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

Update / Add API to query for open orders with pagination #463

Open
4 of 8 tasks
abitmore opened this issue Nov 6, 2017 · 10 comments
Open
4 of 8 tasks

Update / Add API to query for open orders with pagination #463

abitmore opened this issue Nov 6, 2017 · 10 comments
Assignees
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2a Discussion Needed Prompt for team to discuss at next stand up. 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application api cli feature

Comments

@abitmore
Copy link
Member

abitmore commented Nov 6, 2017

IIRC we have 2 sets of API's related to query for open orders of one account in one market:

  • get_full_accounts
  • get_limit_orders + get_settle_orders + get_call_orders

However, both will return extra data and have some limitations, E.G. no pagination.

IMHO we need these API's:

  • for limit_order
    • get_limit_orders_by_market: one market, for all accounts, with pagination
    • get_limit_orders_by_account: all markets, for one account, with pagination
    • get_limit_orders_by_account_market get_account_limit_orders: one market, for one account, with pagination
  • for call_order
    • get_call_orders_by_asset: one asset, for all accounts, with pagination
    • get_call_orders_by_account: all assets, for one account, with pagination
      - [ ] get_call_orders_by_account_asset: one asset, for one account, with pagination (note: get_call_orders_by_account can be used for this)
  • for settle_order
    • get_settle_orders_by_asset: one asset, for all accounts, with pagination
    • get_settle_orders_by_account: all assets, for one account, with pagination
    • get_settle_orders_by_account_asset: one asset, for one account, with pagination
@abitmore abitmore added the api label Nov 6, 2017
@abitmore
Copy link
Member Author

abitmore commented Nov 6, 2017

Related issue: bitshares/bitshares-ui#393, which was closed by using get_full_accounts API. For common use case, get_full_accounts will be called once with subscription to receive future changes, so it should be good enough. So the feature described in this ticket is low priority thus can be postponed.

@abitmore abitmore added this to the Future Non-Consensus-Changing Release milestone Nov 29, 2017
@oxarbitrage
Copy link
Member

it will need to be by_asset in order to get the 3 orders types, by_market will not get the settlement orders as the force_settlement_object haves 1 asset only.

call should be added to the cli wallet.

@abitmore abitmore modified the milestones: Future Non-Consensus-Changing Release, 201806 - Non-Consensus-Changing Release, 201807 - Next Non-Consensus-Changing Release May 21, 2018
@ryanRfox ryanRfox added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2h Accepted Status indicating the solution passed final review, and is ready for implementation 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application labels Jun 13, 2018
@cryptocifer
Copy link
Member

Hi @ryanRfox, as talked in pr #849, neither the name nor the usage(except for get_limit_orders_by_account) of theses apis are confirmed. Maybe first refine requirements and prioritize them?

@ryanRfox
Copy link
Contributor

ryanRfox commented Jun 14, 2018

I'll add my opinion here and rely on other Core Team members to comment further (looking at you: @abitmore @oxarbitrage @pmconrad ). I will also request the UI Team to review and add their thoughts on API naming and the dataset returned (@wmbutler @svk31 ).

The OP defines the API names in a format I support. I'm not sure if we have a naming convention defined, but this seems to work in this case:
get_<this_data>_by_<filter_set> where <this_data> is the specific order type (limit, call, settle) and <filter_set> is an ordered list of various filters thereof (account, market, asset).

I will suggest to change the API names (but, please await others to comment).

As for the ordering of the parameters for each call, I feel they should begin with and iterate through the <filter_set>. It seems in this case that we have either _by_market or by_account_market, etc. Of course account requires only a single value, whereas market requires multiple parameters itself. In this case the parameter ordering is intuitive. However, we may have a <filter_set> where multiple filters require a set of parameters. That becomes a bit more difficult to intuit what order the parameters should be. As I believe it's out of scope for this Issue, let's table that discussion.

Note: One struggle I had (as the new guy here) with this Issue/PR was most of the refactoring/comments taking place within the PR rather than the Issue. My preference is for the Review to happen in the PR, while keeping the refinement dialog within the Issue, so those reviewing the Issue can more quickly be up to date on progress. Thanks.

@pmconrad
Copy link
Contributor

Agree with the proposed APIs. One question:

get_call_orders_by_account_asset: one asset, for one account, with pagination

There can be at most one call_order per account and asset. Do we still want to return a list, with pagination, for symmetry?

@abitmore abitmore added 2a Discussion Needed Prompt for team to discuss at next stand up. and removed 2h Accepted Status indicating the solution passed final review, and is ready for implementation labels Jun 18, 2018
@abitmore
Copy link
Member Author

@pmconrad right now we don't have many MPA's or PM's in the system, so pagination for get_call_orders_by_account_asset API is not very useful. The pagination is to avoid changing it again in the future.

@pmconrad
Copy link
Contributor

There can be at most one call_order per account and asset.

@abitmore
Copy link
Member Author

There can be at most one call_order per account and asset.

Oh. Thought you're talking about another API. Yes, this one will return one or zero entry. I'm fine if change it to return an optional, and remove the pagination.

@abitmore
Copy link
Member Author

Due to #1749, the get_limit_orders_by_account API is needed asap.

@abitmore abitmore changed the title Add API to query for open orders of one account in one market Update / Add API to query for open orders with pagination Apr 16, 2020
@abitmore
Copy link
Member Author

Implemented get_limit_orders_by_account API via #2146 for 4.0 release. Moving the issue to the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2a Discussion Needed Prompt for team to discuss at next stand up. 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application api cli feature
Projects
None yet
Development

No branches or pull requests

5 participants