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 all asset-related API's to support asset symbol as parameter #1051

Closed
abitmore opened this issue Jun 13, 2018 · 5 comments

Comments

Projects
2 participants
@abitmore
Copy link
Member

commented Jun 13, 2018

User Story
As an API user I want to use asset symbols directly for all APIs. Some APIs support asset symbols, e.g. get_ticker, but others don't support, e.g. get_market_history.

Additional Context (optional)
https://bitsharestalk.org/index.php?topic=26699.0

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

to be done after #989

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

changes will be made at the following api calls.

in database_api:

  • get_assets
  • get_limit_orders
  • get_call_orders
  • get_settle_orders
  • get_collateral_bids
  • subscribe_to_market
  • unsubscribe_from_market
  • get_required_fees

in api:

  • get_fill_order_history
  • get_market_history
  • get_asset_holders
  • get_asset_holders_count
  • get_grouped_limit_orders

cli wallet changes:

other impacts

  • get_ticker, get_24_volume, etc are fine as they use lookup_asset_symbols with the base and quote string, it can be an ID or a name. no changed needed.
  • test cases that use asset_id_type(), asset_id_type(1), etc as an argument of one of the modified calls will need to be casted to a string with the asset id on it.

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jul 21, 2018

@abitmore abitmore removed this from To Do in Feature Release (201808) Jul 21, 2018

@abitmore abitmore added this to To do in Feature release (201810) via automation Jul 29, 2018

@abitmore abitmore removed this from New -Awaiting Core Team Evaluation in Project Backlog Jul 29, 2018

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Please check ee0a8ce for support asset symbol or name in get_limit_orders

Additionally, commit also do the following(please check carefully as the same changes will be added to to at least get_call_orders and get_settle_orders:

  • current version of get_limit_orders accepting ids is moved to implementation for internal use.
  • changed database api to new version, after converting the strings to ids with get_asset_from_string it will call internal impl version.
  • limit was not checked in this call so added a check into database api call get_limit_orders with a max of 100(this will end up in 200 results by keeping the current behaviour).
  • added reserve of the results to increase performance a bit with a size of limit*2 in both calls(internal and api).
@abitmore

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

get_limit_orders with a max of 100

I think it may break bitshares-ui.

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

closed by #1272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.