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.

Sign up for GitHub

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

Jump to bottom

Amendment fixReducedOffersV2 fixes issue #4937 #5032

Merged
merged 6 commits into from
Jun 13, 2024
Merged

Amendment fixReducedOffersV2 fixes issue #4937 #5032

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ddf547
Amendment fixReducedOffersV2 fixes issue #4937
scottschurr Apr 3, 2024
186eda8
[FOLD] Address review comments
scottschurr Jun 4, 2024
af7849c
[FOLD] Remove unneeded test
scottschurr Jun 5, 2024
aecda42
[FOLD] Prefer getCurrentTransactionRules() over passing Rules
scottschurr Jun 12, 2024
52a04e6
[FOLD] Address comments on the Quality implementation
scottschurr Jun 12, 2024
e5dbc1d
Merge branch 'develop' into blocked-book
seelabs Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/ripple/app/paths/AMMOffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <ripple/ledger/ApplyView.h>
#include <ripple/ledger/View.h>
#include <ripple/protocol/Quality.h>
#include <ripple/protocol/Rules.h>
#include <ripple/protocol/TER.h>

namespace ripple {
Expand Down Expand Up @@ -103,15 +104,19 @@ class AMMOffer
limitOut(
TAmounts<TIn, TOut> const& offrAmt,
TOut const& limit,
bool fixReducedOffers,
Rules const& rules,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using the globally available rules rather than passing this as a parameter. (also fine as-is, but I think I'd like to get away from passing rules around as much as we do and just using the new global).

bool roundUp) const;

/** Limit in of the provided offer. If one-path then swapIn
* using current balances. If multi-path then ceil_in using
* current quality.
*/
TAmounts<TIn, TOut>
limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit) const;
limitIn(
TAmounts<TIn, TOut> const& offrAmt,
TIn const& limit,
Rules const& rules,
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment above about passing rules as a param.

bool roundUp) const;

QualityFunction
getQualityFunc() const;
Expand Down
13 changes: 10 additions & 3 deletions src/ripple/app/paths/impl/AMMOffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ TAmounts<TIn, TOut>
AMMOffer<TIn, TOut>::limitOut(
TAmounts<TIn, TOut> const& offrAmt,
TOut const& limit,
bool fixReducedOffers,
Rules const& rules,
bool roundUp) const
{
// Change the offer size proportionally to the original offer quality
Expand All @@ -92,7 +92,7 @@ AMMOffer<TIn, TOut>::limitOut(
// poolPays * poolGets < (poolPays - assetOut) * (poolGets + assetIn)
if (ammLiquidity_.multiPath())
{
if (fixReducedOffers)
if (rules.enabled(fixReducedOffersV1))
// It turns out that the ceil_out implementation has some slop in
// it. ceil_out_strict removes that slop. But removing that slop
// affects transaction outcomes, so the change must be made using
Expand All @@ -110,11 +110,18 @@ template <typename TIn, typename TOut>
TAmounts<TIn, TOut>
AMMOffer<TIn, TOut>::limitIn(
TAmounts<TIn, TOut> const& offrAmt,
TIn const& limit) const
TIn const& limit,
Rules const& rules,
bool roundUp) const
{
// See the comments above in limitOut().
if (ammLiquidity_.multiPath())
{
if (rules.enabled(fixReducedOffersV2))
return quality().ceil_in_strict(offrAmt, limit, roundUp);

return quality().ceil_in(offrAmt, limit);
}
return {limit, swapAssetIn(balances_, limit, ammLiquidity_.tradingFee())};
}

Expand Down
18 changes: 11 additions & 7 deletions src/ripple/app/paths/impl/BookStep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,14 +661,18 @@ limitStepIn(
TOut& ownerGives,
std::uint32_t transferRateIn,
std::uint32_t transferRateOut,
TIn const& limit)
TIn const& limit,
Rules const& rules)
{
if (limit < stpAmt.in)
{
stpAmt.in = limit;
auto const inLmt =
mulRatio(stpAmt.in, QUALITY_ONE, transferRateIn, /*roundUp*/ false);
ofrAmt = offer.limitIn(ofrAmt, inLmt);
// It turns out we can prevent order book blocking by (strictly)
// rounding down the ceil_in() result. This adjustment changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be beneficial to add a comment here and in limitStepOut explaining why the choice of the rounding in each case prevents order book blocking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Maybe something like...

// It turns out we can prevent order book blocking by (strictly)
// rounding down the ceil_in() result.  By rounding down we
// guarantee that the quality of an offer left in the ledger is
// as good or better than the quality of the containing order
// book page.

// transaction outcomes, so it must be made under an amendment.
ofrAmt = offer.limitIn(ofrAmt, inLmt, rules, /* roundUp */ false);
stpAmt.out = ofrAmt.out;
ownerGives = mulRatio(
ofrAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false);
Expand Down Expand Up @@ -696,7 +700,7 @@ limitStepOut(
ofrAmt = offer.limitOut(
ofrAmt,
stpAmt.out,
rules.enabled(fixReducedOffersV1),
rules,
/*roundUp*/ true);
stpAmt.in =
mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true);
Expand Down Expand Up @@ -736,7 +740,6 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
sb, afView, book_, sb.parentCloseTime(), counter, j_);

bool const flowCross = afView.rules().enabled(featureFlowCross);
bool const fixReduced = afView.rules().enabled(fixReducedOffersV1);
bool offerAttempted = false;
std::optional<Quality> ofrQ;
auto execOffer = [&](auto& offer) {
Expand Down Expand Up @@ -818,7 +821,7 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
// rounding down the ceil_out() result. This adjustment changes
// transaction outcomes, so it must be made under an amendment.
ofrAmt = offer.limitOut(
ofrAmt, stpAmt.out, fixReduced, /*roundUp*/ false);
ofrAmt, stpAmt.out, afView.rules(), /*roundUp*/ false);

stpAmt.in =
mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true);
Expand Down Expand Up @@ -1177,7 +1180,8 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
ownerGivesAdj,
transferRateIn,
transferRateOut,
remainingIn);
remainingIn,
afView.rules());
savedIns.insert(remainingIn);
lastOut = savedOuts.insert(stpAdjAmt.out);
result.out = sum(savedOuts);
Expand Down Expand Up @@ -1228,7 +1232,7 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
}
else
{
// This is (likely) a problem case, and wil be caught
// This is (likely) a problem case, and will be caught
// with later checks
savedOuts.insert(lastOutAmt);
}
Expand Down
26 changes: 20 additions & 6 deletions src/ripple/app/tx/impl/Offer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <ripple/basics/contract.h>
#include <ripple/ledger/View.h>
#include <ripple/protocol/Quality.h>
#include <ripple/protocol/Rules.h>
#include <ripple/protocol/SField.h>
#include <ripple/protocol/STLedgerEntry.h>
#include <ostream>
Expand Down Expand Up @@ -140,11 +141,15 @@ class TOffer : private TOfferBase<TIn, TOut>
limitOut(
TAmounts<TIn, TOut> const& offrAmt,
TOut const& limit,
bool fixReducedOffers,
Rules const& rules,
bool roundUp) const;

TAmounts<TIn, TOut>
limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit) const;
limitIn(
TAmounts<TIn, TOut> const& offrAmt,
TIn const& limit,
Rules const& rules,
bool roundUp) const;

template <typename... Args>
static TER
Expand Down Expand Up @@ -219,10 +224,10 @@ TAmounts<TIn, TOut>
TOffer<TIn, TOut>::limitOut(
TAmounts<TIn, TOut> const& offrAmt,
TOut const& limit,
bool fixReducedOffers,
Rules const& rules,
bool roundUp) const
{
if (fixReducedOffers)
if (rules.enabled(fixReducedOffersV1))
// It turns out that the ceil_out implementation has some slop in
// it. ceil_out_strict removes that slop. But removing that slop
// affects transaction outcomes, so the change must be made using
Expand All @@ -233,9 +238,18 @@ TOffer<TIn, TOut>::limitOut(

template <class TIn, class TOut>
TAmounts<TIn, TOut>
TOffer<TIn, TOut>::limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit)
const
TOffer<TIn, TOut>::limitIn(
TAmounts<TIn, TOut> const& offrAmt,
TIn const& limit,
Rules const& rules,
bool roundUp) const
{
if (rules.enabled(fixReducedOffersV2))
// It turns out that the ceil_in implementation has some slop in
// it. ceil_in_strict removes that slop. But removing that slop
// affects transaction outcomes, so the change must be made using
// an amendment.
return quality().ceil_in_strict(offrAmt, limit, roundUp);
return m_quality.ceil_in(offrAmt, limit);
}

Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 73;
static constexpr std::size_t numFeatures = 74;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -360,6 +360,7 @@ extern uint256 const fixEmptyDID;
extern uint256 const fixXChainRewardRounding;
extern uint256 const fixPreviousTxnID;
extern uint256 const fixAMMv1_1;
extern uint256 const fixReducedOffersV2;

} // namespace ripple

Expand Down
23 changes: 23 additions & 0 deletions src/ripple/protocol/Quality.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,29 @@
toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
}

Amounts
ceil_in_strict(Amounts const& amount, STAmount const& limit, bool roundUp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use a comment for how "strict" differs from the non-strict function.

const;

template <class In, class Out>
TAmounts<In, Out>
ceil_in_strict(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually don't define functions inline in the class. I know this is a member template and that gets a little tedious outside of the class. I'd prefer it was defined out of the class, but it's a minor nit.

TAmounts<In, Out> const& amount,
In const& limit,
bool roundUp) const
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a copy of ceil_in with one function called changed.

If ceil_in is being deprecated and will be replaced with ceil_in_strict then I actually prefer the code duplication. And I think this is the case. If this is not the case, then let's look at merging the two.

On the same topic, if the ceil_in is being deprecated, consider renaming ceil_in to ceil_in_deprecated or somesuch and renaming ceil_in_strict to ceil_in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about ceil_in being deprecated. But it certainly has an odd behavior regarding rounding. So we might eventually want to replace all uses of it with ceil_in_strict. But I'd like to hold off on renaming either of them until we know that we're actually going to make that change (which would have to go under an amendment). There are a few things in this code base that have been "deprecated" for quite a few years without ever being removed. I'd like to not add to that confusion.

if (amount.in <= limit)
return amount;

Check warning on line 215 in src/ripple/protocol/Quality.h

View check run for this annotation

Codecov / codecov/patch

src/ripple/protocol/Quality.h#L215 

Added line #L215 was not covered by tests

// Use the existing STAmount implementation for now, but consider
// replacing with code specific to IOUAMount and XRPAmount
Amounts stAmt(toSTAmount(amount.in), toSTAmount(amount.out));
STAmount stLim(toSTAmount(limit));
auto const stRes = ceil_in_strict(stAmt, stLim, roundUp);
return TAmounts<In, Out>(
toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
}

/** Returns the scaled amount with out capped.
Math is avoided if the result is exact. The input is clamped
to prevent money creation.
Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ REGISTER_FIX (fixEmptyDID, Supported::yes, VoteBehavior::De
REGISTER_FIX (fixXChainRewardRounding, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixAMMv1_1, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixReducedOffersV2, Supported::yes, VoteBehavior::DefaultNo);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
Expand Down
28 changes: 25 additions & 3 deletions src/ripple/protocol/impl/Quality.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,20 @@ Quality::operator--(int)
return prev;
}

Amounts
Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
template <STAmount (
*DivRoundFunc)(STAmount const&, STAmount const&, Issue const&, bool)>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually like passing the callback as a parameter rather than as a template. But I'm also fine with this as-is.

static Amounts
ceil_in_impl(
Amounts const& amount,
STAmount const& limit,
bool roundUp,
Quality const& quality)
{
if (amount.in > limit)
{
Amounts result(
limit, divRound(limit, rate(), amount.out.issue(), true));
limit,
DivRoundFunc(limit, quality.rate(), amount.out.issue(), roundUp));
// Clamp out
if (result.out > amount.out)
result.out = amount.out;
Expand All @@ -81,6 +88,21 @@ Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
return amount;
}

Amounts
Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
{
return ceil_in_impl<divRound>(amount, limit, /* roundUp */ true, *this);
}

Amounts
Quality::ceil_in_strict(
Amounts const& amount,
STAmount const& limit,
bool roundUp) const
{
return ceil_in_impl<divRoundStrict>(amount, limit, roundUp, *this);
}

template <STAmount (
*MulRoundFunc)(STAmount const&, STAmount const&, Issue const&, bool)>
static Amounts
Expand Down
36 changes: 22 additions & 14 deletions src/test/app/AMM_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3618,27 +3618,33 @@ struct AMM_test : public jtx::AMMTest
STAmount(USD, UINT64_C(9'970'097277662122), -12),
STAmount(EUR, UINT64_C(10'029'99250187452), -11),
ammUSD_EUR.tokens()));
BEAST_EXPECT(expectOffers(
env,
alice,
1,
{{Amounts{
XRPAmount(30'201'749),
STAmount(USD, UINT64_C(29'90272233787818), -14)}}}));

// fixReducedOffersV2 changes the expected results slightly.
Amounts const expectedAmounts =
env.closed()->rules().enabled(fixReducedOffersV2)
? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233787816), -14)}
: Amounts{
XRPAmount(30'201'749),
STAmount(USD, UINT64_C(29'90272233787818), -14)};

BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}}));
}
else
{
BEAST_EXPECT(ammUSD_EUR.expectBalances(
STAmount(USD, UINT64_C(9'970'097277662172), -12),
STAmount(EUR, UINT64_C(10'029'99250187452), -11),
ammUSD_EUR.tokens()));
BEAST_EXPECT(expectOffers(
env,
alice,
1,
{{Amounts{
XRPAmount(30'201'749),
STAmount(USD, UINT64_C(29'9027223378284), -13)}}}));

// fixReducedOffersV2 changes the expected results slightly.
Amounts const expectedAmounts =
env.closed()->rules().enabled(fixReducedOffersV2)
? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233782839), -14)}
: Amounts{
XRPAmount(30'201'749),
STAmount(USD, UINT64_C(29'90272233782840), -14)};

BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}}));
}
// Initial 30,000 + 100
BEAST_EXPECT(expectLine(env, carol, STAmount{USD, 30'100}));
Expand Down Expand Up @@ -6874,6 +6880,8 @@ struct AMM_test : public jtx::AMMTest
testInvalidAMMPayment();
testBasicPaymentEngine(all);
testBasicPaymentEngine(all - fixAMMv1_1);
testBasicPaymentEngine(all - fixReducedOffersV2);
testBasicPaymentEngine(all - fixAMMv1_1 - fixReducedOffersV2);
testAMMTokens();
testAmendment();
testFlags();
Expand Down
Loading
Loading