services/horizon/internal/actions: Add /order_book ingestion endpoint #1761

tamirms · 2019-09-19T11:53:33Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with name of package that is most changed in the PR, ex.
services/friendbot, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've updated the relevant CHANGELOG (here for Horizon) if
needed with deprecations, added features, breaking changes, and DB schema changes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

Summary

Implement /order_book implement using the in memory order book graph populated by the experimental ingestion system.

Fixes #1705

Goal and scope

The current /order_book endpoint queries Stellar Core's database to find all the bids and asks for a given trading pair. The experimental ingestion system provides us with an in memory order book graph. This allows us to implement the /order_book endpoint without querying Stellar Core's database at all.

The goal of the experimental ingestion system is that we reduce the coupling between horizon and Stellar Core's database. Implementing the /order_book endpoint using the in memory order book brings us one step closer towards that goal.

Note that I've still kept the old /order_book handler which queries Stellar Core's database. The old handler will still be used by horizon instances that choose not to run the experimental ingestion system.

Summary of changes

add function to order book graph to find all offers spanning a given number of price levels
implement action for the /order_book endpoint which can be used by both the JSON and SSE handlers
add tests

codecov · 2019-09-19T12:34:33Z

Codecov Report

❗ No coverage uploaded for pull request base (release-horizon-v0.22.0@3c46f3c). Click here to learn what that means.
The diff coverage is 69.48%.

@@                    Coverage Diff                     @@
##             release-horizon-v0.22.0    #1761   +/-   ##
==========================================================
  Coverage                           ?   45.31%           
==========================================================
  Files                              ?      393           
  Lines                              ?    26853           
  Branches                           ?        0           
==========================================================
  Hits                               ?    12168           
  Misses                             ?    13443           
  Partials                           ?     1242

Impacted Files	Coverage Δ
services/horizon/internal/ledger/ledger_source.go	`22.91% <0%> (ø)`
services/horizon/internal/web.go	`86.13% <100%> (ø)`
services/horizon/internal/app.go	`57.01% <100%> (ø)`
services/horizon/internal/handler.go	`40.78% <38.7%> (ø)`
services/horizon/internal/actions/orderbook.go	`83.09% <83.09%> (ø)`
exp/orderbook/graph.go	`88.23% <91.3%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c46f3c...73bd86d. Read the comment docs.

tamirms · 2019-09-20T14:32:05Z

@abuiles please ignore this PR while you are on vacation :) . I added you so you can skim through the PR when you get back

bartekn

Looks good but requires some changes. Haven't checked tests yet.

bartekn · 2019-09-25T10:21:13Z

services/horizon/internal/actions/orderbook.go

+			Amount: amount.String(total),
+		})
+
+		delete(amountForPrice, offerPrice)


Shouldn't we use offer.Price here? offerPrice can be inverted.

Suggested change

delete(amountForPrice, offerPrice)

delete(amountForPrice, offer.Price)

bartekn · 2019-09-25T10:27:45Z

services/horizon/internal/actions/orderbook.go

+	response.Bids = offersToPriceLevels(
+		handler.OrderBookGraph.FindOffers(buying, selling, limit),
+		true,
+	)


I think it's possible that the first call to OrderBookGraph.FindOffers will get offers for ledger X and the second one for ledger X+1. I think we need to make a new function (GetOrderbook) that calls two FindOffers in a locked section of code.

bartekn · 2019-09-25T10:31:27Z

services/horizon/internal/actions/orderbook.go

+	}
+
+	response.Asks = offersToPriceLevels(
+		handler.OrderBookGraph.FindOffers(selling, buying, limit),


I think limit determines the max number of price levels not offers in the old code. If the limit is, say, 10 and there are more than 10 offers at a the first price level the result will be invalid. We need to create GetOrderbook on order book graph anyway (check the other comment) so should be easy to fix.

limit determines the max number of price levels in the new code as well

You're right, sorry!

tamirms · 2019-09-25T16:52:43Z

@bartekn I pushed a commit which addressed your PR comments

bartekn

LGTM except overflow issue. Will do a last check after fix is ready.

services/horizon/internal/actions/orderbook.go

bartekn · 2019-09-25T18:23:31Z

services/horizon/internal/actions/orderbook.go

+
+	amountForPrice := map[xdr.Price]xdr.Int64{}
+	for _, offer := range offers {
+		amountForPrice[offer.Price] += offer.Amount


This reminded me of a similar issue from the past: #436. Basically issuers can send MAX_INT64 to any of it's trustors. If two of them with MAX_INT64 create offers with the same price this can overflow. Looks like we may need to use math/big here and then convert numbers using amount.IntStringToAmount.

It seems that existing /order_book endpoint deals with it because postgres handles overflows properly (so doesn't overflow).

@bartekn great catch, I've pushed a commit which fixes this

bartekn

Thanks, looks great!

tamirms force-pushed the orderbook-ingest-endpoint branch from cd6c791 to 2e80047 Compare September 19, 2019 12:32

tamirms force-pushed the orderbook-ingest-endpoint branch 2 times, most recently from 7bc46ba to 4f2034f Compare September 20, 2019 13:58

Add /order_book ingestion endpoint

f611559

tamirms force-pushed the orderbook-ingest-endpoint branch from 4f2034f to f611559 Compare September 20, 2019 14:13

tamirms marked this pull request as ready for review September 20, 2019 14:29

tamirms requested review from bartekn, abuiles and ire-and-curses September 20, 2019 14:29

tamirms mentioned this pull request Sep 23, 2019

Implement /order_book endpoint using experimental ingestion #1705

Closed

bartekn reviewed Sep 25, 2019

View reviewed changes

Code review fixes

9ce144a

bartekn reviewed Sep 25, 2019

View reviewed changes

tamirms added 2 commits September 25, 2019 21:42

Fixed overflow error and struct comment

6a3263e

Use IntStringToAmount instead of StringFromBigInt

73bd86d

bartekn approved these changes Sep 26, 2019

View reviewed changes

tamirms merged commit 6675d2d into stellar:release-horizon-v0.22.0 Sep 26, 2019

tamirms deleted the orderbook-ingest-endpoint branch September 26, 2019 12:17

bartekn mentioned this pull request Oct 7, 2019

services/horizon/internal/db2/history: Asset stats ingestion #1810

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

services/horizon/internal/actions: Add /order_book ingestion endpoint #1761

services/horizon/internal/actions: Add /order_book ingestion endpoint #1761

tamirms commented Sep 19, 2019 •

edited

Loading

codecov bot commented Sep 19, 2019 •

edited

Loading

tamirms commented Sep 20, 2019

bartekn left a comment

bartekn Sep 25, 2019

bartekn Sep 25, 2019

bartekn Sep 25, 2019

tamirms Sep 25, 2019

bartekn Sep 25, 2019

tamirms commented Sep 25, 2019

bartekn left a comment

bartekn Sep 25, 2019

tamirms Sep 25, 2019

bartekn left a comment

	delete(amountForPrice, offerPrice)
	delete(amountForPrice, offer.Price)

services/horizon/internal/actions: Add /order_book ingestion endpoint #1761

services/horizon/internal/actions: Add /order_book ingestion endpoint #1761

Conversation

tamirms commented Sep 19, 2019 • edited Loading

PR Structure

Thoroughness

Release planning

Summary

Goal and scope

Summary of changes

codecov bot commented Sep 19, 2019 • edited Loading

Codecov Report

tamirms commented Sep 20, 2019

bartekn left a comment

Choose a reason for hiding this comment

bartekn Sep 25, 2019

Choose a reason for hiding this comment

bartekn Sep 25, 2019

Choose a reason for hiding this comment

bartekn Sep 25, 2019

Choose a reason for hiding this comment

tamirms Sep 25, 2019

Choose a reason for hiding this comment

bartekn Sep 25, 2019

Choose a reason for hiding this comment

tamirms commented Sep 25, 2019

bartekn left a comment

Choose a reason for hiding this comment

bartekn Sep 25, 2019

Choose a reason for hiding this comment

tamirms Sep 25, 2019

Choose a reason for hiding this comment

bartekn left a comment

Choose a reason for hiding this comment

tamirms commented Sep 19, 2019 •

edited

Loading

codecov bot commented Sep 19, 2019 •

edited

Loading