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
56 changes: 56 additions & 0 deletions src/test/app/AMM_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,62 @@ 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 AMMHelpers.cpp: adjustLPTokens calculations
// The LPTokens need to have 16 significant digits and a fractional part
for (const Number deltaLPTokens :
{Number{UINT64_C(100000'0000000009), -10},
Number{UINT64_C(100000'0000000001), -10}})
{
testAMM([&](AMM& ammAlice, Env& env) {
// initial LPToken balance
IOUAmount const initLPToken = ammAlice.getLPTokensBalance();
const IOUAmount newLPTokens{
deltaLPTokens.mantissa(), deltaLPTokens.exponent()};

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

IOUAmount const finalLPToken = ammAlice.getLPTokensBalance();

// Change in behavior due to rounding down of LPTokens:
// there is a decrease in the observed return of LPTokens --
// Inputs Number{UINT64_C(100000'0000000001), -10} and
// Number{UINT64_C(100000'0000000009), -10} are both rounded
// down to 1e5
BEAST_EXPECT((finalLPToken - initLPToken == IOUAmount{1, 5}));
BEAST_EXPECT(finalLPToken - initLPToken < deltaLPTokens);

// fraction of newLPTokens/(existing LPToken balance). 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));
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 don't understand how lines 1156 and 1177 can be true at the same. I'm guessing it's due to the loss of precision in the last few digits.

newLPTokens == deltaLPTokens (numerically)
initLPTokens + newLPTokens = finalLPTokens, but finalLPToken - initLPToken < deltaLPTokens


// 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