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

Tick size amendment #1922

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@JoelKatz
Copy link
Member

JoelKatz commented Dec 5, 2016

Add an amendment to allow gateways to set a "tick size" for assets they issue. There are no changes unless the amendment is enabled (since the tick size option cannot be set).

Unit tests are include both for Quality::round itself and the use of the tick size through the AccountSet and OfferCreate transactors.

With the amendment enabled:

AccountSet transactions may set a "TickSize" parameter. Legal values are 0 and 3-15 inclusive. Zero removes the setting. 3-15 allow that many decimal digits of precision in the pricing of offers for assets issued by this account.

For asset pairs with XRP, the tick size imposed, if any, is the tick size of the issuer of the non-XRP
asset. For asset pairs without XRP, the tick size imposed, if any, is the smaller of the two issuer's
configured tick sizes.

The tick size is imposed by rounding the offer quality down to nearest tick and recomputing the
non-critical side of the offer. For a buy, the amount offered is rounded down. For a sell, the amount
charged is rounded up.

Gateways must enable a TickSize on their account for this feature to benefit them.

The primary expected benefit is the elimination of bots fighting over the tip of the order book. This means:

  • Quicker price discovery as outpricing someone by a microscopic amount is made impossible. Currently
    bots can spend hours outbidding each other with no significant price movement.

  • A reduction in offer creation and cancellation spam.

  • More offers left on the books as priority means something when you can't outbid by a microscopic amount.

@JoelKatz JoelKatz force-pushed the JoelKatz:tick_size branch 3 times, most recently from 01f0fc3 to e0d09a0 Dec 5, 2016

@vinniefalco

This comment has been minimized.

Copy link
Contributor

vinniefalco commented Dec 5, 2016

We need a doc preview, @ximinez please coordinate

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 5, 2016

Current coverage is 65.51% (diff: 94.00%)

Merging #1922 into develop will increase coverage by 0.04%

@@            develop      #1922   diff @@
==========================================
  Files           698        698          
  Lines         49745      49797    +52   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          32568      32625    +57   
+ Misses        17177      17172     -5   
  Partials          0          0          

Powered by Codecov. Last update 9e960ff...49cb0c3

@JoelKatz JoelKatz force-pushed the JoelKatz:tick_size branch from e0d09a0 to 7ea5732 Dec 6, 2016

@ximinez

This comment has been minimized.

Copy link
Contributor

ximinez commented Dec 6, 2016

Because @JoelKatz has a BOOST_ROOT related compatibility issue building docs, I went ahead and pushed the preview to https://ximinez.github.io/PR1922/

@nbougalis nbougalis self-assigned this Dec 6, 2016

@ghost

This comment has been minimized.

Copy link

ghost commented Dec 6, 2016

👍

@nbougalis
Copy link
Member

nbougalis left a comment

Micronit: please reword commit slight. I'd change the first line to: "Add support for tick sizes (RIPD-1363):" followed by a blank line, before the exposition text.

@@ -120,4 +120,35 @@ composed_quality (Quality const& lhs, Quality const& rhs)
return Quality ((stored_exponent << (64 - 8)) | stored_mantissa);
}

Quality
Quality::round (int digits) const

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 6, 2016

Member

This function accepts values that are "out of range": the valid range is [3 ... 15]. Is there a reason?

This comment has been minimized.

Copy link
@JoelKatz

JoelKatz Dec 6, 2016

Author Member

It accepts values that it can apply sensibly even if they're outside the normal range. The alternatives are for it to assert, throw, return nonsense values, or return sensible values. I chose the last option.

This comment has been minimized.

Copy link
@nbougalis
(uTickSize).rate();

// We round the side that's not exact,
// just as if your offer happened to execute

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 6, 2016

Member

Micronit: change "your" to "the"

BEAST_EXPECT(q.round(13).rate().getText() == "57719.63525051");
BEAST_EXPECT(q.round(14).rate().getText() == "57719.635250516");
BEAST_EXPECT(q.round(15).rate().getText() == "57719.6352505168");
BEAST_EXPECT(q.round(16).rate().getText() == "57719.63525051682");

This comment has been minimized.

Copy link
@bachase

bachase Dec 6, 2016

Contributor

Per @nbougalis's comment, is it worth testing the 0-2 case since the round function accepts those arguments?

auto const& uGetsIssuerID = saTakerGets.getIssuer ();

std::uint8_t uTickSize = Quality::maxTickSize;
if (!isXRP (uPaysIssuerID))

This comment has been minimized.

Copy link
@bachase

bachase Dec 6, 2016

Contributor

Overkill, but you could wrap this logic in a lambda and call it twice to make it clear the calculation of uTickSize is identical for both issuers.

}
if (! saTakerGets || ! saTakerPays)
{
JLOG (j_.debug()) <<

This comment has been minimized.

Copy link
@bachase

bachase Dec 6, 2016

Contributor

Is it worth having unit test coverage for the rounded to zero case?

if (uTickSize < Quality::maxTickSize)
{
auto const rate =
Quality{saTakerPays, saTakerGets}.round

This comment has been minimized.

Copy link
@bachase

bachase Dec 6, 2016

Contributor

Not specific to this PR, but I'm confused on the order of arguments to Quality for offers. In https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/Quality.h#L39-L40, it states takerPays is always the 'in' argument to a TAmount. For the constructor call here, based on https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/Quality.h#L143-L146 it looks like saTakerPays ends up as the out amount.

This comment has been minimized.

Copy link
@JoelKatz

JoelKatz Dec 6, 2016

Author Member

I may have reversed them all the way through. It will work the same that way, but obviously it's setting a trap for maintaining the code. I'll fix it to be consistent.

This comment has been minimized.

Copy link
@bachase

bachase Dec 6, 2016

Contributor

Not sure if there are already plans, but a future effort could make type wrappers to let the compiler enforce consistency.

JoelKatz added some commits Dec 5, 2016

Add support for tick sizes (RIPD-1363)
Add an amendment to allow gateways to set a "tick size"
for assets they issue. There are no changes unless the
amendment is enabled (since the tick size option cannot
be set).

With the amendment enabled:

AccountSet transactions may set a "TickSize" parameter.
Legal values are 0 and 3-15 inclusive. Zero removes
the setting. 3-15 allow that many decimal digits of
precision in the pricing of offers for assets issued by
this account.

For asset pairs with XRP, the tick size imposed, if
any, is the tick size of the issuer of the non-XRP
asset. For asset pairs without XRP, the tick size
imposed, if any, is the smaller of the two issuer's
configured tick sizes.

The tick size is imposed by rounding the offer
quality down to nearest tick and recomputing the
non-critical side of the offer. For a buy, the amount
offered is rounded down. For a sell, the amount
charged is rounded up.

Gateways must enable a TickSize on their account for
this feature to benefit them.

The primary expected benefit is the elimination of
bots fighting over the tip of the order book. This means:

- Quicker price discovery as outpricing someone by a
microscopic amount is made impossible. Currently
bots can spend hours outbidding each other with no
significant price movement.

- A reduction in offer creation and cancellation spam.

- More offers left on the books as priority means
something when you can't outbid by a microscopic amount.
[FOLD] Suggestions based on code review
Compute quality correctly, round up instead of down.

@JoelKatz JoelKatz force-pushed the JoelKatz:tick_size branch from 7ea5732 to e8b4e33 Dec 7, 2016

@bachase

bachase approved these changes Dec 7, 2016

Copy link
Contributor

bachase left a comment

One last comment nit, but looks great.

// at a slightly better (for the placer) rate
if (bSell)
{
// this is a sell, round the amount we pay

This comment has been minimized.

Copy link
@bachase

bachase Dec 7, 2016

Contributor

From the perspective of the placer, isn't this rounding the amount we get? That is takerPays = placerGets.

This comment has been minimized.

Copy link
@JoelKatz

JoelKatz Dec 13, 2016

Author Member

By "we", I mean the taker. There's an impled "have to" between "we" and "apy". I'll change the comment to make it clearer.

}
else
{
// this is a buy, round the amount we get

This comment has been minimized.

Copy link
@bachase

bachase Dec 7, 2016

Contributor

Likewise, for the placer, isn't this rounding the amount we pay?

@nbougalis

This comment has been minimized.

Copy link
Member

nbougalis commented Dec 25, 2016

Merged as 22a375a.

@nbougalis nbougalis closed this Dec 25, 2016

@JoelKatz JoelKatz deleted the JoelKatz:tick_size branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.