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

AMM Unit tests: rounding down of equal asset deposit LPToken calculation #4982

Merged
merged 12 commits into from
Apr 18, 2024
5 changes: 5 additions & 0 deletions src/ripple/app/misc/impl/AMMHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ adjustAmountsByLPTokens(
std::uint16_t tfee,
bool isDeposit)
{
// If lpTokens contains a fractional part and at least 16 significant
// digits, it is possible that lpTokensActual is rounded down. This is to
// ensure that we don't create/burn more LPTokens (depending on
// whether it's AMMDeposit or AMMWithdraw transaction) than what the trader
// asked for.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seelabs This is one of the points in the codebase where LPTokens are rounded down. Check the function above adjustLPTokens.

Consequently, lpTokensActual < lpTokens by a very small margin.

auto const lpTokensActual =
adjustLPTokens(lptAMMBalance, lpTokens, isDeposit);

Expand Down
40 changes: 40 additions & 0 deletions src/test/app/AMM_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,46 @@ struct AMM_test : public jtx::AMMTest
expectLedgerEntryRoot(env, carol, XRPAmount{28'999'999'990}));
});

// equal asset deposit: unit test to exercise the rounding-down of
// LPTokens in the adjustLPToken calculations
// (AMMHelpers.cpp: adjustLPTokens)
testAMM([&](AMM& ammAlice, Env& env) {
const Number deltaLPTokens{UINT64_C(488088'4817015109), -10};
const IOUAmount newLPTokens{deltaLPTokens.mantissa(),
deltaLPTokens.exponent()};

// carol performs a two-asset deposit
ammAlice.deposit(DepositArg{.account = carol, .tokens =
newLPTokens});

// fraction of newLPTokens/(existing LPToken balance). Carol is
// seeking additional 100000.1 LPTokens. The existing
// LPToken balance is 1e7
const Number fr = deltaLPTokens/1e7;

// The below equations are based on Equation 1, 2 from XLS-30d
// specification, Section: 2.3.1.2
const Number deltaXRP = fr * 1e10;
const Number deltaUSD = fr * 1e4;

const STAmount depositUSD = STAmount{USD, deltaUSD.mantissa(),
deltaUSD.exponent()};

const STAmount depositXRP = STAmount{XRP, deltaXRP.mantissa(),
deltaXRP.exponent()};

// initial LPTokens (1e7) + newLPTokens
BEAST_EXPECT(ammAlice.expectBalances(XRP(10'000) + depositXRP,
USD(10'000) + depositUSD,
IOUAmount{1, 7} + newLPTokens));

// 30,000 less deposited depositUSD
BEAST_EXPECT(expectLine(env, carol, USD(30'000) - depositUSD));
// 30,000 less deposited depositXRP and 10 drops tx fee
BEAST_EXPECT(expectLedgerEntryRoot(env, carol, XRP(30'000) -
depositXRP - txfee(env, 1)));
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe my brain isn't working today, but I'm not following how this tests rounding down. Where would the calculation differ if we rounded to nearest, say? Can you add some clarifying comments to help me follow this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The values computed by AMMHelpers.cpp:adjustLPTokens would be different, if we used the nearest rounding mode. I have tagged that line above ^

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not sure how the test is showing this difference. I think what I'm looking for is a comment in the test that explains how the test proves that the rounding is working as we intended. The test may be doing that, but I'm not sure how. I think I need a comment to explain how the test works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw, I checked the test coverage of AMMHelpers.cpp before and after this PR and this PR improves the coverage in this file, specifically in adjustAmountsByLPTokens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks howard.
@seelabs I have added a test case and asserts on line 1155 to show the rounding down behavior

// Equal limit deposit: deposit USD100 and XRP proportionally
// to the pool composition not to exceed 100XRP. If the amount
// exceeds 100XRP then deposit 100XRP and USD proportionally
Expand Down
Loading