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

refactor get_assets to accept id or name #1272

Merged
merged 18 commits into from Feb 4, 2019

Conversation

Projects
3 participants
@oxarbitrage
Copy link
Member

commented Aug 22, 2018

pull request to start the work in #1051

the first commit refactor get_assets api call to accept name or id on the arguments.

i need you guys to take a look so i can continue in this way for all the calls if i can get a green light here.

this follow the same idea of what we did for all account api calls at: #989

Show resolved Hide resolved libraries/app/database_api.cpp Outdated
optional<asset_object> find_asset(asset_id_type id)const
{
auto rec = _remote_db->get_assets({id}).front();
auto rec = _remote_db->get_assets({asset_id_to_string(id)}).front();

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 22, 2018

Member

This looks ugly as well.

Perhaps better to add another get_assets_by_id() API?

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 22, 2018

Member

Why didn't change find_asset(asset_id_type id) to find(string)? Is it an function that only used internally?

This comment has been minimized.

Copy link
@abitmore

abitmore Aug 22, 2018

Member

Anyway, I think cli_wallet is less important, it's fine to have some ugly code and slightly reduced performance.

Show resolved Hide resolved libraries/app/database_api.cpp Outdated
@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Good job. Thanks.

Show resolved Hide resolved libraries/app/database_api.cpp Outdated
Show resolved Hide resolved libraries/app/database_api.cpp Outdated
Show resolved Hide resolved libraries/app/database_api.cpp
Show resolved Hide resolved libraries/app/database_api.cpp Outdated
Show resolved Hide resolved libraries/app/database_api.cpp Outdated

@abitmore abitmore added this to In progress in Feature Release (201902) via automation Sep 14, 2018

@abitmore abitmore added this to the 201812 - Feature Release milestone Sep 14, 2018

@ryanRfox ryanRfox moved this from In progress to In Review in Feature Release (201902) Jan 23, 2019

oxarbitrage and others added some commits Jan 29, 2019

@jmjatlanta jmjatlanta referenced this pull request Jan 29, 2019

Merged

Implement API changes #4

oxarbitrage added some commits Feb 1, 2019

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2019

I exposed get_asset_id_from_string as we did before withget_account_id_from_string in this commit: d256af6

I think it can be of use and wont hurt to have them available for API users.

@oxarbitrage oxarbitrage merged commit 5b4e51a into bitshares:develop Feb 4, 2019

1 of 2 checks passed

ci/dockercloud Your tests failed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature Release (201902) automation moved this from In Review to Done Feb 4, 2019

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.