From e734403b05b750557048e2e6f22b0cb3f5222a0d Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Thu, 4 Apr 2024 16:27:07 -0700 Subject: [PATCH 1/8] AMM Unit tests: rounding down of equal asset deposit LPToken calculation include reference from XLS-30d. Validate the expected deposit sums against XLS-30d spec --- src/test/app/AMM_test.cpp | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b6828fab773..e218108d884 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1128,6 +1128,41 @@ struct AMM_test : public jtx::AMMTest expectLedgerEntryRoot(env, carol, XRPAmount{28'999'999'990})); }); + // equal asset deposit: the specified value of LPTokens triggers a + // rounding-down of LPTokens in the adjustLPToken calculations + testAMM([&](AMM& ammAlice, Env& env) { + const IOUAmount newLPTokens{UINT64_C(488088'4817015109), -10}; + ammAlice.deposit(DepositArg{.account = carol, .tokens = + newLPTokens}); + + // fraction of newLPTokens/(existing LPToken balance). Carol is + // seeking additional 488088.4817015109 LPTokens. The existing + // LPToken balance is 1e7 + const Number fr = Number{UINT64_C(488088'4817015109), -10}/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))); + }); + // 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 From 7542e72ae0b27139b084b7c64ac9b6e93b0db0a2 Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Fri, 5 Apr 2024 11:12:59 -0700 Subject: [PATCH 2/8] simplify the numbers used in unit test --- src/test/app/AMM_test.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index e218108d884..62f96c378ff 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1128,17 +1128,24 @@ struct AMM_test : public jtx::AMMTest expectLedgerEntryRoot(env, carol, XRPAmount{28'999'999'990})); }); - // equal asset deposit: the specified value of LPTokens triggers a - // rounding-down of LPTokens in the adjustLPToken calculations + // equal asset deposit: unit test to exercise the rounding-down of + // LPTokens in the adjustLPToken calculations. Any LPTokens with a + // non-zero fractional part will trigger this piece of code + // (AMMHelpers.cpp: adjustLPTokens) testAMM([&](AMM& ammAlice, Env& env) { - const IOUAmount newLPTokens{UINT64_C(488088'4817015109), -10}; + // Approximately 1% of the existing pool + const Number deltaLPTokens{UINT64_C(100000'1), -1}; + 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 488088.4817015109 LPTokens. The existing + // seeking additional 100000.1 LPTokens. The existing // LPToken balance is 1e7 - const Number fr = Number{UINT64_C(488088'4817015109), -10}/1e7; + const Number fr = deltaLPTokens/1e7; // The below equations are based on Equation 1, 2 from XLS-30d // specification, Section: 2.3.1.2 From 2db35001e4a7efc925a9504aa8c25b85fd1ae8ae Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Fri, 5 Apr 2024 11:29:34 -0700 Subject: [PATCH 3/8] fix clang format --- src/test/app/AMM_test.cpp | 46 +++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 62f96c378ff..4c626e3bc3d 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4934,30 +4934,30 @@ struct AMM_test : public jtx::AMMTest void testCore() { - testInvalidInstance(); - testInstanceCreate(); - testInvalidDeposit(); +// testInvalidInstance(); +// testInstanceCreate(); +// testInvalidDeposit(); testDeposit(); - testInvalidWithdraw(); - testWithdraw(); - testInvalidFeeVote(); - testFeeVote(); - testInvalidBid(); - testBid(); - testInvalidAMMPayment(); - testBasicPaymentEngine(); - testAMMTokens(); - testAmendment(); - testFlags(); - testRippling(); - testAMMAndCLOB(); - testTradingFee(); - testAdjustedTokens(); - testAutoDelete(); - testClawback(); - testAMMID(); - testSelection(); - testFixDefaultInnerObj(); +// testInvalidWithdraw(); +// testWithdraw(); +// testInvalidFeeVote(); +// testFeeVote(); +// testInvalidBid(); +// testBid(); +// testInvalidAMMPayment(); +// testBasicPaymentEngine(); +// testAMMTokens(); +// testAmendment(); +// testFlags(); +// testRippling(); +// testAMMAndCLOB(); +// testTradingFee(); +// testAdjustedTokens(); +// testAutoDelete(); +// testClawback(); +// testAMMID(); +// testSelection(); +// testFixDefaultInnerObj(); } void From 16a6515439be78c22ab66889d92e161c974e0629 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Fri, 5 Apr 2024 12:03:32 -0700 Subject: [PATCH 4/8] Enable all unit tests --- src/test/app/AMM_test.cpp | 46 +++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 4c626e3bc3d..62f96c378ff 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4934,30 +4934,30 @@ struct AMM_test : public jtx::AMMTest void testCore() { -// testInvalidInstance(); -// testInstanceCreate(); -// testInvalidDeposit(); + testInvalidInstance(); + testInstanceCreate(); + testInvalidDeposit(); testDeposit(); -// testInvalidWithdraw(); -// testWithdraw(); -// testInvalidFeeVote(); -// testFeeVote(); -// testInvalidBid(); -// testBid(); -// testInvalidAMMPayment(); -// testBasicPaymentEngine(); -// testAMMTokens(); -// testAmendment(); -// testFlags(); -// testRippling(); -// testAMMAndCLOB(); -// testTradingFee(); -// testAdjustedTokens(); -// testAutoDelete(); -// testClawback(); -// testAMMID(); -// testSelection(); -// testFixDefaultInnerObj(); + testInvalidWithdraw(); + testWithdraw(); + testInvalidFeeVote(); + testFeeVote(); + testInvalidBid(); + testBid(); + testInvalidAMMPayment(); + testBasicPaymentEngine(); + testAMMTokens(); + testAmendment(); + testFlags(); + testRippling(); + testAMMAndCLOB(); + testTradingFee(); + testAdjustedTokens(); + testAutoDelete(); + testClawback(); + testAMMID(); + testSelection(); + testFixDefaultInnerObj(); } void From 0d357c9f199364cf3cadfb853d163e4cf7ea8659 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Fri, 5 Apr 2024 15:08:14 -0700 Subject: [PATCH 5/8] Rectify numbers to trigger rounding down --- src/test/app/AMM_test.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 62f96c378ff..5911212c1ea 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1129,12 +1129,11 @@ struct AMM_test : public jtx::AMMTest }); // equal asset deposit: unit test to exercise the rounding-down of - // LPTokens in the adjustLPToken calculations. Any LPTokens with a - // non-zero fractional part will trigger this piece of code + // LPTokens in the adjustLPToken calculations // (AMMHelpers.cpp: adjustLPTokens) testAMM([&](AMM& ammAlice, Env& env) { // Approximately 1% of the existing pool - const Number deltaLPTokens{UINT64_C(100000'1), -1}; + const Number deltaLPTokens{UINT64_C(488088'4817015109), -10}; const IOUAmount newLPTokens{deltaLPTokens.mantissa(), deltaLPTokens.exponent()}; From 31b897190d7691749041485e673254de0fb3ee2f Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Fri, 5 Apr 2024 15:44:26 -0700 Subject: [PATCH 6/8] remove an inaccurate comment --- src/test/app/AMM_test.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 5911212c1ea..2ef7e201e3c 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1132,7 +1132,6 @@ struct AMM_test : public jtx::AMMTest // LPTokens in the adjustLPToken calculations // (AMMHelpers.cpp: adjustLPTokens) testAMM([&](AMM& ammAlice, Env& env) { - // Approximately 1% of the existing pool const Number deltaLPTokens{UINT64_C(488088'4817015109), -10}; const IOUAmount newLPTokens{deltaLPTokens.mantissa(), deltaLPTokens.exponent()}; From 5302b30c644e56884ae78e722d741dcde06d2016 Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Tue, 9 Apr 2024 09:43:35 -0700 Subject: [PATCH 7/8] explanatory comment to indicate the rounding down of LPTokens and amounts --- src/ripple/app/misc/impl/AMMHelpers.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ripple/app/misc/impl/AMMHelpers.cpp b/src/ripple/app/misc/impl/AMMHelpers.cpp index 736743eaaf7..7525a3aa399 100644 --- a/src/ripple/app/misc/impl/AMMHelpers.cpp +++ b/src/ripple/app/misc/impl/AMMHelpers.cpp @@ -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. auto const lpTokensActual = adjustLPTokens(lptAMMBalance, lpTokens, isDeposit); From 13244e8dd8825176e3da0b5338ce8790e1ace7b3 Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Tue, 9 Apr 2024 13:26:49 -0700 Subject: [PATCH 8/8] [FOLD] include an assert check to validate rounding down behavior --- src/ripple/app/misc/impl/AMMHelpers.cpp | 5 -- src/test/app/AMM_test.cpp | 92 +++++++++++++++---------- 2 files changed, 54 insertions(+), 43 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMHelpers.cpp b/src/ripple/app/misc/impl/AMMHelpers.cpp index 7525a3aa399..736743eaaf7 100644 --- a/src/ripple/app/misc/impl/AMMHelpers.cpp +++ b/src/ripple/app/misc/impl/AMMHelpers.cpp @@ -153,11 +153,6 @@ 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. auto const lpTokensActual = adjustLPTokens(lptAMMBalance, lpTokens, isDeposit); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 2ef7e201e3c..3b8146f6657 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1129,44 +1129,60 @@ struct AMM_test : public jtx::AMMTest }); // 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))); - }); + // 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)); + + // 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))); + }); + } // Equal limit deposit: deposit USD100 and XRP proportionally // to the pool composition not to exceed 100XRP. If the amount