From 6adb2eca7652cdb35e805d60cab912780ba02047 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 23 Oct 2025 17:58:23 -0400 Subject: [PATCH] Review feedbak from @tequdev, plus helpers - Fix LoanSet.calculateBaseFee with multisign. - Cleanups. - Add View helper functions for Loan and Vault transactions - preflightDestinationAndTag - checkDestinationAndTag - canSendToAccount - Used the helpers in appropriate Loan and Vault transactions. - They could also be used in older transactions, I'll save that for later. --- include/xrpl/ledger/View.h | 47 ++++++++++++- src/libxrpl/ledger/View.cpp | 67 +++++++++++++++++++ .../app/tx/detail/LoanBrokerCoverWithdraw.cpp | 50 +++----------- src/xrpld/app/tx/detail/LoanSet.cpp | 6 +- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 33 +++------ 5 files changed, 136 insertions(+), 67 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index 4343c29b59..c28afd5fdd 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -694,7 +694,7 @@ isPseudoAccount(std::shared_ptr sleAcct); getPseudoAccountFields(); [[nodiscard]] inline bool -isPseudoAccount(ReadView const& view, AccountID accountId) +isPseudoAccount(ReadView const& view, AccountID const& accountId) { return isPseudoAccount(view.read(keylet::account(accountId))); } @@ -702,6 +702,51 @@ isPseudoAccount(ReadView const& view, AccountID accountId) [[nodiscard]] TER canAddHolding(ReadView const& view, Asset const& asset); +/** Checks that the destination and destination tag are valid. + + - If destination is set, it must not be zero. + - If not set, there must not be a destination tag. +*/ +[[nodiscard]] NotTEC +preflightDestinationAndTag( + std::optional const& destination, + bool hasDestinationTag); + +/** Validates that the destination SLE and tag are valid + + - Checks that the SLE is not null. + - If the SLE requires a destination tag, checks that there is a tag. +*/ +[[nodiscard]] TER +checkDestinationAndTag(SLE::const_ref toSle, bool hasDestinationTag); + +/** Checks that from can sent to to (with an SLE) + + - Does the checkDestinationAndTag checks, plus: + - if the destination requires deposit auth, checks that the from account has + that auth. +*/ +[[nodiscard]] TER +canSendToAccount( + AccountID const& from, + ReadView const& view, + SLE::const_ref toSle, + bool hasDestinationTag); + +/** Checks that from can sent to to (with an AccountID) + + - Loads the SLE for "to" + - Checks whether the account is trying to send to itself. + Succeeds if the SLE exists, fails if not. + - Then does the checkDestinationAndTag check with an SLE. +*/ +[[nodiscard]] TER +canSendToAccount( + AccountID const& from, + ReadView const& view, + AccountID const& to, + bool hasDestinationTag); + /// Any transactors that call addEmptyHolding() in doApply must call /// canAddHolding() in preflight with the same View and Asset [[nodiscard]] TER diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index eeb704bc3b..c2d5682765 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -1340,6 +1340,73 @@ canAddHolding(ReadView const& view, Asset const& asset) asset.value()); } +[[nodiscard]] NotTEC +preflightDestinationAndTag( + std::optional const& destination, + bool hasDestinationTag) +{ + if (destination) + { + if (*destination == beast::zero) + { + return temMALFORMED; + } + } + else if (hasDestinationTag) + { + return temMALFORMED; + } + + return tesSUCCESS; +} + +[[nodiscard]] TER +checkDestinationAndTag(SLE::const_ref toSle, bool hasDestinationTag) +{ + if (toSle == nullptr) + return tecNO_DST; + + // The tag is basically account-specific information we don't + // understand, but we can require someone to fill it in. + if (toSle->isFlag(lsfRequireDestTag) && !hasDestinationTag) + return tecDST_TAG_NEEDED; // Cannot send without a tag + + return tesSUCCESS; +} + +[[nodiscard]] TER +canSendToAccount( + AccountID const& from, + ReadView const& view, + SLE::const_ref toSle, + bool hasDestinationTag) +{ + if (auto const ret = checkDestinationAndTag(toSle, hasDestinationTag)) + return ret; + + if (toSle->isFlag(lsfDepositAuth)) + { + if (!view.exists(keylet::depositPreauth(toSle->at(sfAccount), from))) + return tecNO_PERMISSION; + } + + return tesSUCCESS; +} + +[[nodiscard]] TER +canSendToAccount( + AccountID const& from, + ReadView const& view, + AccountID const& to, + bool hasDestinationTag) +{ + auto const toSle = view.read(keylet::account(to)); + if (from == to) + return toSle ? (TER)tesSUCCESS : (TER)tecINTERNAL; + + return canSendToAccount(from, view, toSle, hasDestinationTag); +} + [[nodiscard]] TER addEmptyHolding( ApplyView& view, diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index 50793adb1e..b0f1b98154 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp @@ -43,23 +43,9 @@ LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx) if (!isLegalNet(dstAmount)) return temBAD_AMOUNT; - if (auto const destination = ctx.tx[~sfDestination]; - destination.has_value()) - { - if (*destination == beast::zero) - { - JLOG(ctx.j.debug()) - << "LoanBrokerCoverWithdraw: zero/empty destination account."; - return temMALFORMED; - } - } - else if (ctx.tx.isFieldPresent(sfDestinationTag)) - { - JLOG(ctx.j.debug()) - << "LoanBrokerCoverWithdraw: sfDestinationTag is set but " - "sfDestination is not"; - return temMALFORMED; - } + if (auto const ret = preflightDestinationAndTag( + ctx.tx[~sfDestination], ctx.tx.isFieldPresent(sfDestinationTag))) + return ret; return tesSUCCESS; } @@ -73,11 +59,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) auto const brokerID = tx[sfLoanBrokerID]; auto const amount = tx[sfAmount]; - auto const dstAcct = [&]() -> AccountID { - if (auto const dst = tx[~sfDestination]) - return *dst; - return account; - }(); + auto const dstAcct = tx[~sfDestination].value_or(account); auto const sleBroker = ctx.view.read(keylet::loanbroker(brokerID)); if (!sleBroker) @@ -103,19 +85,13 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) AuthType authType = AuthType::Legacy; if (account != dstAcct) { - auto const sleDst = ctx.view.read(keylet::account(dstAcct)); - if (sleDst == nullptr) - return tecNO_DST; + if (auto const ret = canSendToAccount( + account, + ctx.view, + dstAcct, + tx.isFieldPresent(sfDestinationTag))) + return ret; - if (sleDst->isFlag(lsfRequireDestTag) && - !tx.isFieldPresent(sfDestinationTag)) - return tecDST_TAG_NEEDED; // Cannot send without a tag - - if (sleDst->isFlag(lsfDepositAuth)) - { - if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account))) - return tecNO_PERMISSION; - } // The destination account must have consented to receive the asset by // creating a RippleState or MPToken authType = AuthType::StrongAuth; @@ -171,11 +147,7 @@ LoanBrokerCoverWithdraw::doApply() auto const brokerID = tx[sfLoanBrokerID]; auto const amount = tx[sfAmount]; - auto const dstAcct = [&]() -> AccountID { - if (auto const dst = tx[~sfDestination]) - return *dst; - return account_; - }(); + auto const dstAcct = tx[~sfDestination].value_or(account_); auto broker = view().peek(keylet::loanbroker(brokerID)); if (!broker) diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index af18e12d27..61eff570af 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -172,8 +172,8 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx) // for the transaction. Note that unlike the base class, the single signer // is counted if present. It will only be absent in a batch inner // transaction. - std::size_t const signerCount = tx.isFieldPresent(sfSigners) - ? tx.getFieldArray(sfSigners).size() + std::size_t const signerCount = counterSig.isFieldPresent(sfSigners) + ? counterSig.getFieldArray(sfSigners).size() : (counterSig.isFieldPresent(sfTxnSignature) ? 1 : 0); return normalCost + (signerCount * baseFee); @@ -513,7 +513,7 @@ LoanSet::doApply() brokerOwnerSle->at(sfBalance).value().xrp(), vaultAsset, j_); - !isTesSuccess(ter) && ter != tecDUPLICATE) + ter && ter != tecDUPLICATE) // ignore tecDUPLICATE. That means the holding already exists, // and is fine here return ter; diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index bdf00f5c56..37c0f63e51 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -42,16 +42,9 @@ VaultWithdraw::preflight(PreflightContext const& ctx) if (ctx.tx[sfAmount] <= beast::zero) return temBAD_AMOUNT; - if (auto const destination = ctx.tx[~sfDestination]; - destination.has_value()) - { - if (*destination == beast::zero) - { - JLOG(ctx.j.debug()) - << "VaultWithdraw: zero/empty destination account."; - return temMALFORMED; - } - } + if (auto const ret = preflightDestinationAndTag( + ctx.tx[~sfDestination], ctx.tx.isFieldPresent(sfDestinationTag))) + return ret; return tesSUCCESS; } @@ -111,21 +104,13 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) auto const account = ctx.tx[sfAccount]; auto const dstAcct = ctx.tx[~sfDestination].value_or(account); - auto const sleDst = ctx.view.read(keylet::account(dstAcct)); - if (sleDst == nullptr) - return account == dstAcct ? tecINTERNAL : tecNO_DST; - if (sleDst->isFlag(lsfRequireDestTag) && - !ctx.tx.isFieldPresent(sfDestinationTag)) - return tecDST_TAG_NEEDED; // Cannot send without a tag - - // Withdrawal to a 3rd party destination account is essentially a transfer, - // via shares in the vault. Enforce all the usual asset transfer checks. - if (account != dstAcct && sleDst->isFlag(lsfDepositAuth)) - { - if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account))) - return tecNO_PERMISSION; - } + if (auto const ret = canSendToAccount( + account, + ctx.view, + dstAcct, + ctx.tx.isFieldPresent(sfDestinationTag))) + return ret; // If sending to Account (i.e. not a transfer), we will also create (only // if authorized) a trust line or MPToken as needed, in doApply().