From f47a0f58bca5b3267ffc850d6a914e47a4d2632f Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sun, 3 Nov 2024 17:15:18 -0500 Subject: [PATCH] Add fixAMMv1_3 amendment * Add AMM deposit and withdraw rounding to ensure AMM invariant * Add AMM swap/deposit/withdraw/create invariants * Add Offer invariant to verify consumed amounts * Fix Bid validation --- include/xrpl/protocol/Feature.h | 2 +- include/xrpl/protocol/Rules.h | 3 + include/xrpl/protocol/detail/features.macro | 1 + src/libxrpl/protocol/Rules.cpp | 8 + src/test/app/AMMExtended_test.cpp | 14 +- src/test/app/AMM_test.cpp | 1096 +++++++++++++++---- src/test/jtx/AMM.h | 5 +- src/test/jtx/Env.h | 8 + src/test/jtx/impl/AMM.cpp | 17 +- src/xrpld/app/misc/AMMHelpers.h | 151 ++- src/xrpld/app/misc/detail/AMMHelpers.cpp | 206 +++- src/xrpld/app/paths/detail/AMMOffer.cpp | 4 + src/xrpld/app/tx/detail/AMMBid.cpp | 16 +- src/xrpld/app/tx/detail/AMMDeposit.cpp | 156 ++- src/xrpld/app/tx/detail/AMMWithdraw.cpp | 135 ++- src/xrpld/app/tx/detail/AMMWithdraw.h | 2 +- src/xrpld/app/tx/detail/InvariantCheck.cpp | 223 ++++ src/xrpld/app/tx/detail/InvariantCheck.h | 39 +- src/xrpld/app/tx/detail/Offer.h | 19 +- 19 files changed, 1765 insertions(+), 340 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 90a81c55ef4..df071c69864 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -80,7 +80,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 = 83; +static constexpr std::size_t numFeatures = 84; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated diff --git a/include/xrpl/protocol/Rules.h b/include/xrpl/protocol/Rules.h index 6b22d01afe0..e88083b5ac5 100644 --- a/include/xrpl/protocol/Rules.h +++ b/include/xrpl/protocol/Rules.h @@ -127,5 +127,8 @@ class CurrentTransactionRulesGuard std::optional saved_; }; +bool +isFeatureEnabled(uint256 const& feature); + } // namespace ripple #endif diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 24c6e72ae34..f555b97d5d9 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -29,6 +29,7 @@ // If you add an amendment here, then do not forget to increment `numFeatures` // in include/xrpl/protocol/Feature.h. +XRPL_FIX (AMMv1_3, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Credentials, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(AMMClawback, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (AMMv1_2, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index f47e966e138..784dd7c260e 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -148,4 +148,12 @@ Rules::operator!=(Rules const& other) const { return !(*this == other); } + +bool +isFeatureEnabled(uint256 const& feature) +{ + auto const rules = getCurrentTransactionRules(); + return rules && rules->enabled(feature); +} + } // namespace ripple diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 96053b93b44..e8ab7de3192 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -1444,7 +1444,7 @@ struct AMMExtended_test : public jtx::AMMTest using namespace jtx; FeatureBitset const all{supported_amendments()}; testRmFundedOffer(all); - testRmFundedOffer(all - fixAMMv1_1); + testRmFundedOffer(all - fixAMMv1_1 - fixAMMv1_3); testEnforceNoRipple(all); testFillModes(all); testOfferCrossWithXRP(all); @@ -1458,7 +1458,7 @@ struct AMMExtended_test : public jtx::AMMTest testOfferCreateThenCross(all); testSellFlagExceedLimit(all); testGatewayCrossCurrency(all); - testGatewayCrossCurrency(all - fixAMMv1_1); + testGatewayCrossCurrency(all - fixAMMv1_1 - fixAMMv1_3); testBridgedCross(all); testSellWithFillOrKill(all); testTransferRateOffer(all); @@ -1466,7 +1466,7 @@ struct AMMExtended_test : public jtx::AMMTest testBadPathAssert(all); testSellFlagBasic(all); testDirectToDirectPath(all); - testDirectToDirectPath(all - fixAMMv1_1); + testDirectToDirectPath(all - fixAMMv1_1 - fixAMMv1_3); testRequireAuth(all); testMissingAuth(all); } @@ -4090,9 +4090,9 @@ struct AMMExtended_test : public jtx::AMMTest testBookStep(all); testBookStep(all | ownerPaysFee); testTransferRate(all | ownerPaysFee); - testTransferRate((all - fixAMMv1_1) | ownerPaysFee); + testTransferRate((all - fixAMMv1_1 - fixAMMv1_3) | ownerPaysFee); testTransferRateNoOwnerFee(all); - testTransferRateNoOwnerFee(all - fixAMMv1_1); + testTransferRateNoOwnerFee(all - fixAMMv1_1 - fixAMMv1_3); testLimitQuality(); testXRPPathLoop(); } @@ -4103,7 +4103,7 @@ struct AMMExtended_test : public jtx::AMMTest using namespace jtx; FeatureBitset const all{supported_amendments()}; testStepLimit(all); - testStepLimit(all - fixAMMv1_1); + testStepLimit(all - fixAMMv1_1 - fixAMMv1_3); } void @@ -4112,7 +4112,7 @@ struct AMMExtended_test : public jtx::AMMTest using namespace jtx; FeatureBitset const all{supported_amendments()}; test_convert_all_of_an_asset(all); - test_convert_all_of_an_asset(all - fixAMMv1_1); + test_convert_all_of_an_asset(all - fixAMMv1_1 - fixAMMv1_3); } void diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index f1e81132c5e..79539d1c793 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -24,17 +24,13 @@ #include #include #include -#include #include -#include -#include #include #include #include #include #include -#include #include #include @@ -821,21 +817,6 @@ struct AMM_test : public jtx::AMMTest std::nullopt, ter(tecAMM_FAILED)); - // Tiny deposit - ammAlice.deposit( - carol, - IOUAmount{1, -4}, - std::nullopt, - std::nullopt, - ter(temBAD_AMOUNT)); - ammAlice.deposit( - carol, - STAmount{USD, 1, -12}, - std::nullopt, - std::nullopt, - std::nullopt, - ter(tecAMM_INVALID_TOKENS)); - // Deposit non-empty AMM ammAlice.deposit( carol, @@ -846,6 +827,34 @@ struct AMM_test : public jtx::AMMTest ter(tecAMM_NOT_EMPTY)); }); + // Tiny deposit + testAMM( + [&](AMM& ammAlice, Env& env) { + auto const enabledv1_3 = + env.current()->rules().enabled(fixAMMv1_3); + auto const err = + !enabledv1_3 ? ter(temBAD_AMOUNT) : ter(tesSUCCESS); + // Pre-amendment XRP deposit side is rounded to 0 + // and deposit fails. + // Post-amendment XRP deposit side is rounded to 1 + // and deposit succeeds. + ammAlice.deposit( + carol, IOUAmount{1, -4}, std::nullopt, std::nullopt, err); + // Pre/post-amendment LPTokens is rounded to 0 and deposit + // fails with tecAMM_INVALID_TOKENS. + ammAlice.deposit( + carol, + STAmount{USD, 1, -12}, + std::nullopt, + std::nullopt, + std::nullopt, + ter(tecAMM_INVALID_TOKENS)); + }, + std::nullopt, + 0, + std::nullopt, + {features, features - fixAMMv1_3}); + // Invalid AMM testAMM([&](AMM& ammAlice, Env& env) { ammAlice.withdrawAll(alice); @@ -1309,6 +1318,7 @@ struct AMM_test : public jtx::AMMTest testcase("Deposit"); using namespace jtx; + auto const all = supported_amendments(); // Equal deposit: 1000000 tokens, 10% of the current pool testAMM([&](AMM& ammAlice, Env& env) { @@ -1513,8 +1523,9 @@ struct AMM_test : public jtx::AMMTest }); // Issuer create/deposit + for (auto const& feat : {all, all - fixAMMv1_3}) { - Env env(*this); + Env env(*this, feat); env.fund(XRP(30000), gw); AMM ammGw(env, gw, XRP(10'000), USD(10'000)); BEAST_EXPECT( @@ -1608,6 +1619,7 @@ struct AMM_test : public jtx::AMMTest testcase("Invalid Withdraw"); using namespace jtx; + auto const all = supported_amendments(); testAMM( [&](AMM& ammAlice, Env& env) { @@ -1901,16 +1913,6 @@ struct AMM_test : public jtx::AMMTest ammAlice.withdraw( carol, 10'000, std::nullopt, std::nullopt, ter(tecAMM_BALANCE)); - // Withdraw entire one side of the pool. - // Equal withdraw but due to XRP precision limit, - // this results in full withdraw of XRP pool only, - // while leaving a tiny amount in USD pool. - ammAlice.withdraw( - alice, - IOUAmount{9'999'999'9999, -4}, - std::nullopt, - std::nullopt, - ter(tecAMM_BALANCE)); // Withdrawing from one side. // XRP by tokens ammAlice.withdraw( @@ -1942,6 +1944,57 @@ struct AMM_test : public jtx::AMMTest ter(tecAMM_BALANCE)); }); + testAMM( + [&](AMM& ammAlice, Env& env) { + // Withdraw entire one side of the pool. + // Pre-amendment: + // Equal withdraw but due to XRP rounding + // this results in full withdraw of XRP pool only, + // while leaving a tiny amount in USD pool. + // Post-amendment: + // Most of the pool is withdrawn with remaining tiny amounts + auto err = env.enabled(fixAMMv1_3) ? ter(tesSUCCESS) + : ter(tecAMM_BALANCE); + ammAlice.withdraw( + alice, + IOUAmount{9'999'999'9999, -4}, + std::nullopt, + std::nullopt, + err); + if (env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(1), STAmount{USD, 1, -7}, IOUAmount{1, -4})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); + + testAMM( + [&](AMM& ammAlice, Env& env) { + // Similar to above with even smaller remaining amount + // is it ok that the pool is unbalanced? + // Withdraw entire one side of the pool. + // Equal withdraw but due to XRP precision limit, + // this results in full withdraw of XRP pool only, + // while leaving a tiny amount in USD pool. + auto err = env.enabled(fixAMMv1_3) ? ter(tesSUCCESS) + : ter(tecAMM_BALANCE); + ammAlice.withdraw( + alice, + IOUAmount{9'999'999'999999999, -9}, + std::nullopt, + std::nullopt, + err); + if (env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(1), STAmount{USD, 1, -11}, IOUAmount{1, -8})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); + // Invalid AMM testAMM([&](AMM& ammAlice, Env& env) { ammAlice.withdrawAll(alice); @@ -2005,15 +2058,19 @@ struct AMM_test : public jtx::AMMTest // Withdraw with EPrice limit. Fails to withdraw, calculated tokens // to withdraw are 0. - testAMM([&](AMM& ammAlice, Env&) { - ammAlice.deposit(carol, 1'000'000); - ammAlice.withdraw( - carol, - USD(100), - std::nullopt, - IOUAmount{500, 0}, - ter(tecAMM_FAILED)); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.deposit(carol, 1'000'000); + auto const err = env.enabled(fixAMMv1_3) + ? ter(tecAMM_INVALID_TOKENS) + : ter(tecAMM_FAILED); + ammAlice.withdraw( + carol, USD(100), std::nullopt, IOUAmount{500, 0}, err); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); // Withdraw with EPrice limit. Fails to withdraw, calculated tokens // to withdraw are greater than the LP shares. @@ -2078,14 +2135,22 @@ struct AMM_test : public jtx::AMMTest // Withdraw close to one side of the pool. Account's LP tokens // are rounded to all LP tokens. - testAMM([&](AMM& ammAlice, Env&) { - ammAlice.withdraw( - alice, - STAmount{USD, UINT64_C(9'999'999999999999), -12}, - std::nullopt, - std::nullopt, - ter(tecAMM_BALANCE)); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + auto const err = env.enabled(fixAMMv1_3) + ? ter(tecINVARIANT_FAILED) + : ter(tecAMM_BALANCE); + ammAlice.withdraw( + alice, + STAmount{USD, UINT64_C(9'999'999999999999), -12}, + std::nullopt, + std::nullopt, + err); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); // Tiny withdraw testAMM([&](AMM& ammAlice, Env&) { @@ -2125,6 +2190,7 @@ struct AMM_test : public jtx::AMMTest testcase("Withdraw"); using namespace jtx; + auto const all = supported_amendments(); // Equal withdrawal by Carol: 1000000 of tokens, 10% of the current // pool @@ -2178,11 +2244,24 @@ struct AMM_test : public jtx::AMMTest }); // Single withdrawal by amount XRP1000 - testAMM([&](AMM& ammAlice, Env&) { - ammAlice.withdraw(alice, XRP(1'000)); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(9'000), USD(10'000), IOUAmount{9'486'832'98050514, -8})); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw(alice, XRP(1'000)); + if (!env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(9'000), + USD(10'000), + IOUAmount{9'486'832'98050514, -8})); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{9'000'000'001}, + USD(10'000), + IOUAmount{9'486'832'98050514, -8})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); // Single withdrawal by tokens 10000. testAMM([&](AMM& ammAlice, Env&) { @@ -2233,20 +2312,31 @@ struct AMM_test : public jtx::AMMTest }); // Single deposit/withdraw by the same account - testAMM([&](AMM& ammAlice, Env&) { - // Since a smaller amount might be deposited due to - // the lp tokens adjustment, withdrawing by tokens - // is generally preferred to withdrawing by amount. - auto lpTokens = ammAlice.deposit(carol, USD(1'000)); - ammAlice.withdraw(carol, lpTokens, USD(0)); - lpTokens = ammAlice.deposit(carol, STAmount(USD, 1, -6)); - ammAlice.withdraw(carol, lpTokens, USD(0)); - lpTokens = ammAlice.deposit(carol, XRPAmount(1)); - ammAlice.withdraw(carol, lpTokens, XRPAmount(0)); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'000), USD(10'000), ammAlice.tokens())); - BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{0})); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + // Since a smaller amount might be deposited due to + // the lp tokens adjustment, withdrawing by tokens + // is generally preferred to withdrawing by amount. + auto lpTokens = ammAlice.deposit(carol, USD(1'000)); + ammAlice.withdraw(carol, lpTokens, USD(0)); + lpTokens = ammAlice.deposit(carol, STAmount(USD, 1, -6)); + ammAlice.withdraw(carol, lpTokens, USD(0)); + lpTokens = ammAlice.deposit(carol, XRPAmount(1)); + ammAlice.withdraw(carol, lpTokens, XRPAmount(0)); + if (!env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), USD(10'000), ammAlice.tokens())); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(10'000'000'001), + USD(10'000), + ammAlice.tokens())); + BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{0})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); // Single deposit by different accounts and then withdraw // in reverse. @@ -2289,36 +2379,36 @@ struct AMM_test : public jtx::AMMTest IOUAmount{10'000'000, 0})); }); - auto const all = supported_amendments(); // Withdraw with EPrice limit. testAMM( [&](AMM& ammAlice, Env& env) { ammAlice.deposit(carol, 1'000'000); ammAlice.withdraw( carol, USD(100), std::nullopt, IOUAmount{520, 0}); - if (!env.current()->rules().enabled(fixAMMv1_1)) - BEAST_EXPECT( - ammAlice.expectBalances( - XRPAmount(11'000'000'000), - STAmount{USD, UINT64_C(9'372'781065088757), -12}, - IOUAmount{10'153'846'15384616, -8}) && - ammAlice.expectLPTokens( - carol, IOUAmount{153'846'15384616, -8})); - else - BEAST_EXPECT( - ammAlice.expectBalances( - XRPAmount(11'000'000'000), - STAmount{USD, UINT64_C(9'372'781065088769), -12}, - IOUAmount{10'153'846'15384616, -8}) && - ammAlice.expectLPTokens( - carol, IOUAmount{153'846'15384616, -8})); + BEAST_EXPECT(ammAlice.expectLPTokens( + carol, IOUAmount{153'846'15384616, -8})); + if (!env.enabled(fixAMMv1_1) && !env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'781065088757), -12}, + IOUAmount{10'153'846'15384616, -8})); + else if (env.enabled(fixAMMv1_1) && !env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'781065088769), -12}, + IOUAmount{10'153'846'15384616, -8})); + else if (env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(11'000'000'000), + STAmount{USD, UINT64_C(9'372'78106508877), -11}, + IOUAmount{10'153'846'15384616, -8})); ammAlice.withdrawAll(carol); BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{0})); }, std::nullopt, 0, std::nullopt, - {all, all - fixAMMv1_1}); + {all, all - fixAMMv1_3, all - fixAMMv1_1 - fixAMMv1_3}); // Withdraw with EPrice limit. AssetOut is 0. testAMM( @@ -2326,27 +2416,28 @@ struct AMM_test : public jtx::AMMTest ammAlice.deposit(carol, 1'000'000); ammAlice.withdraw( carol, USD(0), std::nullopt, IOUAmount{520, 0}); - if (!env.current()->rules().enabled(fixAMMv1_1)) - BEAST_EXPECT( - ammAlice.expectBalances( - XRPAmount(11'000'000'000), - STAmount{USD, UINT64_C(9'372'781065088757), -12}, - IOUAmount{10'153'846'15384616, -8}) && - ammAlice.expectLPTokens( - carol, IOUAmount{153'846'15384616, -8})); - else - BEAST_EXPECT( - ammAlice.expectBalances( - XRPAmount(11'000'000'000), - STAmount{USD, UINT64_C(9'372'781065088769), -12}, - IOUAmount{10'153'846'15384616, -8}) && - ammAlice.expectLPTokens( - carol, IOUAmount{153'846'15384616, -8})); + BEAST_EXPECT(ammAlice.expectLPTokens( + carol, IOUAmount{153'846'15384616, -8})); + if (!env.enabled(fixAMMv1_1) && !env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(11'000), + STAmount{USD, UINT64_C(9'372'781065088757), -12}, + IOUAmount{10'153'846'15384616, -8})); + else if (env.enabled(fixAMMv1_1) && !env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(11'000), + STAmount{USD, UINT64_C(9'372'781065088769), -12}, + IOUAmount{10'153'846'15384616, -8})); + else if (env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(11'000), + STAmount{USD, UINT64_C(9'372'78106508877), -11}, + IOUAmount{10'153'846'15384616, -8})); }, std::nullopt, 0, std::nullopt, - {all, all - fixAMMv1_1}); + {all, all - fixAMMv1_3, all - fixAMMv1_1 - fixAMMv1_3}); // IOU to IOU + transfer fee { @@ -2385,14 +2476,25 @@ struct AMM_test : public jtx::AMMTest STAmount{USD, UINT64_C(9'999'999999), -6}, IOUAmount{9'999'999'999, -3})); }); - testAMM([&](AMM& ammAlice, Env&) { - // Single XRP pool - ammAlice.withdraw(alice, std::nullopt, XRPAmount{1}); - BEAST_EXPECT(ammAlice.expectBalances( - XRPAmount{9'999'999'999}, - USD(10'000), - IOUAmount{9'999'999'9995, -4})); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + // Single XRP pool + ammAlice.withdraw(alice, std::nullopt, XRPAmount{1}); + if (!env.enabled(fixAMMv1_3)) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{9'999'999'999}, + USD(10'000), + IOUAmount{9'999'999'9995, -4})); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), + USD(10'000), + IOUAmount{9'999'999'9995, -4})); + }, + std::nullopt, + 0, + std::nullopt, + {all, all - fixAMMv1_3}); testAMM([&](AMM& ammAlice, Env&) { // Single USD pool ammAlice.withdraw(alice, std::nullopt, STAmount{USD, 1, -10}); @@ -2510,6 +2612,7 @@ struct AMM_test : public jtx::AMMTest { testcase("Fee Vote"); using namespace jtx; + auto const all = supported_amendments(); // One vote sets fee to 1%. testAMM([&](AMM& ammAlice, Env& env) { @@ -2527,6 +2630,12 @@ struct AMM_test : public jtx::AMMTest std::uint32_t tokens = 10'000'000, std::vector* accounts = nullptr) { Account a(std::to_string(i)); + // post-amendment the amount to deposit is slightly higher + // in order to ensure AMM invariant sqrt(asset1 * asset2) >= tokens + // fund just one USD higher in this case, which is enough for + // deposit to succeed + if (env.enabled(fixAMMv1_3)) + ++fundUSD; fund(env, gw, {a}, {USD(fundUSD)}, Fund::Acct); ammAlice.deposit(a, tokens); ammAlice.vote(a, 50 * (i + 1)); @@ -2535,11 +2644,16 @@ struct AMM_test : public jtx::AMMTest }; // Eight votes fill all voting slots, set fee 0.175%. - testAMM([&](AMM& ammAlice, Env& env) { - for (int i = 0; i < 7; ++i) - vote(ammAlice, env, i, 10'000); - BEAST_EXPECT(ammAlice.expectTradingFee(175)); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + for (int i = 0; i < 7; ++i) + vote(ammAlice, env, i, 10'000); + BEAST_EXPECT(ammAlice.expectTradingFee(175)); + }, + std::nullopt, + 0, + std::nullopt, + {all}); // Eight votes fill all voting slots, set fee 0.175%. // New vote, same account, sets fee 0.225% @@ -2933,8 +3047,14 @@ struct AMM_test : public jtx::AMMTest fund(env, gw, {bob}, {USD(10'000)}, Fund::Acct); ammAlice.deposit(bob, 1'000'000); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(12'000), USD(12'000), IOUAmount{12'000'000, 0})); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(12'000), USD(12'000), IOUAmount{12'000'000, 0})); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{12'000'000'001}, + USD(12'000), + IOUAmount{12'000'000, 0})); // Initial state. Pay bidMin. env(ammAlice.bid({.account = carol, .bidMin = 110})).close(); @@ -2966,8 +3086,16 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(ammAlice.expectAuctionSlot( 0, std::nullopt, IOUAmount{110})); // ~321.09 tokens burnt on bidding fees. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(12'000), USD(12'000), IOUAmount{11'999'678'91, -2})); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(12'000), + USD(12'000), + IOUAmount{11'999'678'91, -2})); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{12'000'000'001}, + USD(12'000), + IOUAmount{11'999'678'91, -2})); }, std::nullopt, 0, @@ -2996,8 +3124,12 @@ struct AMM_test : public jtx::AMMTest auto const slotPrice = IOUAmount{5'200}; ammTokens -= slotPrice; BEAST_EXPECT(ammAlice.expectAuctionSlot(100, 0, slotPrice)); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), USD(13'000), ammTokens)); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), USD(13'000), ammTokens)); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'000'000'003}, USD(13'000), ammTokens)); // Discounted trade for (int i = 0; i < 10; ++i) { @@ -3038,10 +3170,16 @@ struct AMM_test : public jtx::AMMTest env.balance(ed, USD) == STAmount(USD, UINT64_C(18'999'0057261184), -10)); // USD pool is slightly higher because of the fees. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), - STAmount(USD, UINT64_C(13'002'98282151422), -11), - ammTokens)); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount(USD, UINT64_C(13'002'98282151422), -11), + ammTokens)); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'000'000'003}, + STAmount(USD, UINT64_C(13'002'98282151422), -11), + ammTokens)); } ammTokens = ammAlice.getLPTokensBalance(); // Trade with the fee @@ -3083,31 +3221,54 @@ struct AMM_test : public jtx::AMMTest } else { - BEAST_EXPECT( - env.balance(dan, USD) == - STAmount(USD, UINT64_C(19'490'05672274399), -11)); + if (!features[fixAMMv1_3]) + BEAST_EXPECT( + env.balance(dan, USD) == + STAmount(USD, UINT64_C(19'490'05672274399), -11)); + else + BEAST_EXPECT( + env.balance(dan, USD) == + STAmount(USD, UINT64_C(19'490'05672274398), -11)); // USD pool gains more in dan's fees. - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), - STAmount{USD, UINT64_C(13'012'92609877023), -11}, - ammTokens)); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount{USD, UINT64_C(13'012'92609877023), -11}, + ammTokens)); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'000'000'003}, + STAmount{USD, UINT64_C(13'012'92609877024), -11}, + ammTokens)); // Discounted fee payment ammAlice.deposit(carol, USD(100)); ammTokens = ammAlice.getLPTokensBalance(); - BEAST_EXPECT(ammAlice.expectBalances( - XRP(13'000), - STAmount{USD, UINT64_C(13'112'92609877023), -11}, - ammTokens)); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(13'000), + STAmount{USD, UINT64_C(13'112'92609877023), -11}, + ammTokens)); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'000'000'003}, + STAmount{USD, UINT64_C(13'112'92609877024), -11}, + ammTokens)); env(pay(carol, bob, USD(100)), path(~USD), sendmax(XRP(110))); env.close(); // carol pays 100000 drops in fees // 99900668XRP swapped in for 100USD - BEAST_EXPECT(ammAlice.expectBalances( - XRPAmount{13'100'000'668}, - STAmount{USD, UINT64_C(13'012'92609877023), -11}, - ammTokens)); + if (!features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'100'000'668}, + STAmount{USD, UINT64_C(13'012'92609877023), -11}, + ammTokens)); + else + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'100'000'671}, + STAmount{USD, UINT64_C(13'012'92609877024), -11}, + ammTokens)); } // Payment with the trading fee env(pay(alice, carol, XRP(100)), path(~XRP), sendmax(USD(110))); @@ -3115,20 +3276,27 @@ struct AMM_test : public jtx::AMMTest // alice pays ~1.011USD in fees, which is ~10 times more // than carol's fee // 100.099431529USD swapped in for 100XRP - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'668}, STAmount{USD, UINT64_C(13'114'03663047264), -11}, ammTokens)); } - else + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'668}, STAmount{USD, UINT64_C(13'114'03663047269), -11}, ammTokens)); } + else + { + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'000'000'671}, + STAmount{USD, UINT64_C(13'114'03663044937), -11}, + ammTokens)); + } // Auction slot expired, no discounted fee env.close(seconds(TOTAL_TIME_SLOT_SECS + 1)); // clock is parent's based @@ -3137,7 +3305,7 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT( env.balance(carol, USD) == STAmount(USD, UINT64_C(29'399'00572620545), -11)); - else + else if (!features[fixAMMv1_3]) BEAST_EXPECT( env.balance(carol, USD) == STAmount(USD, UINT64_C(29'399'00572620544), -11)); @@ -3149,7 +3317,7 @@ struct AMM_test : public jtx::AMMTest } // carol pays ~9.94USD in fees, which is ~10 times more in // trading fees vs discounted fee. - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) { BEAST_EXPECT( env.balance(carol, USD) == @@ -3159,7 +3327,7 @@ struct AMM_test : public jtx::AMMTest STAmount{USD, UINT64_C(13'123'98038490681), -11}, ammTokens)); } - else + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) { BEAST_EXPECT( env.balance(carol, USD) == @@ -3169,25 +3337,42 @@ struct AMM_test : public jtx::AMMTest STAmount{USD, UINT64_C(13'123'98038490689), -11}, ammTokens)); } + else + { + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(29'389'06197177129), -11)); + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount{13'000'000'671}, + STAmount{USD, UINT64_C(13'123'98038488352), -11}, + ammTokens)); + } env(pay(carol, bob, USD(100)), path(~USD), sendmax(XRP(110))); env.close(); // carol pays ~1.008XRP in trading fee, which is // ~10 times more than the discounted fee. // 99.815876XRP is swapped in for 100USD - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount(13'100'824'790), STAmount{USD, UINT64_C(13'023'98038490681), -11}, ammTokens)); } - else + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount(13'100'824'790), STAmount{USD, UINT64_C(13'023'98038490689), -11}, ammTokens)); } + else + { + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(13'100'824'793), + STAmount{USD, UINT64_C(13'023'98038488352), -11}, + ammTokens)); + } }, std::nullopt, 1'000, @@ -4481,7 +4666,7 @@ struct AMM_test : public jtx::AMMTest // Offer crossing with AMM LPTokens and XRP. testAMM([&](AMM& ammAlice, Env& env) { auto const token1 = ammAlice.lptIssue(); - auto priceXRP = withdrawByTokens( + auto priceXRP = ammAssetOut( STAmount{XRPAmount{10'000'000'000}}, STAmount{token1, 10'000'000}, STAmount{token1, 5'000'000}, @@ -4506,7 +4691,7 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(ammAlice.expectLPTokens(carol, IOUAmount{4'999'900})); BEAST_EXPECT(ammAlice.expectAuctionSlot(0, 0, IOUAmount{100})); BEAST_EXPECT(accountBalance(env, carol) == "22499999960"); - priceXRP = withdrawByTokens( + priceXRP = ammAssetOut( STAmount{XRPAmount{10'000'000'000}}, STAmount{token1, 9'999'900}, STAmount{token1, 4'999'900}, @@ -4861,9 +5046,12 @@ struct AMM_test : public jtx::AMMTest carol, USD(100), std::nullopt, IOUAmount{520, 0}); // carol withdraws ~1,443.44USD auto const balanceAfterWithdraw = [&]() { - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) return STAmount(USD, UINT64_C(30'443'43891402715), -11); - return STAmount(USD, UINT64_C(30'443'43891402714), -11); + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) + return STAmount(USD, UINT64_C(30'443'43891402714), -11); + else + return STAmount(USD, UINT64_C(30'443'43891402713), -11); }(); BEAST_EXPECT(env.balance(carol, USD) == balanceAfterWithdraw); // Set to original pool size @@ -4873,22 +5061,29 @@ struct AMM_test : public jtx::AMMTest ammAlice.vote(alice, 0); BEAST_EXPECT(ammAlice.expectTradingFee(0)); auto const tokensNoFee = ammAlice.withdraw(carol, deposit); - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT( env.balance(carol, USD) == STAmount(USD, UINT64_C(30'443'43891402717), -11)); - else + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT( env.balance(carol, USD) == STAmount(USD, UINT64_C(30'443'43891402716), -11)); + else + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(30'443'43891402713), -11)); // carol pays ~4008 LPTokens in fees or ~0.5% of the no-fee // LPTokens - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT( tokensNoFee == IOUAmount(746'579'80779913, -8)); - else + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT( tokensNoFee == IOUAmount(746'579'80779912, -8)); + else + BEAST_EXPECT( + tokensNoFee == IOUAmount(746'579'80779911, -8)); BEAST_EXPECT(tokensFee == IOUAmount(750'588'23529411, -8)); }, std::nullopt, @@ -5185,11 +5380,16 @@ struct AMM_test : public jtx::AMMTest // Due to round off some accounts have a tiny gain, while // other have a tiny loss. The last account to withdraw // gets everything in the pool. - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT(ammAlice.expectBalances( XRP(10'000), STAmount{USD, UINT64_C(10'000'0000000013), -10}, IOUAmount{10'000'000})); + else if (features[fixAMMv1_3]) + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), + STAmount{USD, UINT64_C(10'000'0000000003), -10}, + IOUAmount{10'000'000})); else BEAST_EXPECT(ammAlice.expectBalances( XRP(10'000), USD(10'000), IOUAmount{10'000'000})); @@ -5197,25 +5397,29 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(expectLine(env, simon, USD(1'500'000))); BEAST_EXPECT(expectLine(env, chris, USD(1'500'000))); BEAST_EXPECT(expectLine(env, dan, USD(1'500'000))); - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT(expectLine( env, carol, STAmount{USD, UINT64_C(30'000'00000000001), -11})); + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) + BEAST_EXPECT(expectLine(env, carol, USD(30'000))); else BEAST_EXPECT(expectLine(env, carol, USD(30'000))); BEAST_EXPECT(expectLine(env, ed, USD(1'500'000))); BEAST_EXPECT(expectLine(env, paul, USD(1'500'000))); - if (!features[fixAMMv1_1]) + if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT(expectLine( env, nataly, STAmount{USD, UINT64_C(1'500'000'000000002), -9})); - else + else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT(expectLine( env, nataly, STAmount{USD, UINT64_C(1'500'000'000000005), -9})); + else + BEAST_EXPECT(expectLine(env, nataly, USD(1'500'000))); ammAlice.withdrawAll(alice); BEAST_EXPECT(!ammAlice.ammExists()); if (!features[fixAMMv1_1]) @@ -5223,6 +5427,11 @@ struct AMM_test : public jtx::AMMTest env, alice, STAmount{USD, UINT64_C(30'000'0000000013), -10})); + else if (features[fixAMMv1_3]) + BEAST_EXPECT(expectLine( + env, + alice, + STAmount{USD, UINT64_C(30'000'0000000003), -10})); else BEAST_EXPECT(expectLine(env, alice, USD(30'000))); // alice XRP balance is 30,000initial - 50 ammcreate fee - @@ -5235,62 +5444,92 @@ struct AMM_test : public jtx::AMMTest {features}); // Same as above but deposit/withdraw in XRP - testAMM([&](AMM& ammAlice, Env& env) { - Account const bob("bob"); - Account const ed("ed"); - Account const paul("paul"); - Account const dan("dan"); - Account const chris("chris"); - Account const simon("simon"); - Account const ben("ben"); - Account const nataly("nataly"); - fund( - env, - gw, - {bob, ed, paul, dan, chris, simon, ben, nataly}, - XRP(2'000'000), - {}, - Fund::Acct); - for (int i = 0; i < 10; ++i) - { - ammAlice.deposit(ben, XRPAmount{1}); - ammAlice.withdrawAll(ben, XRP(0)); - ammAlice.deposit(simon, XRPAmount(1'000)); - ammAlice.withdrawAll(simon, XRP(0)); - ammAlice.deposit(chris, XRP(1)); - ammAlice.withdrawAll(chris, XRP(0)); - ammAlice.deposit(dan, XRP(10)); - ammAlice.withdrawAll(dan, XRP(0)); - ammAlice.deposit(bob, XRP(100)); - ammAlice.withdrawAll(bob, XRP(0)); - ammAlice.deposit(carol, XRP(1'000)); - ammAlice.withdrawAll(carol, XRP(0)); - ammAlice.deposit(ed, XRP(10'000)); - ammAlice.withdrawAll(ed, XRP(0)); - ammAlice.deposit(paul, XRP(100'000)); - ammAlice.withdrawAll(paul, XRP(0)); - ammAlice.deposit(nataly, XRP(1'000'000)); - ammAlice.withdrawAll(nataly, XRP(0)); - } - // No round off with XRP in this test - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'000), USD(10'000), IOUAmount{10'000'000})); - ammAlice.withdrawAll(alice); - BEAST_EXPECT(!ammAlice.ammExists()); - // 20,000 initial - (deposit+withdraw) * 10 - auto const xrpBalance = (XRP(2'000'000) - txfee(env, 20)).getText(); - BEAST_EXPECT(accountBalance(env, ben) == xrpBalance); - BEAST_EXPECT(accountBalance(env, simon) == xrpBalance); - BEAST_EXPECT(accountBalance(env, chris) == xrpBalance); - BEAST_EXPECT(accountBalance(env, dan) == xrpBalance); - // 30,000 initial - (deposit+withdraw) * 10 - BEAST_EXPECT(accountBalance(env, carol) == "29999999800"); - BEAST_EXPECT(accountBalance(env, ed) == xrpBalance); - BEAST_EXPECT(accountBalance(env, paul) == xrpBalance); - BEAST_EXPECT(accountBalance(env, nataly) == xrpBalance); - // 30,000 initial - 50 ammcreate fee - 10drops withdraw fee - BEAST_EXPECT(accountBalance(env, alice) == "29949999990"); - }); + testAMM( + [&](AMM& ammAlice, Env& env) { + Account const bob("bob"); + Account const ed("ed"); + Account const paul("paul"); + Account const dan("dan"); + Account const chris("chris"); + Account const simon("simon"); + Account const ben("ben"); + Account const nataly("nataly"); + fund( + env, + gw, + {bob, ed, paul, dan, chris, simon, ben, nataly}, + XRP(2'000'000), + {}, + Fund::Acct); + for (int i = 0; i < 10; ++i) + { + ammAlice.deposit(ben, XRPAmount{1}); + ammAlice.withdrawAll(ben, XRP(0)); + ammAlice.deposit(simon, XRPAmount(1'000)); + ammAlice.withdrawAll(simon, XRP(0)); + ammAlice.deposit(chris, XRP(1)); + ammAlice.withdrawAll(chris, XRP(0)); + ammAlice.deposit(dan, XRP(10)); + ammAlice.withdrawAll(dan, XRP(0)); + ammAlice.deposit(bob, XRP(100)); + ammAlice.withdrawAll(bob, XRP(0)); + ammAlice.deposit(carol, XRP(1'000)); + ammAlice.withdrawAll(carol, XRP(0)); + ammAlice.deposit(ed, XRP(10'000)); + ammAlice.withdrawAll(ed, XRP(0)); + ammAlice.deposit(paul, XRP(100'000)); + ammAlice.withdrawAll(paul, XRP(0)); + ammAlice.deposit(nataly, XRP(1'000'000)); + ammAlice.withdrawAll(nataly, XRP(0)); + } + if (!features[fixAMMv1_3]) + { + // No round off with XRP in this test + BEAST_EXPECT(ammAlice.expectBalances( + XRP(10'000), USD(10'000), IOUAmount{10'000'000})); + ammAlice.withdrawAll(alice); + BEAST_EXPECT(!ammAlice.ammExists()); + // 20,000 initial - (deposit+withdraw) * 10 + auto const xrpBalance = + (XRP(2'000'000) - txfee(env, 20)).getText(); + BEAST_EXPECT(accountBalance(env, ben) == xrpBalance); + BEAST_EXPECT(accountBalance(env, simon) == xrpBalance); + BEAST_EXPECT(accountBalance(env, chris) == xrpBalance); + BEAST_EXPECT(accountBalance(env, dan) == xrpBalance); + // 30,000 initial - (deposit+withdraw) * 10 + BEAST_EXPECT(accountBalance(env, carol) == "29999999800"); + BEAST_EXPECT(accountBalance(env, ed) == xrpBalance); + BEAST_EXPECT(accountBalance(env, paul) == xrpBalance); + BEAST_EXPECT(accountBalance(env, nataly) == xrpBalance); + // 30,000 initial - 50 ammcreate fee - 10drops withdraw fee + BEAST_EXPECT(accountBalance(env, alice) == "29949999990"); + } + else + { + // post-amendment the rounding takes place to ensure + // AMM invariant + BEAST_EXPECT(ammAlice.expectBalances( + XRPAmount(10'000'000'080), + USD(10'000), + IOUAmount{10'000'000})); + ammAlice.withdrawAll(alice); + BEAST_EXPECT(!ammAlice.ammExists()); + BEAST_EXPECT(accountBalance(env, ben) == "1999999999790"); + BEAST_EXPECT(accountBalance(env, simon) == "1999999999790"); + BEAST_EXPECT(accountBalance(env, chris) == "1999999999790"); + BEAST_EXPECT(accountBalance(env, dan) == "1999999999790"); + BEAST_EXPECT(accountBalance(env, carol) == "29999999790"); + BEAST_EXPECT(accountBalance(env, ed) == "1999999999792"); + BEAST_EXPECT(accountBalance(env, paul) == "1999999999793"); + BEAST_EXPECT( + accountBalance(env, nataly) == "1999999999795"); + BEAST_EXPECT(accountBalance(env, alice) == "29950000070"); + } + }, + std::nullopt, + 0, + std::nullopt, + {features}); } void @@ -6330,11 +6569,11 @@ struct AMM_test : public jtx::AMMTest } void - testFixOverflowOffer(FeatureBitset features) + testFixOverflowOffer(FeatureBitset featuresInitial) { using namespace jtx; using namespace std::chrono; - FeatureBitset const all{features}; + FeatureBitset const all{featuresInitial}; Account const gatehub{"gatehub"}; Account const bitstamp{"bitstamp"}; @@ -6359,6 +6598,7 @@ struct AMM_test : public jtx::AMMTest STAmount const goodUsdBIT; STAmount const goodUsdBITr; IOUAmount const lpTokenBalance; + std::optional const lpTokenBalanceAlt; double const offer1BtcGH = 0.1; double const offer2BtcGH = 0.1; double const offer2UsdGH = 1; @@ -6384,6 +6624,7 @@ struct AMM_test : public jtx::AMMTest .goodUsdBIT{usdBIT, uint64_t(8'464739069120721), -15}, // .goodUsdBITr{usdBIT, uint64_t(8'464739069098152), -15}, // .lpTokenBalance = {28'61817604250837, -14}, // + .lpTokenBalanceAlt = IOUAmount{28'61817604250836, -14}, // .offer1BtcGH = 0.1, // .offer2BtcGH = 0.1, // .offer2UsdGH = 1, // @@ -6562,7 +6803,7 @@ struct AMM_test : public jtx::AMMTest { testcase(input.testCase); for (auto const& features : - {all - fixAMMOverflowOffer, all | fixAMMOverflowOffer}) + {all - fixAMMOverflowOffer - fixAMMv1_1 - fixAMMv1_3, all}) { Env env(*this, features); @@ -6616,15 +6857,19 @@ struct AMM_test : public jtx::AMMTest features[fixAMMv1_1] ? input.goodUsdGHr : input.goodUsdGH; auto const goodUsdBIT = features[fixAMMv1_1] ? input.goodUsdBITr : input.goodUsdBIT; + auto const lpTokenBalance = + env.enabled(fixAMMv1_3) && input.lpTokenBalanceAlt + ? *input.lpTokenBalanceAlt + : input.lpTokenBalance; if (!features[fixAMMOverflowOffer]) { BEAST_EXPECT(amm.expectBalances( - failUsdGH, failUsdBIT, input.lpTokenBalance)); + failUsdGH, failUsdBIT, lpTokenBalance)); } else { BEAST_EXPECT(amm.expectBalances( - goodUsdGH, goodUsdBIT, input.lpTokenBalance)); + goodUsdGH, goodUsdBIT, lpTokenBalance)); // Invariant: LPToken balance must not change in a // payment or a swap transaction @@ -6820,6 +7065,7 @@ struct AMM_test : public jtx::AMMTest void testLPTokenBalance(FeatureBitset features) { + testcase("LPToken Balance"); using namespace jtx; // Last Liquidity Provider is the issuer of one token @@ -6835,7 +7081,9 @@ struct AMM_test : public jtx::AMMTest amm.deposit(alice, IOUAmount{1'876123487565916, -15}); amm.deposit(carol, IOUAmount{1'000'000}); amm.withdrawAll(alice); + BEAST_EXPECT(amm.expectLPTokens(alice, IOUAmount{0})); amm.withdrawAll(carol); + BEAST_EXPECT(amm.expectLPTokens(carol, IOUAmount{0})); auto const lpToken = getAccountLines( env, gw, amm.lptIssue())[jss::lines][0u][jss::balance]; auto const lpTokenBalance = @@ -7113,6 +7361,378 @@ struct AMM_test : public jtx::AMMTest }); } + void + testDepositAndWithdrawRounding(FeatureBitset features) + { + testcase("Deposit and Withdraw Rounding V2"); + using namespace jtx; + + auto const XPM = gw["XPM"]; + STAmount xrpBalance{XRPAmount(692'614'492'126)}; + STAmount xpmBalance{XPM, UINT64_C(18'610'359'80246901), -8}; + STAmount amount{XPM, UINT64_C(6'566'496939465400), -12}; + std::uint16_t tfee = 941; + + auto test = [&](auto&& cb, std::uint16_t tfee_) { + Env env(*this, features); + env.fund(XRP(1'000'000), gw); + env.fund(XRP(1'000), alice); + env(trust(alice, XPM(7'000))); + env(pay(gw, alice, amount)); + + AMM amm(env, gw, xrpBalance, xpmBalance, CreateArg{.tfee = tfee_}); + // AMM LPToken balance required to replicate single deposit failure + STAmount lptAMMBalance{ + amm.lptIssue(), UINT64_C(3'234'987'266'485968), -6}; + auto const burn = + IOUAmount{amm.getLPTokensBalance() - lptAMMBalance}; + // burn tokens to get to the required AMM state + env(amm.bid(BidArg{.account = gw, .bidMin = burn, .bidMax = burn})); + cb(amm, env); + }; + test( + [&](AMM& amm, Env& env) { + auto const err = env.enabled(fixAMMv1_3) ? ter(tesSUCCESS) + : ter(tecUNFUNDED_AMM); + amm.deposit(DepositArg{ + .account = alice, .asset1In = amount, .err = err}); + }, + tfee); + test( + [&](AMM& amm, Env& env) { + auto const [amount, amount2, lptAMM] = amm.balances(XRP, XPM); + auto const withdraw = STAmount{XPM, 1, -5}; + amm.withdraw(WithdrawArg{.asset1Out = STAmount{XPM, 1, -5}}); + auto const [amount_, amount2_, lptAMM_] = + amm.balances(XRP, XPM); + if (!env.enabled(fixAMMv1_3)) + BEAST_EXPECT((amount2 - amount2_) > withdraw); + else + BEAST_EXPECT((amount2 - amount2_) <= withdraw); + }, + 0); + } + + void + invariant( + jtx::AMM& amm, + jtx::Env& env, + std::string const& msg, + bool shouldFail) + { + auto const [amount, amount2, lptBalance] = amm.balances(GBP, EUR); + auto const res = [&] { + NumberRoundModeGuard g( + env.enabled(fixAMMv1_3) ? Number::upward : Number::getround()); + return root2(amount * amount2); + }(); + if (shouldFail) + BEAST_EXPECT(res < lptBalance); + else + BEAST_EXPECT(res >= lptBalance); + } + + void + testDepositRounding(FeatureBitset all) + { + testcase("Deposit Rounding"); + using namespace jtx; + + // Single asset deposit + for (auto const& deposit : + {STAmount(EUR, 1, 1), + STAmount(EUR, 1, 2), + STAmount(EUR, 1, 5), + STAmount(EUR, 1, -3), // fail + STAmount(EUR, 1, -6), + STAmount(EUR, 1, -9)}) + { + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(100'000)}, + Fund::Acct); + env.close(); + + ammAlice.deposit( + DepositArg{.account = bob, .asset1In = deposit}); + invariant( + ammAlice, + env, + "dep1", + deposit == STAmount{EUR, 1, -3} && + !env.enabled(fixAMMv1_3)); + }, + {{GBP(30'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + } + + // Two-asset proportional deposit (1:1 pool ratio) + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(100'000)}, + Fund::Acct); + env.close(); + + const STAmount depositEuro{ + EUR, UINT64_C(10'1234567890123456), -16}; + const STAmount depositGBP{ + GBP, UINT64_C(10'1234567890123456), -16}; + + ammAlice.deposit(DepositArg{ + .account = bob, + .asset1In = depositEuro, + .asset2In = depositGBP}); + invariant(ammAlice, env, "dep2", false); + }, + {{GBP(30'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // Two-asset proportional deposit (1:3 pool ratio) + for (auto const& exponent : {1, 2, 3, 4, -3 /*fail*/, -6, -9}) + { + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(100'000)}, + Fund::Acct); + env.close(); + + const STAmount depositEuro{EUR, 1, exponent}; + const STAmount depositGBP{GBP, 1, exponent}; + + ammAlice.deposit(DepositArg{ + .account = bob, + .asset1In = depositEuro, + .asset2In = depositGBP}); + invariant( + ammAlice, + env, + "dep3", + exponent != -3 && !env.enabled(fixAMMv1_3)); + }, + {{GBP(10'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + } + + // tfLPToken deposit + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(100'000)}, + Fund::Acct); + env.close(); + + ammAlice.deposit(DepositArg{ + .account = bob, + .tokens = IOUAmount{10'1234567890123456, -16}}); + invariant(ammAlice, env, "dep4", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfOneAssetLPToken deposit + for (auto const& tokens : + {IOUAmount{1, -3}, + IOUAmount{1, -2}, + IOUAmount{1, -1}, + IOUAmount{1}, + IOUAmount{10}, + IOUAmount{100}, + IOUAmount{1'000}, + IOUAmount{10'000}}) + { + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(1'000'000)}, + Fund::Acct); + env.close(); + + ammAlice.deposit(DepositArg{ + .account = bob, + .tokens = tokens, + .asset1In = STAmount{EUR, 1, 6}}); + invariant(ammAlice, env, "dep5", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + } + + // Single deposit with EP not exceeding specified: + // 1'000 GBP with EP not to exceed 5 (GBP/TokensOut) + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(100'000)}, + Fund::Acct); + env.close(); + + ammAlice.deposit( + bob, GBP(1'000), std::nullopt, STAmount{GBP, 5}); + invariant(ammAlice, env, "dep6", false); + }, + {{GBP(30'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + } + + void + testWithdrawRounding(FeatureBitset all) + { + testcase("Withdraw Rounding"); + + using namespace jtx; + + // tfLPToken mode + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw(alice, 1'000); + invariant(ammAlice, env, "with1", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfWithdrawAll mode + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw( + WithdrawArg{.account = alice, .flags = tfWithdrawAll}); + invariant(ammAlice, env, "with2", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfTwoAsset withdraw mode + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw(WithdrawArg{ + .account = alice, + .asset1Out = STAmount{GBP, 3'500}, + .asset2Out = STAmount{EUR, 15'000}, + .flags = tfTwoAsset}); + invariant(ammAlice, env, "with3", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfSingleAsset withdraw mode + // Note: This test fails with 0 trading fees, but doesn't fail if + // trading fees is set to 1'000 -- I suspect the compound operations + // in AMMHelpers.cpp:withdrawByTokens compensate for the rounding + // errors + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw(WithdrawArg{ + .account = alice, + .asset1Out = STAmount{GBP, 1'234}, + .flags = tfSingleAsset}); + invariant(ammAlice, env, "with4", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfOneAssetWithdrawAll mode + testAMM( + [&](AMM& ammAlice, Env& env) { + fund( + env, + gw, + {bob}, + XRP(10'000'000), + {GBP(100'000), EUR(100'000)}, + Fund::Acct); + env.close(); + + ammAlice.deposit(DepositArg{ + .account = bob, .asset1In = STAmount{GBP, 3'456}}); + + ammAlice.withdraw(WithdrawArg{ + .account = bob, + .asset1Out = STAmount{GBP, 1'000}, + .flags = tfOneAssetWithdrawAll}); + invariant(ammAlice, env, "with5", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfOneAssetLPToken mode + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw(WithdrawArg{ + .account = alice, + .tokens = 1'000, + .asset1Out = STAmount{GBP, 100}, + .flags = tfOneAssetLPToken}); + invariant(ammAlice, env, "with6", false); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + + // tfLimitLPToken mode + testAMM( + [&](AMM& ammAlice, Env& env) { + ammAlice.withdraw(WithdrawArg{ + .account = alice, + .asset1Out = STAmount{GBP, 100}, + .maxEP = IOUAmount{2}, + .flags = tfLimitLPToken}); + invariant(ammAlice, env, "with7", true); + }, + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); + } + void run() override { @@ -7128,46 +7748,60 @@ struct AMM_test : public jtx::AMMTest testFeeVote(); testInvalidBid(); testBid(all); - testBid(all - fixAMMv1_1); + testBid(all - fixAMMv1_3); + testBid(all - fixAMMv1_1 - fixAMMv1_3); testInvalidAMMPayment(); testBasicPaymentEngine(all); - testBasicPaymentEngine(all - fixAMMv1_1); + testBasicPaymentEngine(all - fixAMMv1_1 - fixAMMv1_3); testBasicPaymentEngine(all - fixReducedOffersV2); - testBasicPaymentEngine(all - fixAMMv1_1 - fixReducedOffersV2); + testBasicPaymentEngine( + all - fixAMMv1_1 - fixAMMv1_3 - fixReducedOffersV2); testAMMTokens(); testAmendment(); testFlags(); testRippling(); testAMMAndCLOB(all); - testAMMAndCLOB(all - fixAMMv1_1); + testAMMAndCLOB(all - fixAMMv1_1 - fixAMMv1_3); testTradingFee(all); - testTradingFee(all - fixAMMv1_1); + testTradingFee(all - fixAMMv1_3); + testTradingFee(all - fixAMMv1_1 - fixAMMv1_3); testAdjustedTokens(all); - testAdjustedTokens(all - fixAMMv1_1); + testAdjustedTokens(all - fixAMMv1_3); + testAdjustedTokens(all - fixAMMv1_1 - fixAMMv1_3); testAutoDelete(); testClawback(); testAMMID(); testSelection(all); - testSelection(all - fixAMMv1_1); + testSelection(all - fixAMMv1_1 - fixAMMv1_3); testFixDefaultInnerObj(); testMalformed(); testFixOverflowOffer(all); - testFixOverflowOffer(all - fixAMMv1_1); + testFixOverflowOffer(all - fixAMMv1_3); + testFixOverflowOffer(all - fixAMMv1_1 - fixAMMv1_3); testSwapRounding(); testFixChangeSpotPriceQuality(all); - testFixChangeSpotPriceQuality(all - fixAMMv1_1); + testFixChangeSpotPriceQuality(all - fixAMMv1_1 - fixAMMv1_3); testFixAMMOfferBlockedByLOB(all); - testFixAMMOfferBlockedByLOB(all - fixAMMv1_1); + testFixAMMOfferBlockedByLOB(all - fixAMMv1_1 - fixAMMv1_3); testLPTokenBalance(all); - testLPTokenBalance(all - fixAMMv1_1); + testLPTokenBalance(all - fixAMMv1_3); + testLPTokenBalance(all - fixAMMv1_1 - fixAMMv1_3); testAMMClawback(all); testAMMClawback(all - featureAMMClawback); - testAMMClawback(all - fixAMMv1_1 - featureAMMClawback); + testAMMClawback(all - fixAMMv1_1 - fixAMMv1_3 - featureAMMClawback); testAMMDepositWithFrozenAssets(all); testAMMDepositWithFrozenAssets(all - featureAMMClawback); testAMMDepositWithFrozenAssets(all - fixAMMv1_1 - featureAMMClawback); + testAMMDepositWithFrozenAssets( + all - fixAMMv1_1 - fixAMMv1_3 - featureAMMClawback); testFixReserveCheckOnWithdrawal(all); testFixReserveCheckOnWithdrawal(all - fixAMMv1_2); + testDepositAndWithdrawRounding(all); + testDepositAndWithdrawRounding(all - fixAMMv1_3); + testDepositRounding(all); + testDepositRounding(all - fixAMMv1_3); + testWithdrawRounding(all); + testWithdrawRounding(all - fixAMMv1_3); } }; diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 52039f74aea..4328a57ba0f 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -125,7 +125,6 @@ class AMM STAmount const asset1_; STAmount const asset2_; uint256 const ammID_; - IOUAmount const initialLPTokens_; bool log_; bool doClose_; // Predict next purchase price @@ -138,6 +137,7 @@ class AMM std::uint32_t const fee_; AccountID const ammAccount_; Issue const lptIssue_; + IOUAmount const initialLPTokens_; public: AMM(Env& env, @@ -428,6 +428,9 @@ class AMM [[nodiscard]] bool expectAuctionSlot(auto&& cb) const; + + IOUAmount + initialTokens(); }; namespace amm { diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index d90d2bc1228..18bb1c977ab 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -593,6 +593,14 @@ class Env void disableFeature(uint256 const feature); + /** Check if feature is enabled + */ + bool + enabled(uint256 feature) const + { + return current()->rules().enabled(feature); + } + private: void fund(bool setDefaultRipple, STAmount const& amount, Account const& account); diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 089d3508d70..d639db9fa76 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -38,12 +39,16 @@ number(STAmount const& a) return a; } -static IOUAmount -initialTokens(STAmount const& asset1, STAmount const& asset2) +IOUAmount +AMM::initialTokens() { - auto const product = number(asset1) * number(asset2); - return (IOUAmount)(product.mantissa() >= 0 ? root2(product) - : root2(-product)); + if (!env_.enabled(fixAMMv1_3)) + { + auto const product = number(asset1_) * number(asset2_); + return (IOUAmount)(product.mantissa() >= 0 ? root2(product) + : root2(-product)); + } + return getLPTokensBalance(); } AMM::AMM( @@ -64,7 +69,6 @@ AMM::AMM( , asset1_(asset1) , asset2_(asset2) , ammID_(keylet::amm(asset1_.issue(), asset2_.issue()).key) - , initialLPTokens_(initialTokens(asset1, asset2)) , log_(log) , doClose_(close) , lastPurchasePrice_(0) @@ -77,6 +81,7 @@ AMM::AMM( asset1_.issue().currency, asset2_.issue().currency, ammAccount_)) + , initialLPTokens_(initialTokens()) { } diff --git a/src/xrpld/app/misc/AMMHelpers.h b/src/xrpld/app/misc/AMMHelpers.h index 7ad0093a2e4..1ad8b324580 100644 --- a/src/xrpld/app/misc/AMMHelpers.h +++ b/src/xrpld/app/misc/AMMHelpers.h @@ -51,6 +51,8 @@ reduceOffer(auto const& amount) } // namespace detail +enum class IsDeposit : bool { Yes, No }; + /** Calculate LP Tokens given AMM pool reserves. * @param asset1 AMM one side of the pool reserve * @param asset2 AMM another side of the pool reserve @@ -70,7 +72,7 @@ ammLPTokens( * @return tokens */ STAmount -lpTokensIn( +lpTokensOut( STAmount const& asset1Balance, STAmount const& asset1Deposit, STAmount const& lptAMMBalance, @@ -99,7 +101,7 @@ ammAssetIn( * @return tokens out amount */ STAmount -lpTokensOut( +lpTokensIn( STAmount const& asset1Balance, STAmount const& asset1Withdraw, STAmount const& lptAMMBalance, @@ -113,7 +115,7 @@ lpTokensOut( * @return calculated asset amount */ STAmount -withdrawByTokens( +ammAssetOut( STAmount const& assetBalance, STAmount const& lptAMMBalance, STAmount const& lpTokens, @@ -611,13 +613,13 @@ square(Number const& n); * withdraw to cancel out the precision loss. * @param lptAMMBalance LPT AMM Balance * @param lpTokens LP tokens to deposit or withdraw - * @param isDeposit true if deposit, false if withdraw + * @param isDeposit Yes if deposit, No if withdraw */ STAmount adjustLPTokens( STAmount const& lptAMMBalance, STAmount const& lpTokens, - bool isDeposit); + IsDeposit isDeposit); /** Calls adjustLPTokens() and adjusts deposit or withdraw amounts if * the adjusted LP tokens are less than the provided LP tokens. @@ -627,7 +629,7 @@ adjustLPTokens( * @param lptAMMBalance LPT AMM Balance * @param lpTokens LP tokens to deposit or withdraw * @param tfee trading fee in basis points - * @param isDeposit true if deposit, false if withdraw + * @param isDeposit Yes if deposit, No if withdraw * @return */ std::tuple, STAmount> @@ -638,7 +640,7 @@ adjustAmountsByLPTokens( STAmount const& lptAMMBalance, STAmount const& lpTokens, std::uint16_t tfee, - bool isDeposit); + IsDeposit isDeposit); /** Positive solution for quadratic equation: * x = (-b + sqrt(b**2 + 4*a*c))/(2*a) @@ -646,6 +648,141 @@ adjustAmountsByLPTokens( Number solveQuadraticEq(Number const& a, Number const& b, Number const& c); +STAmount +multiply(STAmount const& amount, Number const& frac, Number::rounding_mode rm); + +namespace detail { + +inline Number::rounding_mode +getLPTokenRounding(IsDeposit isDeposit) +{ + // Minimize on deposit, maximize on withdraw to ensure + // AMM invariant sqrt(poolAsset1 * poolAsset2) >= LPTokensBalance + return isDeposit == IsDeposit::Yes ? Number::downward : Number::upward; +} + +inline Number::rounding_mode +getAssetRounding(IsDeposit isDeposit) +{ + // Maximize on deposit, minimize on withdraw to ensure + // AMM invariant sqrt(poolAsset1 * poolAsset2) >= LPTokensBalance + return isDeposit == IsDeposit::Yes ? Number::upward : Number::downward; +} + +} // namespace detail + +/** Round AMM equal deposit/withdrawal amount. Deposit/withdrawal formulas + * calculate the amount as a fractional value of the pool balance. The rounding + * takes place on the last step of multiplying the balance by the fraction if + * AMMv1_3 is enabled. + */ +template +STAmount +getRoundedAsset( + Rules const& rules, + STAmount const& balance, + A const& frac, + IsDeposit isDeposit) +{ + if (!rules.enabled(fixAMMv1_3)) + { + if constexpr (std::is_same_v) + return multiply(balance, frac, balance.issue()); + else + return toSTAmount(balance.issue(), balance * frac); + } + auto const rm = detail::getAssetRounding(isDeposit); + return multiply(balance, frac, rm); +} + +/** Round AMM single deposit/withdrawal amount. + * The lambda's are used to delay evaluation until the function + * is executed so that the calculation is not done twice. noRoundCb() is + * called if AMMv1_3 is disabled. Otherwise, the rounding is set and + * the amount is: + * isDeposit is Yes - the balance multiplied by productCb() + * isDeposit is No - the result of productCb(). The rounding is + * the same for all calculations in productCb() + */ +STAmount +getRoundedAsset( + Rules const& rules, + std::function&& noRoundCb, + STAmount const& balance, + std::function&& productCb, + IsDeposit isDeposit); + +/** Round AMM deposit/withdrawal LPToken amount. Deposit/withdrawal formulas + * calculate the lptokens as a fractional value of the AMM total lptokens. + * The rounding takes place on the last step of multiplying the balance by + * the fraction if AMMv1_3 is enabled. The tokens are then + * adjusted to factor in the loss in precision (we only keep 16 significant + * digits) when adding the lptokens to the balance. + */ +STAmount +getRoundedLPTokens( + Rules const& rules, + STAmount const& balance, + Number const& frac, + IsDeposit isDeposit); + +/** Round AMM single deposit/withdrawal LPToken amount. + * The lambda's are used to delay evaluation until the function is executed + * so that the calculations are not done twice. + * noRoundCb() is called if AMMv1_3 is disabled. Otherwise, the rounding is set + * and the lptokens are: + * if isDeposit is Yes - the result of productCb(). The rounding is + * the same for all calculations in productCb() + * if isDeposit is No - the balance multiplied by productCb() + * The lptokens are then adjusted to factor in the loss in precision + * (we only keep 16 significant digits) when adding the lptokens to the balance. + */ +STAmount +getRoundedLPTokens( + Rules const& rules, + std::function&& noRoundCb, + STAmount const& lptAMMBalance, + std::function&& productCb, + IsDeposit isDeposit); + +/* Next two functions adjust asset in/out amount to factor in the adjusted + * lptokens. The lptokens are calculated from the asset in/out. The lptokens are + * then adjusted to factor in the loss in precision. The adjusted lptokens might + * be less than the initially calculated tokens. Therefore, the asset in/out + * must be adjusted. The rounding might result in the adjusted amount being + * greater than the original asset in/out amount. If this happens, + * then the original amount is reduced by the difference in the adjusted amount + * and the original amount. The actual tokens and the actual adjusted amount + * are then recalculated. The minimum of the original and the actual + * adjusted amount is returned. + */ +std::pair +adjustAssetInByTokens( + Rules const& rules, + STAmount const& balance, + STAmount const& amount, + STAmount const& lptAMMBalance, + STAmount const& tokens, + std::uint16_t tfee); +std::pair +adjustAssetOutByTokens( + Rules const& rules, + STAmount const& balance, + STAmount const& amount, + STAmount const& lptAMMBalance, + STAmount const& tokens, + std::uint16_t tfee); + +/** Find a fraction of tokens after the tokens are adjusted. The fraction + * is used to adjust equal deposit/withdraw amount. + */ +Number +adjustFracByTokens( + Rules const& rules, + STAmount const& lptAMMBalance, + STAmount const& tokens, + Number const& frac); + } // namespace ripple #endif // RIPPLE_APP_MISC_AMMHELPERS_H_INCLUDED diff --git a/src/xrpld/app/misc/detail/AMMHelpers.cpp b/src/xrpld/app/misc/detail/AMMHelpers.cpp index f10b4c15eb0..2ff8d1a296e 100644 --- a/src/xrpld/app/misc/detail/AMMHelpers.cpp +++ b/src/xrpld/app/misc/detail/AMMHelpers.cpp @@ -27,6 +27,10 @@ ammLPTokens( STAmount const& asset2, Issue const& lptIssue) { + // AMM invariant: sqrt(asset1 * asset2) >= LPTokensBalance + auto const rounding = + isFeatureEnabled(fixAMMv1_3) ? Number::downward : Number::getround(); + NumberRoundModeGuard g(rounding); auto const tokens = root2(asset1 * asset2); return toSTAmount(lptIssue, tokens); } @@ -38,7 +42,7 @@ ammLPTokens( * where f1 = 1 - tfee, f2 = (1 - tfee/2)/f1 */ STAmount -lpTokensIn( +lpTokensOut( STAmount const& asset1Balance, STAmount const& asset1Deposit, STAmount const& lptAMMBalance, @@ -48,8 +52,17 @@ lpTokensIn( auto const f2 = feeMultHalf(tfee) / f1; Number const r = asset1Deposit / asset1Balance; auto const c = root2(f2 * f2 + r / f1) - f2; - auto const t = lptAMMBalance * (r - c) / (1 + c); - return toSTAmount(lptAMMBalance.issue(), t); + if (!isFeatureEnabled(fixAMMv1_3)) + { + auto const t = lptAMMBalance * (r - c) / (1 + c); + return toSTAmount(lptAMMBalance.issue(), t); + } + else + { + // minimize tokens out + auto const frac = (r - c) / (1 + c); + return multiply(lptAMMBalance, frac, Number::downward); + } } /* Equation 4 solves equation 3 for b: @@ -78,8 +91,17 @@ ammAssetIn( auto const a = 1 / (t2 * t2); auto const b = 2 * d / t2 - 1 / f1; auto const c = d * d - f2 * f2; - return toSTAmount( - asset1Balance.issue(), asset1Balance * solveQuadraticEq(a, b, c)); + if (!isFeatureEnabled(fixAMMv1_3)) + { + return toSTAmount( + asset1Balance.issue(), asset1Balance * solveQuadraticEq(a, b, c)); + } + else + { + // maximize deposit + auto const frac = solveQuadraticEq(a, b, c); + return multiply(asset1Balance, frac, Number::upward); + } } /* Equation 7: @@ -87,7 +109,7 @@ ammAssetIn( * where R = b/B, c = R*fee + 2 - fee */ STAmount -lpTokensOut( +lpTokensIn( STAmount const& asset1Balance, STAmount const& asset1Withdraw, STAmount const& lptAMMBalance, @@ -96,8 +118,17 @@ lpTokensOut( Number const fr = asset1Withdraw / asset1Balance; auto const f1 = getFee(tfee); auto const c = fr * f1 + 2 - f1; - auto const t = lptAMMBalance * (c - root2(c * c - 4 * fr)) / 2; - return toSTAmount(lptAMMBalance.issue(), t); + if (!isFeatureEnabled(fixAMMv1_3)) + { + auto const t = lptAMMBalance * (c - root2(c * c - 4 * fr)) / 2; + return toSTAmount(lptAMMBalance.issue(), t); + } + else + { + // maximize tokens in + auto const frac = (c - root2(c * c - 4 * fr)) / 2; + return multiply(lptAMMBalance, frac, Number::upward); + } } /* Equation 8 solves equation 7 for b: @@ -111,7 +142,7 @@ lpTokensOut( * R = (t1**2 + t1*(f - 2)) / (t1*f - 1) */ STAmount -withdrawByTokens( +ammAssetOut( STAmount const& assetBalance, STAmount const& lptAMMBalance, STAmount const& lpTokens, @@ -119,8 +150,17 @@ withdrawByTokens( { auto const f = getFee(tfee); Number const t1 = lpTokens / lptAMMBalance; - auto const b = assetBalance * (t1 * t1 - t1 * (2 - f)) / (t1 * f - 1); - return toSTAmount(assetBalance.issue(), b); + if (!isFeatureEnabled(fixAMMv1_3)) + { + auto const b = assetBalance * (t1 * t1 - t1 * (2 - f)) / (t1 * f - 1); + return toSTAmount(assetBalance.issue(), b); + } + else + { + // minimize withdraw + auto const frac = (t1 * t1 - t1 * (2 - f)) / (t1 * f - 1); + return multiply(assetBalance, frac, Number::downward); + } } Number @@ -133,12 +173,12 @@ STAmount adjustLPTokens( STAmount const& lptAMMBalance, STAmount const& lpTokens, - bool isDeposit) + IsDeposit isDeposit) { // Force rounding downward to ensure adjusted tokens are less or equal // to requested tokens. saveNumberRoundMode rm(Number::setround(Number::rounding_mode::downward)); - if (isDeposit) + if (isDeposit == IsDeposit::Yes) return (lptAMMBalance + lpTokens) - lptAMMBalance; return (lpTokens - lptAMMBalance) + lptAMMBalance; } @@ -151,8 +191,12 @@ adjustAmountsByLPTokens( STAmount const& lptAMMBalance, STAmount const& lpTokens, std::uint16_t tfee, - bool isDeposit) + IsDeposit isDeposit) { + // AMMv1_3 amendment adjusts tokens and amounts in deposit/withdraw + if (isFeatureEnabled(fixAMMv1_3)) + return std::make_tuple(amount, amount2, lpTokens); + auto const lpTokensActual = adjustLPTokens(lptAMMBalance, lpTokens, isDeposit); @@ -191,14 +235,14 @@ adjustAmountsByLPTokens( // Single trade auto const amountActual = [&]() { - if (isDeposit) + if (isDeposit == IsDeposit::Yes) return ammAssetIn( amountBalance, lptAMMBalance, lpTokensActual, tfee); else if (!ammRoundingEnabled) - return withdrawByTokens( + return ammAssetOut( amountBalance, lptAMMBalance, lpTokens, tfee); else - return withdrawByTokens( + return ammAssetOut( amountBalance, lptAMMBalance, lpTokensActual, tfee); }(); if (!ammRoundingEnabled) @@ -235,4 +279,132 @@ solveQuadraticEqSmallest(Number const& a, Number const& b, Number const& c) return (2 * c) / (-b + root2(d)); } +STAmount +multiply(STAmount const& amount, Number const& frac, Number::rounding_mode rm) +{ + NumberRoundModeGuard g(rm); + auto const t = amount * frac; + return toSTAmount(amount.issue(), t, rm); +} + +STAmount +getRoundedAsset( + Rules const& rules, + std::function&& noRoundCb, + STAmount const& balance, + std::function&& productCb, + IsDeposit isDeposit) +{ + if (!rules.enabled(fixAMMv1_3)) + return toSTAmount(balance.issue(), noRoundCb()); + + auto const rm = detail::getAssetRounding(isDeposit); + if (isDeposit == IsDeposit::Yes) + return multiply(balance, productCb(), rm); + NumberRoundModeGuard g(rm); + return toSTAmount(balance.issue(), productCb(), rm); +} + +STAmount +getRoundedLPTokens( + Rules const& rules, + STAmount const& balance, + Number const& frac, + IsDeposit isDeposit) +{ + if (!rules.enabled(fixAMMv1_3)) + return toSTAmount(balance.issue(), balance * frac); + + auto const rm = detail::getLPTokenRounding(isDeposit); + auto const tokens = multiply(balance, frac, rm); + return adjustLPTokens(balance, tokens, isDeposit); +} + +STAmount +getRoundedLPTokens( + Rules const& rules, + std::function&& noRoundCb, + STAmount const& lptAMMBalance, + std::function&& productCb, + IsDeposit isDeposit) +{ + if (!rules.enabled(fixAMMv1_3)) + return toSTAmount(lptAMMBalance.issue(), noRoundCb()); + + auto const tokens = [&] { + auto const rm = detail::getLPTokenRounding(isDeposit); + if (isDeposit == IsDeposit::Yes) + { + NumberRoundModeGuard g(rm); + return toSTAmount(lptAMMBalance.issue(), productCb(), rm); + } + return multiply(lptAMMBalance, productCb(), rm); + }(); + return adjustLPTokens(lptAMMBalance, tokens, isDeposit); +} + +std::pair +adjustAssetInByTokens( + Rules const& rules, + STAmount const& balance, + STAmount const& amount, + STAmount const& lptAMMBalance, + STAmount const& tokens, + std::uint16_t tfee) +{ + if (!rules.enabled(fixAMMv1_3)) + return {tokens, amount}; + auto adjAsset = ammAssetIn(balance, lptAMMBalance, tokens, tfee); + auto adjTokens = tokens; + // Rounding didn't work the right way. + // Try to adjust the original deposit amount by difference + // in adjust and original amount. Then adjust tokens and deposit amount. + if (adjAsset > amount) + { + auto const adjAmount = amount - (adjAsset - amount); + auto const t = lpTokensOut(balance, adjAmount, lptAMMBalance, tfee); + adjTokens = adjustLPTokens(lptAMMBalance, t, IsDeposit::Yes); + adjAsset = ammAssetIn(balance, lptAMMBalance, adjTokens, tfee); + } + return {adjTokens, std::min(amount, adjAsset)}; +} + +std::pair +adjustAssetOutByTokens( + Rules const& rules, + STAmount const& balance, + STAmount const& amount, + STAmount const& lptAMMBalance, + STAmount const& tokens, + std::uint16_t tfee) +{ + if (!rules.enabled(fixAMMv1_3)) + return {tokens, amount}; + auto adjAsset = ammAssetOut(balance, lptAMMBalance, tokens, tfee); + auto adjTokens = tokens; + // Rounding didn't work the right way. + // Try to adjust the original deposit amount by difference + // in adjust and original amount. Then adjust tokens and deposit amount. + if (adjAsset > amount) + { + auto const adjAmount = amount - (adjAsset - amount); + auto const t = lpTokensIn(balance, adjAmount, lptAMMBalance, tfee); + adjTokens = adjustLPTokens(lptAMMBalance, t, IsDeposit::No); + adjAsset = ammAssetOut(balance, lptAMMBalance, adjTokens, tfee); + } + return {adjTokens, std::min(amount, adjAsset)}; +} + +Number +adjustFracByTokens( + Rules const& rules, + STAmount const& lptAMMBalance, + STAmount const& tokens, + Number const& frac) +{ + if (!rules.enabled(fixAMMv1_3)) + return frac; + return tokens / lptAMMBalance; +} + } // namespace ripple diff --git a/src/xrpld/app/paths/detail/AMMOffer.cpp b/src/xrpld/app/paths/detail/AMMOffer.cpp index 16ea8628f3b..6b89c453d8b 100644 --- a/src/xrpld/app/paths/detail/AMMOffer.cpp +++ b/src/xrpld/app/paths/detail/AMMOffer.cpp @@ -152,6 +152,10 @@ AMMOffer::checkInvariant( return false; } + // The invariant check below is moved to InvariantCheck + if (isFeatureEnabled(fixAMMv1_3)) + return true; + Number const product = balances_.in * balances_.out; auto const newBalances = TAmounts{ balances_.in + consumed.in, balances_.out - consumed.out}; diff --git a/src/xrpld/app/tx/detail/AMMBid.cpp b/src/xrpld/app/tx/detail/AMMBid.cpp index 9de3762d2e3..0d8cde7c9bf 100644 --- a/src/xrpld/app/tx/detail/AMMBid.cpp +++ b/src/xrpld/app/tx/detail/AMMBid.cpp @@ -78,6 +78,18 @@ AMMBid::preflight(PreflightContext const& ctx) JLOG(ctx.j.debug()) << "AMM Bid: Invalid number of AuthAccounts."; return temMALFORMED; } + else if (ctx.rules.enabled(fixAMMv1_3)) + { + AccountID account = ctx.tx[sfAccount]; + std::set unique; + for (auto const& obj : authAccounts) + { + auto authAccount = obj[sfAccount]; + if (authAccount == account || unique.contains(authAccount)) + return temMALFORMED; + unique.insert(authAccount); + } + } } return preflight2(ctx); @@ -230,7 +242,9 @@ applyBid( auctionSlot.makeFieldAbsent(sfAuthAccounts); // Burn the remaining bid amount auto const saBurn = adjustLPTokens( - lptAMMBalance, toSTAmount(lptAMMBalance.issue(), burn), false); + lptAMMBalance, + toSTAmount(lptAMMBalance.issue(), burn), + IsDeposit::No); if (saBurn >= lptAMMBalance) { // This error case should never occur. diff --git a/src/xrpld/app/tx/detail/AMMDeposit.cpp b/src/xrpld/app/tx/detail/AMMDeposit.cpp index 3448401eb79..08a4f9c2adb 100644 --- a/src/xrpld/app/tx/detail/AMMDeposit.cpp +++ b/src/xrpld/app/tx/detail/AMMDeposit.cpp @@ -543,7 +543,7 @@ AMMDeposit::deposit( lptAMMBalance, lpTokensDeposit, tfee, - true); + IsDeposit::Yes); if (lpTokensDepositActual <= beast::zero) { @@ -626,6 +626,17 @@ AMMDeposit::deposit( return {tesSUCCESS, lptAMMBalance + lpTokensDepositActual}; } +static STAmount +adjustLPTokensOut( + Rules const& rules, + STAmount const& lptAMMBalance, + STAmount const& lpTokensDeposit) +{ + if (!rules.enabled(fixAMMv1_3)) + return lpTokensDeposit; + return adjustLPTokens(lptAMMBalance, lpTokensDeposit, IsDeposit::Yes); +} + /** Proportional deposit of pools assets in exchange for the specified * amount of LPTokens. */ @@ -643,16 +654,25 @@ AMMDeposit::equalDepositTokens( { try { + auto const tokensAdj = + adjustLPTokensOut(view.rules(), lptAMMBalance, lpTokensDeposit); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; auto const frac = - divide(lpTokensDeposit, lptAMMBalance, lptAMMBalance.issue()); + divide(tokensAdj, lptAMMBalance, lptAMMBalance.issue()); + // amounts factor in the adjusted tokens + auto const amountDeposit = + getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::Yes); + auto const amount2Deposit = + getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::Yes); return deposit( view, ammAccount, amountBalance, - multiply(amountBalance, frac, amountBalance.issue()), - multiply(amount2Balance, frac, amount2Balance.issue()), + amountDeposit, + amount2Deposit, lptAMMBalance, - lpTokensDeposit, + tokensAdj, depositMin, deposit2Min, std::nullopt, @@ -709,37 +729,55 @@ AMMDeposit::equalDepositLimit( std::uint16_t tfee) { auto frac = Number{amount} / amountBalance; - auto tokens = toSTAmount(lptAMMBalance.issue(), lptAMMBalance * frac); - if (tokens == beast::zero) - return {tecAMM_FAILED, STAmount{}}; - auto const amount2Deposit = amount2Balance * frac; + auto tokensAdj = + getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes); + if (tokensAdj == beast::zero) + { + if (!view.rules().enabled(fixAMMv1_3)) + return {tecAMM_FAILED, STAmount{}}; + else + return {tecAMM_INVALID_TOKENS, STAmount{}}; + } + // factor in the adjusted tokens + frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac); + auto const amount2Deposit = + getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::Yes); if (amount2Deposit <= amount2) return deposit( view, ammAccount, amountBalance, amount, - toSTAmount(amount2Balance.issue(), amount2Deposit), + amount2Deposit, lptAMMBalance, - tokens, + tokensAdj, std::nullopt, std::nullopt, lpTokensDepositMin, tfee); frac = Number{amount2} / amount2Balance; - tokens = toSTAmount(lptAMMBalance.issue(), lptAMMBalance * frac); - if (tokens == beast::zero) - return {tecAMM_FAILED, STAmount{}}; - auto const amountDeposit = amountBalance * frac; + tokensAdj = + getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes); + if (tokensAdj == beast::zero) + { + if (!view.rules().enabled(fixAMMv1_3)) + return {tecAMM_FAILED, STAmount{}}; + else + return {tecAMM_INVALID_TOKENS, STAmount{}}; + } + // factor in the adjusted tokens + frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac); + auto const amountDeposit = + getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::Yes); if (amountDeposit <= amount) return deposit( view, ammAccount, amountBalance, - toSTAmount(amountBalance.issue(), amountDeposit), + amountDeposit, amount2, lptAMMBalance, - tokens, + tokensAdj, std::nullopt, std::nullopt, lpTokensDepositMin, @@ -765,17 +803,30 @@ AMMDeposit::singleDeposit( std::optional const& lpTokensDepositMin, std::uint16_t tfee) { - auto const tokens = lpTokensIn(amountBalance, amount, lptAMMBalance, tfee); + auto const tokens = adjustLPTokensOut( + view.rules(), + lptAMMBalance, + lpTokensOut(amountBalance, amount, lptAMMBalance, tfee)); if (tokens == beast::zero) - return {tecAMM_FAILED, STAmount{}}; + { + if (!view.rules().enabled(fixAMMv1_3)) + return {tecAMM_FAILED, STAmount{}}; + else + return {tecAMM_INVALID_TOKENS, STAmount{}}; + } + // factor in the adjusted tokens + auto const [tokensAdj, amountDepositAdj] = adjustAssetInByTokens( + view.rules(), amountBalance, amount, lptAMMBalance, tokens, tfee); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; return deposit( view, ammAccount, amountBalance, - amount, + amountDepositAdj, std::nullopt, lptAMMBalance, - tokens, + tokensAdj, std::nullopt, std::nullopt, lpTokensDepositMin, @@ -799,8 +850,13 @@ AMMDeposit::singleDepositTokens( STAmount const& lpTokensDeposit, std::uint16_t tfee) { + auto const tokensAdj = + adjustLPTokensOut(view.rules(), lptAMMBalance, lpTokensDeposit); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; + // the adjusted tokens are factored in auto const amountDeposit = - ammAssetIn(amountBalance, lptAMMBalance, lpTokensDeposit, tfee); + ammAssetIn(amountBalance, lptAMMBalance, tokensAdj, tfee); if (amountDeposit > amount) return {tecAMM_FAILED, STAmount{}}; return deposit( @@ -810,7 +866,7 @@ AMMDeposit::singleDepositTokens( amountDeposit, std::nullopt, lptAMMBalance, - lpTokensDeposit, + tokensAdj, std::nullopt, std::nullopt, std::nullopt, @@ -854,20 +910,32 @@ AMMDeposit::singleDepositEPrice( { if (amount != beast::zero) { - auto const tokens = - lpTokensIn(amountBalance, amount, lptAMMBalance, tfee); + auto const tokens = adjustLPTokensOut( + view.rules(), + lptAMMBalance, + lpTokensOut(amountBalance, amount, lptAMMBalance, tfee)); if (tokens <= beast::zero) - return {tecAMM_FAILED, STAmount{}}; - auto const ep = Number{amount} / tokens; + { + if (!view.rules().enabled(fixAMMv1_3)) + return {tecAMM_FAILED, STAmount{}}; + else + return {tecAMM_INVALID_TOKENS, STAmount{}}; + } + // factor in the adjusted tokens + auto const [tokensAdj, amountDepositAdj] = adjustAssetInByTokens( + view.rules(), amountBalance, amount, lptAMMBalance, tokens, tfee); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; + auto const ep = Number{amountDepositAdj} / tokensAdj; if (ep <= ePrice) return deposit( view, ammAccount, amountBalance, - amount, + amountDepositAdj, std::nullopt, lptAMMBalance, - tokens, + tokensAdj, std::nullopt, std::nullopt, std::nullopt, @@ -898,21 +966,37 @@ AMMDeposit::singleDepositEPrice( auto const a1 = c * c; auto const b1 = c * c * f2 * f2 + 2 * c - d * d; auto const c1 = 2 * c * f2 * f2 + 1 - 2 * d * f2; - auto const amountDeposit = toSTAmount( - amountBalance.issue(), - f1 * amountBalance * solveQuadraticEq(a1, b1, c1)); + auto amtNoRoundCb = [&] { + return f1 * amountBalance * solveQuadraticEq(a1, b1, c1); + }; + auto amtProdCb = [&] { return f1 * solveQuadraticEq(a1, b1, c1); }; + auto const amountDeposit = getRoundedAsset( + view.rules(), amtNoRoundCb, amountBalance, amtProdCb, IsDeposit::Yes); if (amountDeposit <= beast::zero) return {tecAMM_FAILED, STAmount{}}; - auto const tokens = - toSTAmount(lptAMMBalance.issue(), amountDeposit / ePrice); + auto tokNoRoundCb = [&] { return amountDeposit / ePrice; }; + auto tokProdCb = [&] { return amountDeposit / ePrice; }; + auto const tokens = getRoundedLPTokens( + view.rules(), tokNoRoundCb, lptAMMBalance, tokProdCb, IsDeposit::Yes); + // factor in the adjusted tokens + auto const [tokensAdj, amountDepositAdj] = adjustAssetInByTokens( + view.rules(), + amountBalance, + amountDeposit, + lptAMMBalance, + tokens, + tfee); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; + return deposit( view, ammAccount, amountBalance, - amountDeposit, + amountDepositAdj, std::nullopt, lptAMMBalance, - tokens, + tokensAdj, std::nullopt, std::nullopt, std::nullopt, diff --git a/src/xrpld/app/tx/detail/AMMWithdraw.cpp b/src/xrpld/app/tx/detail/AMMWithdraw.cpp index 118262905c1..3f12d4d5930 100644 --- a/src/xrpld/app/tx/detail/AMMWithdraw.cpp +++ b/src/xrpld/app/tx/detail/AMMWithdraw.cpp @@ -520,7 +520,7 @@ AMMWithdraw::withdraw( lpTokensAMMBalance, lpTokensWithdraw, tfee, - false); + IsDeposit::No); return std::make_tuple( amountWithdraw, amount2Withdraw, lpTokensWithdraw); }(); @@ -681,6 +681,20 @@ AMMWithdraw::withdraw( amount2WithdrawActual); } +static STAmount +adjustLPTokensIn( + Rules const& rules, + STAmount const& lptAMMBalance, + STAmount const& lpTokensWithdraw, + WithdrawAll withdrawAll) +{ + if (!rules.enabled(fixAMMv1_3) || withdrawAll == WithdrawAll::Yes) + return lpTokensWithdraw; + return adjustLPTokens(lptAMMBalance, lpTokensWithdraw, IsDeposit::No); +} + +/** Proportional withdrawal of pool assets for the amount of LPTokens. + */ std::pair AMMWithdraw::equalWithdrawTokens( Sandbox& view, @@ -784,16 +798,22 @@ AMMWithdraw::equalWithdrawTokens( journal); } - auto const frac = divide(lpTokensWithdraw, lptAMMBalance, noIssue()); - auto const withdrawAmount = - multiply(amountBalance, frac, amountBalance.issue()); - auto const withdraw2Amount = - multiply(amount2Balance, frac, amount2Balance.issue()); + auto const tokensAdj = adjustLPTokensIn( + view.rules(), lptAMMBalance, lpTokensWithdraw, withdrawAll); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return { + tecAMM_INVALID_TOKENS, STAmount{}, STAmount{}, std::nullopt}; + // the adjusted tokens are factored in + auto const frac = divide(tokensAdj, lptAMMBalance, noIssue()); + auto const amountWithdraw = + getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::No); + auto const amount2Withdraw = + getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::No); // LP is making equal withdrawal by tokens but the requested amount // of LP tokens is likely too small and results in one-sided pool // withdrawal due to round off. Fail so the user withdraws // more tokens. - if (withdrawAmount == beast::zero || withdraw2Amount == beast::zero) + if (amountWithdraw == beast::zero || amount2Withdraw == beast::zero) return {tecAMM_FAILED, STAmount{}, STAmount{}, STAmount{}}; return withdraw( @@ -802,10 +822,10 @@ AMMWithdraw::equalWithdrawTokens( ammAccount, account, amountBalance, - withdrawAmount, - withdraw2Amount, + amountWithdraw, + amount2Withdraw, lptAMMBalance, - lpTokensWithdraw, + tokensAdj, tfee, freezeHanding, withdrawAll, @@ -860,7 +880,16 @@ AMMWithdraw::equalWithdrawLimit( std::uint16_t tfee) { auto frac = Number{amount} / amountBalance; - auto const amount2Withdraw = amount2Balance * frac; + auto amount2Withdraw = + getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::No); + auto tokensAdj = + getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::No); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; + // factor in the adjusted tokens + frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac); + amount2Withdraw = + getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::No); if (amount2Withdraw <= amount2) { return withdraw( @@ -869,24 +898,36 @@ AMMWithdraw::equalWithdrawLimit( ammAccount, amountBalance, amount, - toSTAmount(amount2.issue(), amount2Withdraw), + amount2Withdraw, lptAMMBalance, - toSTAmount(lptAMMBalance.issue(), lptAMMBalance * frac), + tokensAdj, tfee); } frac = Number{amount2} / amount2Balance; - auto const amountWithdraw = amountBalance * frac; - assert(amountWithdraw <= amount); + auto amountWithdraw = + getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::No); + tokensAdj = + getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::No); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; + // factor in the adjusted tokens + frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac); + amountWithdraw = + getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::No); + if (!view.rules().enabled(fixAMMv1_3)) + assert(amountWithdraw <= amount); + else if (amountWithdraw > amount) + return {tecAMM_FAILED, STAmount{}}; return withdraw( view, ammSle, ammAccount, amountBalance, - toSTAmount(amount.issue(), amountWithdraw), + amountWithdraw, amount2, lptAMMBalance, - toSTAmount(lptAMMBalance.issue(), lptAMMBalance * frac), + tokensAdj, tfee); } @@ -905,19 +946,32 @@ AMMWithdraw::singleWithdraw( STAmount const& amount, std::uint16_t tfee) { - auto const tokens = lpTokensOut(amountBalance, amount, lptAMMBalance, tfee); + auto const tokens = adjustLPTokensIn( + view.rules(), + lptAMMBalance, + lpTokensIn(amountBalance, amount, lptAMMBalance, tfee), + isWithdrawAll(ctx_.tx)); if (tokens == beast::zero) - return {tecAMM_FAILED, STAmount{}}; - + { + if (!view.rules().enabled(fixAMMv1_3)) + return {tecAMM_FAILED, STAmount{}}; + else + return {tecAMM_INVALID_TOKENS, STAmount{}}; + } + // factor in the adjusted tokens + auto const [tokensAdj, amountWithdrawAdj] = adjustAssetOutByTokens( + view.rules(), amountBalance, amount, lptAMMBalance, tokens, tfee); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; return withdraw( view, ammSle, ammAccount, amountBalance, - amount, + amountWithdrawAdj, std::nullopt, lptAMMBalance, - tokens, + tokensAdj, tfee); } @@ -942,8 +996,13 @@ AMMWithdraw::singleWithdrawTokens( STAmount const& lpTokensWithdraw, std::uint16_t tfee) { + auto const tokensAdj = adjustLPTokensIn( + view.rules(), lptAMMBalance, lpTokensWithdraw, isWithdrawAll(ctx_.tx)); + if (view.rules().enabled(fixAMMv1_3) && tokensAdj == beast::zero) + return {tecAMM_INVALID_TOKENS, STAmount{}}; + // the adjusted tokens are factored in auto const amountWithdraw = - withdrawByTokens(amountBalance, lptAMMBalance, lpTokensWithdraw, tfee); + ammAssetOut(amountBalance, lptAMMBalance, tokensAdj, tfee); if (amount == beast::zero || amountWithdraw >= amount) { return withdraw( @@ -954,7 +1013,7 @@ AMMWithdraw::singleWithdrawTokens( amountWithdraw, std::nullopt, lptAMMBalance, - lpTokensWithdraw, + tokensAdj, tfee); } @@ -1003,11 +1062,27 @@ AMMWithdraw::singleWithdrawEPrice( // t = T*(T + A*E*(f - 2))/(T*f - A*E) Number const ae = amountBalance * ePrice; auto const f = getFee(tfee); - auto const tokens = lptAMMBalance * (lptAMMBalance + ae * (f - 2)) / - (lptAMMBalance * f - ae); - if (tokens <= 0) - return {tecAMM_FAILED, STAmount{}}; - auto const amountWithdraw = toSTAmount(amount.issue(), tokens / ePrice); + auto tokNoRoundCb = [&] { + return lptAMMBalance * (lptAMMBalance + ae * (f - 2)) / + (lptAMMBalance * f - ae); + }; + auto tokProdCb = [&] { + return (lptAMMBalance + ae * (f - 2)) / (lptAMMBalance * f - ae); + }; + auto const tokensAdj = getRoundedLPTokens( + view.rules(), tokNoRoundCb, lptAMMBalance, tokProdCb, IsDeposit::No); + if (tokensAdj <= beast::zero) + { + if (!view.rules().enabled(fixAMMv1_3)) + return {tecAMM_FAILED, STAmount{}}; + else + return {tecAMM_INVALID_TOKENS, STAmount{}}; + } + auto amtNoRoundCb = [&] { return tokensAdj / ePrice; }; + auto amtProdCb = [&] { return tokensAdj / ePrice; }; + // the adjusted tokens are factored in + auto const amountWithdraw = getRoundedAsset( + view.rules(), amtNoRoundCb, amount, amtProdCb, IsDeposit::No); if (amount == beast::zero || amountWithdraw >= amount) { return withdraw( @@ -1018,7 +1093,7 @@ AMMWithdraw::singleWithdrawEPrice( amountWithdraw, std::nullopt, lptAMMBalance, - toSTAmount(lptAMMBalance.issue(), tokens), + tokensAdj, tfee); } diff --git a/src/xrpld/app/tx/detail/AMMWithdraw.h b/src/xrpld/app/tx/detail/AMMWithdraw.h index ae9328cb05e..1de91fd787f 100644 --- a/src/xrpld/app/tx/detail/AMMWithdraw.h +++ b/src/xrpld/app/tx/detail/AMMWithdraw.h @@ -301,7 +301,7 @@ class AMMWithdraw : public Transactor std::uint16_t tfee); /** Check from the flags if it's withdraw all */ - WithdrawAll + static WithdrawAll isWithdrawAll(STTx const& tx); }; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 90fc399b344..95eb511a9d7 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -19,6 +19,8 @@ #include +#include +#include #include #include #include @@ -1120,4 +1122,225 @@ ValidMPTIssuance::finalize( mptokensCreated_ == 0 && mptokensDeleted_ == 0; } +void +ValidAMM::visitEntry( + bool isDelete, + std::shared_ptr const& before, + std::shared_ptr const& after) +{ + if (!isFeatureEnabled(fixAMMv1_3)) + return; + + using Balance = hash_map>; + auto set = [&](SLE const& sle, + Balance& balance, + STAmount const& a1, + STAmount const& a2) { + STAmount assetBalance = sle[sfBalance]; + // don't know account/issuer + if (assetBalance.negative()) + assetBalance.negate(); + // Don't know which side of the trustline is AMM so save both. + // finalize() does lookup to figure out the AMM side. + balance[a1.getIssuer()][a2.get()] = assetBalance; + balance[a2.getIssuer()][a1.get()] = assetBalance; + }; + auto update = [&](SLE const& sle, Balance& balance) { + auto const leType = sle.getType(); + if (leType == ltACCOUNT_ROOT && sle.isFieldPresent(sfAMMID)) + { + balance[sle[sfAccount]][xrpIssue()] = sle[sfBalance]; + } + else if (leType == ltRIPPLE_STATE && (sle.getFlags() & lsfAMMNode)) + { + auto const& highLimit = sle.getFieldAmount(sfHighLimit); + auto const& lowLimit = sle.getFieldAmount(sfLowLimit); + set(sle, balance, highLimit, lowLimit); + set(sle, balance, lowLimit, highLimit); + } + }; + + if (!isDelete) + { + if (after) + { + if (after->getType() == ltAMM) + { + ammAccount_ = after->getAccountID(sfAccount); + lptAMMBalance_ = after->getFieldAmount(sfLPTokenBalance); + } + else + update(*after, balanceAfter_); + } + if (before) + update(*before, balanceBefore_); + } +} + +bool +ValidAMM::finalize( + STTx const& tx, + TER const result, + XRPAmount const _fee, + ReadView const& _view, + beast::Journal const& j) +{ + if (!_view.rules().enabled(fixAMMv1_3)) + return true; + + auto positiveBalances = [&](STAmount const& amount, + STAmount const& amount2, + STAmount const& lptAMMBalance, + bool zeroAllowed) { + if (zeroAllowed) + return amount >= beast::zero && amount2 >= beast::zero && + lptAMMBalance >= beast::zero; + return amount > beast::zero && amount2 > beast::zero && + lptAMMBalance > beast::zero; + }; + + if (result == tesSUCCESS) + { + auto const txType = tx.getTxnType(); + if ((txType == ttAMM_DEPOSIT || txType == ttAMM_WITHDRAW) && + ammAccount_) + { + auto const [amount, amount2] = ammPoolHolds( + _view, + *ammAccount_, + tx[sfAsset], + tx[sfAsset2], + fhIGNORE_FREEZE, + j); + auto const res = root2(amount * amount2); + if (positiveBalances(amount, amount2, *lptAMMBalance_, true) && + (res >= *lptAMMBalance_ || + (*lptAMMBalance_ != beast::zero && + withinRelativeDistance( + res, Number{*lptAMMBalance_}, Number{1, -11})))) + { + if (*lptAMMBalance_ > beast::zero) + return true; + // last withdraw may have resulted in an empty AMM + if (txType == ttAMM_WITHDRAW && + *lptAMMBalance_ == beast::zero && amount == beast::zero && + amount2 == beast::zero) + return true; + } + + std::string const txt = + txType == ttAMM_DEPOSIT ? "AMMDeposit" : "AMMWithdraw"; + JLOG(j.error()) + << txt << " invariant failed: " << amount << " " << amount2 + << " " << res << " " << lptAMMBalance_->getText() << " " + << ((*lptAMMBalance_ == beast::zero) + ? Number{1} + : ((*lptAMMBalance_ - res) / res)); + return false; + } + else if (txType == ttAMM_CREATE) + { + auto const [amount, amount2] = ammPoolHolds( + _view, + *ammAccount_, + tx[sfAmount].get(), + tx[sfAmount2].get(), + fhIGNORE_FREEZE, + j); + if (positiveBalances(amount, amount2, *lptAMMBalance_, false) && + ammLPTokens(amount, amount2, lptAMMBalance_->issue()) == + *lptAMMBalance_) + return true; + + JLOG(j.error()) << "AMMCreate invariant failed: " << amount << " " + << amount2 << " " << *lptAMMBalance_; + return false; + } + else if ( + txType == ttPAYMENT || txType == ttOFFER_CREATE || + txType == ttCHECK_CASH) + { + for (auto const& [ammAccount, assets] : balanceBefore_) + { + if (auto const root = _view.read(keylet::account(ammAccount)); + root && root->isFieldPresent(sfAMMID)) + { + auto const ammID = (*root)[sfAMMID]; + auto const amm = _view.read(keylet::amm(ammID)); + if (!amm) + { + // LCOV_EXCL_START + JLOG(j.error()) + << "AMM swap invariant failed: invalid AMM " + << to_string(ammID); + return false; + // LCOV_EXCL_STOP + } + auto it = assets.cbegin(); + auto const& asset = it->first; + auto const& asset2 = + (++it) != assets.end() ? it->first : noIssue(); + // AMM must have two balances - one for each pool side + if (assets.size() != 2 || + !balanceAfter_.contains(ammAccount) || + !balanceAfter_[ammAccount].contains(asset) || + !balanceAfter_[ammAccount].contains(asset2)) + { + // LCOV_EXCL_START + JLOG(j.error()) + << "AMM swap invariant failed: inconsistent assets " + << assets.size() << to_string(ammAccount) << " " + << asset << " " << asset2; + return false; + // LCOV_EXCL_STOP + } + + Number const productBefore = + balanceBefore_[ammAccount][asset] * + balanceBefore_[ammAccount][asset2]; + auto const& amount = balanceAfter_[ammAccount][asset]; + auto const& amount2 = balanceAfter_[ammAccount][asset2]; + Number const productAfter = amount * amount2; + auto const& lptAMMBalance = + amm->getFieldAmount(sfLPTokenBalance); + if (!positiveBalances( + amount, amount2, lptAMMBalance, false) || + !(productAfter >= productBefore || + withinRelativeDistance( + productBefore, productAfter, Number{1, -7}))) + { + JLOG(j.error()) + << "AMM swap invariant failed: old balances " + << balanceBefore_[ammAccount][asset].getText() + << " " + << balanceBefore_[ammAccount][asset2].getText() + << " new balances " + << balanceAfter_[ammAccount][asset].getText() << " " + << balanceAfter_[ammAccount][asset2].getText() + << " lptAMMBalance " << lptAMMBalance.getText() + << " tfee " << (*amm)[sfTradingFee] << " diff " + << (productBefore != Number{0} + ? to_string( + (productBefore - productAfter) / + productBefore) + : "undefined") << std::endl; + return false; + } + } + balanceAfter_.erase(ammAccount); + } + if (!balanceAfter_.empty()) + { + // LCOV_EXCL_START + JLOG(j.error()) + << "AMM swap invariant failed: inconsistent assets keys"; + return false; + // LCOV_EXCL_STOP + } + } + } + + return true; +} + } // namespace ripple diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 23ec8005556..15d0a29c021 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -475,6 +475,42 @@ class ValidMPTIssuance beast::Journal const&); }; +class ValidAMM +{ + // deposit/withdraw params + // AMM account + std::optional ammAccount_; + // LPToken balance after + std::optional lptAMMBalance_; + // swap params + // We don't know which side of the trustline is AMM. + // Consequently, we keep highLimit.issuer: and + // lowLimit.issuer: . + // finalize() figures out which account is AMM by checking the root account + // object. + hash_map> balanceBefore_; + hash_map> balanceAfter_; + +public: + ValidAMM() + { + return; + } + void + visitEntry( + bool, + std::shared_ptr const&, + std::shared_ptr const&); + + bool + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); +}; + // additional invariant checks can be declared above and then added to this // tuple using InvariantChecks = std::tuple< @@ -491,7 +527,8 @@ using InvariantChecks = std::tuple< ValidNFTokenPage, NFTokenCountTracking, ValidClawback, - ValidMPTIssuance>; + ValidMPTIssuance, + ValidAMM>; /** * @brief get a tuple of all invariant checks diff --git a/src/xrpld/app/tx/detail/Offer.h b/src/xrpld/app/tx/detail/Offer.h index a6f707ba561..8f091335d2a 100644 --- a/src/xrpld/app/tx/detail/Offer.h +++ b/src/xrpld/app/tx/detail/Offer.h @@ -21,6 +21,7 @@ #define RIPPLE_APP_BOOK_OFFER_H_INCLUDED #include +#include #include #include #include @@ -169,8 +170,24 @@ class TOffer : private TOfferBase * always returns true. */ bool - checkInvariant(TAmounts const&, beast::Journal j) const + checkInvariant(TAmounts const& consumed, beast::Journal j) const { + if (!isFeatureEnabled(fixAMMv1_3)) + return true; + + if (consumed.in > m_amounts.in || consumed.out > m_amounts.out) + { + // LCOV_EXCL_START + JLOG(j.error()) + << "AMMOffer::checkInvariant failed: consumed " + << to_string(consumed.in) << " " << to_string(consumed.out) + << " amounts " << to_string(m_amounts.in) << " " + << to_string(m_amounts.out); + + return false; + // LCOV_EXCL_STOP + } + return true; } };