From 15390bedd5bd042d54a7ab8f0a0a96adbb83431e Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sun, 5 May 2024 07:23:26 -0400 Subject: [PATCH] Fix last Liquidity Provider withdrawal: Due to the rounding, LPTokenBalance of the last Liquidity Provider (LP), might not match this LP's trustline balance. This fix sets LPTokenBalance on last LP withdrawal to this LP's LPToken trustline balance. --- src/ripple/app/misc/AMMUtils.h | 10 ++ src/ripple/app/misc/impl/AMMHelpers.cpp | 2 +- src/ripple/app/misc/impl/AMMUtils.cpp | 82 +++++++++++++++ src/ripple/app/paths/impl/BookStep.cpp | 2 +- src/ripple/app/tx/impl/AMMWithdraw.cpp | 41 +++++++- src/ripple/protocol/impl/Feature.cpp | 2 +- src/test/app/AMM_test.cpp | 127 +++++++++++++++++++++++- src/test/jtx/impl/AMMTest.cpp | 13 ++- 8 files changed, 267 insertions(+), 12 deletions(-) diff --git a/src/ripple/app/misc/AMMUtils.h b/src/ripple/app/misc/AMMUtils.h index c25503ceb9c..9cd4b7d6fec 100644 --- a/src/ripple/app/misc/AMMUtils.h +++ b/src/ripple/app/misc/AMMUtils.h @@ -113,6 +113,16 @@ initializeFeeAuctionVote( Issue const& lptIssue, std::uint16_t tfee); +/** Return true if the Liquidity Provider is the only AMM provider, false + * otherwise. Return tecINTERNAL if encountered an unexpected condition, + * for instance Liquidity Provider has more than one LPToken trustline. + */ +Expected +isOnlyLiquidityProvider( + ReadView const& view, + Issue const& ammIssue, + AccountID const& lpAccount); + } // namespace ripple #endif // RIPPLE_APP_MISC_AMMUTILS_H_INLCUDED diff --git a/src/ripple/app/misc/impl/AMMHelpers.cpp b/src/ripple/app/misc/impl/AMMHelpers.cpp index 33669dcdc1f..5ad59ea1c28 100644 --- a/src/ripple/app/misc/impl/AMMHelpers.cpp +++ b/src/ripple/app/misc/impl/AMMHelpers.cpp @@ -167,7 +167,7 @@ adjustAmountsByLPTokens( { bool const ammRoundingEnabled = [&]() { if (auto const& rules = getCurrentTransactionRules(); - rules && rules->enabled(fixAMMRounding)) + rules && rules->enabled(fixAMMv1_1)) return true; return false; }(); diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index cf4da2c5d85..4f6f0fbd3b5 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -348,4 +348,86 @@ initializeFeeAuctionVote( auctionSlot.makeFieldAbsent(sfDiscountedFee); // LCOV_EXCL_LINE } +Expected +isOnlyLiquidityProvider( + ReadView const& view, + Issue const& ammIssue, + AccountID const& lpAccount) +{ + // Liquidity Provider (LP) must have one LPToken trustline + std::uint8_t nLPTokenTrustLines = 0; + // There are at most two IOU trustlines. One or both could be to the LP + // if LP is the issuer, or a different account if LP is not an issuer. + // For instance, if AMM has two tokens USD and EUR and LP is not the issuer + // of the tokens then the trustlines are between AMM account and the issuer. + std::uint8_t nIOUTrustLines = 0; + // There is only one AMM object + bool hasAMM = false; + // AMM LP has at most three trustlines and only one AMM object must exist. + // If there are more than five objects then it's either an error or + // there are more than one LP. Ten pages should be sufficient to include + // five objects. + std::uint8_t limit = 10; + auto const root = keylet::ownerDir(ammIssue.account); + auto currentIndex = root; + + // Iterate over AMM owner directory objects. + while (limit-- >= 1) + { + auto const ownerDir = view.read(currentIndex); + if (!ownerDir) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + for (auto const& key : ownerDir->getFieldV256(sfIndexes)) + { + auto const sle = view.read(keylet::child(key)); + if (!sle) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + // Only one AMM object + if (sle->getFieldU16(sfLedgerEntryType) == ltAMM) + { + if (hasAMM) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + hasAMM = true; + continue; + } + if (sle->getFieldU16(sfLedgerEntryType) != ltRIPPLE_STATE) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + auto const lowLimit = sle->getFieldAmount(sfLowLimit); + auto const highLimit = sle->getFieldAmount(sfHighLimit); + auto const isLPTrustline = lowLimit.getIssuer() == lpAccount || + highLimit.getIssuer() == lpAccount; + auto const isLPTokenTrustline = + lowLimit.issue() == ammIssue || highLimit.issue() == ammIssue; + + // Liquidity Provider trustline + if (isLPTrustline) + { + // LPToken trustline + if (isLPTokenTrustline) + { + if (++nLPTokenTrustLines > 1) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + } + else if (++nIOUTrustLines > 2) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + } + // Another Liquidity Provider LPToken trustline + else if (isLPTokenTrustline) + return false; + else if (++nIOUTrustLines > 2) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + } + auto const uNodeNext = ownerDir->getFieldU64(sfIndexNext); + if (uNodeNext == 0) + { + if (nLPTokenTrustLines != 1 || nIOUTrustLines == 0 || + nIOUTrustLines > 2) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + return true; + } + currentIndex = keylet::page(root, uNodeNext); + } + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE +} + } // namespace ripple diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index 65dc8351680..4a43d653e0c 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -532,7 +532,7 @@ class BookOfferCrossingStep // Single path AMM offer has to factor in the transfer in rate // when calculating the upper bound quality and the quality function // because single path AMM's offer quality is not constant. - if (!rules.enabled(fixAMMRounding)) + if (!rules.enabled(fixAMMv1_1)) return ofrQ; else if ( offerType == OfferType::CLOB || diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index 839c3b433aa..c8d0d67dc97 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -310,6 +310,31 @@ AMMWithdraw::applyGuts(Sandbox& sb) auto const lpTokensWithdraw = tokensWithdraw(lpTokens, ctx_.tx[~sfLPTokenIn], ctx_.tx.getFlags()); + // Due to rounding, the LPTokenBalance of the last LP + // might not match the LP's trustline balance + if (sb.rules().enabled(fixAMMv1_1)) + { + if (auto const res = + isOnlyLiquidityProvider(sb, lpTokens.issue(), account_); + !res) + return {res.error(), false}; + else if (res.value()) + { + if (withinRelativeDistance( + lpTokens, + ammSle->getFieldAmount(sfLPTokenBalance), + Number{1, -3})) + { + ammSle->setFieldAmount(sfLPTokenBalance, lpTokens); + sb.update(ammSle); + } + else + { + return {tecAMM_INVALID_TOKENS, false}; + } + } + } + auto const tfee = getTradingFee(ctx_.view(), *ammSle, account_); auto const expected = ammHolds( @@ -467,12 +492,24 @@ AMMWithdraw::withdraw( lpTokensWithdrawActual > lpTokens) { JLOG(ctx_.journal.debug()) - << "AMM Withdraw: failed to withdraw, invalid LP tokens " - << " tokens: " << lpTokensWithdrawActual << " " << lpTokens << " " + << "AMM Withdraw: failed to withdraw, invalid LP tokens: " + << lpTokensWithdrawActual << " " << lpTokens << " " << lpTokensAMMBalance; return {tecAMM_INVALID_TOKENS, STAmount{}}; } + // Should not happen since the only LP on last withdraw + // has the balance set to the lp token trustline balance. + if (view.rules().enabled(fixAMMv1_1) && + lpTokensWithdrawActual > lpTokensAMMBalance) + { + JLOG(ctx_.journal.debug()) + << "AMM Withdraw: failed to withdraw, unexpected LP tokens: " + << lpTokensWithdrawActual << " " << lpTokens << " " + << lpTokensAMMBalance; + return {tecINTERNAL, STAmount{}}; + } + // Withdrawing one side of the pool if ((amountWithdrawActual == curBalance && amount2WithdrawActual != curBalance2) || diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 2dd6e361408..ae7a8291bb4 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -466,7 +466,7 @@ REGISTER_FEATURE(PriceOracle, Supported::yes, VoteBehavior::De REGISTER_FIX (fixEmptyDID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixXChainRewardRounding, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo); -REGISTER_FIX (fixAMMv1_1, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixAMMv1_1, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 4e477da6903..b4abd385257 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -17,6 +17,7 @@ */ //============================================================================== #include +#include #include #include #include @@ -3814,7 +3815,7 @@ struct AMM_test : public jtx::AMMTest env.close(); env(offer(carol, XRP(100), USD(55))); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // Pre-amendment the transfer fee is not taken into // account when calculating the limit out based on @@ -3865,7 +3866,7 @@ struct AMM_test : public jtx::AMMTest env.close(); env(offer(carol, XRP(10), USD(5.5))); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { BEAST_EXPECT(amm.expectBalances( XRP(990), @@ -3910,7 +3911,7 @@ struct AMM_test : public jtx::AMMTest env.close(); env(offer(carol, EUR(100), GBP(100))); env.close(); - if (!features[fixAMMRounding]) + if (!features[fixAMMv1_1]) { // After the auto-bridge offers are consumed, single path // AMM offer is generated with the limit out not taking @@ -6737,6 +6738,124 @@ struct AMM_test : public jtx::AMMTest } } + void + testLPTokenBalance(FeatureBitset features) + { + using namespace jtx; + + // Last Liquidity Provider is the issuer of one token + { + Env env(*this, features); + fund( + env, + gw, + {alice, carol}, + XRP(1'000'000'000), + {USD(1'000'000'000)}); + AMM amm(env, gw, XRP(2), USD(1)); + amm.deposit(alice, IOUAmount{1'876123487565916, -15}); + amm.deposit(carol, IOUAmount{1'000'000}); + amm.withdrawAll(alice); + amm.withdrawAll(carol); + auto const lpToken = getAccountLines( + env, gw, amm.lptIssue())[jss::lines][0u][jss::balance]; + auto const lpTokenBalance = + amm.ammRpcInfo()[jss::amm][jss::lp_token][jss::value]; + BEAST_EXPECT( + lpToken == "1414.213562373095" && + lpTokenBalance == "1414.213562373"); + if (!features[fixAMMv1_1]) + { + amm.withdrawAll(gw, std::nullopt, ter(tecAMM_BALANCE)); + BEAST_EXPECT(amm.ammExists()); + } + else + { + amm.withdrawAll(gw); + BEAST_EXPECT(!amm.ammExists()); + } + } + + // Last Liquidity Provider is the issuer of two tokens, or not + // the issuer + for (auto const& lp : {gw, bob}) + { + Env env(*this, features); + auto const ABC = gw["ABC"]; + fund( + env, + gw, + {alice, carol, bob}, + XRP(1'000), + {USD(1'000'000'000), ABC(1'000'000'000'000)}); + AMM amm(env, lp, ABC(2'000'000), USD(1)); + amm.deposit(alice, IOUAmount{1'876123487565916, -15}); + amm.deposit(carol, IOUAmount{1'000'000}); + amm.withdrawAll(alice); + amm.withdrawAll(carol); + auto const lpToken = getAccountLines( + env, lp, amm.lptIssue())[jss::lines][0u][jss::balance]; + auto const lpTokenBalance = + amm.ammRpcInfo()[jss::amm][jss::lp_token][jss::value]; + BEAST_EXPECT( + lpToken == "1414.213562373095" && + lpTokenBalance == "1414.213562373"); + if (!features[fixAMMv1_1]) + { + amm.withdrawAll(lp, std::nullopt, ter(tecAMM_BALANCE)); + BEAST_EXPECT(amm.ammExists()); + } + else + { + amm.withdrawAll(lp); + BEAST_EXPECT(!amm.ammExists()); + } + } + + // More than one Liquidity Provider + // XRP/IOU + { + Env env(*this, features); + fund(env, gw, {alice}, XRP(1'000), {USD(1'000)}); + AMM amm(env, gw, XRP(10), USD(10)); + amm.deposit(alice, 1'000); + auto res = + isOnlyLiquidityProvider(*env.current(), amm.lptIssue(), gw); + BEAST_EXPECT(res && !res.value()); + res = + isOnlyLiquidityProvider(*env.current(), amm.lptIssue(), alice); + BEAST_EXPECT(res && !res.value()); + } + // IOU/IOU, issuer of both IOU + { + Env env(*this, features); + fund(env, gw, {alice}, XRP(1'000), {USD(1'000), EUR(1'000)}); + AMM amm(env, gw, EUR(10), USD(10)); + amm.deposit(alice, 1'000); + auto res = + isOnlyLiquidityProvider(*env.current(), amm.lptIssue(), gw); + BEAST_EXPECT(res && !res.value()); + res = + isOnlyLiquidityProvider(*env.current(), amm.lptIssue(), alice); + BEAST_EXPECT(res && !res.value()); + } + // IOU/IOU, issuer of one IOU + { + Env env(*this, features); + Account const gw1("gw1"); + auto const YAN = gw1["YAN"]; + fund(env, gw, {gw1}, XRP(1'000), {USD(1'000)}); + fund(env, gw1, {gw}, XRP(1'000), {YAN(1'000)}, Fund::IOUOnly); + AMM amm(env, gw1, YAN(10), USD(10)); + amm.deposit(gw, 1'000); + auto res = + isOnlyLiquidityProvider(*env.current(), amm.lptIssue(), gw); + BEAST_EXPECT(res && !res.value()); + res = isOnlyLiquidityProvider(*env.current(), amm.lptIssue(), gw1); + BEAST_EXPECT(res && !res.value()); + } + } + void run() override { @@ -6779,6 +6898,8 @@ struct AMM_test : public jtx::AMMTest testFixChangeSpotPriceQuality(all - fixAMMv1_1); testFixAMMOfferBlockedByLOB(all); testFixAMMOfferBlockedByLOB(all - fixAMMv1_1); + testLPTokenBalance(all); + testLPTokenBalance(all - fixAMMv1_1); } }; diff --git a/src/test/jtx/impl/AMMTest.cpp b/src/test/jtx/impl/AMMTest.cpp index b9dea7d1fc3..187cb3cee05 100644 --- a/src/test/jtx/impl/AMMTest.cpp +++ b/src/test/jtx/impl/AMMTest.cpp @@ -137,10 +137,15 @@ AMMTestBase::testAMM( else if (asset2.native()) fund(env, gw, {alice, carol}, toFund2, {toFund1}, Fund::All); - AMM ammAlice(env, alice, asset1, asset2, false, tfee); - BEAST_EXPECT( - ammAlice.expectBalances(asset1, asset2, ammAlice.tokens())); - cb(ammAlice, env); + AMM ammAlice( + env, + alice, + asset1, + asset2, + CreateArg{.log = false, .tfee = tfee, .err = ter}); + if (BEAST_EXPECT( + ammAlice.expectBalances(asset1, asset2, ammAlice.tokens()))) + cb(ammAlice, env); } }