From ec4a908a4e7e8a6660e46ca77a4bfcbdcb45938c Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 12 Nov 2024 08:02:59 -0500 Subject: [PATCH] Add more adjustment when amount increases while tokens decrease --- src/test/app/AMM_test.cpp | 14 +++--- src/xrpld/app/misc/AMMHelpers.h | 4 +- src/xrpld/app/misc/detail/AMMHelpers.cpp | 42 +++++++++++++++-- src/xrpld/app/tx/detail/AMMDeposit.cpp | 46 ++++++++++++------- src/xrpld/app/tx/detail/AMMWithdraw.cpp | 41 +++++++++++------ src/xrpld/app/tx/detail/InvariantCheck.cpp | 53 +++++++++++++--------- 6 files changed, 136 insertions(+), 64 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 2a40e7493a1..c3c048f5480 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -7392,12 +7392,12 @@ struct AMM_test : public jtx::AMMTest // both LPTokens and asset-amounts are rounded down in AMMHelpers // .cpp:adjustAmountsByLPTokens. The AMM Invariant is violated by ~1e-6 for (STAmount const& depositEuro : - {/*STAmount(EUR, 1, 1), + {STAmount(EUR, 1, 1), STAmount(EUR, 1, 2), - STAmount(EUR, 1, 5),*/ + STAmount(EUR, 1, 5), STAmount(EUR, 1, -3), - /*STAmount(EUR, 1, -6), - STAmount(EUR, 1, -9)*/}) + STAmount(EUR, 1, -6), + STAmount(EUR, 1, -9)}) { testAMM( [&](AMM& ammAlice, Env& env) { @@ -7415,7 +7415,7 @@ struct AMM_test : public jtx::AMMTest }, {{GBP(30'000), EUR(30'000)}}); } - return; + // two-asset proportional deposit (1:1 pool ratio) testAMM( [&](AMM& ammAlice, Env& env) { @@ -7684,8 +7684,8 @@ struct AMM_test : public jtx::AMMTest all - fixAMMv1_1 - fixAMMv1_3 - featureAMMClawback); testDepositAndWithdrawRoundingV2(all); testDepositAndWithdrawRoundingV2(all - fixAMMv1_3); - // testDepositRounding(); - // testWithdrawRounding(); + testDepositRounding(); + testWithdrawRounding(); } }; diff --git a/src/xrpld/app/misc/AMMHelpers.h b/src/xrpld/app/misc/AMMHelpers.h index f6899dec898..f291a48d999 100644 --- a/src/xrpld/app/misc/AMMHelpers.h +++ b/src/xrpld/app/misc/AMMHelpers.h @@ -710,10 +710,10 @@ getRoundedLPTokens( std::function&& noRoundCb, STAmount const& lptAMMBalance, std::function&& productCb, - bool product, + bool productOnly, bool isDeposit); -STAmount +std::pair adjustAssetInByTokens( Rules const& rules, STAmount const& balance, diff --git a/src/xrpld/app/misc/detail/AMMHelpers.cpp b/src/xrpld/app/misc/detail/AMMHelpers.cpp index f208899e796..4a1eeb3bd42 100644 --- a/src/xrpld/app/misc/detail/AMMHelpers.cpp +++ b/src/xrpld/app/misc/detail/AMMHelpers.cpp @@ -347,7 +347,7 @@ getRoundedLPTokens( return adjustLPTokens(lptAMMBalance, tokens, isDeposit); } -STAmount +std::pair adjustAssetInByTokens( Rules const& rules, STAmount const& balance, @@ -357,8 +357,26 @@ adjustAssetInByTokens( std::uint16_t tfee) { if (!rules.enabled(fixAMMv1_3)) - return amount; - return std::min(amount, ammAssetIn(balance, lptAMMBalance, tokens, tfee)); + 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 = [&] { + NumberRoundModeGuard g(Number::getround()); + Number::setround(Number::downward); + auto const diff = adjAsset - amount; + Number::setround(Number::upward); + return amount - diff; + }(); + auto const t = lpTokensOut(balance, adjAmount, lptAMMBalance, tfee); + adjTokens = adjustLPTokens(lptAMMBalance, t, /*isDeposit*/ true); + adjAsset = ammAssetIn(balance, lptAMMBalance, tokens, tfee); + } + return {adjTokens, std::min(amount, adjAsset)}; } STAmount @@ -372,6 +390,24 @@ adjustAssetOutByTokens( { if (!rules.enabled(fixAMMv1_3)) return 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 = [&] { + NumberRoundModeGuard g(Number::getround()); + Number::setround(Number::upward); + auto const diff = adjAsset - amount; + Number::setround(Number::downward); + return amount - diff; + }(); + auto const t = lpTokensIn(balance, adjAmount, lptAMMBalance, tfee); + adjTokens = adjustLPTokens(lptAMMBalance, t, /*isDeposit*/ false); + adjAsset = ammAssetOut(balance, lptAMMBalance, tokens, tfee); + } return std::min(amount, ammAssetOut(balance, lptAMMBalance, tokens, tfee)); } diff --git a/src/xrpld/app/tx/detail/AMMDeposit.cpp b/src/xrpld/app/tx/detail/AMMDeposit.cpp index 633df42f70b..564bfe94117 100644 --- a/src/xrpld/app/tx/detail/AMMDeposit.cpp +++ b/src/xrpld/app/tx/detail/AMMDeposit.cpp @@ -658,10 +658,10 @@ AMMDeposit::equalDepositTokens( adjustLPTokensOut(view.rules(), lptAMMBalance, lpTokensDeposit); auto const frac = divide(tokens, lptAMMBalance, lptAMMBalance.issue()); // amounts factor in the adjusted tokens - auto const amountDeposit = - getRoundedAsset(view.rules(), amountBalance, frac, true); - auto const amount2Deposit = - getRoundedAsset(view.rules(), amount2Balance, frac, true); + auto const amountDeposit = getRoundedAsset( + view.rules(), amountBalance, frac, /*isDeposit*/ true); + auto const amount2Deposit = getRoundedAsset( + view.rules(), amount2Balance, frac, /*isDeposit*/ true); return deposit( view, ammAccount, @@ -726,7 +726,8 @@ AMMDeposit::equalDepositLimit( std::uint16_t tfee) { auto frac = Number{amount} / amountBalance; - auto tokens = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, true); + auto tokens = getRoundedLPTokens( + view.rules(), lptAMMBalance, frac, /*isDeposit*/ true); if (tokens == beast::zero) { if (!view.rules().enabled(fixAMMv1_3)) @@ -737,7 +738,7 @@ AMMDeposit::equalDepositLimit( // factor in the adjusted tokens frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokens, frac); auto const amount2Deposit = - getRoundedAsset(view.rules(), amount2Balance, frac, true); + getRoundedAsset(view.rules(), amount2Balance, frac, /*isDeposit*/ true); if (amount2Deposit <= amount2) return deposit( view, @@ -752,7 +753,8 @@ AMMDeposit::equalDepositLimit( lpTokensDepositMin, tfee); frac = Number{amount2} / amount2Balance; - tokens = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, true); + tokens = getRoundedLPTokens( + view.rules(), lptAMMBalance, frac, /*isDeposit*/ true); if (tokens == beast::zero) { if (!view.rules().enabled(fixAMMv1_3)) @@ -763,7 +765,7 @@ AMMDeposit::equalDepositLimit( // factor in the adjusted tokens frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokens, frac); auto const amountDeposit = - getRoundedAsset(view.rules(), amountBalance, frac, true); + getRoundedAsset(view.rules(), amountBalance, frac, /*isDeposit*/ true); if (amountDeposit <= amount) return deposit( view, @@ -810,7 +812,7 @@ AMMDeposit::singleDeposit( return {tecAMM_INVALID_TOKENS, STAmount{}}; } // factor in the adjusted tokens - auto const amountDeposit = adjustAssetInByTokens( + auto const [tokensActual, amountDeposit] = adjustAssetInByTokens( view.rules(), amountBalance, amount, lptAMMBalance, tokens, tfee); return deposit( view, @@ -819,7 +821,7 @@ AMMDeposit::singleDeposit( amountDeposit, std::nullopt, lptAMMBalance, - tokens, + tokensActual, std::nullopt, std::nullopt, lpTokensDepositMin, @@ -913,9 +915,9 @@ AMMDeposit::singleDepositEPrice( return {tecAMM_INVALID_TOKENS, STAmount{}}; } // factor in the adjusted tokens - auto const amountDeposit = adjustAssetInByTokens( + auto const [tokensActual, amountDeposit] = adjustAssetInByTokens( view.rules(), amountBalance, amount, lptAMMBalance, tokens, tfee); - auto const ep = Number{amountDeposit} / tokens; + auto const ep = Number{amountDeposit} / tokensActual; if (ep <= ePrice) return deposit( view, @@ -924,7 +926,7 @@ AMMDeposit::singleDepositEPrice( amountDeposit, std::nullopt, lptAMMBalance, - tokens, + tokensActual, std::nullopt, std::nullopt, std::nullopt, @@ -960,15 +962,25 @@ AMMDeposit::singleDepositEPrice( }; auto amtProdCb = [&] { return f1 * solveQuadraticEq(a1, b1, c1); }; auto const amountDeposit = getRoundedAsset( - view.rules(), amtNoRoundCb, amountBalance, amtProdCb, false, true); + view.rules(), + amtNoRoundCb, + amountBalance, + amtProdCb, + /*productOnly*/ false, + /*isDeposit*/ true); if (amountDeposit <= beast::zero) return {tecAMM_FAILED, STAmount{}}; auto tokNoRoundCb = [&] { return amountDeposit / ePrice; }; auto tokProdCb = [&] { return amountDeposit / ePrice; }; auto const tokens = getRoundedLPTokens( - view.rules(), tokNoRoundCb, lptAMMBalance, tokProdCb, true, true); + view.rules(), + tokNoRoundCb, + lptAMMBalance, + tokProdCb, + /*productOnly*/ true, + /*isDeposit*/ true); // factor in the adjusted tokens - auto const amountDepositActual = adjustAssetInByTokens( + auto const [tokensActual, amountDepositActual] = adjustAssetInByTokens( view.rules(), amountBalance, amountDeposit, @@ -983,7 +995,7 @@ AMMDeposit::singleDepositEPrice( amountDepositActual, std::nullopt, lptAMMBalance, - tokens, + tokensActual, 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 9af741fc3b8..0e11a1b5989 100644 --- a/src/xrpld/app/tx/detail/AMMWithdraw.cpp +++ b/src/xrpld/app/tx/detail/AMMWithdraw.cpp @@ -766,10 +766,10 @@ AMMWithdraw::equalWithdrawTokens( view.rules(), lptAMMBalance, lpTokensWithdraw, withdrawAll); // the adjusted tokens are factored in auto const frac = divide(tokens, lptAMMBalance, noIssue()); - auto const withdrawAmount = - getRoundedAsset(view.rules(), amountBalance, frac, false); - auto const withdraw2Amount = - getRoundedAsset(view.rules(), amount2Balance, frac, false); + auto const withdrawAmount = getRoundedAsset( + view.rules(), amountBalance, frac, /*isDeposit*/ false); + auto const withdraw2Amount = getRoundedAsset( + view.rules(), amount2Balance, frac, /*isDeposit*/ false); // 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 @@ -840,13 +840,14 @@ AMMWithdraw::equalWithdrawLimit( std::uint16_t tfee) { auto frac = Number{amount} / amountBalance; - auto amount2Withdraw = - getRoundedAsset(view.rules(), amount2Balance, frac, false); - auto tokens = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, false); + auto amount2Withdraw = getRoundedAsset( + view.rules(), amount2Balance, frac, /*isDeposit*/ false); + auto tokens = getRoundedLPTokens( + view.rules(), lptAMMBalance, frac, /*isDeposit*/ false); // factor in the adjusted tokens frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokens, frac); - amount2Withdraw = - getRoundedAsset(view.rules(), amount2Balance, frac, false); + amount2Withdraw = getRoundedAsset( + view.rules(), amount2Balance, frac, /*isDeposit*/ false); if (amount2Withdraw <= amount2) { return withdraw( @@ -863,11 +864,13 @@ AMMWithdraw::equalWithdrawLimit( frac = Number{amount2} / amount2Balance; auto amountWithdraw = - getRoundedAsset(view.rules(), amountBalance, frac, false); - tokens = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, false); + getRoundedAsset(view.rules(), amountBalance, frac, /*isDeposit*/ false); + tokens = getRoundedLPTokens( + view.rules(), lptAMMBalance, frac, /*isDeposit*/ false); // factor in the adjusted tokens frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokens, frac); - amountWithdraw = getRoundedAsset(view.rules(), amountBalance, frac, false); + amountWithdraw = + getRoundedAsset(view.rules(), amountBalance, frac, /*isDeposit*/ false); if (!view.rules().enabled(fixAMMv1_3)) assert(amountWithdraw <= amount); else if (amountWithdraw > amount) @@ -1019,7 +1022,12 @@ AMMWithdraw::singleWithdrawEPrice( return (lptAMMBalance + ae * (f - 2)) / (lptAMMBalance * f - ae); }; auto const tokens = getRoundedLPTokens( - view.rules(), tokNoRoundCb, lptAMMBalance, tokProdCb, false, false); + view.rules(), + tokNoRoundCb, + lptAMMBalance, + tokProdCb, + /*productOnly*/ false, + /*isDeposit*/ false); if (tokens <= beast::zero) { if (!view.rules().enabled(fixAMMv1_3)) @@ -1031,7 +1039,12 @@ AMMWithdraw::singleWithdrawEPrice( auto amtProdCb = [&] { return tokens / ePrice; }; // the adjusted tokens are factored in auto const amountWithdraw = getRoundedAsset( - view.rules(), amtNoRoundCb, amount, amtProdCb, true, false); + view.rules(), + amtNoRoundCb, + amount, + amtProdCb, + /*productOnly*/ true, + /*isDeposit*/ false); if (amount == beast::zero || amountWithdraw >= amount) { return withdraw( diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 5ff69211f0e..ffe688fbdaf 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -1203,7 +1203,10 @@ ValidAMM::finalize( j); NumberRoundModeGuard g(Number::upward); auto const res = root2(amount * amount2); - if (res >= *lptAMMBalance_) + if (res >= *lptAMMBalance_ || + (*lptAMMBalance_ != beast::zero && + withinRelativeDistance( + res, Number{*lptAMMBalance_}, Number{1, -7}))) { if (*lptAMMBalance_ > beast::zero) return true; @@ -1218,7 +1221,10 @@ ValidAMM::finalize( txType == ttAMM_DEPOSIT ? "AMMDeposit" : "AMMWithdraw"; JLOG(j.error()) << txt << " invariant failed: " << amount << " " << amount2 - << " " << res << " " << lptAMMBalance_->getText(); + << " " << res << " " << lptAMMBalance_->getText() << " " + << ((*lptAMMBalance_ == beast::zero) + ? Number{1} + : ((*lptAMMBalance_ - res) / res)); return false; } else if (txType == ttAMM_CREATE) @@ -1250,6 +1256,7 @@ ValidAMM::finalize( if (auto const root = _view.read(keylet::account(k)); root && root->isFieldPresent(sfAMMID)) { + std::uint16_t tfee = 0; if (auto const amm = _view.read(keylet::amm((*root)[sfAMMID])); amm && (*amm)[sfLPTokenBalance] <= beast::zero) @@ -1262,6 +1269,8 @@ ValidAMM::finalize( return false; // LCOV_EXCL_STOP } + else + tfee = (*amm)[sfTradingFee]; // AMM must have two balances - one for each pool side if (!balanceAfter_.contains(k) || v.size() != 2) { @@ -1288,25 +1297,27 @@ ValidAMM::finalize( balanceBefore_[k][asset2]; Number const productAfter = balanceAfter_[k][asset] * balanceAfter_[k][asset2]; - if (productAfter < productBefore) - { - JLOG(j.error()) - << "AMM swap invariant failed: old balances " - << balanceBefore_[k][asset].getText() << " " - << balanceBefore_[k][asset2].getText() - << " new balances " - << balanceAfter_[k][asset].getText() << " " - << balanceAfter_[k][asset2].getText() - << " old/new product " << productBefore << " " - << productAfter << " diff " - << (productBefore != Number{0} - ? to_string( - (productBefore - productAfter) / - productBefore) - : "undefined") - << std::endl; - return false; - } + if (productAfter >= productBefore || + withinRelativeDistance( + productBefore, productAfter, Number{1, -7})) + return true; + + JLOG(j.error()) + << "AMM swap invariant failed: old balances " + << balanceBefore_[k][asset].getText() << " " + << balanceBefore_[k][asset2].getText() + << " new balances " + << balanceAfter_[k][asset].getText() << " " + << balanceAfter_[k][asset2].getText() << " tfee " + << tfee << " old/new product " << productBefore + << " " << productAfter << " diff " + << (productBefore != Number{0} + ? to_string( + (productBefore - productAfter) / + productBefore) + : "undefined") + << std::endl; + return false; } } balanceAfter_.erase(k);