From da44bc85e785a096833dc260e4c9e89235b17d51 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Jul 2023 08:05:11 -0400 Subject: [PATCH 01/18] Cleanup AMM account owner directory on AMM account deletion --- src/ripple/app/tx/impl/AMMCreate.cpp | 4 +- src/ripple/app/tx/impl/AMMWithdraw.cpp | 76 ++++++++++++++++++++-- src/ripple/protocol/impl/LedgerFormats.cpp | 3 +- src/test/app/AMM_test.cpp | 7 ++ 4 files changed, 84 insertions(+), 6 deletions(-) diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index f3596a6f9f4..89376a4eeac 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -282,7 +282,6 @@ applyCreate( TOTAL_TIME_SLOT_SECS; auctionSlot.setFieldU32(sfExpiration, expiration); auctionSlot.setFieldAmount(sfPrice, STAmount{lpTokens.issue(), 0}); - sb.insert(ammSle); // Add owner directory to link the root account and AMM object. if (auto const page = sb.dirInsert( @@ -294,6 +293,9 @@ applyCreate( JLOG(j_.debug()) << "AMM Instance: failed to insert owner dir"; return {tecDIR_FULL, false}; } + else + ammSle->setFieldU64(sfOwnerNode, *page); + sb.insert(ammSle); // Send LPT to LP. auto res = accountSend(sb, *ammAccount, account_, lpTokens, ctx_.journal); diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index 8c583bc6170..544bf2ab35a 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -25,7 +25,6 @@ #include #include #include -#include #include #include @@ -410,9 +409,78 @@ AMMWithdraw::deleteAccount(Sandbox& sb, AccountID const& ammAccountID) if (!sleAMMRoot || !ammSle) return tecINTERNAL; - // Note, the AMM trust lines are deleted since the balance - // goes to 0. It also means there are no linked - // ledger objects. + // Delete directory entry linking the root account and AMM object + Keylet const ownerDirKeylet{keylet::ownerDir(ammAccountID)}; + if (!sb.dirRemove( + ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false)) + { + JLOG(j_.fatal()) << "AMM Withdraw: failed to remove dir link"; + return tecINTERNAL; + } + + // Delete the trustlines + std::shared_ptr sleDirNode{}; + unsigned int uDirEntry{0}; + uint256 dirEntry{beast::zero}; + if (sb.exists(ownerDirKeylet) && + dirFirst(sb, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) + { + do + { + auto sleItem = sb.peek(keylet::child(dirEntry)); + if (!sleItem) + { + // Directory node has an invalid index + JLOG(j_.fatal()) + << "AMM Withdraw: Directory node in ledger " << sb.seq() + << " has index to object that is missing: " + << to_string(dirEntry); + return tefBAD_LEDGER; + } + + // Should only have the trustlines + if (auto const nodeType = safe_cast( + sleItem->getFieldU16(sfLedgerEntryType)); + nodeType != LedgerEntryType::ltRIPPLE_STATE) + { + JLOG(j_.error()) + << "AMM Withdraw: deleting non-trustline " << nodeType; + return tecINTERNAL; + } + + // Trustlines must have zero balance + if (sleItem->getFieldAmount(sfBalance) != beast::zero) + { + JLOG(j_.error()) << "AMM Withdraw: deleting trustline with " + "non-zero balance."; + return tecINTERNAL; + } + + auto const& [lowAccountID, highAccountID] = std::minmax( + sleItem->getFieldAmount(sfHighLimit).getIssuer(), ammAccountID); + if (auto const ter = trustDelete( + sb, sleItem, lowAccountID, highAccountID, ctx_.journal); + ter != tesSUCCESS) + { + JLOG(j_.error()) + << "AMM Withdraw: failed to delete the trustline."; + return ter; + } + + // Update the iterator + assert(uDirEntry == 1); + if (uDirEntry != 1) + { + JLOG(j_.error()) + << "AMM Withdraw: iterator re-validation failed."; + return tefBAD_LEDGER; + } + uDirEntry = 0; + + } while ( + dirNext(sb, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); + } + sb.erase(ammSle); sb.erase(sleAMMRoot); diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index 5228b625bb3..365c8e87c26 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -277,7 +277,8 @@ LedgerFormats::LedgerFormats() {sfAuctionSlot, soeOPTIONAL}, {sfLPTokenBalance, soeREQUIRED}, {sfAsset, soeREQUIRED}, - {sfAsset2, soeREQUIRED} + {sfAsset2, soeREQUIRED}, + {sfOwnerNode, soeREQUIRED} }, commonFields); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 98804e77b74..398bf344478 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1770,9 +1770,16 @@ struct AMM_test : public jtx::AMMTest // Withdraw all tokens. testAMM([&](AMM& ammAlice, Env& env) { + env(trust(carol, STAmount{ammAlice.lptIssue(), 10000})); + env(trust( + carol, + STAmount{Issue{EUR.currency, ammAlice.ammAccount()}, 10000})); + env.close(); ammAlice.withdrawAll(alice); BEAST_EXPECT(!ammAlice.ammExists()); + BEAST_EXPECT(!env.le(keylet::ownerDir(ammAlice.ammAccount()))); + // Can create AMM for the XRP/USD pair AMM ammCarol(env, carol, XRP(10'000), USD(10'000)); BEAST_EXPECT(ammCarol.expectBalances( From 5cead5b3bd0a286af34984c0af872d22ab779ba5 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Jul 2023 08:47:39 -0400 Subject: [PATCH 02/18] Disallow check create to AMM --- src/ripple/app/tx/impl/CreateCheck.cpp | 4 ++++ src/test/app/AMMExtended_test.cpp | 1 - src/test/app/AMM_test.cpp | 7 +++++- src/test/app/CrossingLimits_test.cpp | 1 - src/test/app/Escrow_test.cpp | 1 - src/test/app/Flow_test.cpp | 1 - src/test/app/Freeze_test.cpp | 1 - src/test/app/Offer_test.cpp | 1 - src/test/app/Path_test.cpp | 1 - src/test/app/PayChan_test.cpp | 1 - src/test/app/PayStrand_test.cpp | 1 - src/test/app/TrustAndBalance_test.cpp | 1 - src/test/jtx.h | 1 + src/test/jtx/TestHelpers.h | 33 ++++++++++++++++++++++++++ src/test/jtx/check.h | 7 ------ src/test/jtx/impl/check.cpp | 16 ------------- src/test/rpc/AccountOffers_test.cpp | 1 - src/test/rpc/LedgerData_test.cpp | 1 - src/test/rpc/NoRipple_test.cpp | 1 - 19 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/ripple/app/tx/impl/CreateCheck.cpp b/src/ripple/app/tx/impl/CreateCheck.cpp index f5c2cbfbfd9..e7f0e103bce 100644 --- a/src/ripple/app/tx/impl/CreateCheck.cpp +++ b/src/ripple/app/tx/impl/CreateCheck.cpp @@ -97,6 +97,10 @@ CreateCheck::preclaim(PreclaimContext const& ctx) (flags & lsfDisallowIncomingCheck)) return tecNO_PERMISSION; + // AMM can not cash the check + if (flags & lsfAMM) + return tecNO_PERMISSION; + if ((flags & lsfRequireDestTag) && !ctx.tx.isFieldPresent(sfDestinationTag)) { // The tag is basically account-specific information we don't diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 6b888ce8a2c..ad2c1f67805 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -32,7 +32,6 @@ #include #include #include -#include #include #include diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 398bf344478..48bf90c2d8c 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include @@ -2757,6 +2756,12 @@ struct AMM_test : public jtx::AMMTest ter(tecNO_PERMISSION)); }); + // Can't pay into AMM with checks. + testAMM([&](AMM& ammAlice, Env& env) { + env(check::create(env.master.id(), ammAlice.ammAccount(), XRP(100)), + ter(tecNO_PERMISSION)); + }); + // Pay amounts close to one side of the pool testAMM( [&](AMM& ammAlice, Env& env) { diff --git a/src/test/app/CrossingLimits_test.cpp b/src/test/app/CrossingLimits_test.cpp index d2ae9799ce8..09cca1e82af 100644 --- a/src/test/app/CrossingLimits_test.cpp +++ b/src/test/app/CrossingLimits_test.cpp @@ -18,7 +18,6 @@ #include #include #include -#include namespace ripple { namespace test { diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 255325ca8ac..083764fd371 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -24,7 +24,6 @@ #include #include #include -#include #include #include diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index b9d2b715aa5..131cad6f042 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -28,7 +28,6 @@ #include #include #include -#include namespace ripple { namespace test { diff --git a/src/test/app/Freeze_test.cpp b/src/test/app/Freeze_test.cpp index fc51a6869a6..cb4653c086d 100644 --- a/src/test/app/Freeze_test.cpp +++ b/src/test/app/Freeze_test.cpp @@ -22,7 +22,6 @@ #include #include #include -#include namespace ripple { diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index e54f9706b36..1c8e544af30 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -22,7 +22,6 @@ #include #include #include -#include #include namespace ripple { diff --git a/src/test/app/Path_test.cpp b/src/test/app/Path_test.cpp index ea2dc51132a..0f9916cbcff 100644 --- a/src/test/app/Path_test.cpp +++ b/src/test/app/Path_test.cpp @@ -35,7 +35,6 @@ #include #include #include -#include #include #include diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index dd82a622cc9..f8162dd107d 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -25,7 +25,6 @@ #include #include #include -#include #include diff --git a/src/test/app/PayStrand_test.cpp b/src/test/app/PayStrand_test.cpp index 4eb6c9fecb6..a70ab099745 100644 --- a/src/test/app/PayStrand_test.cpp +++ b/src/test/app/PayStrand_test.cpp @@ -29,7 +29,6 @@ #include "ripple/app/paths/AMMContext.h" #include #include -#include namespace ripple { namespace test { diff --git a/src/test/app/TrustAndBalance_test.cpp b/src/test/app/TrustAndBalance_test.cpp index f83eeac27ec..5b0c1d6480e 100644 --- a/src/test/app/TrustAndBalance_test.cpp +++ b/src/test/app/TrustAndBalance_test.cpp @@ -22,7 +22,6 @@ #include #include #include -#include #include namespace ripple { diff --git a/src/test/jtx.h b/src/test/jtx.h index bcf51398d5c..dd987472cc8 100644 --- a/src/test/jtx.h +++ b/src/test/jtx.h @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/jtx/TestHelpers.h b/src/test/jtx/TestHelpers.h index a29a894452b..ff681ffa50b 100644 --- a/src/test/jtx/TestHelpers.h +++ b/src/test/jtx/TestHelpers.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -405,6 +406,38 @@ STPathElement allpe(AccountID const& a, Issue const& iss); /***************************************************************/ +/* Check */ +/***************************************************************/ +namespace check { + +/** Create a check. */ +// clang-format off +template + requires std::is_same_v +Json::Value +create(A const& account, A const& dest, STAmount const& sendMax) +{ + Json::Value jv; + jv[sfAccount.jsonName] = to_string(account); + jv[sfSendMax.jsonName] = sendMax.getJson(JsonOptions::none); + jv[sfDestination.jsonName] = to_string(dest); + jv[sfTransactionType.jsonName] = jss::CheckCreate; + jv[sfFlags.jsonName] = tfUniversal; + return jv; +} +// clang-format on + +inline Json::Value +create( + jtx::Account const& account, + jtx::Account const& dest, + STAmount const& sendMax) +{ + return create(account.id(), dest.id(), sendMax); +} + +} // namespace check + } // namespace jtx } // namespace test } // namespace ripple diff --git a/src/test/jtx/check.h b/src/test/jtx/check.h index 325e5897258..6bad6b9db5e 100644 --- a/src/test/jtx/check.h +++ b/src/test/jtx/check.h @@ -31,13 +31,6 @@ namespace jtx { /** Check operations. */ namespace check { -/** Create a check. */ -Json::Value -create( - jtx::Account const& account, - jtx::Account const& dest, - STAmount const& sendMax); - /** Cash a check requiring that a specific amount be delivered. */ Json::Value cash(jtx::Account const& dest, uint256 const& checkId, STAmount const& amount); diff --git a/src/test/jtx/impl/check.cpp b/src/test/jtx/impl/check.cpp index 8abcaea1b34..d862130bc70 100644 --- a/src/test/jtx/impl/check.cpp +++ b/src/test/jtx/impl/check.cpp @@ -27,22 +27,6 @@ namespace jtx { namespace check { -// Create a check. -Json::Value -create( - jtx::Account const& account, - jtx::Account const& dest, - STAmount const& sendMax) -{ - Json::Value jv; - jv[sfAccount.jsonName] = account.human(); - jv[sfSendMax.jsonName] = sendMax.getJson(JsonOptions::none); - jv[sfDestination.jsonName] = dest.human(); - jv[sfTransactionType.jsonName] = jss::CheckCreate; - jv[sfFlags.jsonName] = tfUniversal; - return jv; -} - // Cash a check requiring that a specific amount be delivered. Json::Value cash(jtx::Account const& dest, uint256 const& checkId, STAmount const& amount) diff --git a/src/test/rpc/AccountOffers_test.cpp b/src/test/rpc/AccountOffers_test.cpp index 4979ac07216..d94442ea8e5 100644 --- a/src/test/rpc/AccountOffers_test.cpp +++ b/src/test/rpc/AccountOffers_test.cpp @@ -19,7 +19,6 @@ #include #include -#include namespace ripple { namespace test { diff --git a/src/test/rpc/LedgerData_test.cpp b/src/test/rpc/LedgerData_test.cpp index ebc1b9fb553..f0811ba34c4 100644 --- a/src/test/rpc/LedgerData_test.cpp +++ b/src/test/rpc/LedgerData_test.cpp @@ -20,7 +20,6 @@ #include #include #include -#include namespace ripple { diff --git a/src/test/rpc/NoRipple_test.cpp b/src/test/rpc/NoRipple_test.cpp index 268cd1f14a3..472706f0b73 100644 --- a/src/test/rpc/NoRipple_test.cpp +++ b/src/test/rpc/NoRipple_test.cpp @@ -20,7 +20,6 @@ #include #include #include -#include namespace ripple { From eaef4b6e571f4222a68b735b324fce7e7e0dbec6 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Jul 2023 08:55:15 -0400 Subject: [PATCH 03/18] Fix unconstrained entries in AuthAccount --- src/ripple/protocol/impl/InnerObjectFormats.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/ripple/protocol/impl/InnerObjectFormats.cpp b/src/ripple/protocol/impl/InnerObjectFormats.cpp index e6de3cc04e4..ba1a40a87ee 100644 --- a/src/ripple/protocol/impl/InnerObjectFormats.cpp +++ b/src/ripple/protocol/impl/InnerObjectFormats.cpp @@ -77,6 +77,12 @@ InnerObjectFormats::InnerObjectFormats() {sfPrice, soeREQUIRED}, {sfAuthAccounts, soeOPTIONAL}, }); + + add(sfAuthAccount.jsonName.c_str(), + sfAuthAccount.getCode(), + { + {sfAccount, soeREQUIRED}, + }); } InnerObjectFormats const& From b28101f293c376e7a2e9ba1462d2be79f2847137 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 18 Jul 2023 16:00:25 -0400 Subject: [PATCH 04/18] Allow SetTrust on AMM only for LP tokens and other changes * Add AMMID to AMM root account * Remove owner dir entry for ltAMM --- src/ripple/app/tx/impl/AMMCreate.cpp | 15 ++-------- src/ripple/app/tx/impl/AMMWithdraw.cpp | 10 +------ src/ripple/app/tx/impl/SetTrust.cpp | 32 +++++++++++++++++----- src/ripple/protocol/impl/LedgerFormats.cpp | 2 +- src/test/app/AMM_test.cpp | 11 ++++++-- 5 files changed, 37 insertions(+), 33 deletions(-) diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index 89376a4eeac..d1e60bb4774 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -248,6 +248,8 @@ applyCreate( // either an AMMDeposit, TrustSet, crossing an offer, etc. sleAMMRoot->setFieldU32( sfFlags, lsfAMM | lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth); + // Link the root account and AMM object + sleAMMRoot->setFieldH256(sfAMMID, ammKeylet.key); sb.insert(sleAMMRoot); // Calculate initial LPT balance. @@ -282,19 +284,6 @@ applyCreate( TOTAL_TIME_SLOT_SECS; auctionSlot.setFieldU32(sfExpiration, expiration); auctionSlot.setFieldAmount(sfPrice, STAmount{lpTokens.issue(), 0}); - - // Add owner directory to link the root account and AMM object. - if (auto const page = sb.dirInsert( - keylet::ownerDir(*ammAccount), - ammSle->key(), - describeOwnerDir(*ammAccount)); - !page) - { - JLOG(j_.debug()) << "AMM Instance: failed to insert owner dir"; - return {tecDIR_FULL, false}; - } - else - ammSle->setFieldU64(sfOwnerNode, *page); sb.insert(ammSle); // Send LPT to LP. diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index 544bf2ab35a..45787402d0b 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -409,16 +409,8 @@ AMMWithdraw::deleteAccount(Sandbox& sb, AccountID const& ammAccountID) if (!sleAMMRoot || !ammSle) return tecINTERNAL; - // Delete directory entry linking the root account and AMM object - Keylet const ownerDirKeylet{keylet::ownerDir(ammAccountID)}; - if (!sb.dirRemove( - ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false)) - { - JLOG(j_.fatal()) << "AMM Withdraw: failed to remove dir link"; - return tecINTERNAL; - } - // Delete the trustlines + Keylet const ownerDirKeylet{keylet::ownerDir(ammAccountID)}; std::shared_ptr sleDirNode{}; unsigned int uDirEntry{0}; uint256 dirEntry{beast::zero}; diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index acbbedabf10..3d723a8209c 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -128,21 +128,39 @@ SetTrust::preclaim(PreclaimContext const& ctx) } } + auto const sleDst = ctx.view.read(keylet::account(uDstAccountID)); + + if (!sleDst) + return tecNO_DST; + + auto const dstFlags = sleDst->getFlags(); + // If the destination has opted to disallow incoming trustlines // then honour that flag if (ctx.view.rules().enabled(featureDisallowIncoming)) { - auto const sleDst = ctx.view.read(keylet::account(uDstAccountID)); - - if (!sleDst) - return tecNO_DST; - - auto const dstFlags = sleDst->getFlags(); if (dstFlags & lsfDisallowIncomingTrustline) return tecNO_PERMISSION; } - return tesSUCCESS; + // If destination is AMM and the trustline doesn't exist then only + // allow SetTrust if the asset is AMM LP token + TER ter = tesSUCCESS; + if (dstFlags & lsfAMM && + !ctx.view.read(keylet::line(id, uDstAccountID, currency))) + { + if (auto const ammSle = + ctx.view.read({ltAMM, sleDst->getFieldH256(sfAMMID)})) + { + if (ammSle->getFieldAmount(sfLPTokenBalance).getCurrency() != + saLimitAmount.getCurrency()) + ter = tecNO_PERMISSION; + } + else + ter = tecINTERNAL; + } + + return ter; } TER diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index 365c8e87c26..9192513457a 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -56,6 +56,7 @@ LedgerFormats::LedgerFormats() {sfMintedNFTokens, soeDEFAULT}, {sfBurnedNFTokens, soeDEFAULT}, {sfFirstNFTokenSequence, soeOPTIONAL}, + {sfAMMID, soeOPTIONAL}, }, commonFields); @@ -278,7 +279,6 @@ LedgerFormats::LedgerFormats() {sfLPTokenBalance, soeREQUIRED}, {sfAsset, soeREQUIRED}, {sfAsset2, soeREQUIRED}, - {sfOwnerNode, soeREQUIRED} }, commonFields); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 48bf90c2d8c..6727427d72a 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1770,9 +1770,12 @@ struct AMM_test : public jtx::AMMTest // Withdraw all tokens. testAMM([&](AMM& ammAlice, Env& env) { env(trust(carol, STAmount{ammAlice.lptIssue(), 10000})); + // Can SetTrust only for AMM LP tokens env(trust( - carol, - STAmount{Issue{EUR.currency, ammAlice.ammAccount()}, 10000})); + carol, + STAmount{ + Issue{EUR.currency, ammAlice.ammAccount()}, 10000}), + ter(tecNO_PERMISSION)); env.close(); ammAlice.withdrawAll(alice); BEAST_EXPECT(!ammAlice.ammExists()); @@ -3620,9 +3623,11 @@ struct AMM_test : public jtx::AMMTest AMM amm(env, C, TSTA(5'000), TSTB(5'000)); auto const ammIss = Issue(TSTA.currency, amm.ammAccount()); - env.trust(STAmount{ammIss, 10'000}, D); + // Can SetTrust only for AMM LP tokens + env(trust(D, STAmount{ammIss, 10'000}), ter(tecNO_PERMISSION)); env.close(); + // The payment would fail because of above, but check just in case env(pay(C, D, STAmount{ammIss, 10}), sendmax(TSTA(100)), path(amm.ammAccount()), From cbd7144e3d5bb13efe70488c26602c7f058597ea Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sun, 23 Jul 2023 12:43:07 -0400 Subject: [PATCH 05/18] Add AMMDelete to handle amortized deletion and other changes to address reviewer's feedback * Limit number of trustlines to delete on final withdraw and AMMDelete * Enable AMM in empty state when LPTokens is 0 in final withdraw * Disallow trustlines to AMM in empty state * Add utility functions to delete nodes on AccountDelete and AMM account delete * Add tfTwoAssetIfEmpty deposit option in AMM empty state * Fail all AMM transactions in AMM empty state except special deposit * Add tecINCOMPLETE to indicate not all AMM trustlines are deleted; handle it in Transactor similar to deleted offers * Extend the unit-tests --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/misc/AMMUtils.h | 20 ++ src/ripple/app/misc/impl/AMMUtils.cpp | 106 +++++++++ src/ripple/app/paths/impl/BookStep.cpp | 3 +- src/ripple/app/tx/impl/AMMBid.cpp | 11 +- src/ripple/app/tx/impl/AMMCreate.cpp | 25 +-- src/ripple/app/tx/impl/AMMDeposit.cpp | 91 +++++++- src/ripple/app/tx/impl/AMMDeposit.h | 17 ++ src/ripple/app/tx/impl/AMMVote.cpp | 10 +- src/ripple/app/tx/impl/AMMWithdraw.cpp | 114 ++-------- src/ripple/app/tx/impl/AMMWithdraw.h | 8 - src/ripple/app/tx/impl/DeleteAccount.cpp | 77 ++----- src/ripple/app/tx/impl/InvariantCheck.cpp | 6 +- src/ripple/app/tx/impl/SetTrust.cpp | 9 +- src/ripple/app/tx/impl/Transactor.cpp | 66 ++++-- src/ripple/app/tx/impl/applySteps.cpp | 11 + src/ripple/ledger/View.h | 29 +++ src/ripple/ledger/impl/View.cpp | 115 ++++++++++ src/ripple/protocol/Protocol.h | 2 + src/ripple/protocol/TER.h | 4 +- src/ripple/protocol/TxFlags.h | 3 +- src/ripple/protocol/TxFormats.h | 3 + src/ripple/protocol/impl/TER.cpp | 2 + src/ripple/protocol/impl/TxFormats.cpp | 10 + src/ripple/protocol/jss.h | 1 + src/test/app/AMM_test.cpp | 261 ++++++++++++++++++++-- src/test/jtx/AMM.h | 6 + src/test/jtx/impl/AMM.cpp | 17 ++ 28 files changed, 785 insertions(+), 243 deletions(-) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index b676c5ff5e9..6739afcbefa 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -476,6 +476,7 @@ target_sources (rippled PRIVATE src/ripple/app/rdb/impl/Wallet.cpp src/ripple/app/tx/impl/AMMBid.cpp src/ripple/app/tx/impl/AMMCreate.cpp + src/ripple/app/tx/impl/AMMDelete.cpp src/ripple/app/tx/impl/AMMDeposit.cpp src/ripple/app/tx/impl/AMMVote.cpp src/ripple/app/tx/impl/AMMWithdraw.cpp diff --git a/src/ripple/app/misc/AMMUtils.h b/src/ripple/app/misc/AMMUtils.h index 1718df496b8..152229f88cf 100644 --- a/src/ripple/app/misc/AMMUtils.h +++ b/src/ripple/app/misc/AMMUtils.h @@ -93,6 +93,26 @@ ammAccountHolds( AccountID const& ammAccountID, Issue const& issue); +/** Delete trustlines to AMM. If all trustlines are deleted then + * AMM object and account are deleted. Otherwise tecIMPCOMPLETE is returned. + */ +TER +deleteAMMAccount( + ApplyView& view, + Issue const& asset, + Issue const& asset2, + beast::Journal j); + +/** Initialize Auction and Voting slots and set the trading/disconted fee. + */ +void +initializeFeeAuctionVote( + ApplyView& view, + std::shared_ptr& ammSle, + AccountID const& account, + Issue const& lptIssue, + std::uint16_t tfee); + } // namespace ripple #endif // RIPPLE_APP_MISC_AMMUTILS_H_INLCUDED diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 7156c77f21a..fa8a3d656dd 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -188,4 +188,110 @@ ammAccountHolds( return STAmount{issue}; } +static std::pair +deleteAMMTrustLines( + ApplyView& view, + AccountID const& ammAccountID, + std::uint16_t maxTrustlinesToDelete, + beast::Journal j) +{ + return cleanupOnAccountDelete( + view, + keylet::ownerDir(ammAccountID), + [&](LedgerEntryType nodeType, + uint256 const&, + std::shared_ptr& sleItem) -> TER { + // Should only have the trustlines + if (nodeType != LedgerEntryType::ltRIPPLE_STATE) + { + JLOG(j.error()) + << "deleteAMMTrustLines: deleting non-trustline " + << nodeType; + return tecINTERNAL; + } + + // Trustlines must have zero balance + if (sleItem->getFieldAmount(sfBalance) != beast::zero) + { + JLOG(j.error()) + << "deleteAMMTrustLines: deleting trustline with " + "non-zero balance."; + return tecINTERNAL; + } + + return deleteAMMTrustLine(view, sleItem, ammAccountID, j); + }, + j, + maxTrustlinesToDelete); +} + +TER +deleteAMMAccount( + ApplyView& view, + Issue const& asset, + Issue const& asset2, + beast::Journal j) +{ + auto ammSle = view.peek(keylet::amm(asset, asset2)); + if (!ammSle) + return tecINTERNAL; + + auto const ammAccountID = (*ammSle)[sfAccount]; + auto sleAMMRoot = view.peek(keylet::account(ammAccountID)); + if (!sleAMMRoot) + return tecINTERNAL; + + if (auto const res = deleteAMMTrustLines( + view, ammAccountID, maxDeletableAMMTrustLines, j); + std::get(res) != tesSUCCESS) + return std::get(res); + // All trustlines are deleted, can delete ltAMM and the account + else if (res.second == AllNodesDeleted::Yes) + { + view.erase(ammSle); + view.erase(sleAMMRoot); + return tesSUCCESS; + } + + return tecINCOMPLETE; +} + +void +initializeFeeAuctionVote( + ApplyView& view, + std::shared_ptr& ammSle, + AccountID const& account, + Issue const& lptIssue, + std::uint16_t tfee) +{ + // AMM creator gets the voting slot. + STArray voteSlots; + STObject voteEntry{sfVoteEntry}; + if (tfee != 0) + voteEntry.setFieldU16(sfTradingFee, tfee); + voteEntry.setFieldU32(sfVoteWeight, 100000); + voteEntry.setAccountID(sfAccount, account); + voteSlots.push_back(voteEntry); + ammSle->setFieldArray(sfVoteSlots, voteSlots); + // AMM creator gets the auction slot for free. + auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); + auctionSlot.setAccountID(sfAccount, account); + // current + sec in 24h + auto const expiration = std::chrono::duration_cast( + view.info().parentCloseTime.time_since_epoch()) + .count() + + TOTAL_TIME_SLOT_SECS; + auctionSlot.setFieldU32(sfExpiration, expiration); + auctionSlot.setFieldAmount(sfPrice, STAmount{lptIssue, 0}); + // Set the fee + if (tfee != 0) + ammSle->setFieldU16(sfTradingFee, tfee); + else if (ammSle->isFieldPresent(sfTradingFee)) + ammSle->makeFieldAbsent(sfTradingFee); + if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION) + auctionSlot.setFieldU16(sfDiscountedFee, dfee); + else if (auctionSlot.isFieldPresent(sfDiscountedFee)) + auctionSlot.makeFieldAbsent(sfDiscountedFee); +} + } // namespace ripple diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index e82acbde817..dd6abe577f5 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -99,7 +99,8 @@ class BookStep : public StepImp> , ownerPaysTransferFee_(ctx.ownerPaysTransferFee) , j_(ctx.j) { - if (auto const ammSle = ctx.view.read(keylet::amm(in, out))) + if (auto const ammSle = ctx.view.read(keylet::amm(in, out)); + ammSle && ammSle->getFieldAmount(sfLPTokenBalance) != beast::zero) ammLiquidity_.emplace( ctx.view, (*ammSle)[sfAccount], diff --git a/src/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp index 7654e842df1..d059f88c76c 100644 --- a/src/ripple/app/tx/impl/AMMBid.cpp +++ b/src/ripple/app/tx/impl/AMMBid.cpp @@ -94,6 +94,10 @@ AMMBid::preclaim(PreclaimContext const& ctx) return terNO_AMM; } + auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance]; + if (lpTokensBalance == beast::zero) + return tecAMM_EMPTY; + if (ctx.tx.isFieldPresent(sfAuthAccounts)) { for (auto const& account : ctx.tx.getFieldArray(sfAuthAccounts)) @@ -114,7 +118,6 @@ AMMBid::preclaim(PreclaimContext const& ctx) JLOG(ctx.j.debug()) << "AMM Bid: account is not LP."; return tecAMM_INVALID_TOKENS; } - auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance]; auto const bidMin = ctx.tx[~sfBidMin]; @@ -204,10 +207,10 @@ applyBid( Number const& burn) -> TER { auctionSlot.setAccountID(sfAccount, account_); auctionSlot.setFieldU32(sfExpiration, current + TOTAL_TIME_SLOT_SECS); - if (fee == 0) - auctionSlot.makeFieldAbsent(sfDiscountedFee); - else + if (fee != 0) auctionSlot.setFieldU16(sfDiscountedFee, fee); + else if (auctionSlot.isFieldPresent(sfDiscountedFee)) + auctionSlot.makeFieldAbsent(sfDiscountedFee); auctionSlot.setFieldAmount( sfPrice, toSTAmount(lpTokens.issue(), minPrice)); if (ctx_.tx.isFieldPresent(sfAuthAccounts)) diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index d1e60bb4774..0fbb584c1c0 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -257,33 +257,14 @@ applyCreate( // Create ltAMM auto ammSle = std::make_shared(ammKeylet); - if (ctx_.tx[sfTradingFee] != 0) - ammSle->setFieldU16(sfTradingFee, ctx_.tx[sfTradingFee]); ammSle->setAccountID(sfAccount, *ammAccount); ammSle->setFieldAmount(sfLPTokenBalance, lpTokens); auto const& [issue1, issue2] = std::minmax(amount.issue(), amount2.issue()); ammSle->setFieldIssue(sfAsset, STIssue{sfAsset, issue1}); ammSle->setFieldIssue(sfAsset2, STIssue{sfAsset2, issue2}); - // AMM creator gets the voting slot. - STArray voteSlots; - STObject voteEntry{sfVoteEntry}; - if (ctx_.tx[sfTradingFee] != 0) - voteEntry.setFieldU16(sfTradingFee, ctx_.tx[sfTradingFee]); - voteEntry.setFieldU32(sfVoteWeight, 100000); - voteEntry.setAccountID(sfAccount, account_); - voteSlots.push_back(voteEntry); - ammSle->setFieldArray(sfVoteSlots, voteSlots); - // AMM creator gets the auction slot for free. - auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); - auctionSlot.setAccountID(sfAccount, account_); - // current + sec in 24h - auto const expiration = - std::chrono::duration_cast( - ctx_.view().info().parentCloseTime.time_since_epoch()) - .count() + - TOTAL_TIME_SLOT_SECS; - auctionSlot.setFieldU32(sfExpiration, expiration); - auctionSlot.setFieldAmount(sfPrice, STAmount{lpTokens.issue(), 0}); + // AMM creator gets the auction slot and the voting slot. + initializeFeeAuctionVote( + ctx_.view(), ammSle, account_, lptIss, ctx_.tx[sfTradingFee]); sb.insert(ammSle); // Send LPT to LP. diff --git a/src/ripple/app/tx/impl/AMMDeposit.cpp b/src/ripple/app/tx/impl/AMMDeposit.cpp index 2fb9bd1eaab..94da211a318 100644 --- a/src/ripple/app/tx/impl/AMMDeposit.cpp +++ b/src/ripple/app/tx/impl/AMMDeposit.cpp @@ -52,10 +52,12 @@ AMMDeposit::preflight(PreflightContext const& ctx) auto const amount2 = ctx.tx[~sfAmount2]; auto const ePrice = ctx.tx[~sfEPrice]; auto const lpTokens = ctx.tx[~sfLPTokenOut]; + auto const tradingFee = ctx.tx[~sfTradingFee]; // Valid options for the flags are: // tfLPTokens: LPTokenOut, [Amount, Amount2] // tfSingleAsset: Amount, [LPTokenOut] // tfTwoAsset: Amount, Amount2, [LPTokenOut] + // tfTwoAssetIfEmpty: Amount, Amount2, [sfTradingFee] // tfOnAssetLPToken: Amount and LPTokenOut // tfLimitLPToken: Amount and EPrice if (std::popcount(flags & tfDepositSubTx) != 1) @@ -66,29 +68,35 @@ AMMDeposit::preflight(PreflightContext const& ctx) if (flags & tfLPToken) { // if included then both amount and amount2 are deposit min - if (!lpTokens || ePrice || (amount && !amount2) || (!amount && amount2)) + if (!lpTokens || ePrice || (amount && !amount2) || + (!amount && amount2) || tradingFee) return temMALFORMED; } else if (flags & tfSingleAsset) { // if included then lpTokens is deposit min - if (!amount || amount2 || ePrice) + if (!amount || amount2 || ePrice || tradingFee) return temMALFORMED; } else if (flags & tfTwoAsset) { // if included then lpTokens is deposit min - if (!amount || !amount2 || ePrice) + if (!amount || !amount2 || ePrice || tradingFee) return temMALFORMED; } else if (flags & tfOneAssetLPToken) { - if (!amount || !lpTokens || amount2 || ePrice) + if (!amount || !lpTokens || amount2 || ePrice || tradingFee) return temMALFORMED; } else if (flags & tfLimitLPToken) { - if (!amount || !ePrice || lpTokens || amount2) + if (!amount || !ePrice || lpTokens || amount2 || tradingFee) + return temMALFORMED; + } + else if (flags & tfTwoAssetIfEmpty) + { + if (!amount || !amount2 || ePrice || lpTokens) return temMALFORMED; } @@ -148,6 +156,12 @@ AMMDeposit::preflight(PreflightContext const& ctx) } } + if (tradingFee > TRADING_FEE_THRESHOLD) + { + JLOG(ctx.j.debug()) << "AMM Deposit: invalid trading fee."; + return temBAD_FEE; + } + return preflight2(ctx); } @@ -174,12 +188,27 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) if (!expected) return expected.error(); auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected; - if (amountBalance <= beast::zero || amount2Balance <= beast::zero || - lptAMMBalance <= beast::zero) + if (ctx.tx.getFlags() & tfTwoAssetIfEmpty) { - JLOG(ctx.j.debug()) - << "AMM Deposit: reserves or tokens balance is zero."; - return tecINTERNAL; + if (lptAMMBalance != beast::zero) + return tecAMM_EMPTY; + if (amountBalance != beast::zero || amount2Balance != beast::zero) + { + JLOG(ctx.j.debug()) << "AMM Deposit: tokens balance is not zero."; + return tecINTERNAL; + } + } + else + { + if (lptAMMBalance == beast::zero) + return tecAMM_EMPTY; + if (amountBalance <= beast::zero || amount2Balance <= beast::zero || + lptAMMBalance < beast::zero) + { + JLOG(ctx.j.debug()) + << "AMM Deposit: reserves or tokens balance is zero."; + return tecINTERNAL; + } } // Check account has sufficient funds. @@ -313,8 +342,6 @@ AMMDeposit::applyGuts(Sandbox& sb) return {tecINTERNAL, false}; auto const ammAccountID = (*ammSle)[sfAccount]; - auto const tfee = getTradingFee(ctx_.view(), *ammSle, account_); - auto const expected = ammHolds( sb, *ammSle, @@ -325,6 +352,9 @@ AMMDeposit::applyGuts(Sandbox& sb) if (!expected) return {expected.error(), false}; auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected; + auto const tfee = (lptAMMBalance == beast::zero) + ? ctx_.tx[~sfTradingFee].value_or(0) + : getTradingFee(ctx_.view(), *ammSle, account_); auto const subTxType = ctx_.tx.getFlags() & tfDepositSubTx; @@ -382,6 +412,14 @@ AMMDeposit::applyGuts(Sandbox& sb) amount, amount2, tfee); + if (subTxType & tfTwoAssetIfEmpty) + return equalDepositInEmptyState( + sb, + ammAccountID, + *amount, + *amount2, + lptAMMBalance.issue(), + tfee); // should not happen. JLOG(j_.error()) << "AMM Deposit: invalid options."; return std::make_pair(tecINTERNAL, STAmount{}); @@ -391,6 +429,12 @@ AMMDeposit::applyGuts(Sandbox& sb) { assert(newLPTokenBalance > beast::zero); ammSle->setFieldAmount(sfLPTokenBalance, newLPTokenBalance); + // LP depositing into AMM empty state gets the auction slot + // and the voting + if (lptAMMBalance == beast::zero) + initializeFeeAuctionVote( + sb, ammSle, account_, lptAMMBalance.issue(), tfee); + sb.update(ammSle); } @@ -834,4 +878,27 @@ AMMDeposit::singleDepositEPrice( tfee); } +std::pair +AMMDeposit::equalDepositInEmptyState( + Sandbox& view, + AccountID const& ammAccount, + STAmount const& amount, + STAmount const& amount2, + Issue const& lptIssue, + std::uint16_t tfee) +{ + return deposit( + view, + ammAccount, + amount, + amount, + amount2, + STAmount{lptIssue, 0}, + ammLPTokens(amount, amount2, lptIssue), + std::nullopt, + std::nullopt, + std::nullopt, + tfee); +} + } // namespace ripple diff --git a/src/ripple/app/tx/impl/AMMDeposit.h b/src/ripple/app/tx/impl/AMMDeposit.h index 10da43594d3..385ce7c24e5 100644 --- a/src/ripple/app/tx/impl/AMMDeposit.h +++ b/src/ripple/app/tx/impl/AMMDeposit.h @@ -223,6 +223,23 @@ class AMMDeposit : public Transactor STAmount const& lptAMMBalance, STAmount const& ePrice, std::uint16_t tfee); + + /** Equal deposit in empty AMM state (LP tokens balance is 0) + * @param view + * @param ammAccount + * @param amount requested asset1 deposit amount + * @param amount2 requested asset2 deposit amount + * @param tfee + * @return + */ + std::pair + equalDepositInEmptyState( + Sandbox& view, + AccountID const& ammAccount, + STAmount const& amount, + STAmount const& amount2, + Issue const& lptIssue, + std::uint16_t tfee); }; } // namespace ripple diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index 09361356c3a..ff0598aaa40 100644 --- a/src/ripple/app/tx/impl/AMMVote.cpp +++ b/src/ripple/app/tx/impl/AMMVote.cpp @@ -69,6 +69,8 @@ AMMVote::preclaim(PreclaimContext const& ctx) JLOG(ctx.j.debug()) << "AMM Vote: Invalid asset pair."; return terNO_AMM; } + else if (ammSle->getFieldAmount(sfLPTokenBalance) == beast::zero) + return tecAMM_EMPTY; else if (auto const lpTokensNew = ammLPHolds(ctx.view, *ammSle, ctx.tx[sfAccount], ctx.j); lpTokensNew == beast::zero) @@ -208,17 +210,19 @@ applyVote( if (auto const discountedFee = fee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION) auctionSlot.setFieldU16(sfDiscountedFee, discountedFee); - else + else if (auctionSlot.isFieldPresent(sfDiscountedFee)) auctionSlot.makeFieldAbsent(sfDiscountedFee); } } else { - ammSle->makeFieldAbsent(sfTradingFee); + if (ammSle->isFieldPresent(sfTradingFee)) + ammSle->makeFieldAbsent(sfTradingFee); if (ammSle->isFieldPresent(sfAuctionSlot)) { auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); - auctionSlot.makeFieldAbsent(sfDiscountedFee); + if (auctionSlot.isFieldPresent(sfDiscountedFee)) + auctionSlot.makeFieldAbsent(sfDiscountedFee); } } sb.update(ammSle); diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index 45787402d0b..1016b5c3021 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -194,8 +194,10 @@ AMMWithdraw::preclaim(PreclaimContext const& ctx) if (!expected) return expected.error(); auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected; + if (lptAMMBalance == beast::zero) + return tecAMM_EMPTY; if (amountBalance <= beast::zero || amount2Balance <= beast::zero || - lptAMMBalance <= beast::zero) + lptAMMBalance < beast::zero) { JLOG(ctx.j.debug()) << "AMM Withdraw: reserves or tokens balance is zero."; @@ -371,19 +373,31 @@ AMMWithdraw::applyGuts(Sandbox& sb) return std::make_pair(tecINTERNAL, STAmount{}); }(); - // AMM is deleted if zero tokens balance - if (result == tesSUCCESS && newLPTokenBalance != beast::zero) + if (result != tesSUCCESS) + return {result, false}; + + bool updateBalance = true; + if (newLPTokenBalance == beast::zero) + { + if (auto const ter = + deleteAMMAccount(sb, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], j_); + ter != tesSUCCESS && ter != tecINCOMPLETE) + return {ter, false}; + else + updateBalance = (ter == tecINCOMPLETE); + } + + if (updateBalance) { ammSle->setFieldAmount(sfLPTokenBalance, newLPTokenBalance); sb.update(ammSle); - - JLOG(ctx_.journal.trace()) - << "AMM Withdraw: tokens " << to_string(newLPTokenBalance.iou()) - << " " << to_string(lpTokens.iou()) << " " - << to_string(lptAMMBalance.iou()); } - return {result, result == tesSUCCESS}; + JLOG(ctx_.journal.trace()) + << "AMM Withdraw: tokens " << to_string(newLPTokenBalance.iou()) << " " + << to_string(lpTokens.iou()) << " " << to_string(lptAMMBalance.iou()); + + return {tesSUCCESS, true}; } TER @@ -400,85 +414,6 @@ AMMWithdraw::doApply() return result.first; } -TER -AMMWithdraw::deleteAccount(Sandbox& sb, AccountID const& ammAccountID) -{ - auto sleAMMRoot = sb.peek(keylet::account(ammAccountID)); - auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2])); - - if (!sleAMMRoot || !ammSle) - return tecINTERNAL; - - // Delete the trustlines - Keylet const ownerDirKeylet{keylet::ownerDir(ammAccountID)}; - std::shared_ptr sleDirNode{}; - unsigned int uDirEntry{0}; - uint256 dirEntry{beast::zero}; - if (sb.exists(ownerDirKeylet) && - dirFirst(sb, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) - { - do - { - auto sleItem = sb.peek(keylet::child(dirEntry)); - if (!sleItem) - { - // Directory node has an invalid index - JLOG(j_.fatal()) - << "AMM Withdraw: Directory node in ledger " << sb.seq() - << " has index to object that is missing: " - << to_string(dirEntry); - return tefBAD_LEDGER; - } - - // Should only have the trustlines - if (auto const nodeType = safe_cast( - sleItem->getFieldU16(sfLedgerEntryType)); - nodeType != LedgerEntryType::ltRIPPLE_STATE) - { - JLOG(j_.error()) - << "AMM Withdraw: deleting non-trustline " << nodeType; - return tecINTERNAL; - } - - // Trustlines must have zero balance - if (sleItem->getFieldAmount(sfBalance) != beast::zero) - { - JLOG(j_.error()) << "AMM Withdraw: deleting trustline with " - "non-zero balance."; - return tecINTERNAL; - } - - auto const& [lowAccountID, highAccountID] = std::minmax( - sleItem->getFieldAmount(sfHighLimit).getIssuer(), ammAccountID); - if (auto const ter = trustDelete( - sb, sleItem, lowAccountID, highAccountID, ctx_.journal); - ter != tesSUCCESS) - { - JLOG(j_.error()) - << "AMM Withdraw: failed to delete the trustline."; - return ter; - } - - // Update the iterator - assert(uDirEntry == 1); - if (uDirEntry != 1) - { - JLOG(j_.error()) - << "AMM Withdraw: iterator re-validation failed."; - return tefBAD_LEDGER; - } - uDirEntry = 0; - - } while ( - dirNext(sb, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); - } - - sb.erase(ammSle); - sb.erase(sleAMMRoot); - - return tesSUCCESS; -} - std::pair AMMWithdraw::withdraw( Sandbox& view, @@ -622,9 +557,6 @@ AMMWithdraw::withdraw( return {res, STAmount{}}; } - if (lpTokensWithdrawActual == lpTokensAMMBalance) - return {deleteAccount(view, ammAccount), STAmount{}}; - return {tesSUCCESS, lpTokensAMMBalance - lpTokensWithdrawActual}; } diff --git a/src/ripple/app/tx/impl/AMMWithdraw.h b/src/ripple/app/tx/impl/AMMWithdraw.h index 1e5b1a99e4f..40686266315 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.h +++ b/src/ripple/app/tx/impl/AMMWithdraw.h @@ -215,14 +215,6 @@ class AMMWithdraw : public Transactor STAmount const& amount, STAmount const& ePrice, std::uint16_t tfee); - - /** Delete AMM account. - * @param view - * @param ammAccountID - * @return - */ - TER - deleteAccount(Sandbox& view, AccountID const& ammAccountID); }; } // namespace ripple diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp index 62cc9e1fbbf..6d5d25a4a3f 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.cpp +++ b/src/ripple/app/tx/impl/DeleteAccount.cpp @@ -292,76 +292,29 @@ DeleteAccount::doApply() if (!src || !dst) return tefBAD_LEDGER; - // Delete all of the entries in the account directory. Keylet const ownerDirKeylet{keylet::ownerDir(account_)}; - std::shared_ptr sleDirNode{}; - unsigned int uDirEntry{0}; - uint256 dirEntry{beast::zero}; - - if (view().exists(ownerDirKeylet) && - dirFirst(view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) - { - do - { - // Choose the right way to delete each directory node. - auto sleItem = view().peek(keylet::child(dirEntry)); - if (!sleItem) - { - // Directory node has an invalid index. Bail out. - JLOG(j_.fatal()) - << "DeleteAccount: Directory node in ledger " - << view().seq() << " has index to object that is missing: " - << to_string(dirEntry); - return tefBAD_LEDGER; - } - - LedgerEntryType const nodeType{safe_cast( - sleItem->getFieldU16(sfLedgerEntryType))}; - + auto const res = cleanupOnAccountDelete( + view(), + ownerDirKeylet, + [&](LedgerEntryType nodeType, + uint256 const& dirEntry, + std::shared_ptr& sleItem) -> TER { if (auto deleter = nonObligationDeleter(nodeType)) { TER const result{ deleter(ctx_.app, view(), account_, dirEntry, sleItem, j_)}; - if (!isTesSuccess(result)) - return result; - } - else - { - assert(!"Undeletable entry should be found in preclaim."); - JLOG(j_.error()) - << "DeleteAccount undeletable item not found in preclaim."; - return tecHAS_OBLIGATIONS; - } - - // dirFirst() and dirNext() are like iterators with exposed - // internal state. We'll take advantage of that exposed state - // to solve a common C++ problem: iterator invalidation while - // deleting elements from a container. - // - // We have just deleted one directory entry, which means our - // "iterator state" is invalid. - // - // 1. During the process of getting an entry from the - // directory uDirEntry was incremented from 0 to 1. - // - // 2. We then deleted the entry at index 0, which means the - // entry that was at 1 has now moved to 0. - // - // 3. So we verify that uDirEntry is indeed 1. Then we jam it - // back to zero to "un-invalidate" the iterator. - assert(uDirEntry == 1); - if (uDirEntry != 1) - { - JLOG(j_.error()) - << "DeleteAccount iterator re-validation failed."; - return tefBAD_LEDGER; + return result; } - uDirEntry = 0; - } while (dirNext( - view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); - } + assert(!"Undeletable entry should be found in preclaim."); + JLOG(j_.error()) << "DeleteAccount undeletable item not " + "found in preclaim."; + return tecHAS_OBLIGATIONS; + }, + ctx_.journal); + if (std::get(res) != tesSUCCESS) + return std::get(res); // Transfer any XRP remaining after the fee is paid to the destination: (*dst)[sfBalance] = (*dst)[sfBalance] + mSourceBalance; diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 23fa7b17115..a78e35fd1cf 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -321,12 +321,14 @@ AccountRootsNotDeleted::finalize( ReadView const&, beast::Journal const& j) { - // AMM account root can be deleted as the result of AMM withdraw + // AMM account root can be deleted as the result of AMM withdraw/delete // transaction when the total AMM LP Tokens balance goes to 0. // Not every AMM withdraw deletes the AMM account, accountsDeleted_ // is set if it is deleted. if ((tx.getTxnType() == ttACCOUNT_DELETE || - (tx.getTxnType() == ttAMM_WITHDRAW && accountsDeleted_ == 1)) && + ((tx.getTxnType() == ttAMM_WITHDRAW || + tx.getTxnType() == ttAMM_DELETE) && + accountsDeleted_ == 1)) && result == tesSUCCESS) { if (accountsDeleted_ == 1) diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index 3d723a8209c..ed877d3e615 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -144,7 +144,8 @@ SetTrust::preclaim(PreclaimContext const& ctx) } // If destination is AMM and the trustline doesn't exist then only - // allow SetTrust if the asset is AMM LP token + // allow SetTrust if the asset is AMM LP token and AMM is not + // in empty state. TER ter = tesSUCCESS; if (dstFlags & lsfAMM && !ctx.view.read(keylet::line(id, uDstAccountID, currency))) @@ -152,8 +153,10 @@ SetTrust::preclaim(PreclaimContext const& ctx) if (auto const ammSle = ctx.view.read({ltAMM, sleDst->getFieldH256(sfAMMID)})) { - if (ammSle->getFieldAmount(sfLPTokenBalance).getCurrency() != - saLimitAmount.getCurrency()) + if (auto const lpTokens = ammSle->getFieldAmount(sfLPTokenBalance); + lpTokens == beast::zero) + ter = tecAMM_EMPTY; + else if (lpTokens.getCurrency() != saLimitAmount.getCurrency()) ter = tecNO_PERMISSION; } else diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 66aa10227d4..ffdecfdc0c5 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -757,6 +757,25 @@ removeExpiredNFTokenOffers( } } +static void +removeDeletedTrustLines( + ApplyView& view, + std::vector const& trustLines, + beast::Journal viewJ) +{ + std::size_t removed = 0; + + for (auto const& index : trustLines) + { + if (auto const sleState = view.peek({ltRIPPLE_STATE, index})) + { + deleteAMMTrustLine(view, sleState, std::nullopt, viewJ); + if (++removed == maxDeletableAMMTrustLines) + return; + } + } +} + /** Reset the context, discarding any changes made and adjust the fee */ std::pair Transactor::reset(XRPAmount fee) @@ -849,7 +868,8 @@ Transactor::operator()() } else if ( (result == tecOVERSIZE) || (result == tecKILLED) || - (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) + (result == tecINCOMPLETE) || (result == tecEXPIRED) || + (isTecClaimHardFail(result, view().flags()))) { JLOG(j_.trace()) << "reapplying because of " << transToken(result); @@ -858,10 +878,21 @@ Transactor::operator()() // should be used, making it possible to do more useful work // when transactions fail with a `tec` code. std::vector removedOffers; + std::vector removedTrustLines; + std::vector expiredNFTokenOffers; - if ((result == tecOVERSIZE) || (result == tecKILLED)) + bool const doOffers = + ((result == tecOVERSIZE) || (result == tecKILLED)); + bool const doLines = (result == tecINCOMPLETE); + bool const doNFTokenOffers = (result == tecEXPIRED); + if (doOffers || doLines || doNFTokenOffers) { - ctx_.visit([&removedOffers]( + ctx_.visit([&doOffers, + &removedOffers, + &doLines, + &removedTrustLines, + &doNFTokenOffers, + &expiredNFTokenOffers]( uint256 const& index, bool isDelete, std::shared_ptr const& before, @@ -869,30 +900,23 @@ Transactor::operator()() if (isDelete) { assert(before && after); - if (before && after && (before->getType() == ltOFFER) && + if (doOffers && before && after && + (before->getType() == ltOFFER) && (before->getFieldAmount(sfTakerPays) == after->getFieldAmount(sfTakerPays))) { // Removal of offer found or made unfunded removedOffers.push_back(index); } - } - }); - } - std::vector expiredNFTokenOffers; + if (doLines && before && after && + (before->getType() == ltRIPPLE_STATE)) + { + // Removal of obsolete AMM trust line + removedTrustLines.push_back(index); + } - if (result == tecEXPIRED) - { - ctx_.visit([&expiredNFTokenOffers]( - uint256 const& index, - bool isDelete, - std::shared_ptr const& before, - std::shared_ptr const& after) { - if (isDelete) - { - assert(before && after); - if (before && after && + if (doNFTokenOffers && before && after && (before->getType() == ltNFTOKEN_OFFER)) expiredNFTokenOffers.push_back(index); } @@ -917,6 +941,10 @@ Transactor::operator()() removeExpiredNFTokenOffers( view(), expiredNFTokenOffers, ctx_.app.journal("View")); + if (result == tecINCOMPLETE) + removeDeletedTrustLines( + view(), removedTrustLines, ctx_.app.journal("view")); + applied = isTecClaim(result); } diff --git a/src/ripple/app/tx/impl/applySteps.cpp b/src/ripple/app/tx/impl/applySteps.cpp index f0d092d793d..fdb84c271a0 100644 --- a/src/ripple/app/tx/impl/applySteps.cpp +++ b/src/ripple/app/tx/impl/applySteps.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -165,6 +166,8 @@ invoke_preflight(PreflightContext const& ctx) return invoke_preflight_helper(ctx); case ttAMM_BID: return invoke_preflight_helper(ctx); + case ttAMM_DELETE: + return invoke_preflight_helper(ctx); default: assert(false); return {temUNKNOWN, TxConsequences{temUNKNOWN}}; @@ -278,6 +281,8 @@ invoke_preclaim(PreclaimContext const& ctx) return invoke_preclaim(ctx); case ttAMM_BID: return invoke_preclaim(ctx); + case ttAMM_DELETE: + return invoke_preclaim(ctx); default: assert(false); return temUNKNOWN; @@ -353,6 +358,8 @@ invoke_calculateBaseFee(ReadView const& view, STTx const& tx) return AMMVote::calculateBaseFee(view, tx); case ttAMM_BID: return AMMBid::calculateBaseFee(view, tx); + case ttAMM_DELETE: + return AMMDelete::calculateBaseFee(view, tx); default: assert(false); return XRPAmount{0}; @@ -529,6 +536,10 @@ invoke_apply(ApplyContext& ctx) AMMBid p(ctx); return p(); } + case ttAMM_DELETE: { + AMMDelete p(ctx); + return p(); + } default: assert(false); return {temUNKNOWN, false}; diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 939624ae24e..2035709701c 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -43,6 +43,7 @@ namespace ripple { enum class WaiveTransferFee { Yes, No }; +enum class AllNodesDeleted { Yes, No }; //------------------------------------------------------------------------------ // @@ -458,6 +459,34 @@ transferXRP( [[nodiscard]] TER requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); +/** Cleanup owner directory entries on account delete. + * Used for a regular and AMM accounts deletion. The caller + * has to provide the deleter function, which handles details of + * specific account deletion. + * @return {tesSUCCESS, AllNodesDeleted::No} indicates maxNodesToDelete + * are deleted and there remains more nodes to delete. + */ +[[nodiscard]] std::pair +cleanupOnAccountDelete( + ApplyView& view, + Keylet const& ownerDirKeylet, + std::function&)> + deleter, + beast::Journal j, + std::optional maxNodesToDelete = std::nullopt); + +/** Delete trustline to AMM. The passed `sle` must be obtained from a prior + * call to view.peek(). Fail if neither side of the trustline is AMM or + * if ammAccountID is seated and is not one of the trustline's side. + */ +// [[nodiscard]] // nodiscard commented out so Transactor compiles. +TER +deleteAMMTrustLine( + ApplyView& view, + std::shared_ptr sleState, + std::optional const& ammAccountID, + beast::Journal j); + } // namespace ripple #endif diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 2de92bfc20f..b9c9d6bdfae 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1544,4 +1544,119 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account) return tesSUCCESS; } +std::pair +cleanupOnAccountDelete( + ApplyView& view, + Keylet const& ownerDirKeylet, + std::function&)> + deleter, + beast::Journal j, + std::optional maxNodesToDelete) +{ + // Delete all the entries in the account directory. + std::shared_ptr sleDirNode{}; + unsigned int uDirEntry{0}; + uint256 dirEntry{beast::zero}; + std::uint16_t deleted = 0; + + if (view.exists(ownerDirKeylet) && + dirFirst(view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) + { + do + { + if (maxNodesToDelete && ++deleted > *maxNodesToDelete) + return {tesSUCCESS, AllNodesDeleted::No}; + + // Choose the right way to delete each directory node. + auto sleItem = view.peek(keylet::child(dirEntry)); + if (!sleItem) + { + // Directory node has an invalid index. Bail out. + JLOG(j.fatal()) + << "DeleteAccount: Directory node in ledger " << view.seq() + << " has index to object that is missing: " + << to_string(dirEntry); + return {tefBAD_LEDGER, AllNodesDeleted::No}; + } + + LedgerEntryType const nodeType{safe_cast( + sleItem->getFieldU16(sfLedgerEntryType))}; + + // Deleter handles the details of specific account deletion + if (auto const ter = deleter(nodeType, dirEntry, sleItem); + ter != tesSUCCESS) + return {ter, AllNodesDeleted::No}; + + // dirFirst() and dirNext() are like iterators with exposed + // internal state. We'll take advantage of that exposed state + // to solve a common C++ problem: iterator invalidation while + // deleting elements from a container. + // + // We have just deleted one directory entry, which means our + // "iterator state" is invalid. + // + // 1. During the process of getting an entry from the + // directory uDirEntry was incremented from 0 to 1. + // + // 2. We then deleted the entry at index 0, which means the + // entry that was at 1 has now moved to 0. + // + // 3. So we verify that uDirEntry is indeed 1. Then we jam it + // back to zero to "un-invalidate" the iterator. + assert(uDirEntry == 1); + if (uDirEntry != 1) + { + JLOG(j.error()) + << "DeleteAccount iterator re-validation failed."; + return {tefBAD_LEDGER, AllNodesDeleted::No}; + } + uDirEntry = 0; + + } while ( + dirNext(view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); + } + + return {tesSUCCESS, AllNodesDeleted::Yes}; +} + +TER +deleteAMMTrustLine( + ApplyView& view, + std::shared_ptr sleState, + std::optional const& ammAccountID, + beast::Journal j) +{ + if (!sleState) + return tecINTERNAL; + + auto const& [low, high] = std::minmax( + sleState->getFieldAmount(sfLowLimit).getIssuer(), + sleState->getFieldAmount(sfHighLimit).getIssuer()); + auto sleLow = view.peek(keylet::account(low)); + auto sleHigh = view.peek(keylet::account(high)); + if (!sleLow || !sleHigh) + return tecINTERNAL; + bool const ammLow = sleLow->getFlags() & lsfAMM; + bool const ammHigh = sleHigh->getFlags() & lsfAMM; + + // One side must be AMM + if (!(ammLow ^ ammHigh) || + (ammAccountID && + ((ammLow && low != ammAccountID) || + (ammHigh && high != ammAccountID)))) + return terNO_AMM; + + if (auto const ter = trustDelete(view, sleState, low, high, j); + ter != tesSUCCESS) + { + JLOG(j.error()) + << "deleteAMMTrustLine: failed to delete the trustline."; + return ter; + } + + adjustOwnerCount(view, !ammLow ? sleLow : sleHigh, -1, j); + + return tesSUCCESS; +} + } // namespace ripple diff --git a/src/ripple/protocol/Protocol.h b/src/ripple/protocol/Protocol.h index 5df24271f68..d922630f3df 100644 --- a/src/ripple/protocol/Protocol.h +++ b/src/ripple/protocol/Protocol.h @@ -95,6 +95,8 @@ using LedgerIndex = std::uint32_t; */ using TxID = uint256; +std::size_t constexpr maxDeletableAMMTrustLines = 1500; + } // namespace ripple #endif diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 955181de7e2..b7599d74b9c 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -298,7 +298,9 @@ enum TECcodes : TERUnderlyingType { tecUNFUNDED_AMM = 162, tecAMM_BALANCE = 163, tecAMM_FAILED = 164, - tecAMM_INVALID_TOKENS = 165 + tecAMM_INVALID_TOKENS = 165, + tecAMM_EMPTY = 166, + tecINCOMPLETE = 167 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/TxFlags.h b/src/ripple/protocol/TxFlags.h index 75d405cb827..39680e41d95 100644 --- a/src/ripple/protocol/TxFlags.h +++ b/src/ripple/protocol/TxFlags.h @@ -171,12 +171,13 @@ constexpr std::uint32_t tfSingleAsset = 0x00080000; constexpr std::uint32_t tfTwoAsset = 0x00100000; constexpr std::uint32_t tfOneAssetLPToken = 0x00200000; constexpr std::uint32_t tfLimitLPToken = 0x00400000; +constexpr std::uint32_t tfTwoAssetIfEmpty = 0x00800000; constexpr std::uint32_t tfWithdrawSubTx = tfLPToken | tfSingleAsset | tfTwoAsset | tfOneAssetLPToken | tfLimitLPToken | tfWithdrawAll | tfOneAssetWithdrawAll; constexpr std::uint32_t tfDepositSubTx = tfLPToken | tfSingleAsset | tfTwoAsset | tfOneAssetLPToken | - tfLimitLPToken; + tfLimitLPToken | tfTwoAssetIfEmpty; constexpr std::uint32_t tfWithdrawMask = ~(tfUniversal | tfWithdrawSubTx); constexpr std::uint32_t tfDepositMask = ~(tfUniversal | tfDepositSubTx); diff --git a/src/ripple/protocol/TxFormats.h b/src/ripple/protocol/TxFormats.h index a79f7dd79cb..2d7ba40c44c 100644 --- a/src/ripple/protocol/TxFormats.h +++ b/src/ripple/protocol/TxFormats.h @@ -157,6 +157,9 @@ enum TxType : std::uint16_t /** This transaction type bids for the auction slot */ ttAMM_BID = 39, + /** This transaction type deletes AMM in the empty state */ + ttAMM_DELETE = 40, + /** This system-generated transaction type is used to update the status of the various amendments. For details, see: https://xrpl.org/amendments.html diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index c16e9541fbf..a5057b144bb 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -45,6 +45,7 @@ transResults() MAKE_ERROR(tecAMM_BALANCE, "AMM has invalid balance."), MAKE_ERROR(tecAMM_INVALID_TOKENS, "AMM invalid LP tokens."), MAKE_ERROR(tecAMM_FAILED, "AMM transaction failed."), + MAKE_ERROR(tecAMM_EMPTY, "AMM is in empty state."), MAKE_ERROR(tecCLAIM, "Fee claimed. Sequence used. No action."), MAKE_ERROR(tecDIR_FULL, "Can not add entry to full directory."), MAKE_ERROR(tecFAILED_PROCESSING, "Failed to correctly process transaction."), @@ -92,6 +93,7 @@ transResults() MAKE_ERROR(tecINSUFFICIENT_FUNDS, "Not enough funds available to complete requested transaction."), MAKE_ERROR(tecOBJECT_NOT_FOUND, "A requested object could not be located."), MAKE_ERROR(tecINSUFFICIENT_PAYMENT, "The payment is not sufficient."), + MAKE_ERROR(tecINCOMPLETE, "Transaction requires multple submissions to complete the processing."), MAKE_ERROR(tefALREADY, "The exact transaction was already in this ledger."), MAKE_ERROR(tefBAD_ADD_AUTH, "Not authorized to add account."), diff --git a/src/ripple/protocol/impl/TxFormats.cpp b/src/ripple/protocol/impl/TxFormats.cpp index 062174815c9..549e6619c1a 100644 --- a/src/ripple/protocol/impl/TxFormats.cpp +++ b/src/ripple/protocol/impl/TxFormats.cpp @@ -101,6 +101,7 @@ TxFormats::TxFormats() {sfEPrice, soeOPTIONAL}, {sfLPTokenOut, soeOPTIONAL}, {sfTicketSequence, soeOPTIONAL}, + {sfTradingFee, soeOPTIONAL}, }, commonFields); @@ -139,6 +140,15 @@ TxFormats::TxFormats() }, commonFields); + add(jss::AMMDelete, + ttAMM_DELETE, + { + {sfAsset, soeREQUIRED}, + {sfAsset2, soeREQUIRED}, + {sfTicketSequence, soeOPTIONAL}, + }, + commonFields); + add(jss::OfferCancel, ttOFFER_CANCEL, { diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index c7faa4ff98b..a0f81858637 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -52,6 +52,7 @@ JSS(AMMBid); // transaction type JSS(AMMID); // field JSS(AMMCreate); // transaction type JSS(AMMDeposit); // transaction type +JSS(AMMDelete); // transaction type JSS(AMMVote); // transaction type JSS(AMMWithdraw); // transaction type JSS(Amendments); // ledger type. diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 6727427d72a..a7c37067c85 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -108,6 +108,15 @@ struct AMM_test : public jtx::AMMTest env.close(); AMM ammAlice(env, alice, XRP(10'000), USD(10'000)); } + + // Trading fee + testAMM( + [&](AMM& amm, Env&) { + BEAST_EXPECT(amm.expectTradingFee(1'000)); + BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{0})); + }, + std::nullopt, + 1'000); } void @@ -415,96 +424,167 @@ struct AMM_test : public jtx::AMMTest std::optional, std::optional, std::optional, - std::optional>> + std::optional, + std::optional>> invalidOptions = { - // flags, tokens, asset1In, asset2in, EPrice - {tfLPToken, 1'000, std::nullopt, USD(100), std::nullopt}, - {tfLPToken, 1'000, XRP(100), std::nullopt, std::nullopt}, + // flags, tokens, asset1In, asset2in, EPrice, tfee {tfLPToken, 1'000, std::nullopt, + USD(100), std::nullopt, - STAmount{USD, 1, -1}}, + std::nullopt}, + {tfLPToken, + 1'000, + XRP(100), + std::nullopt, + std::nullopt, + std::nullopt}, + {tfLPToken, + 1'000, + std::nullopt, + std::nullopt, + STAmount{USD, 1, -1}, + std::nullopt}, {tfLPToken, std::nullopt, USD(100), std::nullopt, - STAmount{USD, 1, -1}}, + STAmount{USD, 1, -1}, + std::nullopt}, {tfLPToken, 1'000, XRP(100), std::nullopt, - STAmount{USD, 1, -1}}, + STAmount{USD, 1, -1}, + std::nullopt}, + {tfLPToken, + 1'000, + std::nullopt, + std::nullopt, + std::nullopt, + 1'000}, {tfSingleAsset, 1'000, std::nullopt, std::nullopt, + std::nullopt, std::nullopt}, {tfSingleAsset, std::nullopt, std::nullopt, USD(100), + std::nullopt, std::nullopt}, {tfSingleAsset, std::nullopt, std::nullopt, std::nullopt, - STAmount{USD, 1, -1}}, + STAmount{USD, 1, -1}, + std::nullopt}, + {tfSingleAsset, + std::nullopt, + USD(100), + std::nullopt, + std::nullopt, + 1'000}, {tfTwoAsset, 1'000, std::nullopt, std::nullopt, + std::nullopt, std::nullopt}, {tfTwoAsset, std::nullopt, XRP(100), USD(100), - STAmount{USD, 1, -1}}, + STAmount{USD, 1, -1}, + std::nullopt}, {tfTwoAsset, std::nullopt, XRP(100), std::nullopt, + std::nullopt, std::nullopt}, + {tfTwoAsset, + std::nullopt, + XRP(100), + USD(100), + std::nullopt, + 1'000}, {tfTwoAsset, std::nullopt, std::nullopt, USD(100), - STAmount{USD, 1, -1}}, + STAmount{USD, 1, -1}, + std::nullopt}, {tfOneAssetLPToken, 1'000, std::nullopt, std::nullopt, + std::nullopt, std::nullopt}, {tfOneAssetLPToken, std::nullopt, XRP(100), USD(100), + std::nullopt, std::nullopt}, {tfOneAssetLPToken, std::nullopt, XRP(100), std::nullopt, - STAmount{USD, 1, -1}}, + STAmount{USD, 1, -1}, + std::nullopt}, + {tfOneAssetLPToken, + 1'000, + XRP(100), + std::nullopt, + std::nullopt, + 1'000}, {tfLimitLPToken, 1'000, std::nullopt, std::nullopt, + std::nullopt, std::nullopt}, {tfLimitLPToken, 1'000, USD(100), std::nullopt, + std::nullopt, std::nullopt}, {tfLimitLPToken, std::nullopt, USD(100), XRP(100), + std::nullopt, std::nullopt}, {tfLimitLPToken, + std::nullopt, + XRP(100), + std::nullopt, + STAmount{USD, 1, -1}, + 1'000}, + {tfTwoAssetIfEmpty, + std::nullopt, std::nullopt, std::nullopt, std::nullopt, - STAmount{USD, 1, -1}}}; + 1'000}, + {tfTwoAssetIfEmpty, + 1'000, + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt}, + {tfTwoAssetIfEmpty, + std::nullopt, + XRP(100), + USD(100), + STAmount{USD, 1, -1}, + std::nullopt}, + }; for (auto const& it : invalidOptions) { ammAlice.deposit( @@ -516,6 +596,7 @@ struct AMM_test : public jtx::AMMTest std::get<0>(it), std::nullopt, std::nullopt, + std::get<5>(it), ter(temMALFORMED)); } @@ -542,6 +623,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, {{iss1, iss2}}, std::nullopt, + std::nullopt, ter(terNO_AMM)); } @@ -616,6 +698,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, seq(1), + std::nullopt, ter(terNO_ACCOUNT)); // Invalid AMM @@ -628,6 +711,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, {{USD, GBP}}, std::nullopt, + std::nullopt, ter(terNO_AMM)); // Single deposit: 100000 tokens worth of USD @@ -641,6 +725,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, std::nullopt, + std::nullopt, ter(tecAMM_FAILED)); // Single deposit: 100000 tokens worth of XRP @@ -654,6 +739,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, std::nullopt, + std::nullopt, ter(tecAMM_FAILED)); // Deposit amount is invalid @@ -688,6 +774,15 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, ter(tecAMM_INVALID_TOKENS)); + + // Deposit non-empty AMM + ammAlice.deposit( + carol, + XRP(100), + USD(100), + std::nullopt, + tfTwoAssetIfEmpty, + ter(tecAMM_EMPTY)); }); // Invalid AMM @@ -789,6 +884,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, std::nullopt, + std::nullopt, ter(tecUNFUNDED_AMM)); }); @@ -808,6 +904,7 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, std::nullopt, + std::nullopt, ter(tecUNFUNDED_AMM)); }); @@ -883,6 +980,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(temBAD_AMM_TOKENS)); // min amounts can't be <= zero ammAlice.deposit( @@ -894,6 +992,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(temBAD_AMOUNT)); ammAlice.deposit( carol, @@ -904,6 +1003,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(temBAD_AMOUNT)); // min amount bad currency ammAlice.deposit( @@ -915,6 +1015,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(temBAD_CURRENCY)); // min amount bad token pair ammAlice.deposit( @@ -926,6 +1027,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(temBAD_AMM_TOKENS)); ammAlice.deposit( carol, @@ -936,6 +1038,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(temBAD_AMM_TOKENS)); }); @@ -951,6 +1054,7 @@ struct AMM_test : public jtx::AMMTest tfLPToken, std::nullopt, std::nullopt, + std::nullopt, ter(tecAMM_FAILED)); ammAlice.deposit( carol, @@ -961,6 +1065,7 @@ struct AMM_test : public jtx::AMMTest tfLPToken, std::nullopt, std::nullopt, + std::nullopt, ter(tecAMM_FAILED)); // Equal deposit by asset ammAlice.deposit( @@ -972,6 +1077,7 @@ struct AMM_test : public jtx::AMMTest tfTwoAsset, std::nullopt, std::nullopt, + std::nullopt, ter(tecAMM_FAILED)); // Single deposit by asset ammAlice.deposit( @@ -983,6 +1089,7 @@ struct AMM_test : public jtx::AMMTest tfSingleAsset, std::nullopt, std::nullopt, + std::nullopt, ter(tecAMM_FAILED)); }); } @@ -1249,6 +1356,16 @@ struct AMM_test : public jtx::AMMTest std::nullopt, std::nullopt, ter(temINVALID_FLAG)); + ammAlice.withdraw( + alice, + 1'000'000, + std::nullopt, + std::nullopt, + std::nullopt, + tfTwoAssetIfEmpty, + std::nullopt, + std::nullopt, + ter(temINVALID_FLAG)); // Invalid options std::vector cfg) { + cfg->FEES.reference_fee = XRPAmount(1); + return cfg; + }), + all); + fund(env, gw, {alice}, XRP(20'000), {USD(10'000)}); + AMM amm(env, gw, XRP(10'000), USD(10'000)); + for (auto i = 0; i < 1'600; ++i) + { + Account const a{std::to_string(i)}; + env.fund(XRP(1'000), a); + env(trust(a, STAmount{amm.lptIssue(), 10'000})); + env.close(); + } + // The auto-delete deleted 1,500 trustlines and AMM is + // in empty state. + amm.withdrawAll(gw); + BEAST_EXPECT(amm.ammExists()); + + // Bid,Vote,Deposit,Withdraw,SetTrust failing with + // tecAMM_EMPTY. Deposit succeeds with tfTwoAssetIfEmpty option. + amm.bid( + alice, + 1000, + std::nullopt, + {}, + std::nullopt, + std::nullopt, + std::nullopt, + ter(tecAMM_EMPTY)); + amm.vote( + std::nullopt, + 100, + std::nullopt, + std::nullopt, + std::nullopt, + ter(tecAMM_EMPTY)); + amm.withdraw( + alice, 100, std::nullopt, std::nullopt, ter(tecAMM_EMPTY)); + amm.deposit( + alice, + USD(100), + std::nullopt, + std::nullopt, + std::nullopt, + ter(tecAMM_EMPTY)); + env(trust(alice, STAmount{amm.lptIssue(), 10'000}), + ter(tecAMM_EMPTY)); + + // Can deposit with tfTwoAssetIfEmpty option + amm.deposit( + alice, + std::nullopt, + XRP(10'000), + USD(10'000), + std::nullopt, + tfTwoAssetIfEmpty, + std::nullopt, + std::nullopt, + 1'000); + BEAST_EXPECT( + amm.expectBalances(XRP(10'000), USD(10'000), amm.tokens())); + BEAST_EXPECT(amm.expectTradingFee(1'000)); + BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{0})); + + // Withdrawing all tokens deletes AMM since the number + // of remaining trustlines is less than 1,500. + amm.withdrawAll(alice); + BEAST_EXPECT(!amm.ammExists()); + BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount()))); + } + + { + Env env( + *this, + envconfig([](std::unique_ptr cfg) { + cfg->FEES.reference_fee = XRPAmount(1); + return cfg; + }), + all); + fund(env, gw, {alice}, XRP(20'000), {USD(10'000)}); + AMM amm(env, gw, XRP(10'000), USD(10'000)); + for (auto i = 0; i < 3'100; ++i) + { + Account const a{std::to_string(i)}; + env.fund(XRP(1'000), a); + env(trust(a, STAmount{amm.lptIssue(), 10'000})); + env.close(); + } + // Deletes 1,500 trustlines + amm.withdrawAll(gw); + BEAST_EXPECT(amm.ammExists()); + + // AMMDelete has to be called twice to delete AMM. + // Deletes 1,500 trustlines. + amm.ammDelete(alice, ter(tecINCOMPLETE)); + BEAST_EXPECT(amm.ammExists()); + // Deletes remaining trustlines and deletes AMM. + amm.ammDelete(alice); + BEAST_EXPECT(!amm.ammExists()); + BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount()))); + } + } + void testCore() { @@ -4171,6 +4403,7 @@ struct AMM_test : public jtx::AMMTest testAMMAndCLOB(); testTradingFee(); testAdjustedTokens(); + testAutoDelete(); } void diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index dd7e91a3ef8..7803bfba60c 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -180,6 +180,7 @@ class AMM std::optional const& flags, std::optional> const& assets, std::optional const& seq, + std::optional const& tfee = std::nullopt, std::optional const& ter = std::nullopt); IOUAmount @@ -286,6 +287,11 @@ class AMM return ammRpcInfo(lp); } + void + ammDelete( + AccountID const& deleter, + std::optional const& ter = std::nullopt); + private: void setTokens( diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 37efdc112a0..7f9d893df9a 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -382,6 +382,7 @@ AMM::deposit( flags, std::nullopt, std::nullopt, + std::nullopt, ter); } @@ -404,6 +405,7 @@ AMM::deposit( flags, std::nullopt, std::nullopt, + std::nullopt, ter); } @@ -417,6 +419,7 @@ AMM::deposit( std::optional const& flags, std::optional> const& assets, std::optional const& seq, + std::optional const& tfee, std::optional const& ter) { Json::Value jv; @@ -428,6 +431,8 @@ AMM::deposit( asset2In->setJson(jv[jss::Amount2]); if (maxEP) maxEP->setJson(jv[jss::EPrice]); + if (tfee) + jv[jss::TradingFee] = *tfee; std::uint32_t jvflags = 0; if (flags) jvflags = *flags; @@ -697,6 +702,18 @@ AMM::expectAuctionSlot(auto&& cb) const return false; } +void +AMM::ammDelete(AccountID const& deleter, std::optional const& ter) +{ + Json::Value jv; + jv[jss::Account] = to_string(deleter); + setTokens(jv); + jv[jss::TransactionType] = jss::AMMDelete; + if (fee_ != 0) + jv[jss::Fee] = std::to_string(fee_); + submit(jv, std::nullopt, ter); +} + namespace amm { Json::Value trust(AccountID const& account, STAmount const& amount, std::uint32_t flags) From 40317c514f7f83828ac916069ac5e644fdd28f81 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 24 Jul 2023 14:54:48 -0400 Subject: [PATCH 06/18] Fix missing AMMDelete transactor files --- src/ripple/app/tx/impl/AMMDelete.cpp | 89 ++++++++++++++++++++++++++++ src/ripple/app/tx/impl/AMMDelete.h | 54 +++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 src/ripple/app/tx/impl/AMMDelete.cpp create mode 100644 src/ripple/app/tx/impl/AMMDelete.h diff --git a/src/ripple/app/tx/impl/AMMDelete.cpp b/src/ripple/app/tx/impl/AMMDelete.cpp new file mode 100644 index 00000000000..ed9ae7d7471 --- /dev/null +++ b/src/ripple/app/tx/impl/AMMDelete.cpp @@ -0,0 +1,89 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2022 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace ripple { + +NotTEC +AMMDelete::preflight(PreflightContext const& ctx) +{ + if (!ammEnabled(ctx.rules)) + return temDISABLED; + + if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) + return ret; + + if (ctx.tx.getFlags() & tfUniversalMask) + { + JLOG(ctx.j.debug()) << "AMM Delete: invalid flags."; + return temINVALID_FLAG; + } + + if (auto const res = invalidAMMAssetPair(ctx.tx[sfAsset], ctx.tx[sfAsset2])) + { + JLOG(ctx.j.debug()) << "AMM Delete: Invalid asset pair."; + return res; + } + + return preflight2(ctx); +} + +TER +AMMDelete::preclaim(PreclaimContext const& ctx) +{ + auto const ammSle = + ctx.view.read(keylet::amm(ctx.tx[sfAsset], ctx.tx[sfAsset2])); + if (!ammSle) + { + JLOG(ctx.j.debug()) << "AMM Delete: Invalid asset pair."; + return terNO_AMM; + } + + auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance]; + if (lpTokensBalance != beast::zero) + return tecAMM_EMPTY; + + return tesSUCCESS; +} + +TER +AMMDelete::doApply() +{ + // This is the ledger view that we work against. Transactions are applied + // as we go on processing transactions. + Sandbox sb(&ctx_.view()); + + auto const ter = + deleteAMMAccount(sb, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], j_); + if (ter == tesSUCCESS || ter == tecINCOMPLETE) + sb.apply(ctx_.rawView()); + + return ter; +} + +} // namespace ripple diff --git a/src/ripple/app/tx/impl/AMMDelete.h b/src/ripple/app/tx/impl/AMMDelete.h new file mode 100644 index 00000000000..00866914ae2 --- /dev/null +++ b/src/ripple/app/tx/impl/AMMDelete.h @@ -0,0 +1,54 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TX_AMMDELETE_H_INCLUDED +#define RIPPLE_TX_AMMDELETE_H_INCLUDED + +#include + +namespace ripple { + +/** AMMDelete implements AMM delete transactor. This is a mechanism to + * delete AMM in an empty state when the number of LP tokens is 0. + * AMMDelete deletes the truslines up to configured maximum. If all + * trustlines are deleted then AMM ltAMM and root account are deleted. + * Otherwise AMMDelete should be called again. + */ +class AMMDelete : public Transactor +{ +public: + static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; + + explicit AMMDelete(ApplyContext& ctx) : Transactor(ctx) + { + } + + static NotTEC + preflight(PreflightContext const& ctx); + + static TER + preclaim(PreclaimContext const& ctx); + + TER + doApply() override; +}; + +} // namespace ripple + +#endif // RIPPLE_TX_AMMDELETE_H_INCLUDED From 75af923833ad251005b1d3f132da00395a2045de Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 24 Jul 2023 15:50:52 -0400 Subject: [PATCH 07/18] Update api changelog for AMM feature --- API-CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index bbbe71d5545..eaf80448ee7 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -47,6 +47,19 @@ Additions are intended to be non-breaking (because they are purely additive). - Adds the [Clawback transaction type](https://github.com/XRPLF/XRPL-Standards/blob/master/XLS-39d-clawback/README.md#331-clawback-transaction), containing these fields: - `Account`: The issuer of the asset being clawed back. Must also be the sender of the transaction. - `Amount`: The amount being clawed back, with the `Amount.issuer` being the token holder's address. +- Adds [AMM](https://github.com/XRPLF/XRPL-Standards/discussions/78) ([#4294](https://github.com/XRPLF/rippled/pull/4294), [#4626](https://github.com/XRPLF/rippled/pull/4626)) feature: + - Adds `amm_info` API to retrieve AMM information for a given tokens pair. + - Adds `AMMCreate` transaction type to create `AMM` instance. + - Adds `AMMDeposit` transaction type to deposit funds into `AMM` instance. + - Adds `AMMWithdraw` transaction type to withdraw funds from `AMM` instance. + - Adds `AMMVote` transaction type to vote for the trading fee of `AMM` instance. + - Adds `AMMBid` transaction type to bid for the Auction Slot of `AMM` instance. + - Adds `AMMDelete` transaction type to delete `AMM` instance. + - Adds `lsfAMM` `AccountRoot` flag to indicate that the account is `AMM`'s account. + - Adds `lsfAMMNode` `TrustLine` flag to indicate that one side of the `TrustLine` is `AMM` account. + - Adds `tfLPToken`, `tfSingleAsset`, `tfTwoAsset`, `tfOneAssetLPToken`, `tfLimitLPToken`, `tfTwoAssetIfEmpty`, + `tfWithdrawAll`, `tfOneAssetWithdrawAll` which allow a trader to specify different fields combination + for `AMMDeposit` and `AMMWithdraw` transactions. ## XRP Ledger version 1.11.0 From cd88481d68d1c2e246c9bdd962f4301eb83dde1a Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 28 Jul 2023 17:02:30 -0400 Subject: [PATCH 08/18] Maintain AMM trustlines count in AMM root account and other changes addressing reviewer's feedback * AMMWithdraw deletes AMM trustlines and AMM account/object only if number of trustlines is less than max * Add tecAMM_NOT_EMPTY to flag cases that expect AMM in empty state * Don't validate for invalid asset pair in AMMDelete * Use Sandbox instead of ApplyView in some utility functions * Other minor refactoring --- src/ripple/app/misc/AMMUtils.h | 2 +- src/ripple/app/misc/impl/AMMUtils.cpp | 42 ++++++++++++++------------ src/ripple/app/tx/impl/AMMDelete.cpp | 8 +---- src/ripple/app/tx/impl/AMMDeposit.cpp | 2 +- src/ripple/app/tx/impl/AMMWithdraw.cpp | 7 ++++- src/ripple/app/tx/impl/SetTrust.cpp | 4 +++ src/ripple/app/tx/impl/Transactor.cpp | 12 +++++--- src/ripple/ledger/View.h | 5 ++- src/ripple/ledger/impl/View.cpp | 28 ++++++++++------- src/ripple/protocol/Protocol.h | 2 +- src/ripple/protocol/TER.h | 3 +- src/ripple/protocol/impl/TER.cpp | 1 + src/test/app/AMM_test.cpp | 22 ++++++++------ 13 files changed, 79 insertions(+), 59 deletions(-) diff --git a/src/ripple/app/misc/AMMUtils.h b/src/ripple/app/misc/AMMUtils.h index 152229f88cf..a75fd1f43cf 100644 --- a/src/ripple/app/misc/AMMUtils.h +++ b/src/ripple/app/misc/AMMUtils.h @@ -98,7 +98,7 @@ ammAccountHolds( */ TER deleteAMMAccount( - ApplyView& view, + Sandbox& view, Issue const& asset, Issue const& asset2, beast::Journal j); diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index fa8a3d656dd..d4f7bb5458b 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -188,15 +188,15 @@ ammAccountHolds( return STAmount{issue}; } -static std::pair +static std::pair deleteAMMTrustLines( - ApplyView& view, + Sandbox& sb, AccountID const& ammAccountID, std::uint16_t maxTrustlinesToDelete, beast::Journal j) { return cleanupOnAccountDelete( - view, + sb, keylet::ownerDir(ammAccountID), [&](LedgerEntryType nodeType, uint256 const&, @@ -219,7 +219,7 @@ deleteAMMTrustLines( return tecINTERNAL; } - return deleteAMMTrustLine(view, sleItem, ammAccountID, j); + return deleteAMMTrustLine(sb, sleItem, ammAccountID, j); }, j, maxTrustlinesToDelete); @@ -227,33 +227,37 @@ deleteAMMTrustLines( TER deleteAMMAccount( - ApplyView& view, + Sandbox& sb, Issue const& asset, Issue const& asset2, beast::Journal j) { - auto ammSle = view.peek(keylet::amm(asset, asset2)); + auto ammSle = sb.peek(keylet::amm(asset, asset2)); if (!ammSle) return tecINTERNAL; auto const ammAccountID = (*ammSle)[sfAccount]; - auto sleAMMRoot = view.peek(keylet::account(ammAccountID)); + auto sleAMMRoot = sb.peek(keylet::account(ammAccountID)); if (!sleAMMRoot) - return tecINTERNAL; - - if (auto const res = deleteAMMTrustLines( - view, ammAccountID, maxDeletableAMMTrustLines, j); - std::get(res) != tesSUCCESS) - return std::get(res); - // All trustlines are deleted, can delete ltAMM and the account - else if (res.second == AllNodesDeleted::Yes) { - view.erase(ammSle); - view.erase(sleAMMRoot); - return tesSUCCESS; + JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist " + << to_string(ammAccountID); } - return tecINCOMPLETE; + auto const [ter, done] = + deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j); + + if (ter != tesSUCCESS) + return ter; + + if (!done) + return tecINCOMPLETE; + + sb.erase(ammSle); + if (sleAMMRoot) + sb.erase(sleAMMRoot); + + return tesSUCCESS; } void diff --git a/src/ripple/app/tx/impl/AMMDelete.cpp b/src/ripple/app/tx/impl/AMMDelete.cpp index ed9ae7d7471..25502be4f44 100644 --- a/src/ripple/app/tx/impl/AMMDelete.cpp +++ b/src/ripple/app/tx/impl/AMMDelete.cpp @@ -44,12 +44,6 @@ AMMDelete::preflight(PreflightContext const& ctx) return temINVALID_FLAG; } - if (auto const res = invalidAMMAssetPair(ctx.tx[sfAsset], ctx.tx[sfAsset2])) - { - JLOG(ctx.j.debug()) << "AMM Delete: Invalid asset pair."; - return res; - } - return preflight2(ctx); } @@ -66,7 +60,7 @@ AMMDelete::preclaim(PreclaimContext const& ctx) auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance]; if (lpTokensBalance != beast::zero) - return tecAMM_EMPTY; + return tecAMM_NOT_EMPTY; return tesSUCCESS; } diff --git a/src/ripple/app/tx/impl/AMMDeposit.cpp b/src/ripple/app/tx/impl/AMMDeposit.cpp index 94da211a318..5ea49f5445c 100644 --- a/src/ripple/app/tx/impl/AMMDeposit.cpp +++ b/src/ripple/app/tx/impl/AMMDeposit.cpp @@ -191,7 +191,7 @@ AMMDeposit::preclaim(PreclaimContext const& ctx) if (ctx.tx.getFlags() & tfTwoAssetIfEmpty) { if (lptAMMBalance != beast::zero) - return tecAMM_EMPTY; + return tecAMM_NOT_EMPTY; if (amountBalance != beast::zero || amount2Balance != beast::zero) { JLOG(ctx.j.debug()) << "AMM Deposit: tokens balance is not zero."; diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index 1016b5c3021..f040df042c9 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -302,6 +302,9 @@ AMMWithdraw::applyGuts(Sandbox& sb) if (!ammSle) return {tecINTERNAL, false}; auto const ammAccountID = (*ammSle)[sfAccount]; + auto const accountSle = sb.read(keylet::account(ammAccountID)); + if (!accountSle) + return {tecINTERNAL, false}; auto const lpTokens = ammLPHolds(ctx_.view(), *ammSle, ctx_.tx[sfAccount], ctx_.journal); auto const lpTokensWithdraw = @@ -377,7 +380,9 @@ AMMWithdraw::applyGuts(Sandbox& sb) return {result, false}; bool updateBalance = true; - if (newLPTokenBalance == beast::zero) + // Delete only if number of trustlines is less or equal than max + if (newLPTokenBalance == beast::zero && + accountSle->getFieldU32(sfOwnerCount) <= maxDeletableAMMTrustLines) { if (auto const ter = deleteAMMAccount(sb, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], j_); diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index ed877d3e615..7352709a660 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -541,6 +541,10 @@ SetTrust::doApply() uQualityIn, uQualityOut, viewJ); + + // Maintain number of AMM trustlines in AMM root account + if (terResult == tesSUCCESS && (sleDst->getFlags() & lsfAMM)) + adjustOwnerCount(view(), sleDst, 1, j_); } return terResult; diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index ffdecfdc0c5..27ce8ed039e 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -763,16 +763,18 @@ removeDeletedTrustLines( std::vector const& trustLines, beast::Journal viewJ) { - std::size_t removed = 0; + if (trustLines.size() > maxDeletableAMMTrustLines) + { + JLOG(viewJ.error()) + << "removeDeletedTrustLines: deleted trustlines exceed max " + << trustLines.size(); + return; + } for (auto const& index : trustLines) { if (auto const sleState = view.peek({ltRIPPLE_STATE, index})) - { deleteAMMTrustLine(view, sleState, std::nullopt, viewJ); - if (++removed == maxDeletableAMMTrustLines) - return; - } } } diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 2035709701c..27370ebd9fa 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -43,7 +43,6 @@ namespace ripple { enum class WaiveTransferFee { Yes, No }; -enum class AllNodesDeleted { Yes, No }; //------------------------------------------------------------------------------ // @@ -463,10 +462,10 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); * Used for a regular and AMM accounts deletion. The caller * has to provide the deleter function, which handles details of * specific account deletion. - * @return {tesSUCCESS, AllNodesDeleted::No} indicates maxNodesToDelete + * @return {tesSUCCESS, false} indicates maxNodesToDelete * are deleted and there remains more nodes to delete. */ -[[nodiscard]] std::pair +[[nodiscard]] std::pair cleanupOnAccountDelete( ApplyView& view, Keylet const& ownerDirKeylet, diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index b9c9d6bdfae..8aa397f96d3 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1544,7 +1544,7 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account) return tesSUCCESS; } -std::pair +std::pair cleanupOnAccountDelete( ApplyView& view, Keylet const& ownerDirKeylet, @@ -1565,7 +1565,7 @@ cleanupOnAccountDelete( do { if (maxNodesToDelete && ++deleted > *maxNodesToDelete) - return {tesSUCCESS, AllNodesDeleted::No}; + return {tesSUCCESS, false}; // Choose the right way to delete each directory node. auto sleItem = view.peek(keylet::child(dirEntry)); @@ -1576,7 +1576,7 @@ cleanupOnAccountDelete( << "DeleteAccount: Directory node in ledger " << view.seq() << " has index to object that is missing: " << to_string(dirEntry); - return {tefBAD_LEDGER, AllNodesDeleted::No}; + return {tefBAD_LEDGER, false}; } LedgerEntryType const nodeType{safe_cast( @@ -1585,7 +1585,7 @@ cleanupOnAccountDelete( // Deleter handles the details of specific account deletion if (auto const ter = deleter(nodeType, dirEntry, sleItem); ter != tesSUCCESS) - return {ter, AllNodesDeleted::No}; + return {ter, false}; // dirFirst() and dirNext() are like iterators with exposed // internal state. We'll take advantage of that exposed state @@ -1608,7 +1608,7 @@ cleanupOnAccountDelete( { JLOG(j.error()) << "DeleteAccount iterator re-validation failed."; - return {tefBAD_LEDGER, AllNodesDeleted::No}; + return {tefBAD_LEDGER, false}; } uDirEntry = 0; @@ -1616,7 +1616,7 @@ cleanupOnAccountDelete( dirNext(view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); } - return {tesSUCCESS, AllNodesDeleted::Yes}; + return {tesSUCCESS, true}; } TER @@ -1639,11 +1639,16 @@ deleteAMMTrustLine( bool const ammLow = sleLow->getFlags() & lsfAMM; bool const ammHigh = sleHigh->getFlags() & lsfAMM; - // One side must be AMM - if (!(ammLow ^ ammHigh) || - (ammAccountID && - ((ammLow && low != ammAccountID) || - (ammHigh && high != ammAccountID)))) + // can't both be AMM + if (ammLow && ammHigh) + return tecINTERNAL; + + // at least one must be + if (!ammLow && !ammHigh) + return terNO_AMM; + + // one must be the target amm + if (ammAccountID && (low != *ammAccountID && high != *ammAccountID)) return terNO_AMM; if (auto const ter = trustDelete(view, sleState, low, high, j); @@ -1655,6 +1660,7 @@ deleteAMMTrustLine( } adjustOwnerCount(view, !ammLow ? sleLow : sleHigh, -1, j); + adjustOwnerCount(view, ammLow ? sleLow : sleHigh, -1, j); return tesSUCCESS; } diff --git a/src/ripple/protocol/Protocol.h b/src/ripple/protocol/Protocol.h index d922630f3df..1a6b1891bd1 100644 --- a/src/ripple/protocol/Protocol.h +++ b/src/ripple/protocol/Protocol.h @@ -95,7 +95,7 @@ using LedgerIndex = std::uint32_t; */ using TxID = uint256; -std::size_t constexpr maxDeletableAMMTrustLines = 1500; +std::size_t constexpr maxDeletableAMMTrustLines = 512; } // namespace ripple diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index b7599d74b9c..eb7aceb2d2b 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -300,7 +300,8 @@ enum TECcodes : TERUnderlyingType { tecAMM_FAILED = 164, tecAMM_INVALID_TOKENS = 165, tecAMM_EMPTY = 166, - tecINCOMPLETE = 167 + tecAMM_NOT_EMPTY = 167, + tecINCOMPLETE = 168 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index a5057b144bb..3743ac7b12f 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -46,6 +46,7 @@ transResults() MAKE_ERROR(tecAMM_INVALID_TOKENS, "AMM invalid LP tokens."), MAKE_ERROR(tecAMM_FAILED, "AMM transaction failed."), MAKE_ERROR(tecAMM_EMPTY, "AMM is in empty state."), + MAKE_ERROR(tecAMM_NOT_EMPTY, "AMM is not in empty state."), MAKE_ERROR(tecCLAIM, "Fee claimed. Sequence used. No action."), MAKE_ERROR(tecDIR_FULL, "Can not add entry to full directory."), MAKE_ERROR(tecFAILED_PROCESSING, "Failed to correctly process transaction."), diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index a7c37067c85..79401c8d026 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -782,7 +782,7 @@ struct AMM_test : public jtx::AMMTest USD(100), std::nullopt, tfTwoAssetIfEmpty, - ter(tecAMM_EMPTY)); + ter(tecAMM_NOT_EMPTY)); }); // Invalid AMM @@ -4284,18 +4284,20 @@ struct AMM_test : public jtx::AMMTest all); fund(env, gw, {alice}, XRP(20'000), {USD(10'000)}); AMM amm(env, gw, XRP(10'000), USD(10'000)); - for (auto i = 0; i < 1'600; ++i) + for (auto i = 0; i < maxDeletableAMMTrustLines + 10; ++i) { Account const a{std::to_string(i)}; env.fund(XRP(1'000), a); env(trust(a, STAmount{amm.lptIssue(), 10'000})); env.close(); } - // The auto-delete deleted 1,500 trustlines and AMM is - // in empty state. + // The trustlines are not deleted, AMM is set to an empty state. amm.withdrawAll(gw); BEAST_EXPECT(amm.ammExists()); + // Have not deleted all trustlines + amm.ammDelete(alice, ter(tecINCOMPLETE)); + // Bid,Vote,Deposit,Withdraw,SetTrust failing with // tecAMM_EMPTY. Deposit succeeds with tfTwoAssetIfEmpty option. amm.bid( @@ -4343,7 +4345,7 @@ struct AMM_test : public jtx::AMMTest BEAST_EXPECT(amm.expectAuctionSlot(100, 0, IOUAmount{0})); // Withdrawing all tokens deletes AMM since the number - // of remaining trustlines is less than 1,500. + // of remaining trustlines is less than max amm.withdrawAll(alice); BEAST_EXPECT(!amm.ammExists()); BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount()))); @@ -4359,19 +4361,21 @@ struct AMM_test : public jtx::AMMTest all); fund(env, gw, {alice}, XRP(20'000), {USD(10'000)}); AMM amm(env, gw, XRP(10'000), USD(10'000)); - for (auto i = 0; i < 3'100; ++i) + for (auto i = 0; i < maxDeletableAMMTrustLines * 2 + 10; ++i) { Account const a{std::to_string(i)}; env.fund(XRP(1'000), a); env(trust(a, STAmount{amm.lptIssue(), 10'000})); env.close(); } - // Deletes 1,500 trustlines + // Doesn't delete the trustlines amm.withdrawAll(gw); BEAST_EXPECT(amm.ammExists()); - // AMMDelete has to be called twice to delete AMM. - // Deletes 1,500 trustlines. + // AMMDelete has to be called tree times to delete AMM. + // Deletes max trustlines. + amm.ammDelete(alice, ter(tecINCOMPLETE)); + BEAST_EXPECT(amm.ammExists()); amm.ammDelete(alice, ter(tecINCOMPLETE)); BEAST_EXPECT(amm.ammExists()); // Deletes remaining trustlines and deletes AMM. From 9ddff746fed232d7e23bbcccbf0c5181ff6c96a9 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 29 Jul 2023 09:08:54 -0400 Subject: [PATCH 09/18] Check no directory left after AMM trustlines are deleted and other minor fixes --- src/ripple/app/misc/AMMUtils.h | 2 +- src/ripple/app/misc/impl/AMMUtils.cpp | 8 ++++++++ src/ripple/ledger/View.h | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/misc/AMMUtils.h b/src/ripple/app/misc/AMMUtils.h index a75fd1f43cf..c25503ceb9c 100644 --- a/src/ripple/app/misc/AMMUtils.h +++ b/src/ripple/app/misc/AMMUtils.h @@ -103,7 +103,7 @@ deleteAMMAccount( Issue const& asset2, beast::Journal j); -/** Initialize Auction and Voting slots and set the trading/disconted fee. +/** Initialize Auction and Voting slots and set the trading/discounted fee. */ void initializeFeeAuctionVote( diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index d4f7bb5458b..e6091ee563e 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -253,6 +253,14 @@ deleteAMMAccount( if (!done) return tecINCOMPLETE; + auto const ownerDirKeylet = keylet::ownerDir(ammAccountID); + if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet)) + { + JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of " + << toBase58(ammAccountID); + return tecINTERNAL; + } + sb.erase(ammSle); if (sleAMMRoot) sb.erase(sleAMMRoot); diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 27370ebd9fa..493c2b1b67f 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -42,7 +42,7 @@ namespace ripple { -enum class WaiveTransferFee { Yes, No }; +enum class WaiveTransferFee : bool { No = false, Yes }; //------------------------------------------------------------------------------ // From 2b1c8c6e343276fc8d3f436d507c74baac042b79 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sun, 30 Jul 2023 20:02:55 -0400 Subject: [PATCH 10/18] Disallow clawback out of AMM account --- src/ripple/app/tx/impl/Clawback.cpp | 3 +++ src/ripple/protocol/TER.h | 3 ++- src/ripple/protocol/impl/TER.cpp | 3 ++- src/test/app/AMM_test.cpp | 22 ++++++++++++++++++++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 4fb4d4bc8f8..b98cc087345 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -63,6 +63,9 @@ Clawback::preclaim(PreclaimContext const& ctx) if (!sleIssuer || !sleHolder) return terNO_ACCOUNT; + if (sleHolder->getFlags() & lsfAMM) + return tecAMM_ACCOUNT; + std::uint32_t const issuerFlagsIn = sleIssuer->getFieldU32(sfFlags); // If AllowTrustLineClawback is not set or NoFreeze is set, return no diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index eb7aceb2d2b..a2743bace8d 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -301,7 +301,8 @@ enum TECcodes : TERUnderlyingType { tecAMM_INVALID_TOKENS = 165, tecAMM_EMPTY = 166, tecAMM_NOT_EMPTY = 167, - tecINCOMPLETE = 168 + tecAMM_ACCOUNT = 168, + tecINCOMPLETE = 169 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index 3743ac7b12f..c3d8524c17d 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -47,6 +47,7 @@ transResults() MAKE_ERROR(tecAMM_FAILED, "AMM transaction failed."), MAKE_ERROR(tecAMM_EMPTY, "AMM is in empty state."), MAKE_ERROR(tecAMM_NOT_EMPTY, "AMM is not in empty state."), + MAKE_ERROR(tecAMM_ACCOUNT, "AMM Account."), MAKE_ERROR(tecCLAIM, "Fee claimed. Sequence used. No action."), MAKE_ERROR(tecDIR_FULL, "Can not add entry to full directory."), MAKE_ERROR(tecFAILED_PROCESSING, "Failed to correctly process transaction."), @@ -94,7 +95,7 @@ transResults() MAKE_ERROR(tecINSUFFICIENT_FUNDS, "Not enough funds available to complete requested transaction."), MAKE_ERROR(tecOBJECT_NOT_FOUND, "A requested object could not be located."), MAKE_ERROR(tecINSUFFICIENT_PAYMENT, "The payment is not sufficient."), - MAKE_ERROR(tecINCOMPLETE, "Transaction requires multple submissions to complete the processing."), + MAKE_ERROR(tecINCOMPLETE, "Transaction requires multiple submissions to complete the processing."), MAKE_ERROR(tefALREADY, "The exact transaction was already in this ledger."), MAKE_ERROR(tefBAD_ADD_AUTH, "Not authorized to add account."), diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 79401c8d026..403fe590830 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4385,6 +4385,27 @@ struct AMM_test : public jtx::AMMTest } } + void + testClawback() + { + testcase("Clawback"); + using namespace jtx; + FeatureBitset const all{supported_amendments()}; + + for (auto const& features : {all, all - featureClawback}) + { + Env env(*this, features); + env.fund(XRP(2'000), gw); + env.fund(XRP(2'000), alice); + env(fset(gw, asfAllowTrustLineClawback)); + AMM amm(env, gw, XRP(1'000), USD(1'000)); + auto const terr = features[featureClawback] ? ter(tecAMM_ACCOUNT) + : ter(temDISABLED); + env(claw(gw, STAmount{Issue(USD.currency, amm.ammAccount()), 200}), + terr); + } + } + void testCore() { @@ -4408,6 +4429,7 @@ struct AMM_test : public jtx::AMMTest testTradingFee(); testAdjustedTokens(); testAutoDelete(); + testClawback(); } void From c5e3c89001d79c9b0827fe828d5fd42e6c642468 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 1 Aug 2023 16:13:55 -0400 Subject: [PATCH 11/18] Disallow AMM create if issuer has clawback enabled and other changes * Rollback AMM handling in clawback transactor * Use tecINCOMPLETE as indicator of partial deletion in cleanupOnAccountDelete() and deleteAMMTrustLines() * Return tecINTERNAL if AMM root account is nullptr in deleteAMMAccount() * Enable partial trustline deletion in AMMWithdraw * Refactor AccountRootsNotDeleted::finalize() * Refactor SetTrust::preclaim() --- src/ripple/app/misc/impl/AMMUtils.cpp | 20 +++++----- src/ripple/app/tx/impl/AMMCreate.cpp | 17 +++++++- src/ripple/app/tx/impl/AMMDelete.h | 2 +- src/ripple/app/tx/impl/AMMWithdraw.cpp | 4 +- src/ripple/app/tx/impl/Clawback.cpp | 3 -- src/ripple/app/tx/impl/DeleteAccount.cpp | 6 +-- src/ripple/app/tx/impl/InvariantCheck.cpp | 11 ++++-- src/ripple/app/tx/impl/SetTrust.cpp | 44 ++++++++++++--------- src/ripple/app/tx/impl/Transactor.cpp | 9 ++++- src/ripple/ledger/View.h | 9 ++--- src/ripple/ledger/impl/View.cpp | 22 ++++++----- src/ripple/protocol/LedgerFormats.h | 2 +- src/ripple/protocol/TER.h | 3 +- src/ripple/protocol/impl/TER.cpp | 3 +- src/test/app/AMM_test.cpp | 47 ++++++++++++----------- 15 files changed, 115 insertions(+), 87 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index e6091ee563e..b3eb729e080 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -188,7 +188,7 @@ ammAccountHolds( return STAmount{issue}; } -static std::pair +static TER deleteAMMTrustLines( Sandbox& sb, AccountID const& ammAccountID, @@ -234,7 +234,11 @@ deleteAMMAccount( { auto ammSle = sb.peek(keylet::amm(asset, asset2)); if (!ammSle) + { + JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist " + << asset << " " << asset2; return tecINTERNAL; + } auto const ammAccountID = (*ammSle)[sfAccount]; auto sleAMMRoot = sb.peek(keylet::account(ammAccountID)); @@ -242,17 +246,14 @@ deleteAMMAccount( { JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist " << to_string(ammAccountID); + return tecINTERNAL; } - auto const [ter, done] = - deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j); - - if (ter != tesSUCCESS) + if (auto const ter = + deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j); + ter != tesSUCCESS || ter == tecINCOMPLETE) return ter; - if (!done) - return tecINCOMPLETE; - auto const ownerDirKeylet = keylet::ownerDir(ammAccountID); if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet)) { @@ -262,8 +263,7 @@ deleteAMMAccount( } sb.erase(ammSle); - if (sleAMMRoot) - sb.erase(sleAMMRoot); + sb.erase(sleAMMRoot); return tesSUCCESS; } diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index 0fbb584c1c0..6d90bb4d8d5 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -184,7 +184,22 @@ AMMCreate::preclaim(PreclaimContext const& ctx) return tecAMM_INVALID_TOKENS; } - return tesSUCCESS; + // Disallow AMM if the issuer has clawback enabled + auto clawbackDisabled = [&](Issue const& issue) -> TER { + if (!isXRP(issue)) + { + if (auto const sle = ctx.view.read(keylet::account(issue.account)); + !sle) + return tecINTERNAL; + else if (sle->getFlags() & lsfAllowTrustLineClawback) + return tecNO_PERMISSION; + } + return tesSUCCESS; + }; + + if (auto const ter = clawbackDisabled(amount.issue()); ter != tesSUCCESS) + return ter; + return clawbackDisabled(amount2.issue()); } static std::pair diff --git a/src/ripple/app/tx/impl/AMMDelete.h b/src/ripple/app/tx/impl/AMMDelete.h index 00866914ae2..cf7f55cb715 100644 --- a/src/ripple/app/tx/impl/AMMDelete.h +++ b/src/ripple/app/tx/impl/AMMDelete.h @@ -26,7 +26,7 @@ namespace ripple { /** AMMDelete implements AMM delete transactor. This is a mechanism to * delete AMM in an empty state when the number of LP tokens is 0. - * AMMDelete deletes the truslines up to configured maximum. If all + * AMMDelete deletes the trustlines up to configured maximum. If all * trustlines are deleted then AMM ltAMM and root account are deleted. * Otherwise AMMDelete should be called again. */ diff --git a/src/ripple/app/tx/impl/AMMWithdraw.cpp b/src/ripple/app/tx/impl/AMMWithdraw.cpp index f040df042c9..58fc6b549ce 100644 --- a/src/ripple/app/tx/impl/AMMWithdraw.cpp +++ b/src/ripple/app/tx/impl/AMMWithdraw.cpp @@ -380,9 +380,7 @@ AMMWithdraw::applyGuts(Sandbox& sb) return {result, false}; bool updateBalance = true; - // Delete only if number of trustlines is less or equal than max - if (newLPTokenBalance == beast::zero && - accountSle->getFieldU32(sfOwnerCount) <= maxDeletableAMMTrustLines) + if (newLPTokenBalance == beast::zero) { if (auto const ter = deleteAMMAccount(sb, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], j_); diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index b98cc087345..4fb4d4bc8f8 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -63,9 +63,6 @@ Clawback::preclaim(PreclaimContext const& ctx) if (!sleIssuer || !sleHolder) return terNO_ACCOUNT; - if (sleHolder->getFlags() & lsfAMM) - return tecAMM_ACCOUNT; - std::uint32_t const issuerFlagsIn = sleIssuer->getFieldU32(sfFlags); // If AllowTrustLineClawback is not set or NoFreeze is set, return no diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp index 6d5d25a4a3f..eeffc66d382 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.cpp +++ b/src/ripple/app/tx/impl/DeleteAccount.cpp @@ -293,7 +293,7 @@ DeleteAccount::doApply() return tefBAD_LEDGER; Keylet const ownerDirKeylet{keylet::ownerDir(account_)}; - auto const res = cleanupOnAccountDelete( + auto const ter = cleanupOnAccountDelete( view(), ownerDirKeylet, [&](LedgerEntryType nodeType, @@ -313,8 +313,8 @@ DeleteAccount::doApply() return tecHAS_OBLIGATIONS; }, ctx_.journal); - if (std::get(res) != tesSUCCESS) - return std::get(res); + if (ter != tesSUCCESS) + return ter; // Transfer any XRP remaining after the fee is paid to the destination: (*dst)[sfBalance] = (*dst)[sfBalance] + mSourceBalance; diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index a78e35fd1cf..8f202d9be5f 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -326,9 +326,7 @@ AccountRootsNotDeleted::finalize( // Not every AMM withdraw deletes the AMM account, accountsDeleted_ // is set if it is deleted. if ((tx.getTxnType() == ttACCOUNT_DELETE || - ((tx.getTxnType() == ttAMM_WITHDRAW || - tx.getTxnType() == ttAMM_DELETE) && - accountsDeleted_ == 1)) && + tx.getTxnType() == ttAMM_DELETE) && result == tesSUCCESS) { if (accountsDeleted_ == 1) @@ -343,6 +341,13 @@ AccountRootsNotDeleted::finalize( return false; } + // A successful AMMWithdraw MAY delete one account root + // when the total AMM LP Tokens balance goes to 0. Not every AMM withdraw + // deletes the AMM account, accountsDeleted_ is set if it is deleted. + if (tx.getTxnType() == ttAMM_WITHDRAW && result == tesSUCCESS && + accountsDeleted_ == 1) + return true; + if (accountsDeleted_ == 0) return true; diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index 7352709a660..ad84416ef99 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -128,42 +129,47 @@ SetTrust::preclaim(PreclaimContext const& ctx) } } + // This might be nullptr auto const sleDst = ctx.view.read(keylet::account(uDstAccountID)); - if (!sleDst) - return tecNO_DST; - - auto const dstFlags = sleDst->getFlags(); - // If the destination has opted to disallow incoming trustlines // then honour that flag if (ctx.view.rules().enabled(featureDisallowIncoming)) { - if (dstFlags & lsfDisallowIncomingTrustline) + if (!sleDst) + return tecNO_DST; + + if (sleDst->getFlags() & lsfDisallowIncomingTrustline) return tecNO_PERMISSION; } // If destination is AMM and the trustline doesn't exist then only // allow SetTrust if the asset is AMM LP token and AMM is not // in empty state. - TER ter = tesSUCCESS; - if (dstFlags & lsfAMM && - !ctx.view.read(keylet::line(id, uDstAccountID, currency))) + if (ammEnabled(ctx.view.rules())) { - if (auto const ammSle = - ctx.view.read({ltAMM, sleDst->getFieldH256(sfAMMID)})) + if (!sleDst) + return tecNO_DST; + + if (sleDst->getFlags() & lsfAMM && + !ctx.view.read(keylet::line(id, uDstAccountID, currency))) { - if (auto const lpTokens = ammSle->getFieldAmount(sfLPTokenBalance); - lpTokens == beast::zero) - ter = tecAMM_EMPTY; - else if (lpTokens.getCurrency() != saLimitAmount.getCurrency()) - ter = tecNO_PERMISSION; + if (auto const ammSle = + ctx.view.read({ltAMM, sleDst->getFieldH256(sfAMMID)})) + { + if (auto const lpTokens = + ammSle->getFieldAmount(sfLPTokenBalance); + lpTokens == beast::zero) + return tecAMM_EMPTY; + else if (lpTokens.getCurrency() != saLimitAmount.getCurrency()) + return tecNO_PERMISSION; + } + else + return tecINTERNAL; } - else - ter = tecINTERNAL; } - return ter; + return tesSUCCESS; } TER diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 27ce8ed039e..b9d1e6eb8ff 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -773,8 +773,13 @@ removeDeletedTrustLines( for (auto const& index : trustLines) { - if (auto const sleState = view.peek({ltRIPPLE_STATE, index})) - deleteAMMTrustLine(view, sleState, std::nullopt, viewJ); + if (auto const sleState = view.peek({ltRIPPLE_STATE, index}); + deleteAMMTrustLine(view, sleState, std::nullopt, viewJ) != + tesSUCCESS) + { + JLOG(viewJ.error()) + << "removeDeletedTrustLines: failed to delete AMM trustline"; + } } } diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 493c2b1b67f..5983ebd82b1 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -461,11 +461,11 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); /** Cleanup owner directory entries on account delete. * Used for a regular and AMM accounts deletion. The caller * has to provide the deleter function, which handles details of - * specific account deletion. - * @return {tesSUCCESS, false} indicates maxNodesToDelete + * specific account-owned object deletion. + * @return tesINCOMPLETE indicates maxNodesToDelete * are deleted and there remains more nodes to delete. */ -[[nodiscard]] std::pair +[[nodiscard]] TER cleanupOnAccountDelete( ApplyView& view, Keylet const& ownerDirKeylet, @@ -478,8 +478,7 @@ cleanupOnAccountDelete( * call to view.peek(). Fail if neither side of the trustline is AMM or * if ammAccountID is seated and is not one of the trustline's side. */ -// [[nodiscard]] // nodiscard commented out so Transactor compiles. -TER +[[nodiscard]] TER deleteAMMTrustLine( ApplyView& view, std::shared_ptr sleState, diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 8aa397f96d3..b37ba0fab5e 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1544,7 +1544,7 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account) return tesSUCCESS; } -std::pair +TER cleanupOnAccountDelete( ApplyView& view, Keylet const& ownerDirKeylet, @@ -1565,7 +1565,7 @@ cleanupOnAccountDelete( do { if (maxNodesToDelete && ++deleted > *maxNodesToDelete) - return {tesSUCCESS, false}; + return tecINCOMPLETE; // Choose the right way to delete each directory node. auto sleItem = view.peek(keylet::child(dirEntry)); @@ -1576,16 +1576,17 @@ cleanupOnAccountDelete( << "DeleteAccount: Directory node in ledger " << view.seq() << " has index to object that is missing: " << to_string(dirEntry); - return {tefBAD_LEDGER, false}; + return tefBAD_LEDGER; } LedgerEntryType const nodeType{safe_cast( sleItem->getFieldU16(sfLedgerEntryType))}; - // Deleter handles the details of specific account deletion + // Deleter handles the details of specific account-owned object + // deletion if (auto const ter = deleter(nodeType, dirEntry, sleItem); ter != tesSUCCESS) - return {ter, false}; + return ter; // dirFirst() and dirNext() are like iterators with exposed // internal state. We'll take advantage of that exposed state @@ -1608,7 +1609,7 @@ cleanupOnAccountDelete( { JLOG(j.error()) << "DeleteAccount iterator re-validation failed."; - return {tefBAD_LEDGER, false}; + return tefBAD_LEDGER; } uDirEntry = 0; @@ -1616,7 +1617,7 @@ cleanupOnAccountDelete( dirNext(view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); } - return {tesSUCCESS, true}; + return tesSUCCESS; } TER @@ -1626,7 +1627,7 @@ deleteAMMTrustLine( std::optional const& ammAccountID, beast::Journal j) { - if (!sleState) + if (!sleState || sleState->getType() != ltRIPPLE_STATE) return tecINTERNAL; auto const& [low, high] = std::minmax( @@ -1659,8 +1660,11 @@ deleteAMMTrustLine( return ter; } + auto const uFlags = !ammLow ? lsfLowReserve : lsfHighReserve; + if (!(sleState->getFlags() & uFlags)) + return tecINTERNAL; + adjustOwnerCount(view, !ammLow ? sleLow : sleHigh, -1, j); - adjustOwnerCount(view, ammLow ? sleLow : sleHigh, -1, j); return tesSUCCESS; } diff --git a/src/ripple/protocol/LedgerFormats.h b/src/ripple/protocol/LedgerFormats.h index a613c3a470d..6a262f26252 100644 --- a/src/ripple/protocol/LedgerFormats.h +++ b/src/ripple/protocol/LedgerFormats.h @@ -249,7 +249,7 @@ enum LedgerSpecificFlags { 0x10000000, // True, reject new paychans lsfDisallowIncomingTrustline = 0x20000000, // True, reject new trustlines (only if no issued assets) - lsfAMM [[maybe_unused]] = 0x40000000, // True, AMM account + lsfAMM = 0x40000000, // True, AMM account lsfAllowTrustLineClawback = 0x80000000, // True, enable clawback diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index a2743bace8d..eb7aceb2d2b 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -301,8 +301,7 @@ enum TECcodes : TERUnderlyingType { tecAMM_INVALID_TOKENS = 165, tecAMM_EMPTY = 166, tecAMM_NOT_EMPTY = 167, - tecAMM_ACCOUNT = 168, - tecINCOMPLETE = 169 + tecINCOMPLETE = 168 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index c3d8524c17d..aa93be9d67d 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -47,7 +47,6 @@ transResults() MAKE_ERROR(tecAMM_FAILED, "AMM transaction failed."), MAKE_ERROR(tecAMM_EMPTY, "AMM is in empty state."), MAKE_ERROR(tecAMM_NOT_EMPTY, "AMM is not in empty state."), - MAKE_ERROR(tecAMM_ACCOUNT, "AMM Account."), MAKE_ERROR(tecCLAIM, "Fee claimed. Sequence used. No action."), MAKE_ERROR(tecDIR_FULL, "Can not add entry to full directory."), MAKE_ERROR(tecFAILED_PROCESSING, "Failed to correctly process transaction."), @@ -95,7 +94,7 @@ transResults() MAKE_ERROR(tecINSUFFICIENT_FUNDS, "Not enough funds available to complete requested transaction."), MAKE_ERROR(tecOBJECT_NOT_FOUND, "A requested object could not be located."), MAKE_ERROR(tecINSUFFICIENT_PAYMENT, "The payment is not sufficient."), - MAKE_ERROR(tecINCOMPLETE, "Transaction requires multiple submissions to complete the processing."), + MAKE_ERROR(tecINCOMPLETE, "Some work was completed, but more submissions required to finish."), MAKE_ERROR(tefALREADY, "The exact transaction was already in this ledger."), MAKE_ERROR(tefBAD_ADD_AUTH, "Not authorized to add account."), diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 403fe590830..6e5e88f8f38 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -400,6 +400,21 @@ struct AMM_test : public jtx::AMMTest AMM ammAlice1( env, alice, USD(10'000), USD1(10'000), ter(terNO_RIPPLE)); } + + // Issuer has clawback enabled + { + Env env(*this); + env.fund(XRP(1'000), gw); + env(fset(gw, asfAllowTrustLineClawback)); + fund(env, gw, {alice}, XRP(1'000), {USD(1'000)}, Fund::Acct); + env.close(); + AMM amm(env, gw, XRP(100), USD(100), ter(tecNO_PERMISSION)); + AMM amm1(env, alice, USD(100), XRP(100), ter(tecNO_PERMISSION)); + env(fclear(gw, asfAllowTrustLineClawback)); + env.close(); + // Can't be cleared + AMM amm2(env, gw, XRP(100), USD(100), ter(tecNO_PERMISSION)); + } } void @@ -4291,13 +4306,11 @@ struct AMM_test : public jtx::AMMTest env(trust(a, STAmount{amm.lptIssue(), 10'000})); env.close(); } - // The trustlines are not deleted, AMM is set to an empty state. + // The trustlines are partially deleted, + // AMM is set to an empty state. amm.withdrawAll(gw); BEAST_EXPECT(amm.ammExists()); - // Have not deleted all trustlines - amm.ammDelete(alice, ter(tecINCOMPLETE)); - // Bid,Vote,Deposit,Withdraw,SetTrust failing with // tecAMM_EMPTY. Deposit succeeds with tfTwoAssetIfEmpty option. amm.bid( @@ -4368,14 +4381,11 @@ struct AMM_test : public jtx::AMMTest env(trust(a, STAmount{amm.lptIssue(), 10'000})); env.close(); } - // Doesn't delete the trustlines + // The trustlines are partially deleted. amm.withdrawAll(gw); BEAST_EXPECT(amm.ammExists()); - // AMMDelete has to be called tree times to delete AMM. - // Deletes max trustlines. - amm.ammDelete(alice, ter(tecINCOMPLETE)); - BEAST_EXPECT(amm.ammExists()); + // AMMDelete has to be called twice to delete AMM. amm.ammDelete(alice, ter(tecINCOMPLETE)); BEAST_EXPECT(amm.ammExists()); // Deletes remaining trustlines and deletes AMM. @@ -4390,20 +4400,11 @@ struct AMM_test : public jtx::AMMTest { testcase("Clawback"); using namespace jtx; - FeatureBitset const all{supported_amendments()}; - - for (auto const& features : {all, all - featureClawback}) - { - Env env(*this, features); - env.fund(XRP(2'000), gw); - env.fund(XRP(2'000), alice); - env(fset(gw, asfAllowTrustLineClawback)); - AMM amm(env, gw, XRP(1'000), USD(1'000)); - auto const terr = features[featureClawback] ? ter(tecAMM_ACCOUNT) - : ter(temDISABLED); - env(claw(gw, STAmount{Issue(USD.currency, amm.ammAccount()), 200}), - terr); - } + Env env(*this); + env.fund(XRP(2'000), gw); + env.fund(XRP(2'000), alice); + AMM amm(env, gw, XRP(1'000), USD(1'000)); + env(fset(gw, asfAllowTrustLineClawback), ter(tecOWNERS)); } void From b63e6960fc02a6f0d9a6c6f7d48a6cda18a85c84 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Wed, 2 Aug 2023 07:24:12 -0400 Subject: [PATCH 12/18] Remove lsfAMM flag and use sfAMMID instead, plus minor refactoring --- API-CHANGELOG.md | 2 +- src/ripple/app/misc/impl/AMMUtils.cpp | 2 +- src/ripple/app/tx/impl/AMMCreate.cpp | 19 ++++---- src/ripple/app/tx/impl/Clawback.cpp | 3 ++ src/ripple/app/tx/impl/CreateCheck.cpp | 2 +- src/ripple/app/tx/impl/Escrow.cpp | 2 +- src/ripple/app/tx/impl/InvariantCheck.cpp | 4 +- src/ripple/app/tx/impl/PayChan.cpp | 2 +- src/ripple/app/tx/impl/Payment.cpp | 2 +- src/ripple/app/tx/impl/SetTrust.cpp | 6 +-- src/ripple/ledger/View.h | 2 +- src/ripple/ledger/impl/View.cpp | 6 +-- src/ripple/protocol/LedgerFormats.h | 3 +- src/ripple/protocol/TER.h | 3 +- src/ripple/protocol/impl/TER.cpp | 1 + src/test/app/AMM_test.cpp | 56 ++++++++++++++++++++++- src/test/jtx/AMM.h | 13 ++++++ src/test/jtx/impl/AMM.cpp | 4 +- 18 files changed, 99 insertions(+), 33 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index eaf80448ee7..cde3c08f57f 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -55,7 +55,7 @@ Additions are intended to be non-breaking (because they are purely additive). - Adds `AMMVote` transaction type to vote for the trading fee of `AMM` instance. - Adds `AMMBid` transaction type to bid for the Auction Slot of `AMM` instance. - Adds `AMMDelete` transaction type to delete `AMM` instance. - - Adds `lsfAMM` `AccountRoot` flag to indicate that the account is `AMM`'s account. + - Adds `sfAMMID` to `AccountRoot` to indicate that the account is `AMM`'s account. `AMMID` is used to fetch `ltAMM`. - Adds `lsfAMMNode` `TrustLine` flag to indicate that one side of the `TrustLine` is `AMM` account. - Adds `tfLPToken`, `tfSingleAsset`, `tfTwoAsset`, `tfOneAssetLPToken`, `tfLimitLPToken`, `tfTwoAssetIfEmpty`, `tfWithdrawAll`, `tfOneAssetWithdrawAll` which allow a trader to specify different fields combination diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index b3eb729e080..888c658f9b4 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -251,7 +251,7 @@ deleteAMMAccount( if (auto const ter = deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j); - ter != tesSUCCESS || ter == tecINCOMPLETE) + ter != tesSUCCESS) return ter; auto const ownerDirKeylet = keylet::ownerDir(ammAccountID); diff --git a/src/ripple/app/tx/impl/AMMCreate.cpp b/src/ripple/app/tx/impl/AMMCreate.cpp index 6d90bb4d8d5..0c6874953a3 100644 --- a/src/ripple/app/tx/impl/AMMCreate.cpp +++ b/src/ripple/app/tx/impl/AMMCreate.cpp @@ -173,7 +173,7 @@ AMMCreate::preclaim(PreclaimContext const& ctx) auto isLPToken = [&](STAmount const& amount) -> bool { if (auto const sle = ctx.view.read(keylet::account(amount.issue().account))) - return (sle->getFlags() & lsfAMM); + return sle->isFieldPresent(sfAMMID); return false; }; @@ -186,14 +186,13 @@ AMMCreate::preclaim(PreclaimContext const& ctx) // Disallow AMM if the issuer has clawback enabled auto clawbackDisabled = [&](Issue const& issue) -> TER { - if (!isXRP(issue)) - { - if (auto const sle = ctx.view.read(keylet::account(issue.account)); - !sle) - return tecINTERNAL; - else if (sle->getFlags() & lsfAllowTrustLineClawback) - return tecNO_PERMISSION; - } + if (isXRP(issue)) + return tesSUCCESS; + if (auto const sle = ctx.view.read(keylet::account(issue.account)); + !sle) + return tecINTERNAL; + else if (sle->getFlags() & lsfAllowTrustLineClawback) + return tecNO_PERMISSION; return tesSUCCESS; }; @@ -262,7 +261,7 @@ applyCreate( // A user can only receive LPTokens through affirmative action - // either an AMMDeposit, TrustSet, crossing an offer, etc. sleAMMRoot->setFieldU32( - sfFlags, lsfAMM | lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth); + sfFlags, lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth); // Link the root account and AMM object sleAMMRoot->setFieldH256(sfAMMID, ammKeylet.key); sb.insert(sleAMMRoot); diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 4fb4d4bc8f8..58546db5ca7 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -63,6 +63,9 @@ Clawback::preclaim(PreclaimContext const& ctx) if (!sleIssuer || !sleHolder) return terNO_ACCOUNT; + if (sleHolder->isFieldPresent(sfAMMID)) + return tecAMM_ACCOUNT; + std::uint32_t const issuerFlagsIn = sleIssuer->getFieldU32(sfFlags); // If AllowTrustLineClawback is not set or NoFreeze is set, return no diff --git a/src/ripple/app/tx/impl/CreateCheck.cpp b/src/ripple/app/tx/impl/CreateCheck.cpp index e7f0e103bce..77ce4d017a1 100644 --- a/src/ripple/app/tx/impl/CreateCheck.cpp +++ b/src/ripple/app/tx/impl/CreateCheck.cpp @@ -98,7 +98,7 @@ CreateCheck::preclaim(PreclaimContext const& ctx) return tecNO_PERMISSION; // AMM can not cash the check - if (flags & lsfAMM) + if (sleDst->isFieldPresent(sfAMMID)) return tecNO_PERMISSION; if ((flags & lsfRequireDestTag) && !ctx.tx.isFieldPresent(sfDestinationTag)) diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 981cc57e8fe..9d8aa6a2289 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -162,7 +162,7 @@ EscrowCreate::preclaim(PreclaimContext const& ctx) auto const sled = ctx.view.read(keylet::account(ctx.tx[sfDestination])); if (!sled) return tecNO_DST; - if (((*sled)[sfFlags] & lsfAMM)) + if (sled->isFieldPresent(sfAMMID)) return tecNO_PERMISSION; return tesSUCCESS; diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 8f202d9be5f..907611f1c9a 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -323,8 +323,8 @@ AccountRootsNotDeleted::finalize( { // AMM account root can be deleted as the result of AMM withdraw/delete // transaction when the total AMM LP Tokens balance goes to 0. - // Not every AMM withdraw deletes the AMM account, accountsDeleted_ - // is set if it is deleted. + // A successful AccountDelete or AMMDelete MUST delete exactly + // one account root. if ((tx.getTxnType() == ttACCOUNT_DELETE || tx.getTxnType() == ttAMM_DELETE) && result == tesSUCCESS) diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index ac85603957f..3fe2a28a7cf 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -234,7 +234,7 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) (flags & lsfDisallowXRP)) return tecNO_TARGET; - if (flags & lsfAMM) + if (sled->isFieldPresent(sfAMMID)) return tecNO_PERMISSION; } diff --git a/src/ripple/app/tx/impl/Payment.cpp b/src/ripple/app/tx/impl/Payment.cpp index a23e41bac2a..d3c26a37023 100644 --- a/src/ripple/app/tx/impl/Payment.cpp +++ b/src/ripple/app/tx/impl/Payment.cpp @@ -468,7 +468,7 @@ Payment::doApply() // AMMs can never receive an XRP payment. // Must use AMMDeposit transaction instead. - if (sleDst->getFlags() & lsfAMM) + if (sleDst->isFieldPresent(sfAMMID)) return tecNO_PERMISSION; // The source account does have enough money. Make sure the diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index ad84416ef99..7869cc7027d 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -151,7 +151,7 @@ SetTrust::preclaim(PreclaimContext const& ctx) if (!sleDst) return tecNO_DST; - if (sleDst->getFlags() & lsfAMM && + if (sleDst->isFieldPresent(sfAMMID) && !ctx.view.read(keylet::line(id, uDstAccountID, currency))) { if (auto const ammSle = @@ -547,10 +547,6 @@ SetTrust::doApply() uQualityIn, uQualityOut, viewJ); - - // Maintain number of AMM trustlines in AMM root account - if (terResult == tesSUCCESS && (sleDst->getFlags() & lsfAMM)) - adjustOwnerCount(view(), sleDst, 1, j_); } return terResult; diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 5983ebd82b1..2c8d2354e0c 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -462,7 +462,7 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); * Used for a regular and AMM accounts deletion. The caller * has to provide the deleter function, which handles details of * specific account-owned object deletion. - * @return tesINCOMPLETE indicates maxNodesToDelete + * @return tecINCOMPLETE indicates maxNodesToDelete * are deleted and there remains more nodes to delete. */ [[nodiscard]] TER diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index b37ba0fab5e..012cf825f53 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -375,7 +375,7 @@ xrpLiquid( view.ownerCountHook(id, sle->getFieldU32(sfOwnerCount)), ownerCountAdj); // AMMs have no reserve requirement - auto const reserve = (sle->getFlags() & lsfAMM) + auto const reserve = sle->isFieldPresent(sfAMMID) ? XRPAmount{0} : view.fees().accountReserve(ownerCount); @@ -1637,8 +1637,8 @@ deleteAMMTrustLine( auto sleHigh = view.peek(keylet::account(high)); if (!sleLow || !sleHigh) return tecINTERNAL; - bool const ammLow = sleLow->getFlags() & lsfAMM; - bool const ammHigh = sleHigh->getFlags() & lsfAMM; + bool const ammLow = sleLow->isFieldPresent(sfAMMID); + bool const ammHigh = sleHigh->isFieldPresent(sfAMMID); // can't both be AMM if (ammLow && ammHigh) diff --git a/src/ripple/protocol/LedgerFormats.h b/src/ripple/protocol/LedgerFormats.h index 6a262f26252..088cddf7ee9 100644 --- a/src/ripple/protocol/LedgerFormats.h +++ b/src/ripple/protocol/LedgerFormats.h @@ -249,9 +249,8 @@ enum LedgerSpecificFlags { 0x10000000, // True, reject new paychans lsfDisallowIncomingTrustline = 0x20000000, // True, reject new trustlines (only if no issued assets) - lsfAMM = 0x40000000, // True, AMM account lsfAllowTrustLineClawback = - 0x80000000, // True, enable clawback + 0x40000000, // True, enable clawback // ltOFFER lsfPassive = 0x00010000, diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index eb7aceb2d2b..a2743bace8d 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -301,7 +301,8 @@ enum TECcodes : TERUnderlyingType { tecAMM_INVALID_TOKENS = 165, tecAMM_EMPTY = 166, tecAMM_NOT_EMPTY = 167, - tecINCOMPLETE = 168 + tecAMM_ACCOUNT = 168, + tecINCOMPLETE = 169 }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index aa93be9d67d..054980ba671 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -47,6 +47,7 @@ transResults() MAKE_ERROR(tecAMM_FAILED, "AMM transaction failed."), MAKE_ERROR(tecAMM_EMPTY, "AMM is in empty state."), MAKE_ERROR(tecAMM_NOT_EMPTY, "AMM is not in empty state."), + MAKE_ERROR(tecAMM_ACCOUNT, "AMM Account."), MAKE_ERROR(tecCLAIM, "Fee claimed. Sequence used. No action."), MAKE_ERROR(tecDIR_FULL, "Can not add entry to full directory."), MAKE_ERROR(tecFAILED_PROCESSING, "Failed to correctly process transaction."), diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 6e5e88f8f38..dfbe97c4f3f 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -3713,8 +3713,7 @@ struct AMM_test : public jtx::AMMTest info[jss::result][jss::account_data][jss::Flags].asUInt(); BEAST_EXPECT( flags == - (lsfAMM | lsfDisableMaster | lsfDefaultRipple | - lsfDepositAuth)); + (lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth)); }); } @@ -4407,6 +4406,58 @@ struct AMM_test : public jtx::AMMTest env(fset(gw, asfAllowTrustLineClawback), ter(tecOWNERS)); } + void + testAMMID() + { + using namespace jtx; + testAMM([&](AMM& amm, Env& env) { + amm.setClose(false); + auto const info = env.rpc( + "json", + "account_info", + std::string( + "{\"account\": \"" + to_string(amm.ammAccount()) + "\"}")); + try + { + BEAST_EXPECT( + info[jss::result][jss::account_data][jss::AMMID] + .asString() == to_string(amm.ammID())); + } + catch (...) + { + fail(); + } + amm.deposit(carol, 1'000); + auto affected = env.meta()->getJson( + JsonOptions::none)[sfAffectedNodes.fieldName]; + try + { + bool found = false; + for (auto const& node : affected) + { + if (node.isMember(sfModifiedNode.fieldName) && + node[sfModifiedNode.fieldName] + [sfLedgerEntryType.fieldName] + .asString() == "AccountRoot" && + node[sfModifiedNode.fieldName][sfFinalFields.fieldName] + [jss::Account] + .asString() == to_string(amm.ammAccount())) + { + found = node[sfModifiedNode.fieldName] + [sfFinalFields.fieldName][jss::AMMID] + .asString() == to_string(amm.ammID()); + break; + } + } + BEAST_EXPECT(found); + } + catch (...) + { + fail(); + } + }); + } + void testCore() { @@ -4431,6 +4482,7 @@ struct AMM_test : public jtx::AMMTest testAdjustedTokens(); testAutoDelete(); testClawback(); + testAMMID(); } void diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 7803bfba60c..3ad40b0afc4 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -67,6 +67,7 @@ class AMM STAmount const asset2_; IOUAmount const initialLPTokens_; bool log_; + bool doClose_; // Predict next purchase price IOUAmount lastPurchasePrice_; std::optional bidMin_; @@ -292,6 +293,18 @@ class AMM AccountID const& deleter, std::optional const& ter = std::nullopt); + void + setClose(bool close) + { + doClose_ = close; + } + + uint256 + ammID() const + { + return keylet::amm(asset1_.issue(), asset2_.issue()).key; + } + private: void setTokens( diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index 7f9d893df9a..f6113b38672 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -64,6 +64,7 @@ AMM::AMM( , asset2_(asset2) , initialLPTokens_(initialTokens(asset1, asset2)) , log_(log) + , doClose_(true) , lastPurchasePrice_(0) , bidMin_() , bidMax_() @@ -676,7 +677,8 @@ AMM::submit( env_(jv, *ter); else env_(jv); - env_.close(); + if (doClose_) + env_.close(); } bool From 2efa0f8d4c3c8b9061efbd03e4e9041aa99aaa63 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 4 Aug 2023 08:28:10 -0400 Subject: [PATCH 13/18] Rall-back lsfAllowTrustLineClawback flag and other changes * Add const ammID_ to AMM jtx class * Update changelog * Add testcase name --- API-CHANGELOG.md | 9 +++++++++ src/ripple/protocol/LedgerFormats.h | 3 ++- src/test/app/AMM_test.cpp | 1 + src/test/jtx/AMM.h | 3 ++- src/test/jtx/impl/AMM.cpp | 1 + 5 files changed, 15 insertions(+), 2 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index cde3c08f57f..ae988beeb33 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -60,6 +60,15 @@ Additions are intended to be non-breaking (because they are purely additive). - Adds `tfLPToken`, `tfSingleAsset`, `tfTwoAsset`, `tfOneAssetLPToken`, `tfLimitLPToken`, `tfTwoAssetIfEmpty`, `tfWithdrawAll`, `tfOneAssetWithdrawAll` which allow a trader to specify different fields combination for `AMMDeposit` and `AMMWithdraw` transactions. + - Adds new transaction result codes: + - tecUNFUNDED_AMM: insufficient balance to fund AMM. The account does not have funds for liquidity provision. + - tecAMM_BALANCE: AMM has invalid balance. Calculated balances greater than the current pool balances. + - tecAMM_FAILED: AMM transaction failed. Fails due to a processing failure. + - tecAMM_INVALID_TOKENS: AMM invalid LP tokens. Invalid input values, format, or calculated values. + - tecAMM_EMPTY: AMM is in empty state. Transaction expects AMM in non-empty state (LP tokens > 0). + - tecAMM_NOT_EMPTY: AMM is not in empty state. Transaction expects AMM in empty state (LP tokens == 0). + - tecAMM_ACCOUNT: AMM account. Clawback of AMM account. + - tecINCOMPLETE: Some work was completed, but more submissions required to finish. AMMDelete partially deletes the trustlines. ## XRP Ledger version 1.11.0 diff --git a/src/ripple/protocol/LedgerFormats.h b/src/ripple/protocol/LedgerFormats.h index 088cddf7ee9..b9205e7888a 100644 --- a/src/ripple/protocol/LedgerFormats.h +++ b/src/ripple/protocol/LedgerFormats.h @@ -249,8 +249,9 @@ enum LedgerSpecificFlags { 0x10000000, // True, reject new paychans lsfDisallowIncomingTrustline = 0x20000000, // True, reject new trustlines (only if no issued assets) + // 0x40000000 is available lsfAllowTrustLineClawback = - 0x40000000, // True, enable clawback + 0x80000000, // True, enable clawback // ltOFFER lsfPassive = 0x00010000, diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index dfbe97c4f3f..32c73547901 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4409,6 +4409,7 @@ struct AMM_test : public jtx::AMMTest void testAMMID() { + testcase("AMMID"); using namespace jtx; testAMM([&](AMM& amm, Env& env) { amm.setClose(false); diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index 3ad40b0afc4..3cf06bfe401 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -65,6 +65,7 @@ class AMM Account const creatorAccount_; STAmount const asset1_; STAmount const asset2_; + uint256 const ammID_; IOUAmount const initialLPTokens_; bool log_; bool doClose_; @@ -302,7 +303,7 @@ class AMM uint256 ammID() const { - return keylet::amm(asset1_.issue(), asset2_.issue()).key; + return ammID_; } private: diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index f6113b38672..c09d496f439 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -62,6 +62,7 @@ AMM::AMM( , creatorAccount_(account) , asset1_(asset1) , asset2_(asset2) + , ammID_(keylet::amm(asset1_.issue(), asset2_.issue()).key) , initialLPTokens_(initialTokens(asset1, asset2)) , log_(log) , doClose_(true) From fa0d9c47cb68685b4e2f0626d2ee3091963efdb9 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 7 Aug 2023 19:35:17 -0400 Subject: [PATCH 14/18] Address auditor's feedback --- src/ripple/app/misc/impl/AMMUtils.cpp | 2 +- src/ripple/app/tx/impl/Transactor.cpp | 2 +- src/ripple/ledger/impl/View.cpp | 2 +- src/ripple/protocol/Protocol.h | 5 ++++- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 888c658f9b4..dcb403296c1 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -281,7 +281,7 @@ initializeFeeAuctionVote( STObject voteEntry{sfVoteEntry}; if (tfee != 0) voteEntry.setFieldU16(sfTradingFee, tfee); - voteEntry.setFieldU32(sfVoteWeight, 100000); + voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR); voteEntry.setAccountID(sfAccount, account); voteSlots.push_back(voteEntry); ammSle->setFieldArray(sfVoteSlots, voteSlots); diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index b9d1e6eb8ff..449392531b8 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -950,7 +950,7 @@ Transactor::operator()() if (result == tecINCOMPLETE) removeDeletedTrustLines( - view(), removedTrustLines, ctx_.app.journal("view")); + view(), removedTrustLines, ctx_.app.journal("View")); applied = isTecClaim(result); } diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 012cf825f53..8d059aaf4b7 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -1557,7 +1557,7 @@ cleanupOnAccountDelete( std::shared_ptr sleDirNode{}; unsigned int uDirEntry{0}; uint256 dirEntry{beast::zero}; - std::uint16_t deleted = 0; + std::uint32_t deleted = 0; if (view.exists(ownerDirKeylet) && dirFirst(view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) diff --git a/src/ripple/protocol/Protocol.h b/src/ripple/protocol/Protocol.h index 1a6b1891bd1..6e4879cd746 100644 --- a/src/ripple/protocol/Protocol.h +++ b/src/ripple/protocol/Protocol.h @@ -95,7 +95,10 @@ using LedgerIndex = std::uint32_t; */ using TxID = uint256; -std::size_t constexpr maxDeletableAMMTrustLines = 512; +/** The maximum number of trustlines to delete as part of AMM account + * deletion cleanup. + */ +std::uint16_t constexpr maxDeletableAMMTrustLines = 512; } // namespace ripple From 7e2c4541aa3ceeff38cd9c7c1a164530bb41c022 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 11 Aug 2023 09:19:10 -0400 Subject: [PATCH 15/18] Update tecAMM_ACCOUNT error message --- src/ripple/protocol/impl/TER.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index 054980ba671..29b87351204 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -47,7 +47,7 @@ transResults() MAKE_ERROR(tecAMM_FAILED, "AMM transaction failed."), MAKE_ERROR(tecAMM_EMPTY, "AMM is in empty state."), MAKE_ERROR(tecAMM_NOT_EMPTY, "AMM is not in empty state."), - MAKE_ERROR(tecAMM_ACCOUNT, "AMM Account."), + MAKE_ERROR(tecAMM_ACCOUNT, "This operation is not allowed on an AMM Account."), MAKE_ERROR(tecCLAIM, "Fee claimed. Sequence used. No action."), MAKE_ERROR(tecDIR_FULL, "Can not add entry to full directory."), MAKE_ERROR(tecFAILED_PROCESSING, "Failed to correctly process transaction."), From 484e01f43b24c33f43bb3f1eba93371bbeb8a7e5 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 7 Aug 2023 10:51:21 -0400 Subject: [PATCH 16/18] Add unit-tests to verify CLOB/AMM offer and strand selection logic --- src/test/app/AMM_test.cpp | 338 +++++++++++++++++++++++++++++++++++++- 1 file changed, 337 insertions(+), 1 deletion(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 32c73547901..616b32a08ef 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4459,6 +4459,341 @@ struct AMM_test : public jtx::AMMTest }); } + void + testSelection() + { + testcase("Offer/Strand Selection"); + using namespace jtx; + Account const ed("ed"); + Account const gw1("gw1"); + auto const ETH = gw1["ETH"]; + auto const CAN = gw1["CAN"]; + + auto prep = [&](Env& env, auto gwRate, auto gw1Rate) { + fund(env, gw, {alice, carol, bob, ed}, XRP(2'000), {USD(2'000)}); + env.fund(XRP(2'000), gw1); + fund( + env, + gw1, + {alice, carol, bob, ed}, + {ETH(2'000), CAN(2'000)}, + Fund::IOUOnly); + env(rate(gw, gwRate)); + env(rate(gw1, gw1Rate)); + env.close(); + }; + + for (auto const& rates : + {std::make_pair(1.5, 1.9), std::make_pair(1.9, 1.5)}) + { + // Offer Selection + + // Cross-currency payment: AMM has the same spot price quality + // as CLOB's offer and can't generate a better quality offer. + // The transfer fee in this case doesn't change the CLOB quality + // because trIn is ignored on adjustment and trOut on payment is + // also ignored because ownerPaysTransferFee is false in this case. + // Run test for 0) offer, 1) AMM, 2) offer and AMM + // to verify that the quality is better in the first case, + // and CLOB is selected in the second case. + { + std::array q; + for (auto i = 0; i < 3; ++i) + { + Env env(*this); + prep(env, rates.first, rates.second); + std::shared_ptr amm; + if (i == 0 || i == 2) + { + env(offer(ed, ETH(400), USD(400)), txflags(tfPassive)); + env.close(); + } + if (i > 0) + amm = std::make_shared( + env, ed, USD(1'000), ETH(1'000)); + env(pay(carol, bob, USD(100)), + path(~USD), + sendmax(ETH(500)), + txflags(tfPartialPayment)); + env.close(); + // CLOB and AMM, AMM is not selected + if (i == 2) + { + BEAST_EXPECT(amm->expectBalances( + USD(1'000), ETH(1'000), amm->tokens())); + } + BEAST_EXPECT(expectLine(env, bob, USD(2'100))); + q[i] = Quality(Amounts{ + ETH(2'000) - env.balance(carol, ETH), + env.balance(bob, USD) - USD(2'000)}); + } + // CLOB is better quality than AMM + BEAST_EXPECT(q[0] > q[1]); + // AMM is not selected with CLOB + BEAST_EXPECT(q[0] == q[2]); + } + // Offer crossing: AMM has the same spot price quality + // as CLOB's offer and can't generate a better quality offer. + // The transfer fee in this case doesn't change the CLOB quality + // because the quality adjustment is ignored for the offer crossing. + for (auto i = 0; i < 3; ++i) + { + Env env(*this); + prep(env, rates.first, rates.second); + std::shared_ptr amm; + if (i == 0 || i == 2) + { + env(offer(ed, ETH(400), USD(400)), txflags(tfPassive)); + env.close(); + } + if (i > 0) + amm = + std::make_shared(env, ed, USD(1'000), ETH(1'000)); + env(offer(alice, USD(400), ETH(400))); + env.close(); + // AMM is not selected + if (i > 0) + { + BEAST_EXPECT(amm->expectBalances( + USD(1'000), ETH(1'000), amm->tokens())); + } + if (i == 0 || i == 2) + { + // Fully crosses + BEAST_EXPECT(expectOffers(env, ed, 0)); + BEAST_EXPECT(expectOffers(env, alice, 0)); + } + // Fails to cross because AMM is not selected + else + { + BEAST_EXPECT(expectOffers( + env, alice, 1, {Amounts{USD(400), ETH(400)}})); + BEAST_EXPECT(expectOffers(env, ed, 0)); + } + } + + // Show that the CLOB quality reduction + // results in AMM offer selection. + + // Same as the payment but reduced offer quality + { + std::array q; + for (auto i = 0; i < 3; ++i) + { + Env env(*this); + prep(env, rates.first, rates.second); + std::shared_ptr amm; + if (i == 0 || i == 2) + { + env(offer(ed, ETH(400), USD(300)), txflags(tfPassive)); + env.close(); + } + if (i > 0) + amm = std::make_shared( + env, ed, USD(1'000), ETH(1'000)); + env(pay(carol, bob, USD(100)), + path(~USD), + sendmax(ETH(500)), + txflags(tfPartialPayment)); + env.close(); + // AMM and CLOB are selected + if (i > 0) + { + BEAST_EXPECT(!amm->expectBalances( + USD(1'000), ETH(1'000), amm->tokens())); + } + if (i == 2) + { + if (rates.first == 1.5) + { + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, UINT64_C(378'6327949540823), -13}, + STAmount{ + USD, + UINT64_C(283'9745962155617), + -13}}}})); + } + else + { + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ + ETH, UINT64_C(325'299461620749), -12}, + STAmount{ + USD, + UINT64_C(243'9745962155617), + -13}}}})); + } + } + BEAST_EXPECT(expectLine(env, bob, USD(2'100))); + q[i] = Quality(Amounts{ + ETH(2'000) - env.balance(carol, ETH), + env.balance(bob, USD) - USD(2'000)}); + } + // AMM is better quality + BEAST_EXPECT(q[1] > q[0]); + // AMM and CLOB produce better quality + BEAST_EXPECT(q[2] > q[1]); + } + + // Same as the offer-crossing but reduced offer quality + for (auto i = 0; i < 3; ++i) + { + Env env(*this); + prep(env, rates.first, rates.second); + std::shared_ptr amm; + if (i == 0 || i == 2) + { + env(offer(ed, ETH(400), USD(250)), txflags(tfPassive)); + env.close(); + } + if (i > 0) + amm = + std::make_shared(env, ed, USD(1'000), ETH(1'000)); + env(offer(alice, USD(250), ETH(400))); + env.close(); + // AMM is selected in both cases + if (i > 0) + { + BEAST_EXPECT(!amm->expectBalances( + USD(1'000), ETH(1'000), amm->tokens())); + } + // Partially crosses, AMM is selected, CLOB fails limitQuality + if (i == 2) + { + if (rates.first == 1.5) + { + BEAST_EXPECT(expectOffers( + env, ed, 1, {{Amounts{ETH(400), USD(250)}}})); + BEAST_EXPECT(expectOffers( + env, + alice, + 1, + {{Amounts{ + STAmount{USD, UINT64_C(40'5694150420947), -13}, + STAmount{ETH, UINT64_C(64'91106406735152), -14}, + }}})); + } + else + { + // Ed offer is partially crossed. + BEAST_EXPECT(expectOffers( + env, + ed, + 1, + {{Amounts{ + STAmount{ETH, UINT64_C(335'0889359326485), -13}, + STAmount{USD, UINT64_C(209'4305849579053), -13}, + }}})); + BEAST_EXPECT(expectOffers(env, alice, 0)); + } + } + } + + // Strand selection + + // Two book steps strand quality is 1. + // AMM strand's best quality is equal to AMM's spot price + // quality, which is 1. Both strands (steps) are adjusted + // for the transfer fee in qualityUpperBound. In case + // of two strands, AMM offers have better quality and are consumed + // first, remaining liquidity is generated by CLOB offers. + // Liquidity from two strands is better in this case than in case + // of one strand with two book steps. Liquidity from one strand + // with AMM has better quality than either one strand with two book + // steps or two strands. It may appear unintuitive, but one strand + // with AMM is optimized and generates one AMM offer, while in case + // of two strands, multiple AMM offers are generated, which results + // in slightly worse overall quality. + { + std::array q; + for (auto i = 0; i < 3; ++i) + { + Env env(*this); + prep(env, rates.first, rates.second); + std::shared_ptr amm; + + if (i == 0 || i == 2) + { + env(offer(ed, ETH(400), CAN(400)), txflags(tfPassive)); + env(offer(ed, CAN(400), USD(400))), txflags(tfPassive); + env.close(); + } + + if (i > 0) + amm = std::make_shared( + env, ed, ETH(1'000), USD(1'000)); + + env(pay(carol, bob, USD(100)), + path(~USD), + path(~CAN, ~USD), + sendmax(ETH(600)), + txflags(tfPartialPayment)); + env.close(); + + BEAST_EXPECT(expectLine(env, bob, USD(2'100))); + + if (i == 2) + { + // Liquidity is consumed from AMM strand only + if (rates.first == 1.5) + { + BEAST_EXPECT(amm->expectBalances( + STAmount{ETH, UINT64_C(1'176'66038955758), -11}, + USD(850), + amm->tokens())); + } + else + { + BEAST_EXPECT(amm->expectBalances( + STAmount{ + ETH, UINT64_C(1'179'540094339627), -12}, + STAmount{USD, UINT64_C(847'7880529867501), -13}, + amm->tokens())); + BEAST_EXPECT(expectOffers( + env, + ed, + 2, + {{Amounts{ + STAmount{ + ETH, + UINT64_C(343'3179205198749), + -13}, + STAmount{ + CAN, + UINT64_C(343'3179205198749), + -13}, + }, + Amounts{ + STAmount{ + CAN, + UINT64_C(362'2119470132499), + -13}, + STAmount{ + USD, + UINT64_C(362'2119470132499), + -13}, + }}})); + } + } + q[i] = Quality(Amounts{ + ETH(2'000) - env.balance(carol, ETH), + env.balance(bob, USD) - USD(2'000)}); + } + BEAST_EXPECT(q[1] > q[0]); + BEAST_EXPECT(q[2] > q[0] && q[2] < q[1]); + } + } + } + void testCore() { @@ -4484,6 +4819,7 @@ struct AMM_test : public jtx::AMMTest testAutoDelete(); testClawback(); testAMMID(); + testSelection(); } void @@ -4496,4 +4832,4 @@ struct AMM_test : public jtx::AMMTest BEAST_DEFINE_TESTSUITE_PRIO(AMM, app, ripple, 1); } // namespace test -} // namespace ripple \ No newline at end of file +} // namespace ripple From cfa32015afec800e372974cf5b8a9b9e186d8415 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 11 Aug 2023 10:08:29 -0400 Subject: [PATCH 17/18] Address reviewer's feedback --- src/test/app/AMM_test.cpp | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 616b32a08ef..8bf719fa7ec 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4502,19 +4502,17 @@ struct AMM_test : public jtx::AMMTest { Env env(*this); prep(env, rates.first, rates.second); - std::shared_ptr amm; + std::optional amm; if (i == 0 || i == 2) { env(offer(ed, ETH(400), USD(400)), txflags(tfPassive)); env.close(); } if (i > 0) - amm = std::make_shared( - env, ed, USD(1'000), ETH(1'000)); + amm.emplace(env, ed, USD(1'000), ETH(1'000)); env(pay(carol, bob, USD(100)), path(~USD), - sendmax(ETH(500)), - txflags(tfPartialPayment)); + sendmax(ETH(500))); env.close(); // CLOB and AMM, AMM is not selected if (i == 2) @@ -4540,15 +4538,14 @@ struct AMM_test : public jtx::AMMTest { Env env(*this); prep(env, rates.first, rates.second); - std::shared_ptr amm; + std::optional amm; if (i == 0 || i == 2) { env(offer(ed, ETH(400), USD(400)), txflags(tfPassive)); env.close(); } if (i > 0) - amm = - std::make_shared(env, ed, USD(1'000), ETH(1'000)); + amm.emplace(env, ed, USD(1'000), ETH(1'000)); env(offer(alice, USD(400), ETH(400))); env.close(); // AMM is not selected @@ -4560,7 +4557,6 @@ struct AMM_test : public jtx::AMMTest if (i == 0 || i == 2) { // Fully crosses - BEAST_EXPECT(expectOffers(env, ed, 0)); BEAST_EXPECT(expectOffers(env, alice, 0)); } // Fails to cross because AMM is not selected @@ -4568,8 +4564,8 @@ struct AMM_test : public jtx::AMMTest { BEAST_EXPECT(expectOffers( env, alice, 1, {Amounts{USD(400), ETH(400)}})); - BEAST_EXPECT(expectOffers(env, ed, 0)); } + BEAST_EXPECT(expectOffers(env, ed, 0)); } // Show that the CLOB quality reduction @@ -4582,19 +4578,17 @@ struct AMM_test : public jtx::AMMTest { Env env(*this); prep(env, rates.first, rates.second); - std::shared_ptr amm; + std::optional amm; if (i == 0 || i == 2) { env(offer(ed, ETH(400), USD(300)), txflags(tfPassive)); env.close(); } if (i > 0) - amm = std::make_shared( - env, ed, USD(1'000), ETH(1'000)); + amm.emplace(env, ed, USD(1'000), ETH(1'000)); env(pay(carol, bob, USD(100)), path(~USD), - sendmax(ETH(500)), - txflags(tfPartialPayment)); + sendmax(ETH(500))); env.close(); // AMM and CLOB are selected if (i > 0) @@ -4649,15 +4643,14 @@ struct AMM_test : public jtx::AMMTest { Env env(*this); prep(env, rates.first, rates.second); - std::shared_ptr amm; + std::optional amm; if (i == 0 || i == 2) { env(offer(ed, ETH(400), USD(250)), txflags(tfPassive)); env.close(); } if (i > 0) - amm = - std::make_shared(env, ed, USD(1'000), ETH(1'000)); + amm.emplace(env, ed, USD(1'000), ETH(1'000)); env(offer(alice, USD(250), ETH(400))); env.close(); // AMM is selected in both cases @@ -4719,7 +4712,7 @@ struct AMM_test : public jtx::AMMTest { Env env(*this); prep(env, rates.first, rates.second); - std::shared_ptr amm; + std::optional amm; if (i == 0 || i == 2) { @@ -4729,23 +4722,21 @@ struct AMM_test : public jtx::AMMTest } if (i > 0) - amm = std::make_shared( - env, ed, ETH(1'000), USD(1'000)); + amm.emplace(env, ed, ETH(1'000), USD(1'000)); env(pay(carol, bob, USD(100)), path(~USD), path(~CAN, ~USD), - sendmax(ETH(600)), - txflags(tfPartialPayment)); + sendmax(ETH(600))); env.close(); BEAST_EXPECT(expectLine(env, bob, USD(2'100))); if (i == 2) { - // Liquidity is consumed from AMM strand only if (rates.first == 1.5) { + // Liquidity is consumed from AMM strand only BEAST_EXPECT(amm->expectBalances( STAmount{ETH, UINT64_C(1'176'66038955758), -11}, USD(850), From a34d29b3cc37977db64c7251a3cc529ade973e3d Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Fri, 11 Aug 2023 12:53:27 -0400 Subject: [PATCH 18/18] Add comments to stress that the selection tests would have to be updated if OwnerPaysFeature is enabled. --- src/test/app/AMM_test.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 8bf719fa7ec..b0e5106f9eb 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4469,6 +4469,10 @@ struct AMM_test : public jtx::AMMTest auto const ETH = gw1["ETH"]; auto const CAN = gw1["CAN"]; + // These tests are expected to fail if the OwnerPaysFee feature + // is ever supported. Updates will need to be made to AMM handling + // in the payment engine, and these tests will need to be updated. + auto prep = [&](Env& env, auto gwRate, auto gw1Rate) { fund(env, gw, {alice, carol, bob, ed}, XRP(2'000), {USD(2'000)}); env.fund(XRP(2'000), gw1);