expand get_top_markets #924

oxarbitrage · 2018-05-14T12:52:58Z

for issue #842 api call now returning market_ticker object data.

abitmore · 2018-05-14T20:22:20Z

libraries/app/database_api.cpp

-      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.emplace_back( std::move(mv) );
+      market_ticker mt = get_ticker( assets[0]->symbol, assets[1]->symbol, true );


Calling get_ticker directly is inefficient, since it will query the database again for same data. We already have itr here so just need to get the data. I recommend that we move the code in get_ticket to a new static function and call it in both get_ticker and here.

abitmore · 2018-05-15T17:41:14Z

libraries/app/database_api.cpp

@@ -244,6 +244,57 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl>
      graphene::chain::database&                                                                                                            _db;
      const application_options* _app_options = nullptr;

+      static market_ticker get_ticker_data(const market_ticker_object mto,


Actually this is not a typical getter function, more suitable as a constructor of market_ticker class/struct.

Please use constant references as parameters.

better to use optional for order book.

abitmore · 2018-05-16T16:04:20Z

libraries/app/database_api.cpp

-      const auto orders = get_order_book( base, quote, 1 );
-      if( !orders.asks.empty() ) result.lowest_ask = orders.asks[0].price;
-      if( !orders.bids.empty() ) result.highest_bid = orders.bids[0].price;
+      mt = market_ticker(*itr, now, *assets[0], *assets[1], orders);


I think this should be placed out of if( itr != ticker_idx.end() ) block.

you mean the following outside?

order_book orders; if( !skip_order_book ) { orders = get_order_book(assets[0]->symbol, assets[1]->symbol, 1); }

i saw you have that outside the if in the original code inside get_ticker.

or you mean this outside?

mt = market_ticker(*itr, now, *assets[0], *assets[1], orders);

sorry, not fully clear this time with the github inline code comment. pls clarify so i can make the proper changes. thanks!

Yes, as original code, both should be outside.

litepresence · 2018-05-23T15:38:37Z

by human eye we can quickly see there are only a limited number of base currencies which matter in terms of top markets by real volume; namely BTS, bitCNY, bitUSD, OPEN.BTC, USDT, GDEX.BTC; perhaps a few others.

I think best way to sort out "spam" and get a true sense of "top markets by volume" would be to first only consider volume vs one of the whitelisted base coins.

then convert all volume to a common base amongst the whitelist; eg I believe BTS trades to each of those mentioned.

then API returns sorted list of markets that has been normalized to "relative" BTS volume

as additional base currencies become relevant they could then be added to the whitelist in future development

abitmore · 2018-05-25T09:37:18Z

@litepresence thanks for the discussion. Firstly we need to define UI/UX appropriately including logic, then decide what should be done by UI and what be done by the back end. So, actually this discussion especially your comments should be started in the UI project/respository.

abitmore

Note: I haven't finished the review so far.

abitmore · 2018-05-23T09:05:28Z

libraries/app/database_api.cpp


+   order_book orders;


Should use optional<order_book> here.

abitmore · 2018-05-27T01:04:29Z

libraries/app/database_api.cpp

-
-   result.base_volume = uint128_amount_to_string( base_volume, assets[0]->precision );
-   result.quote_volume = uint128_amount_to_string( quote_volume, assets[1]->precision );
+   if( itr == ticker_idx.end() ) return mt;


Here should not return.

Actually should change mt to optional as well.

abitmore

I'd recommend NOT change behavior of existing API's. That said, don't return null if was returning an empty object.

oxarbitrage · 2018-05-27T21:45:35Z

you are right, reversed those changes.

abitmore · 2018-05-28T08:03:40Z

libraries/app/database_api.cpp

-   }
-
-   return result;
+   return mt;


Need to include order data if skip_order_book is false, even when no ticker data.

That's why I said to change mt to optional, actually it was a typo, I meant to use a local variable which is optional type or is a pointer, to store *itr (skip if it's end()), then construct mt with it.

abitmore · 2018-05-28T08:08:35Z

libraries/app/database_api.cpp


   while( itr != volume_idx.rend() && result.size() < limit)
   {
-      market_volume mv;
-      mv.time = now;
      const auto assets = get_assets( { itr->base, itr->quote } );


I'd suggest that we use a small local cache in this get_top_markets function to reduce quantity of queries to the database.

jhtitor · 2018-07-26T09:38:43Z

I have 2 cents from bitshares-qt standpoint.

First of all, the get_top_markets API itself is a great idea, and I'm really glad we finally have such a feature. Going to use it extensively.

Secondly, I think it would be great, if the API could be expanded to allow setting base asset. Say, a user is interested in trading OPEN.BTC -- I want to show top OPEN.BTC markets. The current behavior (listing everything) is also extremely useful, so I'm not proposing to replace it.

Sorry if this is the wrong ticket, which only deals with the return format.

abitmore · 2018-07-27T09:47:31Z

I too want to leave something here for reference, but don't want to write a feature request yet:

expand get_top_markets

a83cbd5

abitmore reviewed May 14, 2018

View reviewed changes

create initial get_ticker_data()

2635cab

abitmore reviewed May 15, 2018

View reviewed changes

move get_ticker_data to constructor

c09bd0c

abitmore reviewed May 16, 2018

View reviewed changes

abitmore modified the milestones: 201806 - Non-Consensus-Changing Release, 201807 - Next Non-Consensus-Changing Release May 21, 2018

move some code outside if

ee53019

abitmore self-requested a review May 23, 2018 09:06

abitmore self-assigned this May 23, 2018

abitmore reviewed May 25, 2018

View reviewed changes

libraries/app/database_api.cpp Outdated

order_book orders;

Copy link

Member

abitmore May 23, 2018

Choose a reason for hiding this comment

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

Should use optional<order_book> here.

change order_book to optional

444c970

abitmore reviewed May 27, 2018

View reviewed changes

add more optionals, fix empty volume

46ffa19

abitmore requested changes May 27, 2018

View reviewed changes

reverse optional in production calls

be0e0c5

abitmore reviewed May 28, 2018

View reviewed changes

ryanRfox assigned oxarbitrage Jun 7, 2018

ryanRfox requested review from jmjatlanta and pmconrad June 14, 2018 15:03

abitmore removed the request for review from jmjatlanta July 21, 2018 20:14

abitmore removed the request for review from pmconrad July 21, 2018 20:14

abitmore modified the milestones: 201807 - Next Non-Consensus-Changing Release, Future Non-Consensus-Changing Release Jul 21, 2018

abitmore modified the milestones: Future Non-Consensus-Changing Release, 201810 - Non-Consensus-Changing Release Jul 29, 2018

oxarbitrage closed this Aug 22, 2018

oxarbitrage deleted the issue842 branch August 22, 2018 23:23

abitmore mentioned this pull request Aug 22, 2018

refactor get_top_markets #1273

Closed

abitmore modified the milestones: 201810 - Feature Release, 201812 - Feature Release Sep 14, 2018

abitmore removed this from the 201901 - Feature Release milestone Jan 1, 2019

This was referenced Feb 1, 2019

Expand get_top_markets API to return more data fields #842

Closed

refactor get_top_markets issue #842 #1549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expand get_top_markets #924

expand get_top_markets #924

oxarbitrage commented May 14, 2018 •

edited

Loading

abitmore May 14, 2018

abitmore May 15, 2018

abitmore May 16, 2018

oxarbitrage May 16, 2018

abitmore May 17, 2018

litepresence commented May 23, 2018 •

edited

Loading

abitmore commented May 25, 2018 •

edited

Loading

abitmore left a comment

abitmore May 23, 2018

abitmore May 27, 2018

abitmore left a comment

oxarbitrage commented May 27, 2018

abitmore May 28, 2018 •

edited

Loading

abitmore May 28, 2018

jhtitor commented Jul 26, 2018

abitmore commented Jul 27, 2018

expand get_top_markets #924

expand get_top_markets #924

Conversation

oxarbitrage commented May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litepresence commented May 23, 2018 • edited Loading

abitmore commented May 25, 2018 • edited Loading

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

oxarbitrage commented May 27, 2018

abitmore May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhtitor commented Jul 26, 2018

abitmore commented Jul 27, 2018

oxarbitrage commented May 14, 2018 •

edited

Loading

litepresence commented May 23, 2018 •

edited

Loading

abitmore commented May 25, 2018 •

edited

Loading

abitmore May 28, 2018 •

edited

Loading