From eeeb90c6f6e4089cfa4b438ccb2b6cb1878e7b7c Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Wed, 10 May 2023 10:17:43 -0400 Subject: [PATCH 01/17] Clawback support --- Builds/CMake/RippledCore.cmake | 2 + src/ripple/app/tx/impl/Clawback.cpp | 173 +++++ src/ripple/app/tx/impl/Clawback.h | 55 ++ src/ripple/app/tx/impl/SetAccount.cpp | 35 + src/ripple/app/tx/impl/applySteps.cpp | 11 + src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/LedgerFormats.h | 5 + src/ripple/protocol/TxFlags.h | 4 + src/ripple/protocol/TxFormats.h | 3 + src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/TxFormats.cpp | 8 + src/ripple/protocol/jss.h | 1 + src/test/app/Clawback_test.cpp | 971 +++++++++++++++++++++++++ src/test/jtx/flags.h | 3 + src/test/jtx/impl/trust.cpp | 11 + src/test/jtx/trust.h | 3 + src/test/rpc/AccountSet_test.cpp | 6 + 17 files changed, 1294 insertions(+), 1 deletion(-) create mode 100644 src/ripple/app/tx/impl/Clawback.cpp create mode 100644 src/ripple/app/tx/impl/Clawback.h create mode 100644 src/test/app/Clawback_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index db7757f9c2f..f772c494e9a 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -427,6 +427,7 @@ target_sources (rippled PRIVATE src/ripple/app/tx/impl/CancelOffer.cpp src/ripple/app/tx/impl/CashCheck.cpp src/ripple/app/tx/impl/Change.cpp + src/ripple/app/tx/impl/Clawback.cpp src/ripple/app/tx/impl/CreateCheck.cpp src/ripple/app/tx/impl/CreateOffer.cpp src/ripple/app/tx/impl/CreateTicket.cpp @@ -692,6 +693,7 @@ if (tests) src/test/app/AccountTxPaging_test.cpp src/test/app/AmendmentTable_test.cpp src/test/app/Check_test.cpp + src/test/app/Clawback_test.cpp src/test/app/CrossingLimits_test.cpp src/test/app/DeliverMin_test.cpp src/test/app/DepositAuth_test.cpp diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp new file mode 100644 index 00000000000..524c513269b --- /dev/null +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -0,0 +1,173 @@ +//------------------------------------------------------------------------------ +/* + 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. +*/ +//============================================================================== + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace ripple { + +NotTEC +Clawback::preflight(PreflightContext const& ctx) +{ + if (!ctx.rules.enabled(featureClawback)) + return temDISABLED; + + if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) + return ret; + + if (ctx.tx.getFlags() & tfClawbackMask) + return temINVALID_FLAG; + + AccountID const issuer = ctx.tx.getAccountID(sfAccount); + STAmount const clawAmount(ctx.tx.getFieldAmount(sfAmount)); + + // The issuer field is used for the token holder instead + AccountID const holder = clawAmount.getIssuer(); + + if (issuer == holder || isXRP(clawAmount) || clawAmount <= beast::zero) + return temBAD_AMOUNT; + + return preflight2(ctx); +} + +TER +Clawback::preclaim(PreclaimContext const& ctx) +{ + AccountID const issuer = ctx.tx.getAccountID(sfAccount); + STAmount const clawAmount(ctx.tx.getFieldAmount(sfAmount)); + AccountID const holder = clawAmount.getIssuer(); + + auto const sleIssuer = ctx.view.read(keylet::account(issuer)); + auto const sleHolder = ctx.view.read(keylet::account(holder)); + if (!sleIssuer || !sleHolder) + return terNO_ACCOUNT; + + std::uint32_t const issuerFlagsIn = sleIssuer->getFieldU32(sfFlags); + + // If AllowClawback is not set or NoFreeze is set, return no permission + if (!(issuerFlagsIn & lsfAllowClawback) || (issuerFlagsIn & lsfNoFreeze)) + return tecNO_PERMISSION; + + auto const sleRippleState = + ctx.view.read(keylet::line(holder, issuer, clawAmount.getCurrency())); + if (!sleRippleState) + return tecNO_LINE; + + STAmount const balance = sleRippleState->getFieldAmount(sfBalance); + + // If balance is positive, issuer must have higher address than holder + if (balance > beast::zero && issuer < holder) + return tecNO_PERMISSION; + + // If balance is negative, issuer must have lower address than holder + if (balance < beast::zero && issuer > holder) + return tecNO_PERMISSION; + + // At this point, we know that issuer and holder accounts + // are correct and a trustline exists between them. + // + // Must now explicitly check the balance to make sure + // available balance is non-zero. + // + // We can't directly check the balance of trustline because + // the available balance of a trustline is prone to new changes (eg. + // XLS-34). So we must use `accountHolds`. + if (accountHolds( + ctx.view, + holder, + clawAmount.getCurrency(), + issuer, + fhIGNORE_FREEZE, + ctx.j) <= beast::zero) + return tecNO_LINE; + + return tesSUCCESS; +} + +TER +Clawback::clawback( + AccountID const& issuer, + AccountID const& holder, + STAmount const& amount) +{ + // This should never happen + if (amount <= beast::zero || issuer != amount.getIssuer()) + return tecINTERNAL; + + STAmount const holderBalance = accountHolds( + view(), + holder, + amount.getCurrency(), + amount.getIssuer(), + fhIGNORE_FREEZE, + j_); + + // The amount to be clawed back can never exceed the amount that + // the token holder current owns + if (amount > holderBalance) + return tecINTERNAL; + + auto const result = accountSend(view(), holder, issuer, amount, j_); + + if (!isTesSuccess(result)) + return result; + + if (accountHolds( + view(), + holder, + amount.getCurrency(), + amount.getIssuer(), + fhIGNORE_FREEZE, + j_) + .signum() < 0) + return tecINTERNAL; + + return tesSUCCESS; +} + +TER +Clawback::doApply() +{ + AccountID const issuer = account_; + STAmount clawAmount(ctx_.tx.getFieldAmount(sfAmount)); + AccountID const holder = clawAmount.getIssuer(); + + // Replace the `issuer` field with holder's account + clawAmount.setIssuer(issuer); + + // Get the spendable balance. Must use `accountHolds`. + STAmount const spendableAmount = accountHolds( + view(), + holder, + clawAmount.getCurrency(), + clawAmount.getIssuer(), + fhIGNORE_FREEZE, + j_); + + return clawback(issuer, holder, std::min(spendableAmount, clawAmount)); +} + +} // namespace ripple diff --git a/src/ripple/app/tx/impl/Clawback.h b/src/ripple/app/tx/impl/Clawback.h new file mode 100644 index 00000000000..5128ee39e94 --- /dev/null +++ b/src/ripple/app/tx/impl/Clawback.h @@ -0,0 +1,55 @@ +//------------------------------------------------------------------------------ +/* + 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_CLAWBACK_H_INCLUDED +#define RIPPLE_TX_CLAWBACK_H_INCLUDED + +#include + +namespace ripple { + +class Clawback : public Transactor +{ +private: + TER + clawback( + AccountID const& issuer, + AccountID const& holder, + STAmount const& amount); + +public: + static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; + + explicit Clawback(ApplyContext& ctx) : Transactor(ctx) + { + } + + static NotTEC + preflight(PreflightContext const& ctx); + + static TER + preclaim(PreclaimContext const& ctx); + + TER + doApply() override; +}; + +} // namespace ripple + +#endif diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index 167644f45fb..80256e7ab28 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -218,6 +218,25 @@ SetAccount::preclaim(PreclaimContext const& ctx) } } + // + // Clawback + // + if (ctx.view.rules().enabled(featureClawback) && + (uSetFlag == asfAllowClawback)) + { + if (uFlagsIn & lsfNoFreeze) + { + JLOG(ctx.j.trace()) << "Can't set Clawback if NoFreeze is set"; + return tecNO_PERMISSION; + } + + if (!dirIsEmpty(ctx.view, keylet::ownerDir(id))) + { + JLOG(ctx.j.trace()) << "Owner directory not empty."; + return tecOWNERS; + } + } + return tesSUCCESS; } @@ -361,6 +380,14 @@ SetAccount::doApply() return tecNEED_MASTER_KEY; } + // Cannot set NoFreeze if clawback is enabled + if (ctx_.view().rules().enabled(featureClawback) && + (uFlagsIn & lsfAllowClawback)) + { + JLOG(j_.trace()) << "Can't set NoFreeze if clawback is enabled"; + return tecNO_PERMISSION; + } + JLOG(j_.trace()) << "Set NoFreeze flag"; uFlagsOut |= lsfNoFreeze; } @@ -562,6 +589,14 @@ SetAccount::doApply() uFlagsOut &= ~lsfDisallowIncomingTrustline; } + // Set flag for clawback + if (ctx_.view().rules().enabled(featureClawback) && + uSetFlag == asfAllowClawback) + { + JLOG(j_.trace()) << "set allow clawback"; + uFlagsOut |= lsfAllowClawback; + } + if (uFlagsIn != uFlagsOut) sle->setFieldU32(sfFlags, uFlagsOut); diff --git a/src/ripple/app/tx/impl/applySteps.cpp b/src/ripple/app/tx/impl/applySteps.cpp index 85959862dba..d0a09c31029 100644 --- a/src/ripple/app/tx/impl/applySteps.cpp +++ b/src/ripple/app/tx/impl/applySteps.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -147,6 +148,8 @@ invoke_preflight(PreflightContext const& ctx) return invoke_preflight_helper(ctx); case ttNFTOKEN_ACCEPT_OFFER: return invoke_preflight_helper(ctx); + case ttCLAWBACK: + return invoke_preflight_helper(ctx); default: assert(false); return {temUNKNOWN, TxConsequences{temUNKNOWN}}; @@ -248,6 +251,8 @@ invoke_preclaim(PreclaimContext const& ctx) return invoke_preclaim(ctx); case ttNFTOKEN_ACCEPT_OFFER: return invoke_preclaim(ctx); + case ttCLAWBACK: + return invoke_preclaim(ctx); default: assert(false); return temUNKNOWN; @@ -311,6 +316,8 @@ invoke_calculateBaseFee(ReadView const& view, STTx const& tx) return NFTokenCancelOffer::calculateBaseFee(view, tx); case ttNFTOKEN_ACCEPT_OFFER: return NFTokenAcceptOffer::calculateBaseFee(view, tx); + case ttCLAWBACK: + return Clawback::calculateBaseFee(view, tx); default: assert(false); return XRPAmount{0}; @@ -463,6 +470,10 @@ invoke_apply(ApplyContext& ctx) NFTokenAcceptOffer p(ctx); return p(); } + case ttCLAWBACK: { + Clawback p(ctx); + return p(); + } default: assert(false); return {temUNKNOWN, false}; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index e4b0e3d4acd..8321d5555ab 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,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 = 58; +static constexpr std::size_t numFeatures = 59; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -345,6 +345,7 @@ extern uint256 const featureXRPFees; extern uint256 const fixUniversalNumber; extern uint256 const fixNonFungibleTokensV1_2; extern uint256 const fixNFTokenRemint; +extern uint256 const featureClawback; } // namespace ripple diff --git a/src/ripple/protocol/LedgerFormats.h b/src/ripple/protocol/LedgerFormats.h index 8245f352c8e..e0b1c122a47 100644 --- a/src/ripple/protocol/LedgerFormats.h +++ b/src/ripple/protocol/LedgerFormats.h @@ -243,6 +243,11 @@ enum LedgerSpecificFlags { 0x10000000, // True, reject new paychans lsfDisallowIncomingTrustline = 0x20000000, // True, reject new trustlines (only if no issued assets) +/* // reserved for AMM amendment + lsfAMM = 0x40000000, // True, AMM account +*/ + lsfAllowClawback = + 0x80000000, // True, enable clawback // ltOFFER lsfPassive = 0x00010000, diff --git a/src/ripple/protocol/TxFlags.h b/src/ripple/protocol/TxFlags.h index c0dd080f6f7..9eaf5de7860 100644 --- a/src/ripple/protocol/TxFlags.h +++ b/src/ripple/protocol/TxFlags.h @@ -88,6 +88,7 @@ constexpr std::uint32_t asfDisallowIncomingNFTokenOffer = 12; constexpr std::uint32_t asfDisallowIncomingCheck = 13; constexpr std::uint32_t asfDisallowIncomingPayChan = 14; constexpr std::uint32_t asfDisallowIncomingTrustline = 15; +constexpr std::uint32_t asfAllowClawback = 16; // OfferCreate flags: constexpr std::uint32_t tfPassive = 0x00010000; @@ -159,6 +160,9 @@ constexpr std::uint32_t const tfNFTokenCancelOfferMask = ~(tfUniversal); // NFTokenAcceptOffer flags: constexpr std::uint32_t const tfNFTokenAcceptOfferMask = ~tfUniversal; +// Clawback flags: +constexpr std::uint32_t const tfClawbackMask = ~tfUniversal; + // clang-format on } // namespace ripple diff --git a/src/ripple/protocol/TxFormats.h b/src/ripple/protocol/TxFormats.h index 250c29d69c1..11df0af7b23 100644 --- a/src/ripple/protocol/TxFormats.h +++ b/src/ripple/protocol/TxFormats.h @@ -139,6 +139,9 @@ enum TxType : std::uint16_t /** This transaction accepts an existing offer to buy or sell an existing NFT. */ ttNFTOKEN_ACCEPT_OFFER = 29, + /** This transaction claws back issued tokens. */ + ttCLAWBACK = 30, + /** 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/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 6b8e7719d00..54864adbaab 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -452,6 +452,7 @@ REGISTER_FEATURE(XRPFees, Supported::yes, VoteBehavior::De REGISTER_FIX (fixUniversalNumber, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNonFungibleTokensV1_2, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenRemint, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/ripple/protocol/impl/TxFormats.cpp b/src/ripple/protocol/impl/TxFormats.cpp index 91a6bcb581b..d6d017508f2 100644 --- a/src/ripple/protocol/impl/TxFormats.cpp +++ b/src/ripple/protocol/impl/TxFormats.cpp @@ -328,6 +328,14 @@ TxFormats::TxFormats() {sfTicketSequence, soeOPTIONAL}, }, commonFields); + + add(jss::Clawback, + ttCLAWBACK, + { + {sfAmount, soeREQUIRED}, + {sfTicketSequence, soeOPTIONAL}, + }, + commonFields); } TxFormats const& diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index 92d9096da92..abe9c6b580b 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -53,6 +53,7 @@ JSS(Check); // ledger type. JSS(CheckCancel); // transaction type. JSS(CheckCash); // transaction type. JSS(CheckCreate); // transaction type. +JSS(Clawback); // transaction type. JSS(ClearFlag); // field. JSS(DeliverMin); // in: TransactionSign JSS(DepositPreauth); // transaction and ledger type. diff --git a/src/test/app/Clawback_test.cpp b/src/test/app/Clawback_test.cpp new file mode 100644 index 00000000000..8f72068d87c --- /dev/null +++ b/src/test/app/Clawback_test.cpp @@ -0,0 +1,971 @@ +//------------------------------------------------------------------------------ +/* + 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. +*/ +//============================================================================== + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace ripple { + +class Clawback_test : public beast::unit_test::suite +{ + template + static std::string + to_string(T const& t) + { + return boost::lexical_cast(t); + } + + // Helper function that returns the owner count of an account root. + static std::uint32_t + ownerCount(test::jtx::Env const& env, test::jtx::Account const& acct) + { + std::uint32_t ret{0}; + if (auto const sleAcct = env.le(acct)) + ret = sleAcct->at(sfOwnerCount); + return ret; + } + + // Helper function that returns the number of tickets held by an account. + static std::uint32_t + ticketCount(test::jtx::Env const& env, test::jtx::Account const& acct) + { + std::uint32_t ret{0}; + if (auto const sleAcct = env.le(acct)) + ret = sleAcct->at(~sfTicketCount).value_or(0); + return ret; + } + + // Helper function that returns the freeze status of a trustline + static bool + getLineFreezeFlag( + test::jtx::Env const& env, + test::jtx::Account const& src, + test::jtx::Account const& dst, + Currency const& cur) + { + if (auto sle = env.le(keylet::line(src, dst, cur))) + { + auto const useHigh = src.id() > dst.id(); + return sle->isFlag(useHigh ? lsfHighFreeze : lsfLowFreeze); + } + Throw("No line in getLineFreezeFlag"); + return false; // silence warning + } + + void + testAllowClawbackFlag(FeatureBitset features) + { + testcase("Enable AllowClawback flag"); + using namespace test::jtx; + + // Test that one can successfully set asfAllowClawback flag. + // If successful, asfNoFreeze can no longer be set. + // Also, asfAllowClawback cannot be cleared. + { + Env env(*this, features); + Account alice{"alice"}; + + env.fund(XRP(1000), alice); + env.close(); + + // set asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // clear asfAllowClawback does nothing + env(fclear(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // asfNoFreeze cannot be set when asfAllowClawback is set + env.require(nflags(alice, asfNoFreeze)); + env(fset(alice, asfNoFreeze), ter(tecNO_PERMISSION)); + env.close(); + } + + // Test that asfAllowClawback cannot be set when + // asfNoFreeze has been set + { + Env env(*this, features); + Account alice{"alice"}; + + env.fund(XRP(1000), alice); + env.close(); + + env.require(nflags(alice, asfNoFreeze)); + + // set asfNoFreeze + env(fset(alice, asfNoFreeze)); + env.close(); + + // NoFreeze is set + env.require(flags(alice, asfNoFreeze)); + + // asfAllowClawback cannot be set if asfNoFreeze is set + env(fset(alice, asfAllowClawback), ter(tecNO_PERMISSION)); + env.close(); + + env.require(nflags(alice, asfAllowClawback)); + } + + // Test that asfAllowClawback is not allowed when owner dir is non-empty + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + env.require(nflags(alice, asfAllowClawback)); + + // alice issues 10 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(10))); + env.close(); + + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // alice fails to enable clawback because she has trustline with bob + env(fset(alice, asfAllowClawback), ter(tecOWNERS)); + env.close(); + + // bob sets trustline to default limit and pays alice back to delete + // the trustline + env(trust(bob, USD(0), 0)); + env(pay(bob, alice, USD(10))); + + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // alice now is able to set asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 0); + } + + // Test that one cannot enable asfAllowClawback when + // featureClawback amendment is disabled + { + Env env(*this, features - featureClawback); + + Account alice{"alice"}; + + env.fund(XRP(1000), alice); + env.close(); + + env.require(nflags(alice, asfAllowClawback)); + + // alice attempts to set asfAllowClawback flag while amendment is + // disabled. no error is returned, but the flag remains to be unset. + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(nflags(alice, asfAllowClawback)); + + // now enable clawback amendment + env.enableFeature(featureClawback); + env.close(); + + // asfAllowClawback can be set + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + } + } + + void + testValidation(FeatureBitset features) + { + testcase("Validation"); + using namespace test::jtx; + + // Test that Clawback tx fails for the following: + // 1. when amendment is disabled + // 2. when asfAllowClawback flag has not been set + { + Env env(*this, features - featureClawback); + + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + env.require(nflags(alice, asfAllowClawback)); + + auto const USD = alice["USD"]; + + // alice issues 10 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(10))); + env.close(); + + env.require(balance(bob, alice["USD"](10))); + env.require(balance(alice, bob["USD"](-10))); + + // clawback fails because amendment is disabled + env(claw(alice, bob["USD"](5)), ter(temDISABLED)); + env.close(); + + // now enable clawback amendment + env.enableFeature(featureClawback); + env.close(); + + // clawback fails because asfAllowClawback has not been set + env(claw(alice, bob["USD"](5)), ter(tecNO_PERMISSION)); + env.close(); + + env.require(balance(bob, alice["USD"](10))); + env.require(balance(alice, bob["USD"](-10))); + } + + // Test that Clawback tx fails for the following: + // 1. invalid flag + // 2. negative STAmount + // 3. zero STAmount + // 4. XRP amount + // 5. `account` and `issuer` fields are same account + // 6. trustline has a balance of 0 + // 7. trustline does not exist + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + auto const USD = alice["USD"]; + + // alice issues 10 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(10))); + env.close(); + + env.require(balance(bob, alice["USD"](10))); + env.require(balance(alice, bob["USD"](-10))); + + // fails due to invalid flag + env(claw(alice, bob["USD"](5)), + txflags(0x00008000), + ter(temINVALID_FLAG)); + env.close(); + + // fails due to negative amount + env(claw(alice, bob["USD"](-5)), ter(temBAD_AMOUNT)); + env.close(); + + // fails due to zero amount + env(claw(alice, bob["USD"](0)), ter(temBAD_AMOUNT)); + env.close(); + + // fails because amount is in XRP + env(claw(alice, XRP(10)), ter(temBAD_AMOUNT)); + env.close(); + + // fails when `issuer` field in `amount` is not token holder + // NOTE: we are using the `issuer` field for the token holder + env(claw(alice, alice["USD"](5)), ter(temBAD_AMOUNT)); + env.close(); + + // bob pays alice back, trustline has a balance of 0 + env(pay(bob, alice, USD(10))); + env.close(); + + // bob still owns the trustline that has 0 balance + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 1); + env.require(balance(bob, alice["USD"](0))); + env.require(balance(alice, bob["USD"](0))); + + // clawback fails because because balance is 0 + env(claw(alice, bob["USD"](5)), ter(tecNO_LINE)); + env.close(); + + // set the limit to default, which should delete the trustline + env(trust(bob, USD(0), 0)); + env.close(); + + // bob no longer owns the trustline + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // clawback fails because trustline does not exist + env(claw(alice, bob["USD"](5)), ter(tecNO_LINE)); + env.close(); + } + } + + void + testPermission(FeatureBitset features) + { + // Checks the tx submitter has the permission to clawback. + // Exercises preclaim code + testcase("Permission"); + using namespace test::jtx; + + // Clawing back from an non-existent account returns error + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + + // bob's account is not funded and does not exist + env.fund(XRP(1000), alice); + env.close(); + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // bob, the token holder, does not exist + env(claw(alice, bob["USD"](5)), ter(terNO_ACCOUNT)); + env.close(); + } + + // Test that trustline cannot be clawed by someone who is + // not the issuer of the currency + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + Account cindy{"cindy"}; + + env.fund(XRP(1000), alice, bob, cindy); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // cindy sets asfAllowClawback + env(fset(cindy, asfAllowClawback)); + env.close(); + env.require(flags(cindy, asfAllowClawback)); + + // alice issues 1000 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(1000))); + env.close(); + + env.require(balance(bob, alice["USD"](1000))); + env.require(balance(alice, bob["USD"](-1000))); + + // cindy tries to claw from bob, and fails because trustline does + // not exist + env(claw(cindy, bob["USD"](200)), ter(tecNO_LINE)); + env.close(); + } + + // When a trustline is created between issuer and holder, + // we must make sure the holder is unable to claw back from + // the issuer by impersonating the issuer account. + // + // This must be tested bidirectionally for both accounts because the + // issuer could be either the low or high account in the trustline + // object + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + auto const CAD = bob["CAD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // bob sets asfAllowClawback + env(fset(bob, asfAllowClawback)); + env.close(); + env.require(flags(bob, asfAllowClawback)); + + // alice issues 10 USD to bob. + // bob then attempts to submit a clawback tx to claw USD from alice. + // this must FAIL, because bob is not the issuer for this + // trustline!!! + { + // bob creates a trustline with alice, and alice sends 10 USD to + // bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(10))); + env.close(); + + env.require(balance(bob, alice["USD"](10))); + env.require(balance(alice, bob["USD"](-10))); + + // bob cannot claw back USD from alice because he's not the + // issuer + env(claw(bob, alice["USD"](5)), ter(tecNO_PERMISSION)); + env.close(); + } + + // bob issues 10 CAD to alice. + // alice then attempts to submit a clawback tx to claw CAD from bob. + // this must FAIL, because alice is not the issuer for this + // trustline!!! + { + // alice creates a trustline with bob, and bob sends 10 CAD to + // alice + env.trust(CAD(1000), alice); + env(pay(bob, alice, CAD(10))); + env.close(); + + env.require(balance(bob, alice["CAD"](-10))); + env.require(balance(alice, bob["CAD"](10))); + + // alice cannot claw back CAD from bob because she's not the + // issuer + env(claw(alice, bob["CAD"](5)), ter(tecNO_PERMISSION)); + env.close(); + } + } + } + + void + testEnabled(FeatureBitset features) + { + testcase("Enable clawback"); + using namespace test::jtx; + + // Test that alice is able to successfully clawback tokens from bob + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // alice issues 1000 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(1000))); + env.close(); + + env.require(balance(bob, alice["USD"](1000))); + env.require(balance(alice, bob["USD"](-1000))); + + // alice claws back 200 USD from bob + env(claw(alice, bob["USD"](200))); + env.close(); + + // bob should have 800 USD left + env.require(balance(bob, alice["USD"](800))); + env.require(balance(alice, bob["USD"](-800))); + + // alice claws back 800 USD from bob again + env(claw(alice, bob["USD"](800))); + env.close(); + + // trustline has a balance of 0 + env.require(balance(bob, alice["USD"](0))); + env.require(balance(alice, bob["USD"](0))); + } + + void + testMultiLine(FeatureBitset features) + { + // Test scenarios where multiple trustlines are involved + testcase("Multi line"); + using namespace test::jtx; + + // Both alice and bob issues their own "USD" to cindy. + // When alice and bob tries to claw back, they will only + // claw back from their respective trustline. + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + Account cindy{"cindy"}; + + env.fund(XRP(1000), alice, bob, cindy); + env.close(); + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // bob sets asfAllowClawback + env(fset(bob, asfAllowClawback)); + env.close(); + env.require(flags(bob, asfAllowClawback)); + + // alice sends 1000 USD to cindy + env.trust(alice["USD"](1000), cindy); + env(pay(alice, cindy, alice["USD"](1000))); + env.close(); + + // bob sends 1000 USD to cindy + env.trust(bob["USD"](1000), cindy); + env(pay(bob, cindy, bob["USD"](1000))); + env.close(); + + // alice claws back 200 USD from cindy + env(claw(alice, cindy["USD"](200))); + env.close(); + + // cindy has 800 USD left in alice's trustline after clawed by alice + env.require(balance(cindy, alice["USD"](800))); + env.require(balance(alice, cindy["USD"](-800))); + + // cindy still has 1000 USD in bob's trustline + env.require(balance(cindy, bob["USD"](1000))); + env.require(balance(bob, cindy["USD"](-1000))); + + // bob claws back 600 USD from cindy + env(claw(bob, cindy["USD"](600))); + env.close(); + + // cindy has 400 USD left in bob's trustline after clawed by bob + env.require(balance(cindy, bob["USD"](400))); + env.require(balance(bob, cindy["USD"](-400))); + + // cindy still has 800 USD in alice's trustline + env.require(balance(cindy, alice["USD"](800))); + env.require(balance(alice, cindy["USD"](-800))); + } + + // alice issues USD to both bob and cindy. + // when alice claws back from bob, only bob's USD balance is + // affected, and cindy's balance remains unchanged, and vice versa. + { + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + Account cindy{"cindy"}; + + env.fund(XRP(1000), alice, bob, cindy); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // alice sends 600 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(600))); + env.close(); + + env.require(balance(alice, bob["USD"](-600))); + env.require(balance(bob, alice["USD"](600))); + + // alice sends 1000 USD to cindy + env.trust(USD(1000), cindy); + env(pay(alice, cindy, USD(1000))); + env.close(); + + env.require(balance(alice, cindy["USD"](-1000))); + env.require(balance(cindy, alice["USD"](1000))); + + // alice claws back 500 USD from bob + env(claw(alice, bob["USD"](500))); + env.close(); + + // bob's balance is reduced + env.require(balance(alice, bob["USD"](-100))); + env.require(balance(bob, alice["USD"](100))); + + // cindy's balance is unchanged + env.require(balance(alice, cindy["USD"](-1000))); + env.require(balance(cindy, alice["USD"](1000))); + + // alice claws back 300 USD from cindy + env(claw(alice, cindy["USD"](300))); + env.close(); + + // bob's balance is unchanged + env.require(balance(alice, bob["USD"](-100))); + env.require(balance(bob, alice["USD"](100))); + + // cindy's balance is reduced + env.require(balance(alice, cindy["USD"](-700))); + env.require(balance(cindy, alice["USD"](700))); + } + } + + void + testBidirectionalLine(FeatureBitset features) + { + testcase("Bidirectional line"); + using namespace test::jtx; + + // Test when both alice and bob issues USD to each other. + // This scenario creates only one trustline. + // In this case, both alice and bob can be seen as the "issuer" + // and they can send however many USDs to each other. + // We test that only the person who has a negative balance from their + // perspective is allowed to clawback + Env env(*this, features); + + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // bob sets asfAllowClawback + env(fset(bob, asfAllowClawback)); + env.close(); + env.require(flags(bob, asfAllowClawback)); + + // alice issues 1000 USD to bob + env.trust(alice["USD"](1000), bob); + env(pay(alice, bob, alice["USD"](1000))); + env.close(); + + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // bob is the holder, and alice is the issuer + env.require(balance(bob, alice["USD"](1000))); + env.require(balance(alice, bob["USD"](-1000))); + + // bob issues 1500 USD to alice + env.trust(bob["USD"](1500), alice); + env(pay(bob, alice, bob["USD"](1500))); + env.close(); + + BEAST_EXPECT(ownerCount(env, alice) == 1); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // bob has negative 500 USD because bob issued 500 USD more than alice + // bob can now been seen as the issuer, while alice is the holder + env.require(balance(bob, alice["USD"](-500))); + env.require(balance(alice, bob["USD"](500))); + + // At this point, both alice and bob are the issuers of USD + // and can send USD to each other through one trustline + + // alice fails to clawback. Even though she is also an issuer, + // the trustline balance is positive from her perspective + env(claw(alice, bob["USD"](200)), ter(tecNO_PERMISSION)); + env.close(); + + // bob is able to successfully clawback from alice because + // the trustline balance is negative from his perspective + env(claw(bob, alice["USD"](200))); + env.close(); + + env.require(balance(bob, alice["USD"](-300))); + env.require(balance(alice, bob["USD"](300))); + + // alice pays bob 1000 USD + env(pay(alice, bob, alice["USD"](1000))); + env.close(); + + // bob's balance becomes positive from his perspective because + // alice issued more USD than the balance + env.require(balance(bob, alice["USD"](700))); + env.require(balance(alice, bob["USD"](-700))); + + // bob is now the holder and fails to clawback + env(claw(bob, alice["USD"](200)), ter(tecNO_PERMISSION)); + env.close(); + + // alice successfully claws back + env(claw(alice, bob["USD"](200))); + env.close(); + + env.require(balance(bob, alice["USD"](500))); + env.require(balance(alice, bob["USD"](-500))); + } + + void + testDeleteDefaultLine(FeatureBitset features) + { + testcase("Delete default trustline"); + using namespace test::jtx; + + // If clawback results the trustline to be default, + // trustline should be automatically deleted + Env env(*this, features); + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // alice issues 1000 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(1000))); + env.close(); + + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + env.require(balance(bob, alice["USD"](1000))); + env.require(balance(alice, bob["USD"](-1000))); + + // set limit to default, + env(trust(bob, USD(0), 0)); + env.close(); + + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // alice claws back full amount from bob, and should also delete + // trustline + env(claw(alice, bob["USD"](1000))); + env.close(); + + // bob no longer owns the trustline because it was deleted + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 0); + } + + void + testFrozenLine(FeatureBitset features) + { + testcase("Frozen trustline"); + using namespace test::jtx; + + // Claws back from frozen trustline + // and the trustline should remain frozen + Env env(*this, features); + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // alice issues 1000 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(1000))); + env.close(); + + env.require(balance(bob, alice["USD"](1000))); + env.require(balance(alice, bob["USD"](-1000))); + + // freeze trustline + env(trust(alice, bob["USD"](0), tfSetFreeze)); + env.close(); + + // alice claws back 200 USD from bob + env(claw(alice, bob["USD"](200))); + env.close(); + + // bob should have 800 USD left + env.require(balance(bob, alice["USD"](800))); + env.require(balance(alice, bob["USD"](-800))); + + // trustline remains frozen + BEAST_EXPECT(getLineFreezeFlag(env, alice, bob, USD.currency)); + } + + void + testAmountExceedsAvailable(FeatureBitset features) + { + testcase("Amount exceeds available"); + using namespace test::jtx; + + // When alice tries to claw back an amount that is greater + // than what bob holds, only the max available balance is clawed + Env env(*this, features); + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // alice issues 1000 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(1000))); + env.close(); + + env.require(balance(bob, alice["USD"](1000))); + env.require(balance(alice, bob["USD"](-1000))); + + // alice tries to claw back 2000 USD + env(claw(alice, bob["USD"](2000))); + env.close(); + + // check alice and bob's balance. + // alice was only able to claw back 1000 USD at maximum + env.require(balance(bob, alice["USD"](0))); + env.require(balance(alice, bob["USD"](0))); + + // bob still owns the trustline because trustline is not in default + // state + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // set limit to default, + env(trust(bob, USD(0), 0)); + env.close(); + + // verify that bob's trustline was deleted + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, bob) == 0); + } + + void + testTickets(FeatureBitset features) + { + testcase("Tickets"); + using namespace test::jtx; + + // Tests clawback with tickets + Env env(*this, features); + Account alice{"alice"}; + Account bob{"bob"}; + + env.fund(XRP(1000), alice, bob); + env.close(); + + auto const USD = alice["USD"]; + + // alice sets asfAllowClawback + env(fset(alice, asfAllowClawback)); + env.close(); + env.require(flags(alice, asfAllowClawback)); + + // alice issues 100 USD to bob + env.trust(USD(1000), bob); + env(pay(alice, bob, USD(100))); + env.close(); + + env.require(balance(bob, alice["USD"](100))); + env.require(balance(alice, bob["USD"](-100))); + + // alice creates 10 tickets + std::uint32_t ticketCnt = 10; + std::uint32_t aliceTicketSeq{env.seq(alice) + 1}; + env(ticket::create(alice, ticketCnt)); + env.close(); + std::uint32_t const aliceSeq{env.seq(alice)}; + BEAST_EXPECT(ticketCount(env, alice) == ticketCnt); + BEAST_EXPECT(ownerCount(env, alice) == ticketCnt); + + while (ticketCnt > 0) + { + // alice claws back 5 USD using a ticket + env(claw(alice, bob["USD"](5)), ticket::use(aliceTicketSeq++)); + env.close(); + + ticketCnt--; + BEAST_EXPECT(ticketCount(env, alice) == ticketCnt); + BEAST_EXPECT(ownerCount(env, alice) == ticketCnt); + } + + // alice clawed back 50 USD total, trustline has 50 USD remaining + env.require(balance(bob, alice["USD"](50))); + env.require(balance(alice, bob["USD"](-50))); + + // Verify that the account sequence numbers did not advance. + BEAST_EXPECT(env.seq(alice) == aliceSeq); + } + + void + testWithFeats(FeatureBitset features) + { + testAllowClawbackFlag(features); + testValidation(features); + testPermission(features); + testEnabled(features); + testMultiLine(features); + testBidirectionalLine(features); + testDeleteDefaultLine(features); + testFrozenLine(features); + testAmountExceedsAvailable(features); + testTickets(features); + } + +public: + void + run() override + { + using namespace test::jtx; + FeatureBitset const all{supported_amendments()}; + + testWithFeats(all); + } +}; + +BEAST_DEFINE_TESTSUITE(Clawback, app, ripple); +} // namespace ripple diff --git a/src/test/jtx/flags.h b/src/test/jtx/flags.h index a9ecaf8e2e0..a6f4345cfd2 100644 --- a/src/test/jtx/flags.h +++ b/src/test/jtx/flags.h @@ -80,6 +80,9 @@ class flags_helper case asfDepositAuth: mask_ |= lsfDepositAuth; break; + case asfAllowClawback: + mask_ |= lsfAllowClawback; + break; default: Throw("unknown flag"); } diff --git a/src/test/jtx/impl/trust.cpp b/src/test/jtx/impl/trust.cpp index 4fd0ad5967d..cce4657e025 100644 --- a/src/test/jtx/impl/trust.cpp +++ b/src/test/jtx/impl/trust.cpp @@ -59,6 +59,17 @@ trust( return jv; } +Json::Value +claw(Account const& account, STAmount const& amount) +{ + Json::Value jv; + jv[jss::Account] = account.human(); + jv[jss::Amount] = amount.getJson(JsonOptions::none); + jv[jss::TransactionType] = jss::Clawback; + + return jv; +} + } // namespace jtx } // namespace test } // namespace ripple diff --git a/src/test/jtx/trust.h b/src/test/jtx/trust.h index ba0bc995914..5b6dd78b3cd 100644 --- a/src/test/jtx/trust.h +++ b/src/test/jtx/trust.h @@ -40,6 +40,9 @@ trust( Account const& peer, std::uint32_t flags); +Json::Value +claw(Account const& account, STAmount const& amount); + } // namespace jtx } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountSet_test.cpp b/src/test/rpc/AccountSet_test.cpp index f935e0a846f..fc2b4ffe116 100644 --- a/src/test/rpc/AccountSet_test.cpp +++ b/src/test/rpc/AccountSet_test.cpp @@ -93,6 +93,12 @@ class AccountSet_test : public beast::unit_test::suite // and are tested elsewhere continue; } + if (flag == asfAllowClawback) + { + // The asfAllowClawback flag can't be cleared. It is tested + // elsewhere. + continue; + } if (std::find(goodFlags.begin(), goodFlags.end(), flag) != goodFlags.end()) From 01f7c6928e63521b307b7458679e640a5de9b8ad Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 1 Jun 2023 10:19:03 -0400 Subject: [PATCH 02/17] remove redundant check and comment --- src/ripple/app/tx/impl/Clawback.cpp | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 524c513269b..3eff5ad4ec7 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -117,19 +117,6 @@ Clawback::clawback( if (amount <= beast::zero || issuer != amount.getIssuer()) return tecINTERNAL; - STAmount const holderBalance = accountHolds( - view(), - holder, - amount.getCurrency(), - amount.getIssuer(), - fhIGNORE_FREEZE, - j_); - - // The amount to be clawed back can never exceed the amount that - // the token holder current owns - if (amount > holderBalance) - return tecINTERNAL; - auto const result = accountSend(view(), holder, issuer, amount, j_); if (!isTesSuccess(result)) @@ -155,7 +142,7 @@ Clawback::doApply() STAmount clawAmount(ctx_.tx.getFieldAmount(sfAmount)); AccountID const holder = clawAmount.getIssuer(); - // Replace the `issuer` field with holder's account + // Replace the `issuer` field with issuer's account clawAmount.setIssuer(issuer); // Get the spendable balance. Must use `accountHolds`. From e17e3cf6881a35b16a9ed20c553e513eca16273e Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 1 Jun 2023 10:53:03 -0400 Subject: [PATCH 03/17] change error code to insufficient fund --- src/ripple/app/tx/impl/Clawback.cpp | 5 ++--- src/test/app/Clawback_test.cpp | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 3eff5ad4ec7..50284434994 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -21,11 +21,10 @@ #include #include #include +#include #include #include #include -#include -#include namespace ripple { @@ -102,7 +101,7 @@ Clawback::preclaim(PreclaimContext const& ctx) issuer, fhIGNORE_FREEZE, ctx.j) <= beast::zero) - return tecNO_LINE; + return tecINSUFFICIENT_FUNDS; return tesSUCCESS; } diff --git a/src/test/app/Clawback_test.cpp b/src/test/app/Clawback_test.cpp index 8f72068d87c..33b9049790e 100644 --- a/src/test/app/Clawback_test.cpp +++ b/src/test/app/Clawback_test.cpp @@ -314,7 +314,7 @@ class Clawback_test : public beast::unit_test::suite env.require(balance(alice, bob["USD"](0))); // clawback fails because because balance is 0 - env(claw(alice, bob["USD"](5)), ter(tecNO_LINE)); + env(claw(alice, bob["USD"](5)), ter(tecINSUFFICIENT_FUNDS)); env.close(); // set the limit to default, which should delete the trustline From 5309c6be0c5d3318be0f93985b6f881e2754d36d Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 1 Jun 2023 14:58:57 -0400 Subject: [PATCH 04/17] add invariant check --- src/ripple/app/tx/impl/Clawback.cpp | 17 +------ src/ripple/app/tx/impl/InvariantCheck.cpp | 62 +++++++++++++++++++++++ src/ripple/app/tx/impl/InvariantCheck.h | 29 ++++++++++- 3 files changed, 91 insertions(+), 17 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 50284434994..6ed9e4f49d3 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -116,22 +116,7 @@ Clawback::clawback( if (amount <= beast::zero || issuer != amount.getIssuer()) return tecINTERNAL; - auto const result = accountSend(view(), holder, issuer, amount, j_); - - if (!isTesSuccess(result)) - return result; - - if (accountHolds( - view(), - holder, - amount.getCurrency(), - amount.getIssuer(), - fhIGNORE_FREEZE, - j_) - .signum() < 0) - return tecINTERNAL; - - return tesSUCCESS; + return accountSend(view(), holder, issuer, amount, j_); } TER diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 8664c6492b9..9fb34fb952a 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -27,6 +27,7 @@ #include #include #include +#include namespace ripple { @@ -717,4 +718,65 @@ NFTokenCountTracking::finalize( return true; } +//------------------------------------------------------------------------------ + +void +ValidClawback::visitEntry( + bool, + std::shared_ptr const& before, + std::shared_ptr const& after) +{ + if (before && before->getType() == ltRIPPLE_STATE) + trustlineChanged++; +} + +bool +ValidClawback::finalize( + STTx const& tx, + TER const result, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) +{ + if (tx.getTxnType() != ttCLAWBACK) + return true; + + if (result == tesSUCCESS) + { + // a successful clawback transaction will ALWAYS claw back some funds + // from ONE trustline + if (trustlineChanged != 1){ + JLOG(j.fatal()) + << "Invariant failed: wrong number of trustline changed."; + return false; + } + + AccountID const issuer = tx.getAccountID(sfAccount); + STAmount const amount = tx.getFieldAmount(sfAmount); + AccountID const holder = amount.getIssuer(); + STAmount const holderBalance = accountHolds( + view, + holder, + amount.getCurrency(), + issuer, + fhIGNORE_FREEZE, + j); + + if (holderBalance.signum() < 0){ + JLOG(j.fatal()) + << "Invariant failed: trustline balance is negative"; + return false; + } + } + else{ + if (trustlineChanged != 0){ + JLOG(j.fatal()) + << "Invariant failed: trustline changed."; + return false; + } + } + + return true; +} + } // namespace ripple diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index c3bb0216426..2a70c5221f6 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -365,6 +365,32 @@ class NFTokenCountTracking beast::Journal const&); }; +/** + * @brief Invariant: Trust line balance cannot be negative after Clawback. + * + * We iterate all the trust lines affected by this transaction and ensure + * that they have non-negative balance. + */ +class ValidClawback +{ + std::uint32_t trustlineChanged = 0; + +public: + void + visitEntry( + bool, + std::shared_ptr const&, + std::shared_ptr const&); + + bool + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); +}; + // additional invariant checks can be declared above and then added to this // tuple using InvariantChecks = std::tuple< @@ -378,7 +404,8 @@ using InvariantChecks = std::tuple< NoZeroEscrow, ValidNewAccountRoot, ValidNFTokenPage, - NFTokenCountTracking>; + NFTokenCountTracking, + ValidClawback>; /** * @brief get a tuple of all invariant checks From f4d1d902e1651896798930ecbcf8878ee2f85b92 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 1 Jun 2023 15:01:14 -0400 Subject: [PATCH 05/17] clang --- src/ripple/app/tx/impl/InvariantCheck.cpp | 32 +++++++++++------------ 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 9fb34fb952a..1ccabd50c26 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -23,11 +23,11 @@ #include #include #include +#include #include #include #include #include -#include namespace ripple { @@ -745,34 +745,32 @@ ValidClawback::finalize( { // a successful clawback transaction will ALWAYS claw back some funds // from ONE trustline - if (trustlineChanged != 1){ + if (trustlineChanged != 1) + { JLOG(j.fatal()) << "Invariant failed: wrong number of trustline changed."; - return false; + return false; } AccountID const issuer = tx.getAccountID(sfAccount); STAmount const amount = tx.getFieldAmount(sfAmount); AccountID const holder = amount.getIssuer(); STAmount const holderBalance = accountHolds( - view, - holder, - amount.getCurrency(), - issuer, - fhIGNORE_FREEZE, - j); - - if (holderBalance.signum() < 0){ + view, holder, amount.getCurrency(), issuer, fhIGNORE_FREEZE, j); + + if (holderBalance.signum() < 0) + { JLOG(j.fatal()) << "Invariant failed: trustline balance is negative"; - return false; + return false; } } - else{ - if (trustlineChanged != 0){ - JLOG(j.fatal()) - << "Invariant failed: trustline changed."; - return false; + else + { + if (trustlineChanged != 0) + { + JLOG(j.fatal()) << "Invariant failed: trustline changed."; + return false; } } From 6981d5d8b0a97643114560756a174adb9f0dd56d Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 1 Jun 2023 15:10:17 -0400 Subject: [PATCH 06/17] plural variable --- src/ripple/app/tx/impl/InvariantCheck.cpp | 6 +++--- src/ripple/app/tx/impl/InvariantCheck.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 1ccabd50c26..ecc143cbacc 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -727,7 +727,7 @@ ValidClawback::visitEntry( std::shared_ptr const& after) { if (before && before->getType() == ltRIPPLE_STATE) - trustlineChanged++; + trustlinesChanged++; } bool @@ -745,7 +745,7 @@ ValidClawback::finalize( { // a successful clawback transaction will ALWAYS claw back some funds // from ONE trustline - if (trustlineChanged != 1) + if (trustlinesChanged != 1) { JLOG(j.fatal()) << "Invariant failed: wrong number of trustline changed."; @@ -767,7 +767,7 @@ ValidClawback::finalize( } else { - if (trustlineChanged != 0) + if (trustlinesChanged != 0) { JLOG(j.fatal()) << "Invariant failed: trustline changed."; return false; diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index 2a70c5221f6..2c73b0d36d8 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -373,7 +373,7 @@ class NFTokenCountTracking */ class ValidClawback { - std::uint32_t trustlineChanged = 0; + std::uint32_t trustlinesChanged = 0; public: void From 113ca51bc1a713d9fd53fc4a29a1d629b2b34ce2 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 2 Jun 2023 10:13:01 -0400 Subject: [PATCH 07/17] amm maybe used --- src/ripple/protocol/LedgerFormats.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ripple/protocol/LedgerFormats.h b/src/ripple/protocol/LedgerFormats.h index e0b1c122a47..c72b769ef2a 100644 --- a/src/ripple/protocol/LedgerFormats.h +++ b/src/ripple/protocol/LedgerFormats.h @@ -243,9 +243,7 @@ enum LedgerSpecificFlags { 0x10000000, // True, reject new paychans lsfDisallowIncomingTrustline = 0x20000000, // True, reject new trustlines (only if no issued assets) -/* // reserved for AMM amendment - lsfAMM = 0x40000000, // True, AMM account -*/ + lsfAMM [[maybe_unused]] = 0x40000000, // True, AMM account lsfAllowClawback = 0x80000000, // True, enable clawback From 43fb0aa6c19a7afdff73f5624063e5217b5f5c42 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 2 Jun 2023 10:35:07 -0400 Subject: [PATCH 08/17] freeze logic to preclaim --- src/ripple/app/tx/impl/SetAccount.cpp | 37 +++++++++++++++------------ 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index 80256e7ab28..e5930fc8725 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -221,19 +221,30 @@ SetAccount::preclaim(PreclaimContext const& ctx) // // Clawback // - if (ctx.view.rules().enabled(featureClawback) && - (uSetFlag == asfAllowClawback)) + if (ctx.view.rules().enabled(featureClawback)) { - if (uFlagsIn & lsfNoFreeze) + if (uSetFlag == asfAllowClawback) { - JLOG(ctx.j.trace()) << "Can't set Clawback if NoFreeze is set"; - return tecNO_PERMISSION; + if (uFlagsIn & lsfNoFreeze) + { + JLOG(ctx.j.trace()) << "Can't set Clawback if NoFreeze is set"; + return tecNO_PERMISSION; + } + + if (!dirIsEmpty(ctx.view, keylet::ownerDir(id))) + { + JLOG(ctx.j.trace()) << "Owner directory not empty."; + return tecOWNERS; + } } - - if (!dirIsEmpty(ctx.view, keylet::ownerDir(id))) + else if (uSetFlag == asfNoFreeze) { - JLOG(ctx.j.trace()) << "Owner directory not empty."; - return tecOWNERS; + // Cannot set NoFreeze if clawback is enabled + if (uFlagsIn & lsfAllowClawback) + { + JLOG(ctx.j.trace()) << "Can't set NoFreeze if clawback is enabled"; + return tecNO_PERMISSION; + } } } @@ -380,14 +391,6 @@ SetAccount::doApply() return tecNEED_MASTER_KEY; } - // Cannot set NoFreeze if clawback is enabled - if (ctx_.view().rules().enabled(featureClawback) && - (uFlagsIn & lsfAllowClawback)) - { - JLOG(j_.trace()) << "Can't set NoFreeze if clawback is enabled"; - return tecNO_PERMISSION; - } - JLOG(j_.trace()) << "Set NoFreeze flag"; uFlagsOut |= lsfNoFreeze; } From 2752b9ba54c3b250cfbf62d6a9be1dfd731c5113 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 2 Jun 2023 10:37:38 -0400 Subject: [PATCH 09/17] clang --- src/ripple/app/tx/impl/SetAccount.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index e5930fc8725..123a5bdc91e 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -235,14 +235,15 @@ SetAccount::preclaim(PreclaimContext const& ctx) { JLOG(ctx.j.trace()) << "Owner directory not empty."; return tecOWNERS; - } + } } else if (uSetFlag == asfNoFreeze) { // Cannot set NoFreeze if clawback is enabled if (uFlagsIn & lsfAllowClawback) { - JLOG(ctx.j.trace()) << "Can't set NoFreeze if clawback is enabled"; + JLOG(ctx.j.trace()) + << "Can't set NoFreeze if clawback is enabled"; return tecNO_PERMISSION; } } From d5e1e92c91174f2d848fd2424d2f8ccef6cc4b79 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 2 Jun 2023 10:42:38 -0400 Subject: [PATCH 10/17] unused var --- src/ripple/app/tx/impl/InvariantCheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index ecc143cbacc..70f099660b9 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -724,7 +724,7 @@ void ValidClawback::visitEntry( bool, std::shared_ptr const& before, - std::shared_ptr const& after) + std::shared_ptr const&) { if (before && before->getType() == ltRIPPLE_STATE) trustlinesChanged++; From 3f32409795826f99857d5b8324e4ef931663d089 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 2 Jun 2023 11:27:02 -0400 Subject: [PATCH 11/17] make clawback inline --- src/ripple/app/tx/impl/Clawback.cpp | 15 +-------------- src/ripple/app/tx/impl/Clawback.h | 7 ------- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 6ed9e4f49d3..e1d74476594 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -106,19 +106,6 @@ Clawback::preclaim(PreclaimContext const& ctx) return tesSUCCESS; } -TER -Clawback::clawback( - AccountID const& issuer, - AccountID const& holder, - STAmount const& amount) -{ - // This should never happen - if (amount <= beast::zero || issuer != amount.getIssuer()) - return tecINTERNAL; - - return accountSend(view(), holder, issuer, amount, j_); -} - TER Clawback::doApply() { @@ -138,7 +125,7 @@ Clawback::doApply() fhIGNORE_FREEZE, j_); - return clawback(issuer, holder, std::min(spendableAmount, clawAmount)); + return accountSend(view(), holder, issuer, std::min(spendableAmount, clawAmount), j_); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/Clawback.h b/src/ripple/app/tx/impl/Clawback.h index 5128ee39e94..c5f072c8463 100644 --- a/src/ripple/app/tx/impl/Clawback.h +++ b/src/ripple/app/tx/impl/Clawback.h @@ -26,13 +26,6 @@ namespace ripple { class Clawback : public Transactor { -private: - TER - clawback( - AccountID const& issuer, - AccountID const& holder, - STAmount const& amount); - public: static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; From 6ea5b73201aae532fb45f701b331d8810171bdd0 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Fri, 2 Jun 2023 11:27:24 -0400 Subject: [PATCH 12/17] clang --- src/ripple/app/tx/impl/Clawback.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index e1d74476594..080a1dc094e 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -125,7 +125,8 @@ Clawback::doApply() fhIGNORE_FREEZE, j_); - return accountSend(view(), holder, issuer, std::min(spendableAmount, clawAmount), j_); + return accountSend( + view(), holder, issuer, std::min(spendableAmount, clawAmount), j_); } } // namespace ripple From 4517eaa12ba11316b15668a6b7fded198c2663e9 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 6 Jun 2023 21:23:09 -0400 Subject: [PATCH 13/17] modify invaraint --- src/ripple/app/tx/impl/InvariantCheck.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 70f099660b9..0be3454d21f 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -743,12 +743,10 @@ ValidClawback::finalize( if (result == tesSUCCESS) { - // a successful clawback transaction will ALWAYS claw back some funds - // from ONE trustline - if (trustlinesChanged != 1) + if (trustlinesChanged > 1) { JLOG(j.fatal()) - << "Invariant failed: wrong number of trustline changed."; + << "Invariant failed: more than one trustline changed."; return false; } From 2f536f3af18f212f684ad91c541722e6de7b0c39 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Mon, 12 Jun 2023 12:33:21 -0400 Subject: [PATCH 14/17] address comments --- src/ripple/app/tx/impl/Clawback.cpp | 1 - src/ripple/app/tx/impl/InvariantCheck.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index 080a1dc094e..e0b8fa89638 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -18,7 +18,6 @@ //============================================================================== #include -#include #include #include #include diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 0be3454d21f..704bddc721b 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -767,7 +767,7 @@ ValidClawback::finalize( { if (trustlinesChanged != 0) { - JLOG(j.fatal()) << "Invariant failed: trustline changed."; + JLOG(j.fatal()) << "Invariant failed: some trustlines were changed despite failure of the transaction."; return false; } } From 5f54117c19be3eabf3a4b5b070628edcc319dcd0 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Mon, 12 Jun 2023 12:33:59 -0400 Subject: [PATCH 15/17] clang --- src/ripple/app/tx/impl/InvariantCheck.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 704bddc721b..6d49b5b7f6b 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -767,7 +767,8 @@ ValidClawback::finalize( { if (trustlinesChanged != 0) { - JLOG(j.fatal()) << "Invariant failed: some trustlines were changed despite failure of the transaction."; + JLOG(j.fatal()) << "Invariant failed: some trustlines were changed " + "despite failure of the transaction."; return false; } } From 6d5dd3edbe17080276dbff1ba26a8e001d91bf2c Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 15 Jun 2023 12:22:05 -0400 Subject: [PATCH 16/17] address comments --- src/ripple/app/tx/impl/Clawback.cpp | 31 ++++++++++++++--------- src/ripple/app/tx/impl/InvariantCheck.cpp | 2 +- src/ripple/app/tx/impl/InvariantCheck.h | 4 +-- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index e0b8fa89638..f3982cc70a2 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -39,11 +39,11 @@ Clawback::preflight(PreflightContext const& ctx) if (ctx.tx.getFlags() & tfClawbackMask) return temINVALID_FLAG; - AccountID const issuer = ctx.tx.getAccountID(sfAccount); - STAmount const clawAmount(ctx.tx.getFieldAmount(sfAmount)); + AccountID const issuer = ctx.tx[sfAccount]; + STAmount const clawAmount = ctx.tx[sfAmount]; // The issuer field is used for the token holder instead - AccountID const holder = clawAmount.getIssuer(); + AccountID const& holder = clawAmount.getIssuer(); if (issuer == holder || isXRP(clawAmount) || clawAmount <= beast::zero) return temBAD_AMOUNT; @@ -54,9 +54,9 @@ Clawback::preflight(PreflightContext const& ctx) TER Clawback::preclaim(PreclaimContext const& ctx) { - AccountID const issuer = ctx.tx.getAccountID(sfAccount); - STAmount const clawAmount(ctx.tx.getFieldAmount(sfAmount)); - AccountID const holder = clawAmount.getIssuer(); + AccountID const issuer = ctx.tx[sfAccount]; + STAmount const clawAmount = ctx.tx[sfAmount]; + AccountID const& holder = clawAmount.getIssuer(); auto const sleIssuer = ctx.view.read(keylet::account(issuer)); auto const sleHolder = ctx.view.read(keylet::account(holder)); @@ -74,7 +74,7 @@ Clawback::preclaim(PreclaimContext const& ctx) if (!sleRippleState) return tecNO_LINE; - STAmount const balance = sleRippleState->getFieldAmount(sfBalance); + STAmount const balance = (*sleRippleState)[sfBalance]; // If balance is positive, issuer must have higher address than holder if (balance > beast::zero && issuer < holder) @@ -108,12 +108,14 @@ Clawback::preclaim(PreclaimContext const& ctx) TER Clawback::doApply() { - AccountID const issuer = account_; - STAmount clawAmount(ctx_.tx.getFieldAmount(sfAmount)); - AccountID const holder = clawAmount.getIssuer(); + AccountID const& issuer = account_; + STAmount clawAmount = ctx_.tx[sfAmount]; + AccountID const holder = clawAmount.getIssuer(); // cannot be reference // Replace the `issuer` field with issuer's account clawAmount.setIssuer(issuer); + if (holder == issuer) + return tecINTERNAL; // Get the spendable balance. Must use `accountHolds`. STAmount const spendableAmount = accountHolds( @@ -124,8 +126,13 @@ Clawback::doApply() fhIGNORE_FREEZE, j_); - return accountSend( - view(), holder, issuer, std::min(spendableAmount, clawAmount), j_); + return rippleCredit( + view(), + holder, + issuer, + std::min(spendableAmount, clawAmount), + true, + j_); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 6d49b5b7f6b..a2e452d4c44 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -752,7 +752,7 @@ ValidClawback::finalize( AccountID const issuer = tx.getAccountID(sfAccount); STAmount const amount = tx.getFieldAmount(sfAmount); - AccountID const holder = amount.getIssuer(); + AccountID const& holder = amount.getIssuer(); STAmount const holderBalance = accountHolds( view, holder, amount.getCurrency(), issuer, fhIGNORE_FREEZE, j); diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index 2c73b0d36d8..e6dd157ae05 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -366,10 +366,10 @@ class NFTokenCountTracking }; /** - * @brief Invariant: Trust line balance cannot be negative after Clawback. + * @brief Invariant: Token holder's trustline balance cannot be negative after Clawback. * * We iterate all the trust lines affected by this transaction and ensure - * that they have non-negative balance. + * that no more than one trustline is modified, and also holder's balance is non-negative. */ class ValidClawback { From 0c0c57a33b839e17727a5bb1a50c63a9cea0d204 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 15 Jun 2023 12:22:46 -0400 Subject: [PATCH 17/17] clang --- src/ripple/app/tx/impl/Clawback.cpp | 2 +- src/ripple/app/tx/impl/InvariantCheck.h | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ripple/app/tx/impl/Clawback.cpp b/src/ripple/app/tx/impl/Clawback.cpp index f3982cc70a2..d4a16e7adbc 100644 --- a/src/ripple/app/tx/impl/Clawback.cpp +++ b/src/ripple/app/tx/impl/Clawback.cpp @@ -110,7 +110,7 @@ Clawback::doApply() { AccountID const& issuer = account_; STAmount clawAmount = ctx_.tx[sfAmount]; - AccountID const holder = clawAmount.getIssuer(); // cannot be reference + AccountID const holder = clawAmount.getIssuer(); // cannot be reference // Replace the `issuer` field with issuer's account clawAmount.setIssuer(issuer); diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index e6dd157ae05..cc689574279 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -366,10 +366,12 @@ class NFTokenCountTracking }; /** - * @brief Invariant: Token holder's trustline balance cannot be negative after Clawback. + * @brief Invariant: Token holder's trustline balance cannot be negative after + * Clawback. * * We iterate all the trust lines affected by this transaction and ensure - * that no more than one trustline is modified, and also holder's balance is non-negative. + * that no more than one trustline is modified, and also holder's balance is + * non-negative. */ class ValidClawback {