add wallet api get_account_history_by_operations for exchanges #430
Conversation
|
the problem here is that you call we tried this and several other methods to make this call and similar calls to resolve similar issues, they all have some flaw and we concluded to move account history from ram into a database(elasticsearch was selected) indexing everything to make all kind of searches directly against the database instead of creating and maintain new api calls for everything needed. this is work in progress and a first version of this work will be posted very soon. |
| @@ -369,7 +382,7 @@ class wallet_api | |||
| vector<call_order_object> get_call_orders(string a, uint32_t limit)const; | |||
| vector<force_settlement_object> get_settle_orders(string a, uint32_t limit)const; | |||
|
|
|||
| /** Returns the collateral_bid object for the given MPA | |||
| /** Returns the most recent operations on the named account, with transaction id. | |||
There was a problem hiding this comment.
OK. I have updated the document.
| @@ -112,16 +112,15 @@ namespace graphene { namespace app { | |||
| operation_history_id_type start = operation_history_id_type())const; | |||
|
|
|||
| /** | |||
| * @brief Get operations relevant to the specificed account | |||
| * @brief Get operations relevant to the specificed account, return a list of operation history objects with transaction id | |||
There was a problem hiding this comment.
"Get operations relevant to the specificed account, return a list of operation history objects with transaction id"
should be something more like:
"Get operations relevant to the specified account filtering by operation type"
same everywhere.
the parameter name i think it should be "operation_types" instead of "operation_indexs" or "operation_ids".
rest looks good.
This code does not affect performance of the node so i don't have any reason for not to add except for the limit thing that is now specified as a max number ... still don't like much calling for 100 results and getting 1. much more better to do it with the elasticsearch plugin and wrapper as: http://209.188.21.157:5000/get_account_history?account_id=1.2.282&from=100&size=10&sort_by=-operation_type&operation_type=0
anyways, i am not going to oppose to it as there is not infrastructure for elasticsearch yet, if you guys think it is needed then lets make that small changes and add it.
thanks @gxchain123
There was a problem hiding this comment.
another limitation of elasticsearch is that the cli_wallet cant make calls into it yet. i think that in the future the cli will access the database for specific calls like this but not present atm. thats another argument to merge this code at least by now.
|
Something to consider, i was testing the cli wallet call. To make it i started a node following the history of 2 accounts Started node as:
I added a counter dump to the while on line https://github.com/gxchain123/bitshares-core/blob/e4fb550e38354da467997d1e8019072509ff86e9/libraries/wallet/wallet.cpp#L2848. when calling In some others it needs to start looping throw the account history like: Now trying some calls against the other account i am following all good until there but when: the cli will get trapped in an infinite loop as account haves 3,5M operations and loop already at 4,7M. we need to at least stop the loop when the count is greater than total_ops. the cpu usage of the witness_node and the cli_wallet go increasing as the infinite loop is running, cli_wallet more drastically but witness_node also goes climbing. what do you think @abitmore ?
let me know what do you think guys, sorry to be so picky here but this are certain problems i already faced before. |
|
how about adding a counter to stop the loop ? |
|
thank you @zhuliting . yes, that will help, the same call on the 3.5 mill account now at least stops at some point: however i still don't get why that request will need to loop throw the account so much. i am missing something or the logic is not very good in that cases ? please note i am just asking to get all the op types from an account, all this calls will loop throw the account and i think it is not needed: |
|
sorry for my mistake. |
delete log
libraries/wallet/wallet.cpp
Outdated
|
|
||
| uint32_t counter = start; | ||
| while (limit > 0 && counter <= stats.total_ops) { | ||
| ilog("counter: ${c}", ("c", counter)); |
There was a problem hiding this comment.
pls lets remove this line to merge. thx.
There was a problem hiding this comment.
good enough now @zhuliting. thank you for your work on this. lets just remove the counter ilog to merge.
|
counter is redundant in current code |
get_account_history_by_operations returns the most recent operations on the named account. txID will be included in returned transactions.
parameters：account_name_or_id [operation ids] start limit_num
operation ids species the operation type you want to retrieve, for example, [0] stands for transfer operation.
cli_wallet call:
walet api call:
results: