Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#463, Add API to query for open orders of one account in one market #849

Merged
merged 18 commits into from
Jul 21, 2018
Merged

#463, Add API to query for open orders of one account in one market #849

merged 18 commits into from
Jul 21, 2018

Conversation

cryptocifer
Copy link
Member

@cryptocifer cryptocifer commented Apr 15, 2018

Local tests ok:

  1. call this api through websocket
  2. call this api through cli_wallet

But I'm not very sure if this pr meets the requests of #463, pls help pointing out if not.

Update:

may fix #811 btw

@abitmore abitmore self-requested a review April 15, 2018 23:58
Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abitmore
Copy link
Member

Wanted parameters: account, market (base and quote) and fields for pagination.

@cryptocifer
Copy link
Member Author

the get_account_orders() api I added mostly copied from get_full_account(), the first parameter is a array of accounts.

Per the requests and for simplicity, I think new api's first parameter can just be one account name (or id), is there any case that will request multiple accounts' orders at one time?

@abitmore
Copy link
Member

@cifer-lee IMHO the new APIs should only use single account, but not an array. Thanks.

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I didn't see why querying by expiration makes sense. Please explain?
  • Need a way to query for the second page and more. Usually we use two parameters: a start and a limit.
  • I think it's best to sort orders by price, and the best field for pagination is order's ID (as start of each page). Please think a bit more.

Thank you.

auto quote_id = assets[1]->id;

// Add the account's orders
auto order_range = _db.get_index_type<limit_order_index>().indices().get<by_account>().equal_range(account->id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps best to add an index for this API? Iterating with continue below is inefficient.

}

vector<limit_order_object> database_api_impl::get_account_limit_orders( const string& name_or_id, const string& base, const string& quote, fc::time_point_sec expire, uint32_t limit)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if limit is too big. Since limit_order is relatively small, I think we can return a bit more per page.

@cryptocifer
Copy link
Member Author

@abitmore Thanks for your comments, I had think before made these changes.

IMHO by expiration maybe simple to use, since client would not know order id? But client could easily fetch account's orders by specifying an timestamp and a limit.

Shortcomings are that expiration is not ordered in limit_order_index, and many orders not specifying expiration may default to year 2038. Actually, I wish limit order could have a create time, so we can querying account's orders this way: query 100 of Sam's limit orders before 2018-04-18, time descending.

@abitmore
Copy link
Member

abitmore commented Apr 19, 2018

Clients software do know order id, although perhaps aren't showing it on UI.

My thoughts about pagination:

  • with a given id, get price of the order;
    * query with by_price index, with lower bound from tuple (price,id) as lower bound, then iterate forward.
  • query with an appropriate index, with lower bound from tuple (price,id) as lower bound, then iterate forward.

// Update:
For this approach, we need a (price,account,id) or (account,price,id) index. Since by_price is related to consensus about order matching, we'll avoiding changing that. But I guess it's fine to change by_account index to (account,price,id), since it's only used by API. Please correct me if I'm wrong.

@cryptocifer
Copy link
Member Author

query with by_price index, with lower bound from tuple (price,id) as lower bound, then iterate forward.

But this means filter the target account iteratively, not this back to the inefficiency issue you pointed above?

@abitmore
Copy link
Member

@cifer-lee sorry, I've noticed the issue then updated the comment later. Please check again.

@cryptocifer
Copy link
Member Author

cryptocifer commented Apr 19, 2018

@abitmore yeah I noticed your updates but no time to fix daytime, now I am working on it.

update:

Sorry i think there still exists a trouble, when we get an index (account, price, id), we can't do pagination by id, since orders will first sorting by account and price, they will mostly not in order by id, thus pagination by id won't work too.

So it seems if we want pagination by id, we shouldn't sort by price, but it also depend on sorting by price to select out target market. And I found no method of just getting orders by market without sorting.

In my opinion, we may need to query orders by (account, id) index, and filter out the target market in iterative way, which means orders not sorted by price. If must, we can sort it by price manually before response to client, while client may need to traverse the received orders to find out the max order id - using it to request the next page

@abitmore
Copy link
Member

@cifer-lee my thoughts about pagination (two steps for one API query):

  • with a given id, get price of the order;
  • query with an appropriate index, with lower bound from tuple (account,price,id) as lower bound, then iterate forward.

@abitmore
Copy link
Member

@cifer-lee some changes in the last commit are not ideal:

  • we need "base" and "quote" in parameter list, otherwise there is no good way to query for the first page;
  • we need (account,price,id) index to query for the 2nd page, but not (account,id,price).

@cryptocifer
Copy link
Member Author

@abitmore Have fixed per to discussion on telegram, please check~

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap long lines which are more than 120 characters.

for ( ; lower_itr != upper_itr ; ++lower_itr) {
const limit_order_object &order = *lower_itr;

if (count++ >= limit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not move this into for?


vector<limit_order_object> database_api_impl::get_account_limit_orders( const string& name_or_id, const string &base, const string &quote, uint32_t limit, optional<limit_order_id_type> ostart_id, optional<price> ostart_price)
{
FC_ASSERT( limit <= 100 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to 101 for easier pagination.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but why 101 easier for pagination?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we use 100, 50 or etc as page size. Since we use lower_bound here, if pass in the last item of last page, with 100 as limit we can get only 99 new items (although it's possible to get around it e.g. pass in last_id+1 but it's not convenient).

if (!ostart_id && !ostart_price) {
lower_itr = index_by_account.lower_bound(std::make_tuple(account->id, price::max(base_id, quote_id)));
upper_itr = index_by_account.upper_bound(std::make_tuple(account->id, price::min(base_id, quote_id)));
} else if (ostart_id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid using optional::bool(). Use ostart_id.is_valid() instead. BTW using operator!() is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, but why avoid using optional::bool()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistent, I will change both these two lines to use ostart_id.valide()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/bitshares/bitshares-fc/blob/master/include/fc/optional.hpp#L190-L192

      // this operation is not safe and can result in unintential 
      // casts and comparisons, use valid() or !! 
      explicit operator bool()const  { return _valid;  }

I can be wrong though. Since operator bool() is explicit, I didn't get what conversion will be done when calling if(ostart_id).

// in case of the order been deleted during page querying
if (!_db.find_object(*ostart_id)) {
if (ostart_price) {
lower_itr = index_by_account.lower_bound(std::make_tuple(account->id, (*ostart_price).max(), *ostart_id));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use ostart_price here but not max().

return results;
}
} else {
const limit_order_object &loo = _db.get(*ostart_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this we searched same object twice, which is unnecessary.

By the way, we need to check if the order is in base:quote and is owned by that account. If not, IMHO best to throw out an exception.

* @param quote Quote asset
* @param limit Max items to fetch
* @param start_id Start order id, fetch orders which price lower than this order
* @param start_price Fetch orders with price lower than this price
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lower is not accurate.

* @param base Base asset
* @param quote Quote asset
* @param limit Max items to fetch
* @param start_id Start order id, fetch orders which price lower than this order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lower is not accurate. BTW if the price is the same, order id should be equal or higher?

* @return List of orders from @ref name_or_id to the corresponding account
*
* @note
* 1. if @ref name_or_id cannot be tied to an account, empty results will be returned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change results to result.

* @param name_or_id Each item must be the name or ID of an account to retrieve
* @param base Base asset
* @param quote Quote asset
* @param limit Max items to fetch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention the limitation of this parameter.

@@ -378,6 +378,7 @@ class wallet_api
vector<operation_detail> get_relative_account_history(string name, uint32_t stop, int limit, uint32_t start)const;

vector<bucket_object> get_market_history(string symbol, string symbol2, uint32_t bucket, fc::time_point_sec start, fc::time_point_sec end)const;
vector<limit_order_object> get_account_limit_orders( const string& name_or_id, const string &base, const string &quote, uint32_t limit = 100, optional<limit_order_id_type> ostart_id = optional<limit_order_id_type>(), optional<price> ostart_price = optional<price>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add docs.

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job so far.

vector<limit_order_object> get_account_limit_orders( const string& name_or_id,
const string &base,
const string &quote,
uint32_t limit = 100,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

101.

// in case of the order been deleted during page querying
const limit_order_object *p_loo = nullptr;

if (!(p_loo = static_cast<const limit_order_object *>(_db.find_object(*ostart_id)))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the assignment to the line above?

By the way, no need to convert with static_cast if use _db.find().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_db.find() will throw exception on fail, give me no change to use ostart_price getting lower bound

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think find() won't throw exception, but get() will.

Copy link
Member Author

@cryptocifer cryptocifer May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I'm wrong? I found _db.find() has roughly the same logic with _db.get(), especially both of them have an assert clause, maybe not throw exception like FC_ASSERT, but also abort the program.

https://github.com/bitshares/bitshares-core/blob/master/libraries/db/include/graphene/db/object_database.hpp#L109-L122

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find() can return nullptr, so should be fine.

In regards to the use of assert(), actually I'm not sure if it's better to change them to FC_ASSERT. Main reason is assert() will be skipped when building in Release mode for better performance, but by doing so we may have skipped some important checking; another reason is the abortion behavior you've noticed. More info in #511.

const account_object* account = nullptr;
uint32_t count = 0;
limit_order_id_type start_id;
price start_price;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variables?

if (!(p_loo = static_cast<const limit_order_object *>(_db.find_object(*ostart_id)))) {
if (ostart_price.valid()) {
lower_itr = index_by_account.lower_bound(std::make_tuple(account->id, *ostart_price, *ostart_id));
upper_itr = index_by_account.upper_bound(std::make_tuple(account->id, (*ostart_price).min()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(*x). can be replaced with x->.

* @param name_or_id The name or ID of an account to retrieve
* @param base Base asset
* @param quote Quote asset
* @param limit The limitation of items each query can fetch, currently 101
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limit can be 101 or smaller.

* @param base Base asset
* @param quote Quote asset
* @param limit The limitation of items each query can fetch, currently 101
* @param start_id Start order id, fetch orders which price are lower than or equal to this order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a clearer description here. Perhaps something like "price lower than this order, or price equal to this order but order ID greater than this order".

* @note
* 1. if @ref name_or_id cannot be tied to an account, empty result will be returned
* 2. @ref start_id and @ref start_price can be empty, if so the api will return the "first page" of orders;
* if start_id is specified and valid, its price will be used to do page query preferentially, otherwise the start_price will be used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need description about the scenario when both start_id and start_price will be used.

@@ -268,6 +268,25 @@ class database_api
*/
vector<optional<account_object>> get_accounts(const vector<account_id_type>& account_ids)const;

/**
* @brief Fetch all orders relevant to the specified account sorted descendingly by price
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to describe somewhere that this API is to query for orders of one specified market but not all markets.

By the way, do we need an API to query for orders of all markets?

Copy link
Member Author

@cryptocifer cryptocifer May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depend on wether ui need it?
if needed I can add this api

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are use cases, but not popular: "I have many open orders, I don't remember what markets are they in, just want to get them all". I think it's low priority.

@abitmore abitmore added this to the 201805 - Non-Consensus-Changing Release milestone May 2, 2018
Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job.

BTW best if coding style can be same to most of existing code:

  • { in new line
  • 3 white spaces as indentation
  • white spaces e.g. if( condition ) and call_function( a, b );
  • wrap long lines


const auto& index_by_account = _db.get_index_type<limit_order_index>().indices().get<by_account>();
limit_order_multi_index_type::index<by_account>::type::const_iterator lower_itr;
limit_order_multi_index_type::index<by_account>::type::const_iterator upper_itr;
Copy link
Member

@abitmore abitmore May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually upper_itr can be initialized here, no need to be initialized in if/else block below, since the results will be the same.

Update: actually it's better to initialize it after the if/else block (than before the block).

Copy link
Member Author

@cryptocifer cryptocifer May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has initialize upper_itr in each of if/else if/else branch, is it ok?

// query with no constraint, expected:
// 1. up to 101 orders returned
// 2. orders were sorted by price desendingly
results = db_api.get_account_limit_orders(seller.name, "BTS", "CNY");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "BTS" to the corresponding macro or use a local variable, otherwise this test case will fail in testnet branch.

BOOST_CHECK(results.size() == 101);
for (int i = 0 ; i < results.size() - 1 ; ++i) {
BOOST_CHECK(results[i].sell_price >= results[i+1].sell_price);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if the first item and the last item are correct (also best to add this check to test cases below).

// 1. up to specified amount of orders returned
// 2. orders were sorted by price desendingly
results = db_api.get_account_limit_orders(seller.name, "BTS", "CNY", 50);
results = db_api.get_account_limit_orders(seller.name, "BTS", "CNY", 50);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant code?

// of the lowest price 150 orders
results = db_api.get_account_limit_orders(seller.name, "BTS", "CNY", 101,
limit_order_id_type(o.id), o.sell_price);
BOOST_CHECK(results.size() <= 101);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know how many records should be returned, so not good to use <=.

for (int i = 0 ; i < results.size() - 1 ; ++i) {
BOOST_CHECK(results[i].sell_price >= results[i+1].sell_price);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test the scenarios that will return an empty vector or throw an exception?

@cryptocifer
Copy link
Member Author

@abitmore have fixed reviews so far except the initialization of upper_itr, hard to initialize it at defining point because it depend on wether ostart_price is valid. And IMO it's ok to assign upper_itr in each if/else branch since more clearly to read.

btw, about code formatting, maybe we can uniform code style using an auto-formatting tool?

@abitmore
Copy link
Member

abitmore commented May 20, 2018

@abitmore have fixed reviews so far except the initialization of upper_itr, hard to initialize it at defining point because it depend on wether ostart_price is valid. And IMO it's ok to assign upper_itr in each if/else branch since more clearly to read.

upper_itr is only related to base and quote.

Update: actually it's better to initialize it after the if/else block (than before the block).

btw, about code formatting, maybe we can uniform code style using an auto-formatting tool?

Of course you can use a tool. However, usually we only format new code and sometimes related old code for easier review (via diffing tool).

@abitmore
Copy link
Member

@cifer-lee your commit history is not clear (duplicated commits and unrelated commits), please rebase.

@@ -680,12 +680,13 @@ vector<limit_order_object> database_api_impl::get_account_limit_orders( const st
{
// in case of the order been deleted during page querying
const limit_order_object *p_loo = _db.find(*ostart_id);
upper_itr = index_by_account.upper_bound(std::make_tuple(account->id, price::min(base_id, quote_id)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change slightly reduced performance under certain circumstances :)

Before this change, if an exception is thrown due to FC_ASSERT below, this database query won't be executed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance reduction means if run into FC_ASSERT, upper_itr initialization is vain here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

@abitmore abitmore modified the milestones: 201806 - Non-Consensus-Changing Release, 201807 - Next Non-Consensus-Changing Release May 21, 2018
@litepresence
Copy link
Contributor

litepresence commented May 24, 2018

not sure if this helps, but this is how I sort my limit orders in one market / one account; sorted by orderNumber... on client side using websocket-client w/ call get_full_accounts, in python, without pybitshares

https://pastebin.com/Bg2L78qB

returns of list of dicts with keys:
price, amount, market, orderNumber, orderType

@cryptocifer
Copy link
Member Author

@litepresence Thanks~ but as have talked above, better to sorted first by price, then by order id

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Code looks fine.
  • Test cases didn't cover all FC_ASSERTs in the code.
  • (In my own opinion) name of the API is debatable.

@ryanRfox and other core devs please take a look.

Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with abit. The code looks good, and compiling/testing works.

As for the naming, I'm assuming "limit" is talking about "limit orders" and not the size of the returned vector. As to how the name fits with the rest of the API, I'll defer to those with more maturity in the project.

soapbox:
I believe that we have to be reasonable and not overly pedantic about testing / code coverage. But the more we cover in our tests the better. A bug caught in a test is much cheaper than a bug caught by the user. (even more-so in the blockchain world). But the corollary is also true: if you try to make it idiot proof, someone will build a better idiot.
/soapbox

@cryptocifer
Copy link
Member Author

@abitmore @jmjatlanta Thanks for your comments, I feel sorry for the partially covered test cases and the ambiguous api name. I have never written test cases before, so have no conscious about coverage, I will supplement the missing cases sooner.

The api name is derived from get_limit_orders api, I have no good idea how to make it clearer, maybe get_limit_orders_by_account looks clearer but the parameters indicates it was obviously not just "by account". Hearing from maturity too..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants