From 78ef800e30f603d78507e6df8a7ca0b273a894e5 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 24 Oct 2025 00:03:32 -0400 Subject: [PATCH] Fix more problems introduced by 6adb2ec - Renamed canSendToAccount to canWithdraw, because the semantics are a little different from a payment. Notably, if withdrawing to self, you can still include a destination tag. - Simplified the interface to canWithdraw to just pass in the STTx. - preflightDestinationAndTag is pretty pointless now, so removed it. --- include/xrpl/ledger/View.h | 66 ++++++++++++------- src/libxrpl/ledger/View.cpp | 35 ++++------ src/test/app/LoanBroker_test.cpp | 7 -- .../app/tx/detail/LoanBrokerCoverWithdraw.cpp | 16 ++--- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 16 ++--- 5 files changed, 69 insertions(+), 71 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index 47dc8743bc..8de5a8decb 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -702,16 +702,6 @@ isPseudoAccount(ReadView const& view, AccountID const& 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. @@ -720,34 +710,60 @@ preflightDestinationAndTag( [[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. -*/ +/** Checks that can withdraw funds from an object to itself or a destination. + * + * The receiver may be either the submitting account (sfAccount) or a different + * destination account (sfDestination). + * + * - Checks that the receiver account exists. + * - If the receiver requires a destination tag, check that one exists, even + * if withdrawing to self. + * - If withdrawing to self, succeed. + * - If not, checks if the receiver requires deposit authorization, and if + * the sender has it. + */ [[nodiscard]] TER -canSendToAccount( +canWithdraw( AccountID const& from, ReadView const& view, AccountID const& to, 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. -*/ +/** Checks that can withdraw funds from an object to itself or a destination. + * + * The receiver may be either the submitting account (sfAccount) or a different + * destination account (sfDestination). + * + * - Checks that the receiver account exists. + * - If the receiver requires a destination tag, check that one exists, even + * if withdrawing to self. + * - If withdrawing to self, succeed. + * - If not, checks if the receiver requires deposit authorization, and if + * the sender has it. + */ [[nodiscard]] TER -canSendToAccount( +canWithdraw( AccountID const& from, ReadView const& view, AccountID const& to, bool hasDestinationTag); +/** Checks that can withdraw funds from an object to itself or a destination. + * + * The receiver may be either the submitting account (sfAccount) or a different + * destination account (sfDestination). + * + * - Checks that the receiver account exists. + * - If the receiver requires a destination tag, check that one exists, even + * if withdrawing to self. + * - If withdrawing to self, succeed. + * - If not, checks if the receiver requires deposit authorization, and if + * the sender has it. + */ +[[nodiscard]] TER +canWithdraw(ReadView const& view, STTx const& tx); + /// 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 98c1b25dfa..c52b313df8 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -1340,26 +1340,6 @@ 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) { @@ -1375,7 +1355,7 @@ checkDestinationAndTag(SLE::const_ref toSle, bool hasDestinationTag) } [[nodiscard]] TER -canSendToAccount( +canWithdraw( AccountID const& from, ReadView const& view, AccountID const& to, @@ -1398,7 +1378,7 @@ canSendToAccount( } [[nodiscard]] TER -canSendToAccount( +canWithdraw( AccountID const& from, ReadView const& view, AccountID const& to, @@ -1406,7 +1386,16 @@ canSendToAccount( { auto const toSle = view.read(keylet::account(to)); - return canSendToAccount(from, view, to, toSle, hasDestinationTag); + return canWithdraw(from, view, to, toSle, hasDestinationTag); +} + +[[nodiscard]] TER +canWithdraw(ReadView const& view, STTx const& tx) +{ + auto const from = tx[sfAccount]; + auto const to = tx[~sfDestination].value_or(from); + + return canWithdraw(from, view, to, tx.isFieldPresent(sfDestinationTag)); } [[nodiscard]] TER diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 259be872ed..57bff0bbfa 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -384,13 +384,6 @@ class LoanBroker_test : public beast::unit_test::suite destination(AccountID{}), ter(temMALFORMED)); - // If a destination tag is specified, a destination must be - // specified, too - env(coverWithdraw(alice, keylet.key, vault.asset(1)), - dtag(123), - ter(temMALFORMED)); - verifyCoverAmount(10); - // Withdraw some of the cover amount env(coverWithdraw(alice, keylet.key, vault.asset(7))); env.close(); diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index b0f1b98154..e7143bcf50 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp @@ -43,9 +43,13 @@ LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx) if (!isLegalNet(dstAmount)) return temBAD_AMOUNT; - if (auto const ret = preflightDestinationAndTag( - ctx.tx[~sfDestination], ctx.tx.isFieldPresent(sfDestinationTag))) - return ret; + if (auto const destination = ctx.tx[~sfDestination]) + { + if (*destination == beast::zero) + { + return temMALFORMED; + } + } return tesSUCCESS; } @@ -85,11 +89,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) AuthType authType = AuthType::Legacy; if (account != dstAcct) { - if (auto const ret = canSendToAccount( - account, - ctx.view, - dstAcct, - tx.isFieldPresent(sfDestinationTag))) + if (auto const ret = canWithdraw(ctx.view, tx)) return ret; // The destination account must have consented to receive the asset by diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 37c0f63e51..d35fc56741 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -42,9 +42,13 @@ VaultWithdraw::preflight(PreflightContext const& ctx) if (ctx.tx[sfAmount] <= beast::zero) return temBAD_AMOUNT; - if (auto const ret = preflightDestinationAndTag( - ctx.tx[~sfDestination], ctx.tx.isFieldPresent(sfDestinationTag))) - return ret; + if (auto const destination = ctx.tx[~sfDestination]) + { + if (*destination == beast::zero) + { + return temMALFORMED; + } + } return tesSUCCESS; } @@ -105,11 +109,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) auto const account = ctx.tx[sfAccount]; auto const dstAcct = ctx.tx[~sfDestination].value_or(account); - if (auto const ret = canSendToAccount( - account, - ctx.view, - dstAcct, - ctx.tx.isFieldPresent(sfDestinationTag))) + if (auto const ret = canWithdraw(ctx.view, ctx.tx)) return ret; // If sending to Account (i.e. not a transfer), we will also create (only