diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index c3c048f5480..337c6611784 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -24,10 +24,8 @@ #include #include #include -#include #include -#include -#include +#include #include #include #include @@ -3164,7 +3162,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT( env.balance(carol, USD) == - STAmount(USD, UINT64_C(29'499'00572620545), -11)); + STAmount(USD, UINT64_C(29'499'00572620543), -11)); if (!features[fixAMMv1_3]) BEAST_EXPECT( env.balance(bob, USD) == @@ -3172,7 +3170,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT( env.balance(bob, USD) == - STAmount(USD, UINT64_C(18'999'00572616195), -11)); + STAmount(USD, UINT64_C(18'999'00572616191), -11)); if (!features[fixAMMv1_3]) BEAST_EXPECT( env.balance(ed, USD) == @@ -3180,7 +3178,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT( env.balance(ed, USD) == - STAmount(USD, UINT64_C(18'999'00572611841), -11)); + STAmount(USD, UINT64_C(18'999'00572611839), -11)); // USD pool is slightly higher because of the fees. if (!features[fixAMMv1_3]) BEAST_EXPECT(ammAlice.expectBalances( @@ -3190,7 +3188,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'003}, - STAmount(USD, UINT64_C(13'002'98282151419), -11), + STAmount(USD, UINT64_C(13'002'98282151427), -11), ammTokens)); } ammTokens = ammAlice.getLPTokensBalance(); @@ -3240,7 +3238,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT( env.balance(dan, USD) == - STAmount(USD, UINT64_C(19'490'056722744), -9)); + STAmount(USD, UINT64_C(19'490'05672274393), -11)); // USD pool gains more in dan's fees. if (!features[fixAMMv1_3]) BEAST_EXPECT(ammAlice.expectBalances( @@ -3250,7 +3248,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'003}, - STAmount{USD, UINT64_C(13'012'92609877019), -11}, + STAmount{USD, UINT64_C(13'012'92609877034), -11}, ammTokens)); // Discounted fee payment ammAlice.deposit(carol, USD(100)); @@ -3263,7 +3261,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'003}, - STAmount{USD, UINT64_C(13'112'92609877019), -11}, + STAmount{USD, UINT64_C(13'112'92609877034), -11}, ammTokens)); env(pay(carol, bob, USD(100)), path(~USD), @@ -3279,7 +3277,7 @@ struct AMM_test : public jtx::AMMTest else BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'100'000'671}, - STAmount{USD, UINT64_C(13'012'92609877019), -11}, + STAmount{USD, UINT64_C(13'012'92609877034), -11}, ammTokens)); } // Payment with the trading fee @@ -3306,14 +3304,14 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'671}, - STAmount{USD, UINT64_C(13'114'03663044932), -11}, + STAmount{USD, UINT64_C(13'114'03663044947), -11}, ammTokens)); } // Auction slot expired, no discounted fee env.close(seconds(TOTAL_TIME_SLOT_SECS + 1)); // clock is parent's based env.close(); - if (!features[fixAMMv1_1] || features[fixAMMv1_3]) + if (!features[fixAMMv1_1]) BEAST_EXPECT( env.balance(carol, USD) == STAmount(USD, UINT64_C(29'399'00572620545), -11)); @@ -3321,6 +3319,10 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT( env.balance(carol, USD) == STAmount(USD, UINT64_C(29'399'00572620544), -11)); + else + BEAST_EXPECT( + env.balance(carol, USD) == + STAmount(USD, UINT64_C(29'399'00572620543), -11)); ammTokens = ammAlice.getLPTokensBalance(); for (int i = 0; i < 10; ++i) { @@ -3353,10 +3355,10 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT( env.balance(carol, USD) == - STAmount(USD, UINT64_C(29'389'06197177128), -11)); + STAmount(USD, UINT64_C(29'389'06197177119), -11)); BEAST_EXPECT(ammAlice.expectBalances( XRPAmount{13'000'000'671}, - STAmount{USD, UINT64_C(13'123'98038488349), -11}, + STAmount{USD, UINT64_C(13'123'98038488371), -11}, ammTokens)); } env(pay(carol, bob, USD(100)), path(~USD), sendmax(XRP(110))); @@ -3382,7 +3384,7 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT(ammAlice.expectBalances( XRPAmount(13'100'824'793), - STAmount{USD, UINT64_C(13'023'98038488349), -11}, + STAmount{USD, UINT64_C(13'023'98038488371), -11}, ammTokens)); } }, @@ -5392,7 +5394,7 @@ 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] && !features[fixAMMv1_3]) + if (!features[fixAMMv1_1] || features[fixAMMv1_3]) BEAST_EXPECT(ammAlice.expectBalances( XRP(10'000), STAmount{USD, UINT64_C(10'000'0000000013), -10}, @@ -5400,11 +5402,6 @@ struct AMM_test : public jtx::AMMTest else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) BEAST_EXPECT(ammAlice.expectBalances( XRP(10'000), USD(10'000), IOUAmount{10'000'000})); - else - BEAST_EXPECT(ammAlice.expectBalances( - XRP(10'000), - STAmount{USD, UINT64_C(10'000'000000001), -9}, - IOUAmount{10'000'000})); BEAST_EXPECT(expectLine(env, ben, USD(1'500'000))); BEAST_EXPECT(expectLine(env, simon, USD(1'500'000))); BEAST_EXPECT(expectLine(env, chris, USD(1'500'000))); @@ -5431,24 +5428,16 @@ struct AMM_test : public jtx::AMMTest nataly, STAmount{USD, UINT64_C(1'500'000'000000005), -9})); else - BEAST_EXPECT(expectLine( - env, - nataly, - STAmount{USD, UINT64_C(1'499'999'999999998), -9})); + BEAST_EXPECT(expectLine(env, nataly, USD(1'500'000))); ammAlice.withdrawAll(alice); BEAST_EXPECT(!ammAlice.ammExists()); - if (!features[fixAMMv1_1] && !features[fixAMMv1_3]) + if (!features[fixAMMv1_1] || features[fixAMMv1_3]) BEAST_EXPECT(expectLine( env, alice, STAmount{USD, UINT64_C(30'000'0000000013), -10})); - else if (features[fixAMMv1_1] && !features[fixAMMv1_3]) - BEAST_EXPECT(expectLine(env, alice, USD(30'000))); else - BEAST_EXPECT(expectLine( - env, - alice, - STAmount{USD, UINT64_C(30'000'000000001), -9})); + BEAST_EXPECT(expectLine(env, alice, USD(30'000))); // alice XRP balance is 30,000initial - 50 ammcreate fee - // 10drops fee BEAST_EXPECT(accountBalance(env, alice) == "29949999990"); @@ -5524,21 +5513,21 @@ struct AMM_test : public jtx::AMMTest // post-amendment the rounding takes place to ensure // AMM invariant BEAST_EXPECT(ammAlice.expectBalances( - XRPAmount(10'000'000'079), + 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) == "1999999999794"); - BEAST_EXPECT(accountBalance(env, dan) == "1999999999791"); + BEAST_EXPECT(accountBalance(env, chris) == "1999999999790"); + BEAST_EXPECT(accountBalance(env, dan) == "1999999999790"); BEAST_EXPECT(accountBalance(env, carol) == "29999999790"); BEAST_EXPECT(accountBalance(env, ed) == "1999999999791"); - BEAST_EXPECT(accountBalance(env, paul) == "1999999999792"); + BEAST_EXPECT(accountBalance(env, paul) == "1999999999794"); BEAST_EXPECT( - accountBalance(env, nataly) == "1999999999793"); - BEAST_EXPECT(accountBalance(env, alice) == "29950000069"); + accountBalance(env, nataly) == "1999999999795"); + BEAST_EXPECT(accountBalance(env, alice) == "29950000070"); } }, std::nullopt, @@ -7328,74 +7317,88 @@ struct AMM_test : public jtx::AMMTest } void - testDepositAndWithdrawRoundingV2(FeatureBitset features) + testDepositAndWithdrawRounding(FeatureBitset features) { - testcase("Deposit and Withddraw Rouding V2"); + testcase("Deposit and Withdraw Rounding V2"); using namespace jtx; - auto const LPT = gw["LPT"]; + + auto const XPM = gw["XPM"]; STAmount xrpBalance{XRPAmount(692'614'492'126)}; - STAmount xpmBalance{USD, UINT64_C(18'610'359'80246901), -8}; - STAmount lptAMMBalance{LPT, UINT64_C(3'234'987'266'485968), -6}; - STAmount amount{USD, UINT64_C(6'566'496939465400), -12}; + 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 = [&](bool isDeposit) { + auto test = [&](auto&& cb, std::uint16_t tfee_) { Env env(*this, features); - CurrentTransactionRulesGuard r(env.current()->rules()); - - auto const tokens = [&] { - auto const tokens = [&] { - if (isDeposit) - return lpTokensOut( - xpmBalance, amount, lptAMMBalance, tfee); - return lpTokensIn(xpmBalance, amount, lptAMMBalance, tfee); - }(); - if (!features[fixAMMv1_3]) - return tokens; - return adjustLPTokens(lptAMMBalance, tokens, isDeposit); - }(); - auto const [amountActual, amount2Actual, lpTokensActual] = - adjustAmountsByLPTokens( - xpmBalance, - amount, - std::nullopt, - lptAMMBalance, - tokens, - tfee, - isDeposit); - (void)amount2Actual; - if (!features[fixAMMv1_3]) - BEAST_EXPECT(lpTokensActual != tokens); - else - BEAST_EXPECT(lpTokensActual == tokens); - // Deposit/Withdraw amount is rounded up if amendment is disabled - if (!features[fixAMMv1_3]) - BEAST_EXPECT(amountActual > amount); - // Deposit/Withdraw amount is capped if amendment is disabled - else - BEAST_EXPECT(amountActual == amount); + 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); + } - // Deposit - test(true); - // Withdrawal - test(false); + 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() + testDepositRounding(FeatureBitset all) { testcase("Deposit Rounding"); using namespace jtx; - // single asset deposits into IOU/IOU AMM pool - // both LPTokens and asset-amounts are rounded down in AMMHelpers - // .cpp:adjustAmountsByLPTokens. The AMM Invariant is violated by ~1e-6 - for (STAmount const& depositEuro : + // Single asset deposit + for (auto const& deposit : {STAmount(EUR, 1, 1), STAmount(EUR, 1, 2), STAmount(EUR, 1, 5), - STAmount(EUR, 1, -3), + STAmount(EUR, 1, -3), // fail STAmount(EUR, 1, -6), STAmount(EUR, 1, -9)}) { @@ -7411,12 +7414,21 @@ struct AMM_test : public jtx::AMMTest env.close(); ammAlice.deposit( - DepositArg{.account = bob, .asset1In = depositEuro}); + DepositArg{.account = bob, .asset1In = deposit}); + invariant( + ammAlice, + env, + "dep1", + deposit == STAmount{EUR, 1, -3} && + !env.enabled(fixAMMv1_3)); }, - {{GBP(30'000), EUR(30'000)}}); + {{GBP(30'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); } - // two-asset proportional deposit (1:1 pool ratio) + // Two-asset proportional deposit (1:1 pool ratio) testAMM( [&](AMM& ammAlice, Env& env) { fund( @@ -7428,8 +7440,6 @@ struct AMM_test : public jtx::AMMTest Fund::Acct); env.close(); - // although the following inputs are crafted to cause rounding, - // the deposit transaction does not violate the AMM invariants const STAmount depositEuro{ EUR, UINT64_C(10'1234567890123456), -16}; const STAmount depositGBP{ @@ -7439,13 +7449,15 @@ struct AMM_test : public jtx::AMMTest .account = bob, .asset1In = depositEuro, .asset2In = depositGBP}); + invariant(ammAlice, env, "dep2", false); }, - {{GBP(30'000), EUR(30'000)}}); + {{GBP(30'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); - // two-asset proportional deposit (1:3 pool ratio) - // Since the pool assets are not in identical quantities, computation - // of resulting LPTokens could cause rounding adjustments - for (const int exponent : {1, 2, 3, 4, -3, -6, -9}) + // 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) { @@ -7465,13 +7477,19 @@ struct AMM_test : public jtx::AMMTest .account = bob, .asset1In = depositEuro, .asset2In = depositGBP}); + invariant( + ammAlice, + env, + "dep3", + exponent != -3 && !env.enabled(fixAMMv1_3)); }, - {{GBP(10'000), EUR(30'000)}}); + {{GBP(10'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); } - // tfLPToken based deposits - // Observation: adjustLPTokens does not make any changes to LPTokens - // in this deposit mode. No rounding-based violations are seen either. + // tfLPToken deposit testAMM( [&](AMM& ammAlice, Env& env) { fund( @@ -7486,14 +7504,23 @@ struct AMM_test : public jtx::AMMTest ammAlice.deposit(DepositArg{ .account = bob, .tokens = IOUAmount{10'1234567890123456, -16}}); + invariant(ammAlice, env, "dep4", false); }, - {{GBP(7'000), EUR(30'000)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); - // tfOneAssetLPToken deposits -- AMM invariant is violated - // This error isn't observed with inputs of 10, 100. - // ammAssetsIn is responsible for handling the LPTokens and amount1 - // for this deposit mode. - for (const unsigned int tokens : {1'000, 10'000}) + // 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) { @@ -7510,8 +7537,12 @@ struct AMM_test : public jtx::AMMTest .account = bob, .tokens = tokens, .asset1In = STAmount{EUR, 1, 6}}); + invariant(ammAlice, env, "dep5", false); }, - {{GBP(7'000), EUR(30'000)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); } // Single deposit with EP not exceeding specified: @@ -7529,12 +7560,16 @@ struct AMM_test : public jtx::AMMTest ammAlice.deposit( bob, GBP(1'000), std::nullopt, STAmount{GBP, 5}); + invariant(ammAlice, env, "dep6", false); }, - {{GBP(30'000), EUR(30'000)}}); + {{GBP(30'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); } void - testWithdrawRounding() + testWithdrawRounding(FeatureBitset all) { testcase("Withdraw Rounding"); @@ -7542,16 +7577,26 @@ struct AMM_test : public jtx::AMMTest // tfLPToken mode testAMM( - [&](AMM& ammAlice, Env& env) { ammAlice.withdraw(alice, 1'000); }, - {{GBP(7'000), EUR(30'000)}}); + [&](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)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); // tfTwoAsset withdraw mode testAMM( @@ -7561,21 +7606,30 @@ struct AMM_test : public jtx::AMMTest .asset1Out = STAmount{GBP, 3'500}, .asset2Out = STAmount{EUR, 15'000}, .flags = tfTwoAsset}); + invariant(ammAlice, env, "with3", false); }, - {{GBP(7'000), EUR(30'000)}}); + {{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 + // 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)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); // tfOneAssetWithdrawAll mode testAMM( @@ -7596,8 +7650,12 @@ struct AMM_test : public jtx::AMMTest .account = bob, .asset1Out = STAmount{GBP, 1'000}, .flags = tfOneAssetWithdrawAll}); + invariant(ammAlice, env, "with5", false); }, - {{GBP(7'000), EUR(30'000)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); // tfOneAssetLPToken mode testAMM( @@ -7607,8 +7665,12 @@ struct AMM_test : public jtx::AMMTest .tokens = 1'000, .asset1Out = STAmount{GBP, 100}, .flags = tfOneAssetLPToken}); + invariant(ammAlice, env, "with6", false); }, - {{GBP(7'000), EUR(30'000)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); // tfLimitLPToken mode testAMM( @@ -7618,8 +7680,12 @@ struct AMM_test : public jtx::AMMTest .asset1Out = STAmount{GBP, 100}, .maxEP = IOUAmount{2}, .flags = tfLimitLPToken}); + invariant(ammAlice, env, "with7", true); }, - {{GBP(7'000), EUR(30'000)}}); + {{GBP(7'000), EUR(30'000)}}, + 0, + std::nullopt, + {all}); } void @@ -7682,10 +7748,12 @@ struct AMM_test : public jtx::AMMTest testAMMDepositWithFrozenAssets(all - featureAMMClawback); testAMMDepositWithFrozenAssets( all - fixAMMv1_1 - fixAMMv1_3 - featureAMMClawback); - testDepositAndWithdrawRoundingV2(all); - testDepositAndWithdrawRoundingV2(all - fixAMMv1_3); - testDepositRounding(); - testWithdrawRounding(); + testDepositAndWithdrawRounding(all); + testDepositAndWithdrawRounding(all - fixAMMv1_3); + testDepositRounding(all); + testDepositRounding(all - fixAMMv1_3); + testWithdrawRounding(all); + testWithdrawRounding(all - fixAMMv1_3); } }; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index ffe688fbdaf..292424d0664 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -1206,7 +1206,7 @@ ValidAMM::finalize( if (res >= *lptAMMBalance_ || (*lptAMMBalance_ != beast::zero && withinRelativeDistance( - res, Number{*lptAMMBalance_}, Number{1, -7}))) + res, Number{*lptAMMBalance_}, Number{1, -11}))) { if (*lptAMMBalance_ > beast::zero) return true; @@ -1299,7 +1299,7 @@ ValidAMM::finalize( balanceAfter_[k][asset] * balanceAfter_[k][asset2]; if (productAfter >= productBefore || withinRelativeDistance( - productBefore, productAfter, Number{1, -7})) + productBefore, productAfter, Number{1, -11})) return true; JLOG(j.error())