-
Notifications
You must be signed in to change notification settings - Fork 649
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please submit fix for how to use database API in cli_wallet for example get_full_accounts method? #811 as a new PR.
- Pagination is needed for Update / Add API to query for open orders with pagination #463. I was thinking about limit_order only, since you've coded call_order and settle_order, I think we can add 3 APIs in total.
Wanted parameters: account, market (base and quote) and fields for pagination. |
the 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? |
@cifer-lee IMHO the new APIs should only use single account, but not an array. Thanks. |
There was a problem hiding this 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 alimit
. - 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.
libraries/app/database_api.cpp
Outdated
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); |
There was a problem hiding this comment.
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.
libraries/app/database_api.cpp
Outdated
} | ||
|
||
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) | ||
{ |
There was a problem hiding this comment.
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.
@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. |
Clients software do know order id, although perhaps aren't showing it on UI. My thoughts about pagination:
// Update: |
But this means filter the target account iteratively, not this back to the inefficiency issue you pointed above? |
@cifer-lee sorry, I've noticed the issue then updated the comment later. Please check again. |
@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 So it seems if we want pagination by 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 |
@cifer-lee my thoughts about pagination (two steps for one API query):
|
@cifer-lee some changes in the last commit are not ideal:
|
@abitmore Have fixed per to discussion on telegram, please check~ |
There was a problem hiding this 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.
libraries/app/database_api.cpp
Outdated
for ( ; lower_itr != upper_itr ; ++lower_itr) { | ||
const limit_order_object &order = *lower_itr; | ||
|
||
if (count++ >= limit) |
There was a problem hiding this comment.
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
?
libraries/app/database_api.cpp
Outdated
|
||
vector<limit_order_object> database_api_impl::get_account_limit_orders( const string& name_or_id, const string &base, const string "e, uint32_t limit, optional<limit_order_id_type> ostart_id, optional<price> ostart_price) | ||
{ | ||
FC_ASSERT( limit <= 100 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
libraries/app/database_api.cpp
Outdated
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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)
.
libraries/app/database_api.cpp
Outdated
// 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)); |
There was a problem hiding this comment.
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()
.
libraries/app/database_api.cpp
Outdated
return results; | ||
} | ||
} else { | ||
const limit_order_object &loo = _db.get(*ostart_id); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 "e, uint32_t limit = 100, optional<limit_order_id_type> ostart_id = optional<limit_order_id_type>(), optional<price> ostart_price = optional<price>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add docs.
There was a problem hiding this 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.
- Please wrap long lines to fit this page in screen: https://github.com/bitshares/bitshares-core/pull/849/files;
- best if test cases are added.
vector<limit_order_object> get_account_limit_orders( const string& name_or_id, | ||
const string &base, | ||
const string "e, | ||
uint32_t limit = 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
101
.
libraries/app/database_api.cpp
Outdated
// 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)))) { |
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
libraries/app/database_api.cpp
Outdated
const account_object* account = nullptr; | ||
uint32_t count = 0; | ||
limit_order_id_type start_id; | ||
price start_price; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variables?
libraries/app/database_api.cpp
Outdated
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())); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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 )
andcall_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; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
tests/tests/database_api_tests.cpp
Outdated
// 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"); |
There was a problem hiding this comment.
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.
tests/tests/database_api_tests.cpp
Outdated
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); | ||
} |
There was a problem hiding this comment.
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).
tests/tests/database_api_tests.cpp
Outdated
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant code?
tests/tests/database_api_tests.cpp
Outdated
// 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); |
There was a problem hiding this comment.
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 <=
.
tests/tests/database_api_tests.cpp
Outdated
for (int i = 0 ; i < results.size() - 1 ; ++i) { | ||
BOOST_CHECK(results[i].sell_price >= results[i+1].sell_price); | ||
} | ||
|
There was a problem hiding this comment.
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?
@abitmore have fixed reviews so far except the initialization of btw, about code formatting, maybe we can uniform code style using an auto-formatting tool? |
Update: actually it's better to initialize it after the
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). |
@cifer-lee your commit history is not clear (duplicated commits and unrelated commits), please rebase. |
libraries/app/database_api.cpp
Outdated
@@ -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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
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 returns of list of dicts with keys: |
@litepresence Thanks~ but as have talked above, better to sorted first by price, then by order id |
There was a problem hiding this 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_ASSERT
s in the code. - (In my own opinion) name of the API is debatable.
@ryanRfox and other core devs please take a look.
There was a problem hiding this 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
@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 |
Local tests ok:
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