From 4fc080a14ee47dd9476f0d615dc67e407ea32e3d Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 23 Dec 2024 17:35:26 -0500 Subject: [PATCH] Delete AMM MPToken on AMM delete. Update for MPT isOnlyLiquidityProvider(). Update checkReserve() in AMMWithdraw. Update invariant for AMMWithdraw. Add deleteMPToken to View. --- include/xrpl/protocol/Indexes.h | 2 +- src/libxrpl/protocol/Indexes.cpp | 4 +- src/test/app/MPToken_test.cpp | 48 +++++++++++++ src/test/rpc/AMMInfo_test.cpp | 2 +- src/xrpld/app/misc/detail/AMMUtils.cpp | 60 ++++++----------- src/xrpld/app/tx/detail/AMMCreate.cpp | 2 + src/xrpld/app/tx/detail/AMMWithdraw.cpp | 78 +++++++++++----------- src/xrpld/app/tx/detail/InvariantCheck.cpp | 30 +++++---- src/xrpld/app/tx/detail/Transactor.cpp | 1 + src/xrpld/ledger/View.h | 10 +++ src/xrpld/ledger/detail/View.cpp | 19 ++++++ src/xrpld/rpc/handlers/AMMInfo.cpp | 6 +- 12 files changed, 165 insertions(+), 97 deletions(-) diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h index 3ce6ef8e836..e0bddd023ca 100644 --- a/include/xrpl/protocol/Indexes.h +++ b/include/xrpl/protocol/Indexes.h @@ -310,7 +310,7 @@ Keylet mptIssuance(std::uint32_t seq, AccountID const& issuer) noexcept; Keylet -mptIssuance(MPTID const& issuanceID) noexcept; +mptIssuance(MPTIssue const& mptIssue) noexcept; inline Keylet mptIssuance(uint256 const& issuanceKey) diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index 596f8314093..499e6c354d9 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -549,11 +549,11 @@ mptIssuance(std::uint32_t seq, AccountID const& issuer) noexcept } Keylet -mptIssuance(MPTID const& issuanceID) noexcept +mptIssuance(MPTIssue const& mptIssue) noexcept { return { ltMPTOKEN_ISSUANCE, - indexHash(LedgerNameSpace::MPTOKEN_ISSUANCE, issuanceID)}; + indexHash(LedgerNameSpace::MPTOKEN_ISSUANCE, mptIssue.getMptID())}; } Keylet diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 423595ed18d..b1a906dc13a 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -3583,6 +3583,51 @@ class MPToken_test : public beast::unit_test::suite } } + void + testBasicAMM(FeatureBitset features) + { + testcase("Basic AMM"); + using namespace jtx; + Account const gw{"gw"}; + Account const alice{"alice"}; + Account const carol{"carol"}; + auto const USD = gw["USD"]; + Env env{*this}; + + fund(env, gw, {alice, carol}, XRP(1'000), {USD(1'000)}); + + MPTTester mpt(env, gw, {.fund = false}); + mpt.create({.flags = tfMPTCanTransfer | tfMPTCanTrade}); + auto const MPT = mpt["MPT"]; + mpt.authorize({.account = alice}); + mpt.authorize({.account = carol}); + mpt.pay(gw, alice, 1'000); + mpt.pay(gw, carol, 1'000); + + MPTTester mpt1(env, gw, {.fund = false}); + mpt1.create({.flags = tfMPTCanTransfer | tfMPTCanTrade}); + auto const MPT1 = mpt1["MPT1"]; + mpt1.authorize({.account = alice}); + mpt1.authorize({.account = carol}); + mpt1.pay(gw, alice, 1'000); + mpt1.pay(gw, carol, 1'000); + + std::vector> pools = { + {XRP(100), MPT(100), IOUAmount{100'000}}, + {USD(100), MPT(100), IOUAmount{100}}, + {MPT(100), MPT1(100), IOUAmount{100}}}; + for (auto& pool : pools) + { + AMM amm(env, gw, std::get<0>(pool), std::get<1>(pool)); + amm.deposit(alice, std::get<2>(pool)); + amm.deposit(carol, std::get<2>(pool)); + amm.withdrawAll(alice); + amm.withdrawAll(carol); + amm.withdrawAll(gw); + BEAST_EXPECT(!amm.ammExists()); + } + } + public: void run() override @@ -3647,6 +3692,9 @@ class MPToken_test : public beast::unit_test::suite // Add AMMClawback testAMMClawback(all); + + // Test AMM + testBasicAMM(all); } }; diff --git a/src/test/rpc/AMMInfo_test.cpp b/src/test/rpc/AMMInfo_test.cpp index fd72de804d5..f03bf18434e 100644 --- a/src/test/rpc/AMMInfo_test.cpp +++ b/src/test/rpc/AMMInfo_test.cpp @@ -210,7 +210,7 @@ class AMMInfo_test : public jtx::AMMTestBase auto const MPT1 = mpt1["MPT"]; std::vector> pools = { - {XRP(100), MPT(100), IOUAmount{100000}}, + {XRP(100), MPT(100), IOUAmount{100'000}}, {USD(100), MPT(100), IOUAmount{100}}, {MPT(100), MPT1(100), IOUAmount{100}}}; for (auto& pool : pools) diff --git a/src/xrpld/app/misc/detail/AMMUtils.cpp b/src/xrpld/app/misc/detail/AMMUtils.cpp index 6971d1f806c..90603735fd2 100644 --- a/src/xrpld/app/misc/detail/AMMUtils.cpp +++ b/src/xrpld/app/misc/detail/AMMUtils.cpp @@ -218,7 +218,7 @@ ammAccountHolds( } static TER -deleteAMMTrustLines( +deleteAMMObjects( Sandbox& sb, AccountID const& ammAccountID, std::uint16_t maxTrustlinesToDelete, @@ -233,7 +233,13 @@ deleteAMMTrustLines( // Skip AMM if (nodeType == LedgerEntryType::ltAMM) return {tesSUCCESS, SkipEntry::Yes}; - // Should only have the trustlines + + if (nodeType == ltMPTOKEN) + return { + deleteAMMMPToken(sb, sleItem, ammAccountID, j), + SkipEntry::No}; + + // Only the trustlines should remain if (nodeType != LedgerEntryType::ltRIPPLE_STATE) { // LCOV_EXCL_START @@ -292,41 +298,10 @@ deleteAMMAccount( } if (auto const ter = - deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j); + deleteAMMObjects(sb, ammAccountID, maxDeletableAMMTrustLines, j); ter != tesSUCCESS) return ter; - auto checkDeleteMPToken = [&](Asset const& asset_) -> TER { - if (asset_.holds()) - { - auto const mptIssuanceID = - keylet::mptIssuance(asset_.get().getMptID()); - auto const mptokenKey = - keylet::mptoken(mptIssuanceID.key, ammAccountID); - - auto const sleMpt = sb.peek(mptokenKey); - if (!sleMpt) - return tecINTERNAL; - - if (!sb.dirRemove( - keylet::ownerDir(ammAccountID), - (*sleMpt)[sfOwnerNode], - sleMpt->key(), - false)) - return tecINTERNAL; - - sb.erase(sleMpt); - } - - return tesSUCCESS; - }; - - if (auto const err = checkDeleteMPToken(asset)) - return err; - - if (auto const err = checkDeleteMPToken(asset2)) - return err; - auto const ownerDirKeylet = keylet::ownerDir(ammAccountID); if (!sb.dirRemove( ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false)) @@ -411,6 +386,9 @@ isOnlyLiquidityProvider( // 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 are at most two MPT objects, one for each side of the pool. + // TODO MPT, check logic with MPT introduction + std::uint8_t nMPT = 0; // There is only one AMM object bool hasAMM = false; // AMM LP has at most three trustlines and only one AMM object must exist. @@ -432,15 +410,21 @@ isOnlyLiquidityProvider( auto const sle = view.read(keylet::child(key)); if (!sle) return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + auto const entryType = sle->getFieldU16(sfLedgerEntryType); // Only one AMM object - if (sle->getFieldU16(sfLedgerEntryType) == ltAMM) + if (entryType == ltAMM) { if (hasAMM) return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE hasAMM = true; continue; } - if (sle->getFieldU16(sfLedgerEntryType) != ltRIPPLE_STATE) + if (entryType == ltMPTOKEN) + { + ++nMPT; + continue; + } + if (entryType != ltRIPPLE_STATE) return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE auto const lowLimit = sle->getFieldAmount(sfLowLimit); auto const highLimit = sle->getFieldAmount(sfHighLimit); @@ -470,8 +454,8 @@ isOnlyLiquidityProvider( auto const uNodeNext = ownerDir->getFieldU64(sfIndexNext); if (uNodeNext == 0) { - if (nLPTokenTrustLines != 1 || nIOUTrustLines == 0 || - nIOUTrustLines > 2) + if (nLPTokenTrustLines != 1 || (nIOUTrustLines == 0 && nMPT == 0) || + (nIOUTrustLines > 2 || nMPT > 2)) return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE return true; } diff --git a/src/xrpld/app/tx/detail/AMMCreate.cpp b/src/xrpld/app/tx/detail/AMMCreate.cpp index 1b37836cc7d..34ca7187013 100644 --- a/src/xrpld/app/tx/detail/AMMCreate.cpp +++ b/src/xrpld/app/tx/detail/AMMCreate.cpp @@ -368,6 +368,8 @@ applyCreate( (*mptoken)[sfFlags] = 0; (*mptoken)[sfOwnerNode] = *ownerNode; sb.insert(mptoken); + // TODO MPT we don't adjust the owner count? + // didn't adjust for AMM object } if (auto const res = accountSend( diff --git a/src/xrpld/app/tx/detail/AMMWithdraw.cpp b/src/xrpld/app/tx/detail/AMMWithdraw.cpp index de9a45655d4..c6a3a967885 100644 --- a/src/xrpld/app/tx/detail/AMMWithdraw.cpp +++ b/src/xrpld/app/tx/detail/AMMWithdraw.cpp @@ -521,7 +521,7 @@ AMMWithdraw::withdraw( auto const expected = ammHolds( view, ammSle, - amountWithdraw.issue(), + amountWithdraw.asset(), std::nullopt, freezeHandling, AuthHandling::ahIGNORE_AUTH, // ??? @@ -615,17 +615,32 @@ AMMWithdraw::withdraw( return {tecAMM_BALANCE, STAmount{}, STAmount{}, STAmount{}}; } - // Check the reserve in case a trustline has to be created + // Check the reserve in case a trustline or MPT has to be created bool const enabledFixAMMv1_2 = view.rules().enabled(fixAMMv1_2); - auto sufficientReserve = [&](Issue const& issue) -> TER { - if (!enabledFixAMMv1_2 || isXRP(issue)) + // If seated after a call to sufficientReserve() then MPToken must be + // authorized + std::optional mptokenKey; + auto sufficientReserve = [&](Asset const& asset) -> TER { + mptokenKey = std::nullopt; + if (!enabledFixAMMv1_2 || isXRP(asset)) return tesSUCCESS; - if (!view.exists(keylet::line(account, issue))) + bool const isIssue = asset.holds(); + bool const checkReserve = [&] { + if (isIssue) + return !view.exists(keylet::line(account, asset.get())); + auto const issuanceKey = keylet::mptIssuance(asset.get()); + mptokenKey = keylet::mptoken(issuanceKey.key, account); + if (!view.exists(keylet::mptoken(issuanceKey.key, account))) + return true; + mptokenKey = std::nullopt; + return false; + }(); + if (checkReserve) { - auto const sleAccount = view.read(keylet::account(account)); + auto sleAccount = view.peek(keylet::account(account)); if (!sleAccount) return tecINTERNAL; // LCOV_EXCL_LINE - auto const balance = (*sleAccount)[sfBalance].xrp(); + STAmount const balance = (*sleAccount)[sfBalance]; std::uint32_t const ownerCount = sleAccount->at(sfOwnerCount); // See also SetTrust::doApply() @@ -633,62 +648,47 @@ AMMWithdraw::withdraw( (ownerCount < 2) ? XRPAmount(beast::zero) : view.fees().accountReserve(ownerCount + 1)); - if (std::max(priorBalance, balance) < reserve) + auto const balance_ = + isIssue ? std::max(priorBalance, balance.xrp()) : balance.xrp(); + if (balance_ < reserve) return tecINSUFFICIENT_RESERVE; + + // Update owner count. + if (!isIssue) + adjustOwnerCount(view, sleAccount, 1, journal); } return tesSUCCESS; }; - if (auto const err = sufficientReserve(amountWithdrawActual.issue())) + if (auto const err = sufficientReserve(amountWithdrawActual.asset())) return {err, STAmount{}, STAmount{}, STAmount{}}; // Create MPToken if doesn't exist // TODO make a library, AMMCreate, AMMAuthorize use almost identical code auto createMPToken = [&](Asset const& asset) -> TER { - if (asset.holds()) + // If mptoken is seated then must authorize + if (mptokenKey) { auto const& mptIssue = asset.get(); - auto const issuanceKey = keylet::mptIssuance(mptIssue.getMptID()); - auto const mptokenKey = keylet::mptoken(issuanceKey.key, account); - if (!view.exists(mptokenKey)) - { - if (auto err = requireAuth( - view, mptIssue, account, MPTAuthType::WeakAuth); - err != tesSUCCESS) - return err; - } - - auto const sleAcct = view.peek(keylet::account(account)); - if (!sleAcct) - return tefINTERNAL; - - std::uint32_t const uOwnerCount = - sleAcct->getFieldU32(sfOwnerCount); - XRPAmount const reserveCreate( - (uOwnerCount < 2) - ? XRPAmount(beast::zero) - : view.fees().accountReserve(uOwnerCount + 1)); - - if (priorBalance < reserveCreate) - return tecINSUFFICIENT_RESERVE; + if (auto err = + requireAuth(view, mptIssue, account, MPTAuthType::WeakAuth); + err != tesSUCCESS) + return err; auto const ownerNode = view.dirInsert( keylet::ownerDir(account), - mptokenKey, + *mptokenKey, describeOwnerDir(account)); if (!ownerNode) return tecDIR_FULL; - auto mptoken = std::make_shared(mptokenKey); + auto mptoken = std::make_shared(*mptokenKey); (*mptoken)[sfAccount] = account; (*mptoken)[sfMPTokenIssuanceID] = mptIssue.getMptID(); (*mptoken)[sfFlags] = 0; (*mptoken)[sfOwnerNode] = *ownerNode; view.insert(mptoken); - - // Update owner count. - adjustOwnerCount(view, sleAcct, 1, journal); } return tesSUCCESS; }; @@ -717,7 +717,7 @@ AMMWithdraw::withdraw( // Withdraw amount2Withdraw if (amount2WithdrawActual) { - if (auto const err = sufficientReserve(amount2WithdrawActual->issue()); + if (auto const err = sufficientReserve(amount2WithdrawActual->asset()); err != tesSUCCESS) return {err, STAmount{}, STAmount{}, STAmount{}}; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 6f971286f8d..c1d7a38853f 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -1007,7 +1007,8 @@ ValidMPTIssuance::finalize( { if (result == tesSUCCESS) { - if (tx.getTxnType() == ttMPTOKEN_ISSUANCE_CREATE) + auto const txnType = tx.getTxnType(); + if (txnType == ttMPTOKEN_ISSUANCE_CREATE) { if (mptIssuancesCreated_ == 0) { @@ -1028,7 +1029,7 @@ ValidMPTIssuance::finalize( return mptIssuancesCreated_ == 1 && mptIssuancesDeleted_ == 0; } - if (tx.getTxnType() == ttMPTOKEN_ISSUANCE_DESTROY) + if (txnType == ttMPTOKEN_ISSUANCE_DESTROY) { if (mptIssuancesDeleted_ == 0) { @@ -1049,7 +1050,7 @@ ValidMPTIssuance::finalize( return mptIssuancesCreated_ == 0 && mptIssuancesDeleted_ == 1; } - if (tx.getTxnType() == ttMPTOKEN_AUTHORIZE) + if (txnType == ttMPTOKEN_AUTHORIZE) { bool const submittedByIssuer = tx.isFieldPresent(sfHolder); @@ -1089,7 +1090,7 @@ ValidMPTIssuance::finalize( return true; } - if (tx.getTxnType() == ttMPTOKEN_ISSUANCE_SET) + if (txnType == ttMPTOKEN_ISSUANCE_SET) { if (mptIssuancesDeleted_ > 0) { @@ -1116,7 +1117,7 @@ ValidMPTIssuance::finalize( mptokensCreated_ == 0 && mptokensDeleted_ == 0; } - if (tx.getTxnType() == ttAMM_CREATE || tx.getTxnType() == ttCHECK_CASH) + if (txnType == ttAMM_CREATE || txnType == ttCHECK_CASH) { if (mptIssuancesDeleted_ > 0) { @@ -1135,21 +1136,20 @@ ValidMPTIssuance::finalize( } // AMM can be created with IOU/MPT or MPT/MPT else if ( - (tx.getTxnType() == ttAMM_CREATE && mptokensCreated_ > 2) || - (tx.getTxnType() == ttCHECK_CASH && mptokensCreated_ > 1)) + (txnType == ttAMM_CREATE && mptokensCreated_ > 2) || + (txnType == ttCHECK_CASH && mptokensCreated_ > 1)) { JLOG(j.fatal()) << "Invariant failed: MPT issuance set " "succeeded while creating MPTokens"; } return mptIssuancesCreated_ == 0 && mptIssuancesDeleted_ == 0 && - ((tx.getTxnType() == ttAMM_CREATE && mptokensCreated_ <= 2) || - (tx.getTxnType() == ttCHECK_CASH && mptokensCreated_ <= 1)) && + ((txnType == ttAMM_CREATE && mptokensCreated_ <= 2) || + (txnType == ttCHECK_CASH && mptokensCreated_ <= 1)) && mptokensDeleted_ == 0; } - if (tx.getTxnType() == ttAMM_DELETE || - tx.getTxnType() == ttAMM_WITHDRAW) + if (txnType == ttAMM_DELETE || txnType == ttAMM_WITHDRAW) { if (mptIssuancesDeleted_ > 0) { @@ -1166,14 +1166,18 @@ ValidMPTIssuance::finalize( JLOG(j.fatal()) << "Invariant failed: MPT issuance set " "succeeded while removing MPTokens"; } - else if (mptokensCreated_ > 0) + // MPToken can be created if LP withdraws from MPT pool, + // and doesn't own MPToken object for this MPT + else if (txnType == ttAMM_WITHDRAW && mptokensCreated_ > 1) { JLOG(j.fatal()) << "Invariant failed: MPT issuance set " "succeeded while creating MPTokens"; } return mptIssuancesCreated_ == 0 && mptIssuancesDeleted_ == 0 && - mptokensCreated_ == 0 && mptokensDeleted_ <= 2; + ((txnType == ttAMM_DELETE && mptokensCreated_ == 0) || + (txnType == ttAMM_WITHDRAW && mptokensCreated_ <= 1)) && + mptokensDeleted_ <= 2; } } diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index df3d32f8a58..d97cdf411bf 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -1008,6 +1008,7 @@ Transactor::operator()() removeExpiredNFTokenOffers( view(), expiredNFTokenOffers, ctx_.app.journal("View")); + // TODO MPT? if (result == tecINCOMPLETE) removeDeletedTrustLines( view(), removedTrustLines, ctx_.app.journal("View")); diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index 4d0fc7798b5..d22e74a1405 100644 --- a/src/xrpld/ledger/View.h +++ b/src/xrpld/ledger/View.h @@ -626,6 +626,16 @@ deleteAMMTrustLine( std::optional const& ammAccountID, beast::Journal j); +/** Delete AMMs MPToken. The passed `sle` must be obtained from a prior + * call to view.peek(). + */ +[[nodiscard]] TER +deleteAMMMPToken( + ApplyView& view, + std::shared_ptr sleMPT, + AccountID const& ammAccountID, + beast::Journal j); + } // namespace ripple #endif diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 054c6b0bd15..ffb1dadf255 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -2048,6 +2048,25 @@ deleteAMMTrustLine( return tesSUCCESS; } +TER +deleteAMMMPToken( + ApplyView& view, + std::shared_ptr sleMpt, + AccountID const& ammAccountID, + beast::Journal j) +{ + if (!view.dirRemove( + keylet::ownerDir(ammAccountID), + (*sleMpt)[sfOwnerNode], + sleMpt->key(), + false)) + return tecINTERNAL; + + view.erase(sleMpt); + + return tesSUCCESS; +} + TER rippleCredit( ApplyView& view, diff --git a/src/xrpld/rpc/handlers/AMMInfo.cpp b/src/xrpld/rpc/handlers/AMMInfo.cpp index 384aad4616a..44bbd3a6676 100644 --- a/src/xrpld/rpc/handlers/AMMInfo.cpp +++ b/src/xrpld/rpc/handlers/AMMInfo.cpp @@ -148,9 +148,9 @@ doAMMInfo(RPC::JsonContext& context) return Unexpected(rpcINVALID_PARAMS); XRPL_ASSERT( - (issue1.has_value() == issue2.has_value()) && - (issue1.has_value() != ammID.has_value()), - "ripple::doAMMInfo : issue1 and issue2 do match"); + (asset1.has_value() == asset2.has_value()) && + (asset1.has_value() != ammID.has_value()), + "ripple::doAMMInfo : asset1 and asset2 do match"); auto const ammKeylet = [&]() { if (asset1 && asset2)