Refactor Payment transactor.

src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp

@@ -3,6 +3,8 @@
 #include <xrpld/app/misc/LendingHelpers.h>
 #include <xrpld/app/tx/detail/Payment.h>
 
+#include "test/jtx/pay.h"
+
 namespace ripple {
 
 bool
@@ -163,33 +165,19 @@ LoanBrokerCoverWithdraw::doApply()
         // 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<MPTIssue>();
-        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,
+            .sendMax = amount,
             .srcAccountID = brokerPseudoID,
             .dstAccountID = dstAcct,
-            .sleDst = sleDst,
             .dstAmount = amount,
-            .paths = STPathSet{},
+            .sourceBalance = mSourceBalance,
+            .priorBalance = mPriorBalance,
+            .paths = {},
             .deliverMin = std::nullopt,
-            .j = j_};
+            .domainID = std::nullopt
-
+    };
-        TER ret;
+        TER const ret = Payment::makePayment(paymentParams);
-        if (mptDirect)
-        {
-            ret = Payment::makeMPTDirectPayment(paymentParams);
-        }
-        else
-        {
-            ret = Payment::makeRipplePayment(paymentParams);
-        }
         // Always claim a fee
         if (!isTesSuccess(ret) && !isTecClaim(ret))
         {

src/xrpld/app/tx/detail/Payment.cpp

@@ -28,8 +28,8 @@ Payment::makeTxConsequences(PreflightContext const& ctx)
     return TxConsequences{ctx.tx, calculateMaxXRPSpend(ctx.tx)};
 }
 
-STAmount
+static STAmount
-Payment::getMaxSourceAmount(
+getMaxSourceAmount(
     AccountID const& senderAccount,
     STAmount const& dstAmount,
     std::optional<STAmount> const& sendMax)
@@ -371,20 +371,26 @@ Payment::preclaim(PreclaimContext const& ctx)
 }
 
 TER
-Payment::doApply()
+Payment::makePayment(RipplePaymentParams& p)
 {
-    auto const deliverMin = ctx_.tx[~sfDeliverMin];
+    auto const deliverMin = p.deliverMin;
 
     // Ripple if source or destination is non-native or if there are paths.
-    std::uint32_t const txFlags = ctx_.tx.getFlags();
+    std::uint32_t const txFlags = p.flags;
     bool const partialPaymentAllowed = txFlags & tfPartialPayment;
     bool const limitQuality = txFlags & tfLimitQuality;
     bool const defaultPathsAllowed = !(txFlags & tfNoRippleDirect);
-    auto const hasPaths = ctx_.tx.isFieldPresent(sfPaths);
+    auto const hasPaths = !p.paths.empty();
-    auto const sendMax = ctx_.tx[~sfSendMax];
+    auto const sendMax = p.sendMax;
+    auto j_ = p.ctx.journal;
+    auto& ctx_ = p.ctx;
+    auto& view = ctx_.view();
+    auto const& account_ = p.srcAccountID;
+    auto const& mSourceBalance = p.sourceBalance;
+    auto const& mPriorBalance = p.priorBalance;
 
-    AccountID const dstAccountID(ctx_.tx.getAccountID(sfDestination));
+    AccountID const dstAccountID(p.dstAccountID);
-    STAmount const dstAmount(ctx_.tx.getFieldAmount(sfAmount));
+    STAmount const dstAmount(p.dstAmount);
     bool const mptDirect = dstAmount.holds<MPTIssue>();
     STAmount const maxSourceAmount =
         getMaxSourceAmount(account_, dstAmount, sendMax);
@@ -394,12 +400,12 @@ Payment::doApply()
 
     // Open a ledger for editing.
     auto const k = keylet::account(dstAccountID);
-    SLE::pointer sleDst = view().peek(k);
+    SLE::pointer sleDst = view.peek(k);
 
     if (!sleDst)
     {
         std::uint32_t const seqno{
-            view().rules().enabled(featureDeletableAccounts) ? view().seq()
+            view.rules().enabled(featureDeletableAccounts) ? view.seq()
                                                              : 1};
 
         // Create the account.
@@ -407,14 +413,14 @@ Payment::doApply()
         sleDst->setAccountID(sfAccount, dstAccountID);
         sleDst->setFieldU32(sfSequence, seqno);
 
-        view().insert(sleDst);
+        view.insert(sleDst);
     }
     else
     {
         // Tell the engine that we are intending to change the destination
         // account.  The source account gets always charged a fee so it's always
         // marked as modified.
-        view().update(sleDst);
+        view.update(sleDst);
     }
 
     bool const ripple =
@@ -422,42 +428,162 @@ Payment::doApply()
 
     if (ripple)
     {
-        return makeRipplePayment(RipplePaymentParams{
+        // Ripple payment with at least one intermediate step and uses
-            .ctx = ctx_,
+        // transitive balances.
-            .maxSourceAmount = maxSourceAmount,
+
-            .srcAccountID = account_,
+        // An account that requires authorization has two ways to get an
-            .dstAccountID = dstAccountID,
+        // IOU Payment in:
-            .sleDst = sleDst,
+        //  1. If Account == Destination, or
-            .dstAmount = dstAmount,
+        //  2. If Account is deposit preauthorized by destination.
-            .paths = ctx_.tx.getFieldPathSet(sfPaths),
+
-            .deliverMin = deliverMin,
+        if (auto err = verifyDepositPreauth(
-            .partialPaymentAllowed = partialPaymentAllowed,
+                ctx_.tx,
-            .defaultPathsAllowed = defaultPathsAllowed,
+                ctx_.view(),
-            .limitQuality = limitQuality,
+                account_,
-            .j = j_});
+                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_,
+                p.paths,
+                p.domainID,
+                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{
+        JLOG(j_.trace()) << " dstAmount=" << dstAmount.getFullText();
-            .ctx = ctx_,
+        auto const& mptIssue = dstAmount.get<MPTIssue>();
-            .maxSourceAmount = maxSourceAmount,
+
-            .srcAccountID = account_,
+        if (auto const ter = requireAuth(view, mptIssue, account_);
-            .dstAccountID = dstAccountID,
+            ter != tesSUCCESS)
-            .sleDst = sleDst,
+            return ter;
-            .dstAmount = dstAmount,
+
-            .paths = ctx_.tx.getFieldPathSet(sfPaths),
+        if (auto const ter = requireAuth(view, mptIssue, dstAccountID);
-            .deliverMin = deliverMin,
+            ter != tesSUCCESS)
-            .partialPaymentAllowed = partialPaymentAllowed,
+            return ter;
-            .defaultPathsAllowed = defaultPathsAllowed,
+
-            .limitQuality = limitQuality,
+        if (auto const ter =
-            .j = j_});
+                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());
         }
 
-    XRPL_ASSERT(dstAmount.native(), "ripple::Payment::doApply : amount is XRP");
+        // 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::makePayment : amount is XRP");
 
     // Direct XRP payment.
 
-    auto const sleSrc = view().peek(keylet::account(account_));
+    auto const sleSrc = view.peek(keylet::account(account_));
     if (!sleSrc)
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
@@ -466,7 +592,7 @@ Payment::doApply()
     auto const ownerCount = sleSrc->getFieldU32(sfOwnerCount);
 
     // This is the total reserve in drops.
-    auto const reserve = view().fees().accountReserve(ownerCount);
+    auto const reserve = view.fees().accountReserve(ownerCount);
 
     // mPriorBalance is the balance on the sending account BEFORE the
     // fees were charged. We want to make sure we have enough reserve
@@ -515,7 +641,7 @@ Payment::doApply()
     // to get the account un-wedged.
 
     // Get the base reserve.
-    XRPAmount const dstReserve{view().fees().reserve};
+    XRPAmount const dstReserve{view.fees().reserve};
 
     if (dstAmount > dstReserve ||
         sleDst->getFieldAmount(sfBalance) > dstReserve)
@@ -543,199 +669,23 @@ Payment::doApply()
     return tesSUCCESS;
 }
 
-// Reusable helpers
 TER
-Payment::makeRipplePayment(Payment::RipplePaymentParams const& p)
+Payment::doApply()
 {
-    // Set up some copies/references so the code can be moved from
+    RipplePaymentParams paymentParams{
-    // Payment::doApply otherwise unmodified
+        .ctx = ctx_,
-    //
+        .sendMax = ctx_.tx[~sfSendMax],
-    // Note that some of these variable names use trailing '_', which is
+        .srcAccountID = ctx_.tx[sfAccount],
-    // usually reserved for member variables. After, or just before these
+        .dstAccountID = ctx_.tx[sfDestination],
-    // changes are merged, a follow-up can clean up the names for consistency.
+        .dstAmount = ctx_.tx[sfAmount],
-    ApplyContext& ctx_ = p.ctx;
+        .sourceBalance = mSourceBalance,
-    STAmount const& maxSourceAmount = p.maxSourceAmount;
+        .priorBalance = mPriorBalance,
-    AccountID const& account_ = p.srcAccountID;
+        .paths = ctx_.tx.getFieldPathSet(sfPaths),
-    AccountID const& dstAccountID = p.dstAccountID;
+        .deliverMin = ctx_.tx[~sfDeliverMin],
-    SLE::pointer sleDst = p.sleDst;
+        .domainID = ctx_.tx[~sfDomainID],
-    STAmount const& dstAmount = p.dstAmount;
+        .flags = ctx_.tx.getFlags(),
-    STPathSet const& paths = p.paths;
+    };
-    std::optional<STAmount> const& deliverMin = p.deliverMin;
+    return makePayment(paymentParams);
-    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<STAmount> 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<MPTIssue>();
-
-    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

src/xrpld/app/tx/detail/Payment.h

@@ -45,32 +45,20 @@ public:
     struct RipplePaymentParams
     {
         ApplyContext& ctx;
-        STAmount const& maxSourceAmount;
+        std::optional<STAmount> const& sendMax;
         AccountID const& srcAccountID;
         AccountID const& dstAccountID;
-        SLE::pointer sleDst;
         STAmount const& dstAmount;
-        // Paths need to be explicitly included because other transactions don't
+        XRPAmount const& sourceBalance;
-        // have them defined
+        XRPAmount const& priorBalance;
         STPathSet const& paths;
         std::optional<STAmount> const& deliverMin;
-        bool partialPaymentAllowed = false;
+        std::optional<uint256> const& domainID;
-        bool defaultPathsAllowed = true;
+        std::uint32_t flags = 0;
-        bool limitQuality = false;
-        beast::Journal j;
     };
 
-    static STAmount
-    getMaxSourceAmount(
-        AccountID const& senderAccount,
-        STAmount const& dstAmount,
-        std::optional<STAmount> const& sendMax = {});
-
     static TER
-    makeRipplePayment(RipplePaymentParams const& p);
+    makePayment(RipplePaymentParams& p);
-
-    static TER
-    makeMPTDirectPayment(RipplePaymentParams const& p);
 };
 
 }  // namespace ripple