Get CoverWithdraw IOU payments working

- Clean up some of the payment parameters
- Also factor out Payment::makeMPTDirectPayment for future use

src/test/app/LoanBroker_test.cpp

@@ -244,9 +244,16 @@ class LoanBroker_test : public beast::unit_test::suite
             env(coverWithdraw(alice, keylet.key, vault.asset(900)),
                 ter(tecINSUFFICIENT_FUNDS));
 
-            env(coverWithdraw(alice, keylet.key, vault.asset(1)),
-                destination(bystander),
-                ter(tecNO_LINE));
+            // Skip this test for XRP, because that can always be sent
+            if (!vault.asset.raw().native())
+            {
+                TER const expected = vault.asset.raw().holds<MPTIssue>()
+                    ? tecNO_AUTH
+                    : tecNO_LINE;
+                env(coverWithdraw(alice, keylet.key, vault.asset(1)),
+                    destination(bystander),
+                    ter(expected));
+            }
             verifyCoverAmount(10);
 
             // Withdraw some of the cover amount

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

@@ -55,7 +55,11 @@ LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx)
     if (ctx.tx[sfLoanBrokerID] == beast::zero)
         return temINVALID;
 
-    if (ctx.tx[sfAmount] <= beast::zero)
+    auto const dstAmount = ctx.tx[sfAmount];
+    if (dstAmount <= beast::zero)
+        return temBAD_AMOUNT;
+
+    if (!isLegalNet(dstAmount))
         return temBAD_AMOUNT;
 
     if (auto const destination = ctx.tx[~sfDestination];
@@ -183,41 +187,54 @@ LoanBrokerCoverWithdraw::doApply()
     if (!broker)
         return tecINTERNAL;  // LCOV_EXCL_LINE
 
-    auto const brokerPseudoID = broker->at(sfAccount);
+    auto const brokerPseudoID = *broker->at(sfAccount);
 
     // Decrease the LoanBroker's CoverAvailable by Amount
     broker->at(sfCoverAvailable) -= amount;
     view().update(broker);
 
-    if (dstAcct != account_ && !amount.native() && !amount.holds<MPTIssue>())
+    // Move the funds from the broker's pseudo-account to the dstAcct
+
+    if (dstAcct != account_ && !amount.native())
     {
-        STAmount const maxSourceAmount(
-            Issue{amount.get<Issue>().currency, brokerPseudoID},
-            amount.mantissa(),
-            amount.exponent(),
-            amount < beast::zero);
+        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;
 
-        auto ret = Payment::makeRipplePayment(Payment::RipplePaymentParams{
-            .ctx = ctx_,
-            .maxSourceAmount = maxSourceAmount,
-            .srcAccountID = account_,
-            .dstAccountID = dstAcct,
-            .sleDst = sleDst,
-            .dstAmount = amount,
-            .deliverMin = std::nullopt,
-            .j = j_});
-
-        // Always claim a fee
-        if (!isTesSuccess(ret) && !isTecClaim(ret))
+        if (!mptDirect)
         {
-            JLOG(j_.info())
-                << "LoanBrokerCoverWithdraw: changing result from "
-                << transToken(ret)
-                << " to tecPATH_DRY for IOU payment with Destination";
-            ret = tecPATH_DRY;
+            // 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.
+
+            auto const ret =
+                Payment::makeRipplePayment(Payment::RipplePaymentParams{
+                    .ctx = ctx_,
+                    .maxSourceAmount = maxSourceAmount,
+                    .srcAccountID = brokerPseudoID,
+                    .dstAccountID = dstAcct,
+                    .sleDst = sleDst,
+                    .dstAmount = amount,
+                    .paths = STPathSet{},
+                    .deliverMin = std::nullopt,
+                    .j = j_});
+
+            // 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;
         }
-        return ret;
     }
 
     // Transfer assets from pseudo-account to depositor.

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

@@ -48,8 +48,8 @@ Payment::makeTxConsequences(PreflightContext const& ctx)
 }
 
 STAmount
-getMaxSourceAmount(
-    AccountID const& account,
+Payment::getMaxSourceAmount(
+    AccountID const& senderAccount,
     STAmount const& dstAmount,
     std::optional<STAmount> const& sendMax)
 {
@@ -59,7 +59,7 @@ getMaxSourceAmount(
         return dstAmount;
     else
         return STAmount(
-            Issue{dstAmount.get<Issue>().currency, account},
+            Issue{dstAmount.get<Issue>().currency, senderAccount},
             dstAmount.mantissa(),
             dstAmount.exponent(),
             dstAmount < beast::zero);
@@ -454,6 +454,7 @@ Payment::doApply()
             .dstAccountID = dstAccountID,
             .sleDst = sleDst,
             .dstAmount = dstAmount,
+            .paths = ctx_.tx.getFieldPathSet(sfPaths),
             .deliverMin = deliverMin,
             .partialPaymentAllowed = partialPaymentAllowed,
             .defaultPathsAllowed = defaultPathsAllowed,
@@ -462,79 +463,19 @@ Payment::doApply()
     }
     else if (mptDirect)
     {
-        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());
-        else if (res == tecINSUFFICIENT_FUNDS || res == tecPATH_DRY)
-            res = tecPATH_PARTIAL;
-
-        return res;
+        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_});
     }
 
     XRPL_ASSERT(dstAmount.native(), "ripple::Payment::doApply : amount is XRP");
@@ -676,7 +617,7 @@ Payment::makeRipplePayment(Payment::RipplePaymentParams const& p)
             p.dstAmount,
             p.dstAccountID,
             p.srcAccountID,
-            p.ctx.tx.getFieldPathSet(sfPaths),
+            p.paths,
             p.ctx.tx[~sfDomainID],
             p.ctx.app.logs(),
             &rcInput);
@@ -707,4 +648,83 @@ Payment::makeRipplePayment(Payment::RipplePaymentParams const& p)
     return terResult;
 }
 
+TER
+Payment::makeMPTDirectPayment(Payment::RipplePaymentParams const& p)
+{
+    JLOG(p.j.trace()) << " p.dstAmount=" << p.dstAmount.getFullText();
+    auto const& mptIssue = p.dstAmount.get<MPTIssue>();
+
+    if (auto const ter = requireAuth(p.ctx.view(), mptIssue, p.srcAccountID);
+        ter != tesSUCCESS)
+        return ter;
+
+    if (auto const ter = requireAuth(p.ctx.view(), mptIssue, p.dstAccountID);
+        ter != tesSUCCESS)
+        return ter;
+
+    if (auto const ter =
+            canTransfer(p.ctx.view(), mptIssue, p.srcAccountID, p.dstAccountID);
+        ter != tesSUCCESS)
+        return ter;
+
+    if (auto err = verifyDepositPreauth(
+            p.ctx.tx,
+            p.ctx.view(),
+            p.srcAccountID,
+            p.dstAccountID,
+            p.sleDst,
+            p.ctx.journal);
+        !isTesSuccess(err))
+        return err;
+
+    auto const& issuer = mptIssue.getIssuer();
+
+    // Transfer rate
+    Rate rate{QUALITY_ONE};
+    // Payment between the holders
+    if (p.srcAccountID != issuer && p.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(
+                p.ctx.view(), {p.srcAccountID, p.dstAccountID}, mptIssue))
+            return tecLOCKED;
+
+        // Get the rate for a payment between the holders.
+        rate = transferRate(p.ctx.view(), mptIssue.getMptID());
+    }
+
+    // Amount to deliver.
+    STAmount amountDeliver = p.dstAmount;
+    // Factor in the transfer rate.
+    // No rounding. It'll change once MPT integrated into DEX.
+    STAmount requiredMaxSourceAmount = multiply(p.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 (p.partialPaymentAllowed && requiredMaxSourceAmount > p.maxSourceAmount)
+    {
+        requiredMaxSourceAmount = p.maxSourceAmount;
+        // No rounding. It'll change once MPT integrated into DEX.
+        amountDeliver = divide(p.maxSourceAmount, rate);
+    }
+
+    if (requiredMaxSourceAmount > p.maxSourceAmount ||
+        (p.deliverMin && amountDeliver < *p.deliverMin))
+        return tecPATH_PARTIAL;
+
+    PaymentSandbox pv(&p.ctx.view());
+    auto res = accountSend(
+        pv, p.srcAccountID, p.dstAccountID, amountDeliver, p.ctx.journal);
+    if (res == tesSUCCESS)
+        pv.apply(p.ctx.rawView());
+    else if (res == tecINSUFFICIENT_FUNDS || res == tecPATH_DRY)
+        res = tecPATH_PARTIAL;
+
+    return res;
+}
+
 }  // namespace ripple

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

@@ -69,6 +69,9 @@ public:
         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<STAmount> const& deliverMin;
         bool partialPaymentAllowed = false;
         bool defaultPathsAllowed = true;
@@ -76,8 +79,17 @@ public:
         beast::Journal j;
     };
 
+    static STAmount
+    getMaxSourceAmount(
+        AccountID const& senderAccount,
+        STAmount const& dstAmount,
+        std::optional<STAmount> const& sendMax = {});
+
     static TER
     makeRipplePayment(RipplePaymentParams const& p);
+
+    static TER
+    makeMPTDirectPayment(RipplePaymentParams const& p);
 };
 
 }  // namespace ripple