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.

Sign up for GitHub

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

Jump to bottom

add api calls to get non expired withdrawls #657

Closed

oxarbitrage wants to merge 9 commits into bitshares:develop from oxarbitrage:issue611

Member

oxarbitrage commented Feb 9, 2018

pull request exposes 2 new api calls to get withdraw objects from accounts: get_withdrawals_giver and get_withdrawals_recipient

they can only can get non expired objects as they will be removed from the index when they expire at https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/db_update.cpp#L485

i think history will be needed eventually but we don't want to add it to core so we probably can add this objects here: #500

also, if merged, the calls will need to be added to cli_wallet so i can create some documentation on how a business can use this feature as it is now fully from the cli.


          add api calls to get non expired withdrawls

d822ebe

abitmore requested changes

View reviewed changes

Member

abitmore left a comment

I'd recommend that change type of start to object_id_type or withdraw_permission_object_id_type.

Member

abitmore commented Feb 9, 2018

Having CLI commands for WP is good but not a priority. Businesses will build GUI's with the API's.

abitmore added this to the Next Non-Consensus-Changing Release milestone

abitmore added the api label


          change start parameter type and loop

2677dfb

Member Author

oxarbitrage commented Feb 9, 2018

changed start type. change in the loop was needed to make it work with the object id.

abitmore reviewed

View reviewed changes

libraries/app/database_api.cpp Outdated

-                               });
+                 auto withdraw_itr = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>().lower_bound(boost::make_tuple(account, start));
+                 while( withdraw_itr->withdraw_from_account == account && result.size() < limit)

Member

abitmore Feb 9, 2018

Need to check for end()

Member Author

oxarbitrage Feb 9, 2018

added end of index check at 845eb37

for some reason i cant do _db.get_index_type<withdraw_permission_index>().indices().get<by_from>() alone. when i do it i get a compiler error as:

...
sion_object> >; TagList = boost::mpl::v_item<graphene::chain::by_from, boost::mpl::vector0<mpl_::na>, 0>; Category = boost::multi_index::detail::ordered_unique_tag]’ is protected
   ~ordered_index()
   ^
/home/alfredo/CLionProjects/issue611/libraries/app/database_api.cpp:2074:95: error: within this context
    auto withdraw_idx = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>();
...

so i had to code that twice, as far as i remember i never saw the issue in other indexes.

libraries/app/database_api.cpp Outdated

-                                  result.emplace_back(withdraw);
-                               });
+                 auto withdraw_itr = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>().lower_bound(boost::make_tuple(account, start));

Member

abitmore Feb 9, 2018

Perhaps use upper_bound for pagination, otherwise we will have effectively 99 rows per page, which will impact performance somehow.

Member Author

oxarbitrage Feb 10, 2018

make sense change to upper_bound but i had to add a special case to be able to retrieve first object. this will not make too much sense in the live change as 1.12.0 is already gone but as i am in a private testnet i think the right thing is to consider it.


          add index end check

845eb37

abitmore reviewed

View reviewed changes

libraries/app/database_api.cpp Outdated

+                 auto withdraw_index_end = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>().end();
+                 auto withdraw_itr = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>().lower_bound(boost::make_tuple(account, start));
+                 while( withdraw_itr->withdraw_from_account == account && result.size() < limit && withdraw_itr != withdraw_index_end)

Member

abitmore Feb 9, 2018

end check should be before dereferencing (withdraw_itr->)

Member Author

oxarbitrage Feb 10, 2018

sorry, changed.

abitmore reviewed

View reviewed changes

libraries/app/database_api.cpp Outdated

                  FC_ASSERT( limit <= 100 );
                  vector<withdraw_permission_object> result;
+                 auto withdraw_index_end = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>().end();

Member

abitmore Feb 9, 2018

Two lines of _db.get_index_type<withdraw_permission_index>().indices().get<by_from>() is ugly.

Member Author

oxarbitrage Feb 9, 2018

yea, found the issue. testing now and will upload with all the last suggestions.

Member Author

oxarbitrage Feb 10, 2018

was just needing const auto& to make it work.


          change to upper bound, move end check, add support for 1.12.0 object

f52bdc5

abitmore reviewed

View reviewed changes

libraries/app/database_api.cpp Outdated

-                 while( withdraw_itr->withdraw_from_account == account && result.size() < limit && withdraw_itr != withdraw_index_end)
+                 const auto& withdraw_idx = _db.get_index_type<withdraw_permission_index>().indices().get<by_from>();
+                 auto withdraw_index_end = withdraw_idx.end();
+                 auto withdraw_itr = withdraw_idx.upper_bound(boost::make_tuple(account, start));

Member

abitmore Feb 10, 2018

After changed start to withdraw_permission_id_type and using upper_bound at same time, it would be unable to return the object whose id is 1.12.0 (although in production it's been removed long before).

Member Author

oxarbitrage Feb 10, 2018

yes but i added an extra block for it 2 calls in the. do yo think we should remove it ?

Member

abitmore Feb 10, 2018

But it's ambiguous when start is 1.12.0 (want it to be returned or not). IMHO we can just change start to object_id_type so users can pass in 0.0.0 for example.

Member

abitmore Feb 10, 2018

And make the in-code document about start clearer.

Member Author

oxarbitrage Feb 11, 2018

extended start doc line.

Member

abitmore commented Feb 11, 2018

Another option would be: keep lower_bound and withdraw_permission_id_type, but increase the maximum value of limit to 101, so clients can get 100 new rows per page.


          return to lower bound, change limit to 101

bd7e7a4

Member Author

oxarbitrage commented Feb 11, 2018 •

edited

Loading

Another option would be: keep lower_bound and withdraw_permission_id_type, but increase the maximum value of limit to 101, so clients can get 100 new rows per page.

Used this option.

abitmore requested a review from ryanRfox

February 11, 2018 23:17

Member

abitmore commented Feb 11, 2018

The code looks OK now. Other concerns:

the API names are not beautiful enough
the API may be enough for payers, but we may need more powerful API's for recipients which are usually service providers therefore may generate large quantities of objects and need to query with complex filters. To serve them better, need to implement plugins or even external data processing tools (E.G. ElasticSearch), although this discussion does not belong to this ticket.

@TheTaconator @ryanRfox please advise.

abitmore approved these changes

View reviewed changes

lubos.ilcik and others added 2 commits

February 13, 2018 06:46


          remove "temp for testing" code using BTS string literal

a34d4bd


          Merge commit 'refs/pull/665/head' of https://github.com/bitshares/bit…

06f574e

…shares-core into issue611

abitmore requested changes

View reviewed changes

Member

abitmore left a comment

The API names are ambiguous, they may refer to query for histories, may refer to query by the opposites. Please change the names to get_withdraw_permissions_by_giver and get_withdraw_permissions_by_recipient.

oxarbitrage added 2 commits

February 19, 2018 08:45


          change call names

de1ca7d


          chnage fc_api call names

d4fa8d0

Member Author

oxarbitrage commented Feb 19, 2018

i am sending a new pull request for this as i sent unwanted commits when changed the names here. will reference to this pull.

oxarbitrage closed this

oxarbitrage mentioned this pull request

add withdrawl permission api calls #676

Merged

oxarbitrage deleted the issue611 branch

February 16, 2019 14:06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api