diff --git a/src/ripple/app/misc/AMMUtils.h b/src/ripple/app/misc/AMMUtils.h index c25503ceb..9cd4b7d6f 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 33669dcdc..5ad59ea1c 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 cf4da2c5d..4f6f0fbd3 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 091ab9f3f..6aec801ae 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -532,7 +532,7 @@ public: // 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 839c3b433..c8d0d67dc 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 3f40b0c0b..e90818dfd 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -499,7 +499,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 c3c43d493..ed5f8a31d 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 @@ private: 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 @@ private: 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 @@ private: 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 @@ private: } } + 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 @@ private: 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 b9dea7d1f..187cb3cee 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); } }