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

Review limitations on API's #783

Closed
6 of 14 tasks
abitmore opened this issue Mar 25, 2018 · 22 comments
Closed
6 of 14 tasks

Review limitations on API's #783

abitmore opened this issue Mar 25, 2018 · 22 comments
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) api performance

Comments

@abitmore
Copy link
Member

abitmore commented Mar 25, 2018

Some API's have a limit parameter but don't check whether it's too big. Inspired by #781 (comment).

Be careful since changes may break client applications.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
    • Assigned: @manikey123
      • get_limit_orders
      • get_call_orders
      • get_settle_orders
      • get_order_book
    • Estimated:
      • 3 hours per API (12 hours total for research, implementation, testing and documentation)
    • Remittance: Pending
  • Perform QA/Testing
  • Update Documentation
@ryanRfox ryanRfox added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) labels May 24, 2018
@manikey123
Copy link
Contributor

Claiming this issue

@ryanRfox
Copy link
Contributor

Thank you @manikey123 for picking this up. May I request you spend an hour on this and #844 to research and document your proposed solution along with an estimate for each.

@manikey123
Copy link
Contributor

manikey123 commented Jan 14, 2019

From #781
"i am wondering if we should FC_ASSERT the limit in get_limit_orders and similar too as we do in most calls."
So focusing on get_limit_orders. Currently in below line for get_limit_orders the limit is external set from the api input
https://github.com/bitshares/bitshares-core/blob/master/libraries/app/database_api.cpp#L1152

Please do provide clarifications for the below points:

  1. Should FC_ASSET be present for the limit value
  2. what should be the default value of the limit
  3. Should the limit values be configurable from the config.ini

let me know your feedback

@pmconrad
Copy link
Contributor

IMO FC_ASSERT should be added, with a default value of 100, and the value should be configurable in the sense of #782 .

@abitmore
Copy link
Member Author

bitshares-ui calls get_limit_orders, get_call_orders, get_settle_orders with limit = 300. See bitshares/bitshares-ui#1796

@manikey123
Copy link
Contributor

783 has 3 API's in scope.
Initial research : 0 hours ( this ticket is previously researched part of 782)
Estimating 3 hours per API (development & research, unit testing, review notes)
Total hours estimate : 3*3=9 hours (estimate)
@ryanRfox @abitmore @pmconrad : let me know if I can proceed with development. Thanks

@manikey123
Copy link
Contributor

Starting with above development

@ryanRfox
Copy link
Contributor

ryanRfox commented Feb 6, 2019

Assigned to @manikey123 for 3 hours per API. Added a Task List to the Description above. Also, added the get_order_book from #782 (comment) provided by @bitfab

@ryanRfox ryanRfox added the 2d Developing Status indicating currently designing and developing a solution label Feb 6, 2019
@manikey123
Copy link
Contributor

updated code for review #1598

@abitmore
Copy link
Member Author

The purpose was to mitigate DoS. Every API call should not cause too much pressure on an API node.

@manikey123
Copy link
Contributor

manikey123 commented Mar 23, 2019

based on above notes, went through code and below are API that can be configured:

get_withdraw_permissions_by_giver --> limit=101
get_withdraw_permissions_by_recipient --> limit=101
get_fill_order_history --> limte = to be updated

lookup_witness_accounts --> limit=1000
lookup_committee_member_accounts --> limit=1000
lookup_vote_ids --> limit=1000
lookup_accounts --> limit=1000

list_assets --> limit=101

@abitmore @pmconrad : let me know your feedback

@pmconrad
Copy link
Contributor

IMO get_fill_order_history should have a maximum as well, and IMO all of these should be configurable.

@manikey123
Copy link
Contributor

Based on above discussion, do I proceed with development ? @pmconrad @abitmore @ryanRfox

@ryanRfox
Copy link
Contributor

Yes. Please proceed. Thank you for your efforts @manikey123

@abitmore
Copy link
Member Author

Perhaps we can make this configurable as well?

if(_subscribed_accounts.size() < 100) {
Need a new ticket?

@abitmore
Copy link
Member Author

abitmore commented Apr 24, 2019

And these:

@abitmore
Copy link
Member Author

Created new issue #1733 for above items.

@manikey123
Copy link
Contributor

2 hours spent on updating PR comments

@abitmore
Copy link
Member Author

Docs are needed to close this.

@abitmore
Copy link
Member Author

@ryanRfox added a "docs needed" label for this issue but didn't remove it. I don't know what docs are needed. I think this issue can be closed since we've merged #1673. Please feel free to reopen if need more work.

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) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) api performance
Projects
None yet
Development

No branches or pull requests

5 participants