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

add get_top_markets #737

Merged
merged 4 commits into from
Mar 17, 2018
Merged

Conversation

oxarbitrage
Copy link
Member

@oxarbitrage oxarbitrage commented Mar 16, 2018

replaces #560

Edited by @abitmore: this is new PR for #512.

vector<market_volume> result;

fc::uint128 base_volume;
fc::uint128 quote_volume;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variables?

fc::uint128 base_volume;
fc::uint128 quote_volume;

uint c = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable?

@@ -115,6 +115,7 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl>
market_ticker get_ticker( const string& base, const string& quote, bool skip_order_book = false )const;
market_volume get_24_volume( const string& base, const string& quote )const;
order_book get_order_book( const string& base, const string& quote, unsigned limit = 50 )const;
vector<market_volume> get_top_markets(uint limit)const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style: add spaces.

By the way, I don't like uint, perhaps change to uint32_t?

@oxarbitrage
Copy link
Member Author

cleaned that, thanks.

mv.quote = assets[1]->symbol;
mv.base_volume = uint128_amount_to_string( itr->base_volume, assets[0]->precision );
mv.quote_volume = uint128_amount_to_string( itr->quote_volume, assets[1]->precision );
result.push_back(mv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think result.emplace_back( std::move(mv) ) would have better performance.


const auto& volume_idx = _db.get_index_type<graphene::market_history::market_ticker_index>().indices().get<by_volume>();
auto itr = volume_idx.rbegin();
vector<market_volume> result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a result.reserve(limit); here would improve performance.

@oxarbitrage
Copy link
Member Author

thank you, added the performance changes at eaefc59

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure this API will need to be revised later. Please add a note in the in-code doc, e.g. this API is experimental and subject to change in next releases or similar, to leave us some room for future improvement. Then we can merge it and move on.

@oxarbitrage oxarbitrage merged commit 70695b5 into bitshares:develop Mar 17, 2018
@oxarbitrage
Copy link
Member Author

agree, added and merged.

@oxarbitrage oxarbitrage mentioned this pull request Mar 17, 2018
@oxarbitrage oxarbitrage deleted the get_top_markets2 branch June 22, 2019 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants