support account names or ids in all api calls #989
Conversation
libraries/app/database_api.cpp
Outdated
| @@ -198,6 +198,24 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl> | |||
| return tmp; | |||
| } | |||
|
|
|||
| const account_object* get_account_from_string( const std::string& name_or_id ) | |||
There was a problem hiding this comment.
I do not believe this method changes anything internally. So you should be able to mark this const, and keep the const qualifier on the methods that call it.
There was a problem hiding this comment.
hey John, thank you for that. i had the intention of fix that before submitting but it seems i forgot. fixed now at 30eda4a thanks again!
libraries/app/database_api.cpp
Outdated
| { | ||
| FC_ASSERT( limit <= 101 ); | ||
| vector<withdraw_permission_object> result; | ||
|
|
||
| const auto& withdraw_idx = _db.get_index_type<withdraw_permission_index>().indices().get<by_authorized>(); | ||
| auto withdraw_index_end = withdraw_idx.end(); | ||
| const account_id_type account = get_account_from_string(account_id_or_name)->id; // ask this, maybe not safe |
There was a problem hiding this comment.
If the account is not found, get_account_from_string throws. So this is safe, at least from the common error.
There was a problem hiding this comment.
thank you, removed the comments from code.
libraries/app/database_api.cpp
Outdated
| @@ -198,6 +198,24 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl> | |||
| return tmp; | |||
| } | |||
|
|
|||
| const account_object* get_account_from_string( const std::string& name_or_id ) const | |||
| { | |||
| // TODO cache the result to avoid repeatly fetching from db | |||
There was a problem hiding this comment.
Possible solution: Put the result in a static, always check to see if the names/ids match.
Caution: It is my understanding that references to db objects can turn stale. That could mean that you'll be pointing to an address that could now be non-null and not an account_object.
Question: Would keeping a copy be a better choice? While the data within it could grow stale, at least you would control its lifetime.
|
@abitmore , @pmconrad I will like you guys to take a look to this when you get the time. I know is a lot of changes but here is a summary about that this pull do:
Thank you guys. |
|
I forgot to mention i had to made changes to Apart from that i don't have test cases for everything changed but all tests are passing with the current state and i had tried all the calls impacted by |
The following is an initial work in #969 giving support of account by name to a few selected calls to discuss if it is the right approach or something else will be better.
In this approach, non exposed function
get_account_from_stringis created, the scope is only database_api.cpp file. Code for it is present(and duplicated) in several api calls likeverify_account_authorityorget_full_accounts. This duplicated code is what it is added to the new function.Next,
verify_account_authoritywas refactored to use the new created call and alsoget_full_accounts. Please note this 2 calls were already supporting account as a string.Next, api call
get_account_balancesis changed to support account by name. In this change, the wallet is impacted so had to make changes there too.There are actually a lot of calls that need to be changed just inside
database_api.cpp, more inapi.cppand some requirewallet.cppchanges so decided to submit pull now before going further as the approach can be wrong.I will post a list of all the impacted apis in the issue soon.
Note: There is a similar issue with assets, it should be better to be able to use all api calls by asset_id or name, probably for another issue.
Note 2: A local cache will be a good idea for
get_account_from_stringas it will be called constantly, other suggestions to improve performance of it will be great.