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

get_full_account only contains given withdrawal_permissions #1654

Closed
sschiessl-bcp opened this issue Mar 15, 2019 · 16 comments
Closed

get_full_account only contains given withdrawal_permissions #1654

sschiessl-bcp opened this issue Mar 15, 2019 · 16 comments
Assignees
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 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) 9a Tiny Effort estimation indicating TBD

Comments

@sschiessl-bcp
Copy link

sschiessl-bcp commented Mar 15, 2019

If I get a full account via get_full_accounts call, I can see a list of objects withdraws which contains all given withdraw_permission s.

Full account should also contain all received ones (or contain nothing and a separate api call is necessary). One suggestion could be to call it withdraw_permissions, which is a list of received and given ones (giver/receiver defined by authorized account id).

@pmconrad pmconrad 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 6 API Impact flag identifying the application programing interface (API) 9a Tiny Effort estimation indicating TBD labels Mar 15, 2019
@pmconrad
Copy link
Contributor

Please use the "Feature request" template next time.

@pmconrad pmconrad added the 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added label Mar 15, 2019
@abitmore abitmore added this to the Future Feature Release milestone Apr 8, 2019
@oxarbitrage oxarbitrage self-assigned this May 4, 2019
@abitmore
Copy link
Member

abitmore commented May 5, 2019

Usually if a business is established, the operational account can have lots (e.g. thousands) of authorized withdraw permission objects, thus it's not good to query via get_full_accounts since the API will likely only returns the first X rows.

There is already another API get_withdraw_permissions_by_recipient which has pagination.

@pmconrad
Copy link
Contributor

pmconrad commented May 5, 2019

How about limiting the list in get_full_accounts to one page and perhaps add a flag if additional entries exist?

@abitmore
Copy link
Member

abitmore commented May 5, 2019

@pmconrad that's a good idea. IMHO all lists returned by get_full_accounts should do so.

@oxarbitrage
Copy link
Member

We have a lot of lists so a flag for each into the full account object will be too much IMO.
Please note we have list of proposals, balances, limit orders, call orders, settle_orders, assets issued, withdraw permissions and htlc.
We can change so each list can just return first page(100 by default - configurable)but this could affect current clients relaying in the full list results.

My suggestion is to only limit to default the withdraws and maybe the htlc as it is new. Add flag for those 2 only.

@abitmore
Copy link
Member

abitmore commented May 5, 2019

My recommendation is to find out an appropriate value as default, even if it changes current behavior. Currently there are a few accounts (e.g. funkit-bithares) with hundreds even thousands of open orders in the system, lots of clients (e.g. reference UI) are already broken when fetching their accounts. API nodes are also under pressure.

@oxarbitrage
Copy link
Member

Ok, what do you think of:

  • Add limit option api-limit-get-full-accounts-lists default 100 into configuration following Fix #782: Change hard-coded limitations in API's to configurable #1513
  • Limit all list output(proposals, balances, limit orders, call orders, settle_orders, assets issued, withdraw permissions and htlc) of get_full_accounts with this max value.
  • No flag, if users is getting 100 results he probably need to use the dedicated functions to get the rest. Probably this need documentation.

Let me know what do you think to continue the development. Appreciated.

@abitmore
Copy link
Member

abitmore commented May 6, 2019

@oxarbitrage I'm fine with your solution. @pm your opinion?

@abitmore
Copy link
Member

abitmore commented May 6, 2019

By the way, after we have bitshares/bitshares-fc#126, we can add parameters to get_full_accounts API to query for needed object types only, to specify limits or etc.

@oxarbitrage
Copy link
Member

Yes, i considered that too but i think we should better discuss it in a new issue after the FC code is merged.

@pmconrad
Copy link
Contributor

pmconrad commented May 7, 2019

These two don't go together. If you get 50 results you don't know if that's because of a low configured limit or because there aren't any more.

@sschiessl-bcp
Copy link
Author

By the way, after we have bitshares/bitshares-fc#126, we can add parameters to get_full_accounts API to query for needed object types only, to specify limits or etc.

I like that idea, also full accounts is getting abused because people are lazy.

In default, full accounts could only return the count of objects instead of the actual objects (e.g. withdrawal permissions), which indicates to the UI that there is need to query if interested. If you add the flag to full account, you get the actual objects (up to X) together with the total object count to decide if further query is necessary

@abitmore
Copy link
Member

abitmore commented May 7, 2019

@sschiessl-bcp's idea is good.

@oxarbitrage: How about adding new count fields to full_account object? Perhaps counts are too expensive to compute, how about just adding flag fields to indicate there is more data? Seems doable.

@abitmore
Copy link
Member

abitmore commented May 7, 2019

If you get 50 results you don't know if that's because of a low configured limit or because there aren't any more.

Need the server info API in this case. #626

@sschiessl-bcp
Copy link
Author

sschiessl-bcp commented May 7, 2019

@sschiessl-bcp's idea is good. How about adding new count fields to full_account object? Perhaps counts are too expensive to compute, how about just adding flag fields to indicate there is more data? Seems doable.

For the user it would be a "Load more" button, it's fine not to display the number of missing entries in case we only get a flag that there is more

@oxarbitrage
Copy link
Member

closed by #1749

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 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) 9a Tiny Effort estimation indicating TBD
Projects
None yet
Development

No branches or pull requests

4 participants