From dd5464d92170a2ce54a3efc7ba90b845c0a033fa Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sun, 23 Nov 2025 23:06:44 -0500 Subject: [PATCH] From discussion with @gregtatcam: Revert "Payment" - Revert Payment transactor (Payment.*) back to what is in "develop". - Change LoanBrokerCoverWithdraw to do a direct transfer, whether it's too the account or the destination. Similar to VaultWithdraw, and as specified - https://github.com/XRPLF/rippled/pull/5270#discussion_r2554560222 --- .../app/tx/detail/LoanBrokerCoverWithdraw.cpp | 67 +--- src/xrpld/app/tx/detail/Payment.cpp | 373 +++++++----------- src/xrpld/app/tx/detail/Payment.h | 31 -- 3 files changed, 165 insertions(+), 306 deletions(-) diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index 58204b70d5..ef4cd68420 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp @@ -159,60 +159,25 @@ LoanBrokerCoverWithdraw::doApply() return ter; } - // Move the funds from the broker's pseudo-account to the dstAcct - if (dstAcct == account_ || amount.native()) + // Sanity check + if (accountHolds( + view(), + brokerPseudoID, + amount.asset(), + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j_) < amount) { - // Transfer assets directly from pseudo-account to depositor. - // Because this is either a self-transfer or an XRP payment, there is no - // need to use the payment engine. - return accountSend( - view(), brokerPseudoID, dstAcct, amount, j_, WaiveTransferFee::Yes); + // LCOV_EXCL_START + JLOG(j_.error()) << "LoanBrokerCoverWithdraw: negative balance of " + "broker cover assets."; + return tefINTERNAL; + // LCOV_EXCL_STOP } - { - // If sending the Cover to a different account, then this is - // effectively a payment. Use the Payment transaction code to call - // the payment engine, though only a subset of the functionality is - // supported in this transaction. e.g. No paths, no partial - // payments. - bool const mptDirect = amount.holds(); - STAmount const maxSourceAmount = - Payment::getMaxSourceAmount(brokerPseudoID, amount); - SLE::pointer sleDst = view().peek(keylet::account(dstAcct)); - if (!sleDst) - return tecINTERNAL; // LCOV_EXCL_LINE - - Payment::RipplePaymentParams paymentParams{ - .ctx = ctx_, - .maxSourceAmount = maxSourceAmount, - .srcAccountID = brokerPseudoID, - .dstAccountID = dstAcct, - .sleDst = sleDst, - .dstAmount = amount, - .paths = STPathSet{}, - .deliverMin = std::nullopt, - .j = j_}; - - TER ret; - if (mptDirect) - { - ret = Payment::makeMPTDirectPayment(paymentParams); - } - else - { - ret = Payment::makeRipplePayment(paymentParams); - } - // Always claim a fee - if (!isTesSuccess(ret) && !isTecClaim(ret)) - { - JLOG(j_.info()) - << "LoanBrokerCoverWithdraw: changing result from " - << transToken(ret) - << " to tecPATH_DRY for IOU payment with Destination"; - return tecPATH_DRY; - } - return ret; - } + // Move the funds directly from the broker's pseudo-account to the dstAcct + return accountSend( + view(), brokerPseudoID, dstAcct, amount, j_, WaiveTransferFee::Yes); } //------------------------------------------------------------------------------ diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 9d6a5e5783..3aff4db5e1 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -29,8 +29,8 @@ Payment::makeTxConsequences(PreflightContext const& ctx) } STAmount -Payment::getMaxSourceAmount( - AccountID const& senderAccount, +getMaxSourceAmount( + AccountID const& account, STAmount const& dstAmount, std::optional const& sendMax) { @@ -40,7 +40,7 @@ Payment::getMaxSourceAmount( return dstAmount; else return STAmount( - Issue{dstAmount.get().currency, senderAccount}, + Issue{dstAmount.get().currency, account}, dstAmount.mantissa(), dstAmount.exponent(), dstAmount < beast::zero); @@ -422,35 +422,155 @@ Payment::doApply() if (ripple) { - return makeRipplePayment(RipplePaymentParams{ - .ctx = ctx_, - .maxSourceAmount = maxSourceAmount, - .srcAccountID = account_, - .dstAccountID = dstAccountID, - .sleDst = sleDst, - .dstAmount = dstAmount, - .paths = ctx_.tx.getFieldPathSet(sfPaths), - .deliverMin = deliverMin, - .partialPaymentAllowed = partialPaymentAllowed, - .defaultPathsAllowed = defaultPathsAllowed, - .limitQuality = limitQuality, - .j = j_}); + // Ripple payment with at least one intermediate step and uses + // transitive balances. + + // An account that requires authorization has two ways to get an + // IOU Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination. + + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; + + path::RippleCalc::Input rcInput; + rcInput.partialPaymentAllowed = partialPaymentAllowed; + rcInput.defaultPathsAllowed = defaultPathsAllowed; + rcInput.limitQuality = limitQuality; + rcInput.isLedgerOpen = view().open(); + + path::RippleCalc::Output rc; + { + PaymentSandbox pv(&view()); + JLOG(j_.debug()) << "Entering RippleCalc in payment: " + << ctx_.tx.getTransactionID(); + rc = path::RippleCalc::rippleCalculate( + pv, + maxSourceAmount, + dstAmount, + dstAccountID, + account_, + ctx_.tx.getFieldPathSet(sfPaths), + ctx_.tx[~sfDomainID], + ctx_.app.logs(), + &rcInput); + // VFALCO NOTE We might not need to apply, depending + // on the TER. But always applying *should* + // be safe. + pv.apply(ctx_.rawView()); + } + + // TODO: is this right? If the amount is the correct amount, was + // the delivered amount previously set? + if (rc.result() == tesSUCCESS && rc.actualAmountOut != dstAmount) + { + if (deliverMin && rc.actualAmountOut < *deliverMin) + rc.setResult(tecPATH_PARTIAL); + else + ctx_.deliver(rc.actualAmountOut); + } + + auto terResult = rc.result(); + + // Because of its overhead, if RippleCalc + // fails with a retry code, claim a fee + // instead. Maybe the user will be more + // careful with their path spec next time. + if (isTerRetry(terResult)) + terResult = tecPATH_DRY; + return terResult; } else if (mptDirect) { - return makeMPTDirectPayment(RipplePaymentParams{ - .ctx = ctx_, - .maxSourceAmount = maxSourceAmount, - .srcAccountID = account_, - .dstAccountID = dstAccountID, - .sleDst = sleDst, - .dstAmount = dstAmount, - .paths = ctx_.tx.getFieldPathSet(sfPaths), - .deliverMin = deliverMin, - .partialPaymentAllowed = partialPaymentAllowed, - .defaultPathsAllowed = defaultPathsAllowed, - .limitQuality = limitQuality, - .j = j_}); + JLOG(j_.trace()) << " dstAmount=" << dstAmount.getFullText(); + auto const& mptIssue = dstAmount.get(); + + if (auto const ter = requireAuth(view(), mptIssue, account_); + ter != tesSUCCESS) + return ter; + + if (auto const ter = requireAuth(view(), mptIssue, dstAccountID); + ter != tesSUCCESS) + return ter; + + if (auto const ter = + canTransfer(view(), mptIssue, account_, dstAccountID); + ter != tesSUCCESS) + return ter; + + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; + + auto const& issuer = mptIssue.getIssuer(); + + // Transfer rate + Rate rate{QUALITY_ONE}; + // Payment between the holders + if (account_ != issuer && dstAccountID != issuer) + { + // If globally/individually locked then + // - can't send between holders + // - holder can send back to issuer + // - issuer can send to holder + if (isAnyFrozen(view(), {account_, dstAccountID}, mptIssue)) + return tecLOCKED; + + // Get the rate for a payment between the holders. + rate = transferRate(view(), mptIssue.getMptID()); + } + + // Amount to deliver. + STAmount amountDeliver = dstAmount; + // Factor in the transfer rate. + // No rounding. It'll change once MPT integrated into DEX. + STAmount requiredMaxSourceAmount = multiply(dstAmount, rate); + + // Send more than the account wants to pay or less than + // the account wants to deliver (if no SendMax). + // Adjust the amount to deliver. + if (partialPaymentAllowed && requiredMaxSourceAmount > maxSourceAmount) + { + requiredMaxSourceAmount = maxSourceAmount; + // No rounding. It'll change once MPT integrated into DEX. + amountDeliver = divide(maxSourceAmount, rate); + } + + if (requiredMaxSourceAmount > maxSourceAmount || + (deliverMin && amountDeliver < *deliverMin)) + return tecPATH_PARTIAL; + + PaymentSandbox pv(&view()); + auto res = accountSend( + pv, account_, dstAccountID, amountDeliver, ctx_.journal); + if (res == tesSUCCESS) + { + pv.apply(ctx_.rawView()); + + // If the actual amount delivered is different from the original + // amount due to partial payment or transfer fee, we need to update + // DelieveredAmount using the actual delivered amount + if (view().rules().enabled(fixMPTDeliveredAmount) && + amountDeliver != dstAmount) + ctx_.deliver(amountDeliver); + } + else if (res == tecINSUFFICIENT_FUNDS || res == tecPATH_DRY) + res = tecPATH_PARTIAL; + + return res; } XRPL_ASSERT(dstAmount.native(), "ripple::Payment::doApply : amount is XRP"); @@ -543,199 +663,4 @@ Payment::doApply() return tesSUCCESS; } -// Reusable helpers -TER -Payment::makeRipplePayment(Payment::RipplePaymentParams const& p) -{ - // Set up some copies/references so the code can be moved from - // Payment::doApply otherwise unmodified - // - // Note that some of these variable names use trailing '_', which is - // usually reserved for member variables. After, or just before these - // changes are merged, a follow-up can clean up the names for consistency. - ApplyContext& ctx_ = p.ctx; - STAmount const& maxSourceAmount = p.maxSourceAmount; - AccountID const& account_ = p.srcAccountID; - AccountID const& dstAccountID = p.dstAccountID; - SLE::pointer sleDst = p.sleDst; - STAmount const& dstAmount = p.dstAmount; - STPathSet const& paths = p.paths; - std::optional const& deliverMin = p.deliverMin; - bool partialPaymentAllowed = p.partialPaymentAllowed; - bool defaultPathsAllowed = p.defaultPathsAllowed; - bool limitQuality = p.limitQuality; - beast::Journal j_ = p.j; - - auto view = [&p]() -> ApplyView& { return p.ctx.view(); }; - - // Below this line, copied straight from Payment::doApply - // except `ctx_.tx.getFieldPathSet(sfPaths)` replaced with `paths`, - // because not all transactions have that field available, and using - // it will throw - //------------------------------------------------------- - // Ripple payment with at least one intermediate step and uses - // transitive balances. - - // An account that requires authorization has two ways to get an - // IOU Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination. - - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, dstAccountID, sleDst, ctx_.journal); - !isTesSuccess(err)) - return err; - - path::RippleCalc::Input rcInput; - rcInput.partialPaymentAllowed = partialPaymentAllowed; - rcInput.defaultPathsAllowed = defaultPathsAllowed; - rcInput.limitQuality = limitQuality; - rcInput.isLedgerOpen = view().open(); - - path::RippleCalc::Output rc; - { - PaymentSandbox pv(&view()); - JLOG(j_.debug()) << "Entering RippleCalc in payment: " - << ctx_.tx.getTransactionID(); - rc = path::RippleCalc::rippleCalculate( - pv, - maxSourceAmount, - dstAmount, - dstAccountID, - account_, - paths, - ctx_.tx[~sfDomainID], - ctx_.app.logs(), - &rcInput); - // VFALCO NOTE We might not need to apply, depending - // on the TER. But always applying *should* - // be safe. - pv.apply(ctx_.rawView()); - } - - // TODO: is this right? If the amount is the correct amount, was - // the delivered amount previously set? - if (rc.result() == tesSUCCESS && rc.actualAmountOut != dstAmount) - { - if (deliverMin && rc.actualAmountOut < *deliverMin) - rc.setResult(tecPATH_PARTIAL); - else - ctx_.deliver(rc.actualAmountOut); - } - - auto terResult = rc.result(); - - // Because of its overhead, if RippleCalc - // fails with a retry code, claim a fee - // instead. Maybe the user will be more - // careful with their path spec next time. - if (isTerRetry(terResult)) - terResult = tecPATH_DRY; - return terResult; -} - -TER -Payment::makeMPTDirectPayment(Payment::RipplePaymentParams const& p) -{ - // Set up some copies/references so the code can be moved from - // Payment::doApply otherwise unmodified - // - // Note that some of these variable names use trailing '_', which is - // usually reserved for member variables. After, or just before these - // changes are merged, a follow-up can clean up the names for consistency. - ApplyContext& ctx_ = p.ctx; - STAmount const& maxSourceAmount = p.maxSourceAmount; - AccountID const& account_ = p.srcAccountID; - AccountID const& dstAccountID = p.dstAccountID; - SLE::pointer sleDst = p.sleDst; - STAmount const& dstAmount = p.dstAmount; - // STPathSet const& paths = p.paths; - std::optional const& deliverMin = p.deliverMin; - bool partialPaymentAllowed = p.partialPaymentAllowed; - // bool defaultPathsAllowed = p.defaultPathsAllowed; - // bool limitQuality = p.limitQuality; - beast::Journal j_ = p.j; - - auto view = [&p]() -> ApplyView& { return p.ctx.view(); }; - - // Below this line, copied straight from Payment::doApply - //------------------------------------------------------- - JLOG(j_.trace()) << " dstAmount=" << dstAmount.getFullText(); - auto const& mptIssue = dstAmount.get(); - - if (auto const ter = requireAuth(view(), mptIssue, account_); - ter != tesSUCCESS) - return ter; - - if (auto const ter = requireAuth(view(), mptIssue, dstAccountID); - ter != tesSUCCESS) - return ter; - - if (auto const ter = canTransfer(view(), mptIssue, account_, dstAccountID); - ter != tesSUCCESS) - return ter; - - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, dstAccountID, sleDst, ctx_.journal); - !isTesSuccess(err)) - return err; - - auto const& issuer = mptIssue.getIssuer(); - - // Transfer rate - Rate rate{QUALITY_ONE}; - // Payment between the holders - if (account_ != issuer && dstAccountID != issuer) - { - // If globally/individually locked then - // - can't send between holders - // - holder can send back to issuer - // - issuer can send to holder - if (isAnyFrozen(view(), {account_, dstAccountID}, mptIssue)) - return tecLOCKED; - - // Get the rate for a payment between the holders. - rate = transferRate(view(), mptIssue.getMptID()); - } - - // Amount to deliver. - STAmount amountDeliver = dstAmount; - // Factor in the transfer rate. - // No rounding. It'll change once MPT integrated into DEX. - STAmount requiredMaxSourceAmount = multiply(dstAmount, rate); - - // Send more than the account wants to pay or less than - // the account wants to deliver (if no SendMax). - // Adjust the amount to deliver. - if (partialPaymentAllowed && requiredMaxSourceAmount > maxSourceAmount) - { - requiredMaxSourceAmount = maxSourceAmount; - // No rounding. It'll change once MPT integrated into DEX. - amountDeliver = divide(maxSourceAmount, rate); - } - - if (requiredMaxSourceAmount > maxSourceAmount || - (deliverMin && amountDeliver < *deliverMin)) - return tecPATH_PARTIAL; - - PaymentSandbox pv(&view()); - auto res = - accountSend(pv, account_, dstAccountID, amountDeliver, ctx_.journal); - if (res == tesSUCCESS) - { - pv.apply(ctx_.rawView()); - - // If the actual amount delivered is different from the original - // amount due to partial payment or transfer fee, we need to update - // DelieveredAmount using the actual delivered amount - if (view().rules().enabled(fixMPTDeliveredAmount) && - amountDeliver != dstAmount) - ctx_.deliver(amountDeliver); - } - else if (res == tecINSUFFICIENT_FUNDS || res == tecPATH_DRY) - res = tecPATH_PARTIAL; - - return res; -} - } // namespace ripple diff --git a/src/xrpld/app/tx/detail/Payment.h b/src/xrpld/app/tx/detail/Payment.h index c532b6efb2..f41326ecf6 100644 --- a/src/xrpld/app/tx/detail/Payment.h +++ b/src/xrpld/app/tx/detail/Payment.h @@ -40,37 +40,6 @@ public: TER doApply() override; - - // Helpers for this class and other transactors that make "Payments" - struct RipplePaymentParams - { - ApplyContext& ctx; - STAmount const& maxSourceAmount; - AccountID const& srcAccountID; - AccountID const& dstAccountID; - SLE::pointer sleDst; - STAmount const& dstAmount; - // Paths need to be explicitly included because other transactions don't - // have them defined - STPathSet const& paths; - std::optional const& deliverMin; - bool partialPaymentAllowed = false; - bool defaultPathsAllowed = true; - bool limitQuality = false; - beast::Journal j; - }; - - static STAmount - getMaxSourceAmount( - AccountID const& senderAccount, - STAmount const& dstAmount, - std::optional const& sendMax = {}); - - static TER - makeRipplePayment(RipplePaymentParams const& p); - - static TER - makeMPTDirectPayment(RipplePaymentParams const& p); }; } // namespace ripple