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

services/horizon: Gracefully handle non-canonical prices in GET /order_book #2400

Merged
merged 2 commits into from
Mar 20, 2020

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Mar 19, 2020

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.

Fixes #2010

Due to the non-canonical rational representation of offer prices, they were not being compared properly in a bunch of places, leading to an incorrect result of GET order_book (the limit
parameter wasn't respected and levels were not being coalesced properly) when equal prices with different representation were involved .

This happened in:

  1. graph.findOffers(), which treated equal prices with different
    representation as different price levels.
  2. offersToPriceLevels(), which computed multiple levels for
    equal prices with different representation.

This change fixes the problems above by adding and using comparison operations which take non-canonical representations into account.

In addition, the prices output in order_book are now normalized (which is a requirement in order to properly coalesce prices with identical value but different representation).

Alternatively, we could use Go's math/big.Rat for storing ingested offer prices (since Rats are always normalized) but it may be useful to keep the original values from the ledger as is for debugging purposes.

@cla-bot cla-bot bot added the cla: yes label Mar 19, 2020
@2opremio 2opremio force-pushed the 2010-fix-order-book-price-levels branch from 3ac08ea to a8da652 Compare March 19, 2020 22:07
@2opremio
Copy link
Contributor Author

2opremio commented Mar 19, 2020

I am missing tests, but the approach is ready to review.

@2opremio 2opremio requested a review from tamirms March 19, 2020 22:08
@tamirms
Copy link
Contributor

tamirms commented Mar 19, 2020

@2opremio this looks great! I can approve once you add tests

xdr/price.go Outdated Show resolved Hide resolved
Due to the non-canonical rational representation of offer prices, they were not
being compared properly in a bunch of places, leading to an incorrect result of
`GET order_book` (the `limit` parameter wasn't respected and levels not being
coalesced properly) when equal prices with different representation were
involved.

This happened in:

1. `graph.findOffers()`, which treated equal prices with different
   representation as different price levels.
2. `offersToPriceLevels()`, which computed multiple levels for equal prices with
   different representation.

This change fixes the problems above by adding and using comparison operations
which take non-canonical representations into account.

In addition, the prices output in order_book are now normalized (which is a
requirement in order to properly coalesce prices with identical value but
different representation).

Alternatively, we could use Go's `math/big.Rat` to ingesting offers (since
`Rat`s are always normalized) but it may be useful to keep the original values
from the ledger as is for debugging purposes.
@2opremio 2opremio force-pushed the 2010-fix-order-book-price-levels branch from 8960199 to 7533207 Compare March 20, 2020 18:04
@2opremio 2opremio marked this pull request as ready for review March 20, 2020 18:04
@2opremio
Copy link
Contributor Author

@tamirms this is ready to review, I have added the tests.

@tamirms
Copy link
Contributor

tamirms commented Mar 20, 2020

@2opremio can you add update graph_test.go to make sure FindAsksAndBids() that determines price levels correctly?

@2opremio
Copy link
Contributor Author

@tamirms Done!

@2opremio 2opremio merged commit cbfdb32 into release-horizon-v1.1.0 Mar 20, 2020
@2opremio 2opremio deleted the 2010-fix-order-book-price-levels branch March 20, 2020 19:58
@ire-and-curses ire-and-curses modified the milestone: Horizon 1.1.0 Apr 7, 2020
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.

3 participants