From efa182377356a6ac0162540428cf5edf168cab76 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 3 Jan 2025 11:56:54 -0500 Subject: [PATCH 1/5] lpt freeze --- include/xrpl/protocol/Feature.h | 2 +- include/xrpl/protocol/detail/features.macro | 1 + src/test/app/LPTokenTransfer_test.cpp | 499 ++++++++++++++++++++ src/xrpld/app/misc/detail/AMMUtils.cpp | 39 +- src/xrpld/app/paths/detail/DirectStep.cpp | 7 +- src/xrpld/app/paths/detail/StepChecks.h | 22 + src/xrpld/ledger/View.h | 7 + src/xrpld/ledger/detail/View.cpp | 44 +- 8 files changed, 608 insertions(+), 13 deletions(-) create mode 100644 src/test/app/LPTokenTransfer_test.cpp diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 18a9b9498aa..8821d531ff0 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -80,7 +80,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 83; +static constexpr std::size_t numFeatures = 84; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 31fc90cef80..27c7ce83550 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -29,6 +29,7 @@ // If you add an amendment here, then do not forget to increment `numFeatures` // in include/xrpl/protocol/Feature.h. +XRPL_FIX (FrozenLPTokenTransfer, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Credentials, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(AMMClawback, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (AMMv1_2, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/test/app/LPTokenTransfer_test.cpp b/src/test/app/LPTokenTransfer_test.cpp new file mode 100644 index 00000000000..1b5309b75fd --- /dev/null +++ b/src/test/app/LPTokenTransfer_test.cpp @@ -0,0 +1,499 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "xrpl/protocol/TER.h" +#include "xrpl/protocol/TxFlags.h" +#include +#include +#include + +#include +namespace ripple { +namespace test { + +class LPTokenTransfer_test : public jtx::AMMTest +{ + void + testDirectStep(FeatureBitset features) + { + testcase("DirectStep"); + + using namespace jtx; + Env env{*this, features}; + fund(env, gw, {alice}, {USD(20'000), BTC(0.5)}, Fund::All); + env.close(); + + AMM ammAlice(env, alice, USD(20'000), BTC(0.5)); + BEAST_EXPECT( + ammAlice.expectBalances(USD(20'000), BTC(0.5), IOUAmount{100, 0})); + + fund(env, gw, {carol}, {USD(4'000), BTC(1)}, Fund::Acct); + ammAlice.deposit(carol, 10); + BEAST_EXPECT( + ammAlice.expectBalances(USD(22'000), BTC(0.55), IOUAmount{110, 0})); + + fund(env, gw, {bob}, {USD(4'000), BTC(1)}, Fund::Acct); + ammAlice.deposit(bob, 10); + BEAST_EXPECT( + ammAlice.expectBalances(USD(24'000), BTC(0.60), IOUAmount{120, 0})); + + auto const lpIssue = ammAlice.lptIssue(); + env.trust(STAmount{lpIssue, 500}, alice); + env.trust(STAmount{lpIssue, 500}, bob); + env.trust(STAmount{lpIssue, 500}, carol); + env.close(); + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // bob can still send to lptoken to carol even tho carol's USD is + // frozen, regardless of whether fixFrozenLPTokenTransfer is enabled or + // not + // Note: Deep freeze is not considered for LPToken transfer + env(pay(bob, carol, STAmount{lpIssue, 5})); + env.close(); + + // cannot transfer to an amm account + env(pay(carol, lpIssue.getIssuer(), STAmount{lpIssue, 5}), + ter(tecNO_PERMISSION)); + env.close(); + + if (features[fixFrozenLPTokenTransfer]) + { + // carol is frozen on USD and therefore can't send lptoken to bob + env(pay(carol, bob, STAmount{lpIssue, 5}), ter(tecPATH_DRY)); + } + else + { + // carol can still send lptoken with frozen USD + env(pay(carol, bob, STAmount{lpIssue, 5})); + } + } + + void + testBookStep(FeatureBitset features) + { + testcase("BookStep"); + + using namespace jtx; + Env env{*this, features}; + + fund( + env, + gw, + {alice, bob, carol}, + {USD(10'000), EUR(10'000)}, + Fund::All); + AMM ammAlice(env, alice, USD(10'000), EUR(10'000)); + ammAlice.deposit(carol, 1'000); + ammAlice.deposit(bob, 1'000); + + auto const lpIssue = ammAlice.lptIssue(); + + // carols creates an offer to sell lptoken + env(offer(carol, XRP(10), STAmount{lpIssue, 10}), txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 1)); + + env.trust(STAmount{lpIssue, 1'000'000'000}, alice); + env.trust(STAmount{lpIssue, 1'000'000'000}, bob); + env.trust(STAmount{lpIssue, 1'000'000'000}, carol); + env.close(); + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // exercises alice's ability to consume carol's offer to sell lptoken + // when carol's USD is frozen pre/post fixFrozenLPTokenTransfer + // amendment + if (features[fixFrozenLPTokenTransfer]) + { + // with fixFrozenLPTokenTransfer, alice fails to consume carol's + // offer since carol's USD is frozen + env(pay(alice, bob, STAmount{lpIssue, 10}), + txflags(tfPartialPayment), + sendmax(XRP(10)), + ter(tecPATH_DRY)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 1)); + + // gateway unfreezes carol's USD + env(trust(gw, carol["USD"](1'000'000'000), tfClearFreeze)); + env.close(); + + // alice successfully consumes carol's offer + env(pay(alice, bob, STAmount{lpIssue, 10}), + txflags(tfPartialPayment), + sendmax(XRP(10))); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 0)); + } + else + { + // without fixFrozenLPTokenTransfer, alice can consume carol's offer + // even when carol's USD is frozen + env(pay(alice, bob, STAmount{lpIssue, 10}), + txflags(tfPartialPayment), + sendmax(XRP(10))); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 0)); + } + + // make sure carol's USD is not frozen + env(trust(gw, carol["USD"](1'000'000'000), tfClearFreeze)); + env.close(); + + // ensure that carol's offer to buy lptoken can be consumed by alice + // even when carol's USD is frozen + { + // carol creates an offer to buy lptoken + env(offer(carol, STAmount{lpIssue, 10}, XRP(10)), + txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 1)); + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // alice successfully consumes carol's offer + env(pay(alice, bob, XRP(10)), + txflags(tfPartialPayment), + sendmax(STAmount{lpIssue, 10})); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 0)); + } + } + + void + testOfferCreation(FeatureBitset features) + { + testcase("Create offer"); + + using namespace jtx; + Env env{*this, features}; + + fund( + env, + gw, + {alice, bob, carol}, + {USD(10'000), EUR(10'000)}, + Fund::All); + AMM ammAlice(env, alice, USD(10'000), EUR(10'000)); + ammAlice.deposit(carol, 1'000); + ammAlice.deposit(bob, 1'000); + + auto const lpIssue = ammAlice.lptIssue(); + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // exercises carol's ability to create a new offer to sell lptoken with + // frozen USD, before and after fixFrozenLPTokenTransfer + if (features[fixFrozenLPTokenTransfer]) + { + // with fixFrozenLPTokenTransfer, carol can't create an offer to + // sell lptoken when one of the assets is frozen + + // carol can't create an offer to sell lptoken + env(offer(carol, XRP(10), STAmount{lpIssue, 10}), + txflags(tfPassive), + ter(tecUNFUNDED_OFFER)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 0)); + + // gateway unfreezes carol's USD + env(trust(gw, carol["USD"](1'000'000'000), tfClearFreeze)); + env.close(); + + // carol can create an offer to sell lptoken after USD is unfrozen + env(offer(carol, XRP(10), STAmount{lpIssue, 10}), + txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 1)); + } + else + { + // without fixFrozenLPTokenTransfer, carol can create an offer + env(offer(carol, XRP(10), STAmount{lpIssue, 10}), + txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 1)); + } + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // carol can create offer to buy lptoken even if USD is frozen + env(offer(carol, STAmount{lpIssue, 10}, XRP(5)), txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 2)); + } + + void + testOfferCrossing(FeatureBitset features) + { + testcase("Offer crossing"); + + using namespace jtx; + Env env{*this, features}; + + // Offer crossing with two AMM LPTokens. + fund(env, gw, {alice, carol}, {USD(10'000)}, Fund::All); + AMM ammAlice1(env, alice, XRP(10'000), USD(10'000)); + ammAlice1.deposit(carol, 10'000'000); + + fund(env, gw, {alice, carol}, {EUR(10'000)}, Fund::IOUOnly); + AMM ammAlice2(env, alice, XRP(10'000), EUR(10'000)); + ammAlice2.deposit(carol, 10'000'000); + auto const token1 = ammAlice1.lptIssue(); + auto const token2 = ammAlice2.lptIssue(); + + // carol creates offer + env(offer(carol, STAmount{token2, 100}, STAmount{token1, 100})); + env.close(); + BEAST_EXPECT(expectOffers(env, carol, 1)); + + // gateway freezes carol's USD, carol's token1 should be frozen as well + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // alice creates an offer which exhibits different behavior on offer + // crossing depending on if fixFrozenLPTokenTransfer is enabled + env(offer(alice, STAmount{token1, 100}, STAmount{token2, 100})); + env.close(); + + // exercises carol's offer's ability to cross with alice's offer when + // carol's USD is frozen, before and after fixFrozenLPTokenTransfer + if (features[fixFrozenLPTokenTransfer]) + { + // with fixFrozenLPTokenTransfer enabled, alice's offer can no + // longer cross with carol's offer + BEAST_EXPECT( + expectLine(env, alice, STAmount{token1, 10'000'000}) && + expectLine(env, alice, STAmount{token2, 10'000'000})); + BEAST_EXPECT( + expectLine(env, carol, STAmount{token2, 10'000'000}) && + expectLine(env, carol, STAmount{token1, 10'000'000})); + BEAST_EXPECT( + expectOffers(env, alice, 1) && expectOffers(env, carol, 0)); + } + else + { + // alice's offer still crosses with carol's offer despite carol's + // token1 is frozen + BEAST_EXPECT( + expectLine(env, alice, STAmount{token1, 10'000'100}) && + expectLine(env, alice, STAmount{token2, 9'999'900})); + BEAST_EXPECT( + expectLine(env, carol, STAmount{token2, 10'000'100}) && + expectLine(env, carol, STAmount{token1, 9'999'900})); + BEAST_EXPECT( + expectOffers(env, alice, 0) && expectOffers(env, carol, 0)); + } + } + + void + testCheck(FeatureBitset features) + { + testcase("Check"); + + using namespace jtx; + Env env{*this, features}; + + fund( + env, + gw, + {alice, bob, carol}, + {USD(10'000), EUR(10'000)}, + Fund::All); + AMM ammAlice(env, alice, USD(10'000), EUR(10'000)); + ammAlice.deposit(carol, 1'000); + ammAlice.deposit(bob, 1'000); + + auto const lpIssue = ammAlice.lptIssue(); + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // carol can always create a check with lptoken that has frozen + // token + uint256 const chkId{keylet::check(carol, env.seq(carol)).key}; + env(check::create(carol, bob, STAmount{lpIssue, 10})); + env.close(); + + // with fixFrozenLPTokenTransfer enabled, bob fails to cash the check + if (features[fixFrozenLPTokenTransfer]) + env(check::cash(bob, chkId, STAmount{lpIssue, 10}), + ter(tecPATH_PARTIAL)); + else + env(check::cash(bob, chkId, STAmount{lpIssue, 10})); + + env.close(); + } + + void + testNFTOffers(FeatureBitset features) + { + testcase("NFT Offers"); + using namespace test::jtx; + + Env env{*this, features}; + + // Setup AMM + fund( + env, + gw, + {alice, bob, carol}, + {USD(10'000), EUR(10'000)}, + Fund::All); + AMM ammAlice(env, alice, USD(10'000), EUR(10'000)); + ammAlice.deposit(carol, 1'000); + ammAlice.deposit(bob, 1'000); + + auto const lpIssue = ammAlice.lptIssue(); + + // bob mints a nft + uint256 const nftID{token::getNextID(env, bob, 0u, tfTransferable)}; + env(token::mint(bob, 0), txflags(tfTransferable)); + env.close(); + + // bob creates a sell offer for lptoken + uint256 const sellOfferIndex = keylet::nftoffer(bob, env.seq(bob)).key; + env(token::createOffer(bob, nftID, STAmount{lpIssue, 10}), + txflags(tfSellNFToken)); + env.close(); + + // gateway freezes carol's USD + env(trust(gw, carol["USD"](0), tfSetFreeze)); + env.close(); + + // exercises one's ability to transfer NFT using lptoken when one of the + // assets is frozen + if (features[fixFrozenLPTokenTransfer]) + { + // with fixFrozenLPTokenTransfer, freezing USD will prevent buy/sell + // offers with lptokens from being created/accepted + + // carol fails to accept bob's offer with lptoken because carol's + // USD is frozen + env(token::acceptSellOffer(carol, sellOfferIndex), + ter(tecINSUFFICIENT_FUNDS)); + env.close(); + + // gateway unfreezes carol's USD + env(trust(gw, carol["USD"](1'000'000), tfClearFreeze)); + env.close(); + + // carol can now accept the offer and own the nft + env(token::acceptSellOffer(carol, sellOfferIndex)); + env.close(); + + // gateway freezes bobs's USD + env(trust(gw, bob["USD"](0), tfSetFreeze)); + env.close(); + + // bob fails to create a buy offer with lptoken for carol's nft + // since bob's USD is frozen + env(token::createOffer(bob, nftID, STAmount{lpIssue, 10}), + token::owner(carol), + ter(tecUNFUNDED_OFFER)); + env.close(); + + // gateway unfreezes bob's USD + env(trust(gw, bob["USD"](1'000'000), tfClearFreeze)); + env.close(); + + // bob can now create a buy offer + env(token::createOffer(bob, nftID, STAmount{lpIssue, 10}), + token::owner(carol)); + env.close(); + } + else + { + // without fixFrozenLPTokenTransfer, freezing USD will still allow + // buy/sell offers to be created/accepted with lptoken + + // carol can still accept bob's offer despite carol's USD is frozen + env(token::acceptSellOffer(carol, sellOfferIndex)); + env.close(); + + // gateway freezes bob's USD + env(trust(gw, bob["USD"](0), tfSetFreeze)); + env.close(); + + // bob creates a buy offer with lptoken despite bob's USD is frozen + uint256 const buyOfferIndex = + keylet::nftoffer(bob, env.seq(bob)).key; + env(token::createOffer(bob, nftID, STAmount{lpIssue, 10}), + token::owner(carol)); + env.close(); + + // carol accepts bob's offer + env(token::acceptBuyOffer(carol, buyOfferIndex)); + env.close(); + } + } + +public: + void + run() override + { + using namespace test::jtx; + FeatureBitset const all{supported_amendments()}; + + for (auto const features : {all, all - fixFrozenLPTokenTransfer}) + { + testDirectStep(features); + testBookStep(features); + testOfferCreation(features); + testOfferCrossing(features); + testCheck(features); + testNFTOffers(features); + } + } +}; + +BEAST_DEFINE_TESTSUITE(LPTokenTransfer, app, ripple); +} // namespace test +} // namespace ripple diff --git a/src/xrpld/app/misc/detail/AMMUtils.cpp b/src/xrpld/app/misc/detail/AMMUtils.cpp index f5f6ae6612c..e5cdb3a4504 100644 --- a/src/xrpld/app/misc/detail/AMMUtils.cpp +++ b/src/xrpld/app/misc/detail/AMMUtils.cpp @@ -116,13 +116,38 @@ ammLPHolds( AccountID const& lpAccount, beast::Journal const j) { - return accountHolds( - view, - lpAccount, - ammLPTCurrency(cur1, cur2), - ammAccount, - FreezeHandling::fhZERO_IF_FROZEN, - j); + // This function looks similar to `accountHolds`. However, it only checks if + // a LPToken holder has enough balance. On the other hand, `accountHolds` + // checks if the underlying assets of LPToken are frozen with the + // fixFrozenLPTokenTransfer amendment + + auto const currency = ammLPTCurrency(cur1, cur2); + STAmount amount; + + auto const sle = view.read(keylet::line(lpAccount, ammAccount, currency)); + if (!sle) + { + amount.clear(Issue{currency, ammAccount}); + } + else if (isFrozen(view, lpAccount, currency, ammAccount)) + { + amount.clear(Issue{currency, ammAccount}); + } + else + { + amount = sle->getFieldAmount(sfBalance); + if (lpAccount > ammAccount) + { + // Put balance in account terms. + amount.negate(); + } + amount.setIssuer(ammAccount); + } + JLOG(j.trace()) << "ammLPHolds:" + << " lpAccount=" << to_string(lpAccount) + << " amount=" << amount.getFullText(); + + return view.balanceHook(lpAccount, ammAccount, amount); } STAmount diff --git a/src/xrpld/app/paths/detail/DirectStep.cpp b/src/xrpld/app/paths/detail/DirectStep.cpp index 95e64b337bc..20d482a2b31 100644 --- a/src/xrpld/app/paths/detail/DirectStep.cpp +++ b/src/xrpld/app/paths/detail/DirectStep.cpp @@ -204,7 +204,8 @@ class DirectStepI : public StepImp> logStringImpl(char const* name) const { std::ostringstream ostr; - ostr << name << ": " << "\nSrc: " << src_ << "\nDst: " << dst_; + ostr << name << ": " + << "\nSrc: " << src_ << "\nDst: " << dst_; return ostr.str(); } @@ -904,8 +905,8 @@ DirectStepI::check(StrandContext const& ctx) const // pure issue/redeem can't be frozen if (!(ctx.isLast && ctx.isFirst)) { - auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); - if (ter != tesSUCCESS) + if (auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); + ter != tesSUCCESS) return ter; } diff --git a/src/xrpld/app/paths/detail/StepChecks.h b/src/xrpld/app/paths/detail/StepChecks.h index 2c2fee91cf9..9d4dd366031 100644 --- a/src/xrpld/app/paths/detail/StepChecks.h +++ b/src/xrpld/app/paths/detail/StepChecks.h @@ -21,10 +21,12 @@ #define RIPPLE_APP_PATHS_IMPL_STEP_CHECKS_H_INCLUDED #include +#include #include #include #include #include +#include "xrpl/protocol/TER.h" namespace ripple { @@ -54,6 +56,26 @@ checkFreeze( } } + if (view.rules().enabled(fixFrozenLPTokenTransfer)) + { + if (auto const sleDst = view.read(keylet::account(dst)); + sleDst && sleDst->isFieldPresent(sfAMMID)) + { + auto const sleAmm = view.read(keylet::amm((*sleDst)[sfAMMID])); + if (!sleAmm) + return tecINTERNAL; + + if (isLPTokenFrozen( + view, + src, + (*sleAmm)[sfAsset].get(), + (*sleAmm)[sfAsset2].get())) + { + return terNO_LINE; + } + } + } + return tesSUCCESS; } diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index 74027752486..8bf56ef3a68 100644 --- a/src/xrpld/ledger/View.h +++ b/src/xrpld/ledger/View.h @@ -153,6 +153,13 @@ isFrozen(ReadView const& view, AccountID const& account, Asset const& asset) asset.value()); } +[[nodiscard]] bool +isLPTokenFrozen( + ReadView const& view, + AccountID const& account, + Issue asset, + Issue asset2); + // Returns the amount an account can spend without going into debt. // // <-- saAmount: amount of currency held by account. May be negative. diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index ebf307f1535..ffdebfa57ae 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -267,6 +267,17 @@ isFrozen( isIndividualFrozen(view, account, mptIssue); } +bool +isLPTokenFrozen( + ReadView const& view, + AccountID const& account, + Issue asset, + Issue asset2) +{ + return isFrozen(view, account, asset.currency, asset.account) || + isFrozen(view, account, asset2.currency, asset2.account); +} + STAmount accountHolds( ReadView const& view, @@ -303,8 +314,36 @@ accountHolds( amount.negate(); } amount.setIssuer(issuer); + + // if it's a LPToken, also need to check if issuers of the asset pair + // has frozen holder's trustline + if (view.rules().enabled(fixFrozenLPTokenTransfer) && + zeroIfFrozen == fhZERO_IF_FROZEN) + { + auto const sleIssuer = view.read(keylet::account(issuer)); + if (!sleIssuer) + { + amount.clear(Issue{currency, issuer}); + } + else if (sleIssuer->isFieldPresent(sfAMMID)) + { + auto const sleAmm = + view.read(keylet::amm((*sleIssuer)[sfAMMID])); + + if (!sleAmm || + isLPTokenFrozen( + view, + account, + (*sleAmm)[sfAsset].get(), + (*sleAmm)[sfAsset2].get())) + { + amount.clear(Issue{currency, issuer}); + } + } + } } - JLOG(j.trace()) << "accountHolds:" << " account=" << to_string(account) + JLOG(j.trace()) << "accountHolds:" + << " account=" << to_string(account) << " amount=" << amount.getFullText(); return view.balanceHook(account, issuer, amount); @@ -453,7 +492,8 @@ xrpLiquid( STAmount const amount = (balance < reserve) ? STAmount{0} : balance - reserve; - JLOG(j.trace()) << "accountHolds:" << " account=" << to_string(id) + JLOG(j.trace()) << "accountHolds:" + << " account=" << to_string(id) << " amount=" << amount.getFullText() << " fullBalance=" << fullBalance.getFullText() << " balance=" << balance.getFullText() From 01ce1e36389e60b6b3c599d447024cfac2a172aa Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 14 Jan 2025 14:36:28 -0500 Subject: [PATCH 2/5] refactor accountholds --- src/xrpld/ledger/detail/View.cpp | 79 ++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index ffdebfa57ae..d3e2ec1ef3c 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -295,53 +295,62 @@ accountHolds( // IOU: Return balance on trust line modulo freeze auto const sle = view.read(keylet::line(account, issuer, currency)); - if (!sle) - { - amount.clear(Issue{currency, issuer}); - } - else if ( - (zeroIfFrozen == fhZERO_IF_FROZEN) && - isFrozen(view, account, currency, issuer)) - { - amount.clear(Issue{currency, issuer}); - } - else - { - amount = sle->getFieldAmount(sfBalance); - if (account > issuer) + auto const allowBalance = [&]() { + if (!sle) { - // Put balance in account terms. - amount.negate(); + return false; } - amount.setIssuer(issuer); - // if it's a LPToken, also need to check if issuers of the asset pair - // has frozen holder's trustline - if (view.rules().enabled(fixFrozenLPTokenTransfer) && - zeroIfFrozen == fhZERO_IF_FROZEN) + if (zeroIfFrozen == fhZERO_IF_FROZEN) { - auto const sleIssuer = view.read(keylet::account(issuer)); - if (!sleIssuer) + if (isFrozen(view, account, currency, issuer)) { - amount.clear(Issue{currency, issuer}); + return false; } - else if (sleIssuer->isFieldPresent(sfAMMID)) + + if (view.rules().enabled(fixFrozenLPTokenTransfer)) { - auto const sleAmm = - view.read(keylet::amm((*sleIssuer)[sfAMMID])); - - if (!sleAmm || - isLPTokenFrozen( - view, - account, - (*sleAmm)[sfAsset].get(), - (*sleAmm)[sfAsset2].get())) + auto const sleIssuer = view.read(keylet::account(issuer)); + if (!sleIssuer) { - amount.clear(Issue{currency, issuer}); + return false; + } + else if (sleIssuer->isFieldPresent(sfAMMID)) + { + auto const sleAmm = + view.read(keylet::amm((*sleIssuer)[sfAMMID])); + + if (!sleAmm || + isLPTokenFrozen( + view, + account, + (*sleAmm)[sfAsset].get(), + (*sleAmm)[sfAsset2].get())) + { + return false; + } } } } + + return true; + }(); + + if (allowBalance) + { + amount = sle->getFieldAmount(sfBalance); + if (account > issuer) + { + // Put balance in account terms. + amount.negate(); + } + amount.setIssuer(issuer); } + else + { + amount.clear(Issue{currency, issuer}); + } + JLOG(j.trace()) << "accountHolds:" << " account=" << to_string(account) << " amount=" << amount.getFullText(); From ea97b086788660d622a0718918ff1a4130020e03 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 14 Jan 2025 14:39:33 -0500 Subject: [PATCH 3/5] comment --- src/xrpld/ledger/detail/View.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index d3e2ec1ef3c..b8efcfeccc6 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -308,6 +308,8 @@ accountHolds( return false; } + // when fixFrozenLPTokenTransfer is enabled, if currency is lptoken, + // we need to check if the associated assets have been frozen if (view.rules().enabled(fixFrozenLPTokenTransfer)) { auto const sleIssuer = view.read(keylet::account(issuer)); From 0409265c69609d9e59f9b6ff7ea64ad2d1924392 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Wed, 22 Jan 2025 12:21:12 -0500 Subject: [PATCH 4/5] addressc comments --- src/test/app/LPTokenTransfer_test.cpp | 29 +++-------------------- src/xrpld/app/misc/detail/AMMUtils.cpp | 4 ---- src/xrpld/app/paths/detail/DirectStep.cpp | 4 ++-- src/xrpld/app/paths/detail/StepChecks.h | 3 +-- src/xrpld/ledger/detail/View.cpp | 6 +---- 5 files changed, 7 insertions(+), 39 deletions(-) diff --git a/src/test/app/LPTokenTransfer_test.cpp b/src/test/app/LPTokenTransfer_test.cpp index 1b5309b75fd..efb403ae7f1 100644 --- a/src/test/app/LPTokenTransfer_test.cpp +++ b/src/test/app/LPTokenTransfer_test.cpp @@ -20,29 +20,7 @@ #include #include #include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include "xrpl/protocol/TER.h" -#include "xrpl/protocol/TxFlags.h" -#include -#include -#include - -#include + namespace ripple { namespace test { @@ -82,7 +60,7 @@ class LPTokenTransfer_test : public jtx::AMMTest env(trust(gw, carol["USD"](0), tfSetFreeze)); env.close(); - // bob can still send to lptoken to carol even tho carol's USD is + // bob can still send lptoken to carol even tho carol's USD is // frozen, regardless of whether fixFrozenLPTokenTransfer is enabled or // not // Note: Deep freeze is not considered for LPToken transfer @@ -479,8 +457,7 @@ class LPTokenTransfer_test : public jtx::AMMTest void run() override { - using namespace test::jtx; - FeatureBitset const all{supported_amendments()}; + FeatureBitset const all{jtx::supported_amendments()}; for (auto const features : {all, all - fixFrozenLPTokenTransfer}) { diff --git a/src/xrpld/app/misc/detail/AMMUtils.cpp b/src/xrpld/app/misc/detail/AMMUtils.cpp index e5cdb3a4504..d99eaf1f284 100644 --- a/src/xrpld/app/misc/detail/AMMUtils.cpp +++ b/src/xrpld/app/misc/detail/AMMUtils.cpp @@ -126,13 +126,9 @@ ammLPHolds( auto const sle = view.read(keylet::line(lpAccount, ammAccount, currency)); if (!sle) - { amount.clear(Issue{currency, ammAccount}); - } else if (isFrozen(view, lpAccount, currency, ammAccount)) - { amount.clear(Issue{currency, ammAccount}); - } else { amount = sle->getFieldAmount(sfBalance); diff --git a/src/xrpld/app/paths/detail/DirectStep.cpp b/src/xrpld/app/paths/detail/DirectStep.cpp index 20d482a2b31..34723d36bca 100644 --- a/src/xrpld/app/paths/detail/DirectStep.cpp +++ b/src/xrpld/app/paths/detail/DirectStep.cpp @@ -905,8 +905,8 @@ DirectStepI::check(StrandContext const& ctx) const // pure issue/redeem can't be frozen if (!(ctx.isLast && ctx.isFirst)) { - if (auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); - ter != tesSUCCESS) + auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); + if (ter != tesSUCCESS) return ter; } diff --git a/src/xrpld/app/paths/detail/StepChecks.h b/src/xrpld/app/paths/detail/StepChecks.h index 9d4dd366031..536dfdae94a 100644 --- a/src/xrpld/app/paths/detail/StepChecks.h +++ b/src/xrpld/app/paths/detail/StepChecks.h @@ -26,7 +26,6 @@ #include #include #include -#include "xrpl/protocol/TER.h" namespace ripple { @@ -63,7 +62,7 @@ checkFreeze( { auto const sleAmm = view.read(keylet::amm((*sleDst)[sfAMMID])); if (!sleAmm) - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE if (isLPTokenFrozen( view, diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index b8efcfeccc6..1e1158ad984 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -297,16 +297,12 @@ accountHolds( auto const sle = view.read(keylet::line(account, issuer, currency)); auto const allowBalance = [&]() { if (!sle) - { return false; - } if (zeroIfFrozen == fhZERO_IF_FROZEN) { if (isFrozen(view, account, currency, issuer)) - { return false; - } // when fixFrozenLPTokenTransfer is enabled, if currency is lptoken, // we need to check if the associated assets have been frozen @@ -315,7 +311,7 @@ accountHolds( auto const sleIssuer = view.read(keylet::account(issuer)); if (!sleIssuer) { - return false; + return false; // LCOV_EXCL_LINE } else if (sleIssuer->isFieldPresent(sfAMMID)) { From d1d0abb230f85d80a8dafe475ffd7925f3fb3a70 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Wed, 22 Jan 2025 13:53:03 -0500 Subject: [PATCH 5/5] const ref param --- src/xrpld/ledger/View.h | 4 ++-- src/xrpld/ledger/detail/View.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index 8bf56ef3a68..d626d79fd45 100644 --- a/src/xrpld/ledger/View.h +++ b/src/xrpld/ledger/View.h @@ -157,8 +157,8 @@ isFrozen(ReadView const& view, AccountID const& account, Asset const& asset) isLPTokenFrozen( ReadView const& view, AccountID const& account, - Issue asset, - Issue asset2); + Issue const& asset, + Issue const& asset2); // Returns the amount an account can spend without going into debt. // diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 1e1158ad984..9b959601fbf 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -271,8 +271,8 @@ bool isLPTokenFrozen( ReadView const& view, AccountID const& account, - Issue asset, - Issue asset2) + Issue const& asset, + Issue const& asset2) { return isFrozen(view, account, asset.currency, asset.account) || isFrozen(view, account, asset2.currency, asset2.account);