Get remaining Loan unit tests working

- Rename some of the helper functions / lambdas. - Update tracked interestOwed field better at final payoff. - Add checks in LoanSet that the fields can be represented in the asset type, and update test that fails those checks (testLoanPayComputePeriodicPaymentValidRateInvariant) - Also check that the computed periodic payment can be represented as the asset type, and doesn't round _UP_ to 0. - Update asserts to account for more scenarios, including initial loan computation.
2026-04-29 15:37:57 +00:00 · 2025-10-12 19:18:14 -04:00
parent bda6bb46d1
commit d7196a8e56
4 changed files with 146 additions and 87 deletions
--- a/src/test/app/Loan_test.cpp
+++ b/src/test/app/Loan_test.cpp
@@ -1369,7 +1369,7 @@ class Loan_test : public beast::unit_test::suite
                ter(tecNO_PERMISSION));
        };

-        auto immediatePayoff = [&](std::uint32_t baseFlag) {
+        auto fullPayment = [&](std::uint32_t baseFlag) {
            return [&, baseFlag](
                       Keylet const& loanKeylet,
                       VerifyLoanStatus const& verifyLoanStatus) {
@@ -1432,7 +1432,7 @@ class Loan_test : public beast::unit_test::suite
            };
        };

-        auto multiplePayoff = [&](std::uint32_t baseFlag) {
+        auto combineAllPayments = [&](std::uint32_t baseFlag) {
            return [&, baseFlag](
                       Keylet const& loanKeylet,
                       VerifyLoanStatus const& verifyLoanStatus) {
@@ -1536,7 +1536,7 @@ class Loan_test : public beast::unit_test::suite
            broker,
            pseudoAcct,
            0,
-            immediatePayoff(0));
+            fullPayment(0));

        lifecycle(
            caseLabel,
@@ -1550,11 +1550,11 @@ class Loan_test : public beast::unit_test::suite
            broker,
            pseudoAcct,
            tfLoanOverpayment,
-            immediatePayoff(lsfLoanOverpayment));
+            fullPayment(lsfLoanOverpayment));

        lifecycle(
            caseLabel,
-            "Loan overpayment prohibited - Make multiple payments",
+            "Loan overpayment prohibited - Combine all payments",
            env,
            loanAmount,
            interestExponent,
@@ -1564,11 +1564,11 @@ class Loan_test : public beast::unit_test::suite
            broker,
            pseudoAcct,
            0,
-            multiplePayoff(0));
+            combineAllPayments(0));

        lifecycle(
            caseLabel,
-            "Loan overpayment allowed - Make multiple payments",
+            "Loan overpayment allowed - Combine all payments",
            env,
            loanAmount,
            interestExponent,
@@ -1578,7 +1578,7 @@ class Loan_test : public beast::unit_test::suite
            broker,
            pseudoAcct,
            tfLoanOverpayment,
-            multiplePayoff(lsfLoanOverpayment));
+            combineAllPayments(lsfLoanOverpayment));

        lifecycle(
            caseLabel,
@@ -1775,20 +1775,21 @@ class Loan_test : public beast::unit_test::suite
                    if (paymentComponents.final)
                    {
                        state.paymentRemaining = 0;
+                        state.interestOwed = 0;
                    }
                    else
                    {
                        state.nextPaymentDate += state.paymentInterval;
+                        state.interestOwed -= valueMinusFee(
+                            broker.asset.raw(),
+                            paymentComponents.roundedInterest,
+                            managementFeeRateParameter,
+                            state.loanScale);
                    }
                    state.principalOutstanding -=
                        paymentComponents.roundedPrincipal;
                    state.totalValue -= paymentComponents.roundedPrincipal +
                        paymentComponents.roundedInterest;
-                    state.interestOwed -= valueMinusFee(
-                        broker.asset.raw(),
-                        paymentComponents.roundedInterest,
-                        managementFeeRateParameter,
-                        state.loanScale);

                    verifyLoanStatus(state);
                }
@@ -2214,38 +2215,28 @@ class Loan_test : public beast::unit_test::suite
        createJson["OverpaymentInterestRate"] = 1360;
        createJson["PaymentInterval"] = 727;

-        Number const actualPrincipal{6};
-
        auto const brokerStateBefore =
            env.le(keylet::loanbroker(broker.brokerID));
        auto const loanSequence = brokerStateBefore->at(sfLoanSequence);
        auto const keylet = keylet::loan(broker.brokerID, loanSequence);

        createJson = env.json(createJson, sig(sfCounterpartySignature, lender));
-        env(createJson, ter(tesSUCCESS));
+        // Fails in preclaim because principal requested can't be represented as
+        // XRP
+        env(createJson, ter(tecPRECISION_LOSS));
        env.close();

-        if (auto const loan = env.le(keylet); BEAST_EXPECT(loan))
-        {
-            // Verify the payment decreased the principal
-            BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments);
-            BEAST_EXPECT(loan->at(sfLoanScale) == actualPrincipal.exponent());
-            BEAST_EXPECT(loan->at(sfPrincipalOutstanding) == actualPrincipal);
-        }
+        BEAST_EXPECT(!env.le(keylet));

-        auto loanPayTx = env.json(
-            pay(borrower, keylet.key, STAmount{broker.asset, serviceFee + 6}));
-        env(loanPayTx, ter(tesSUCCESS));
+        Number const actualPrincipal{6};
+
+        createJson[sfPrincipalRequested] = actualPrincipal;
+        createJson.removeMember(sfSequence.jsonName);
+        createJson = env.json(createJson, sig(sfCounterpartySignature, lender));
+        // Fails in doApply because the payment is too small to be represented
+        // as XRP.
+        env(createJson, ter(tecPRECISION_LOSS));
        env.close();
-
-        if (auto const loan = env.le(keylet); BEAST_EXPECT(loan))
-        {
-            // Verify the payment decreased the principal
-            BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments - 1);
-            BEAST_EXPECT(loan->at(sfLoanScale) == actualPrincipal.exponent());
-            BEAST_EXPECT(
-                loan->at(sfPrincipalOutstanding) == actualPrincipal - 1);
-        }
    }

    void
--- a/src/xrpld/app/misc/LendingHelpers.h
+++ b/src/xrpld/app/misc/LendingHelpers.h
@@ -225,16 +225,19 @@ computePaymentComponents(
        auto const p = roundToAsset(
            asset,
            // Compute the delta that will get the tracked principalOutstanding
-            // amount as close to the raw principal amount after the payment as
+            // amount as close to the true principal amount after the payment as
            // possible.
            principalOutstanding - (rawPrincipalOutstanding - rawPrincipal),
            scale,
            Number::downward);

+        // The principal part can only be 0 during intial loan validation. If it
+        // is 0, the Loan will not be created, but we don't want an assert
+        // aborting the process before we get that far.
        XRPL_ASSERT_PARTS(
-            p > 0,
+            p >= 0,
            "rippled::detail::computePaymentComponents",
-            "principal part positive");
+            "principal part not negative");
        XRPL_ASSERT_PARTS(
            p <= principalOutstanding,
            "rippled::detail::computePaymentComponents",
@@ -266,7 +269,7 @@ computePaymentComponents(
        auto const iDiff = roundedPeriodicPayment - roundedPrincipal;

        // Compute the delta that will get the untracked interestOutstanding
-        // amount as close as possible to the raw interest amount after the
+        // amount as close as possible to the true interest amount after the
        // payment as possible.
        auto const iSync = interestOutstanding -
            (roundToAsset(asset, rawInterestOutstanding, scale) -
@@ -691,7 +694,7 @@ template <AssetType A>
 LoanProperties
 computeLoanProperties(
    A const& asset,
-    Number const& principalOutstanding,
+    Number principalOutstanding,
    TenthBips32 interestRate,
    std::uint32_t paymentInterval,
    std::uint32_t paymentsRemaining,
@@ -723,8 +726,14 @@ computeLoanProperties(
    // over payments)
    auto const loanScale = totalValueOutstanding.exponent();

+    // Since we just figured out the loan scale, we haven't been able to
+    // validate that the principal fits in it, so to allow this function to
+    // succeed, round it here, and let the caller do the validation.
+    principalOutstanding = roundToAsset(
+        asset, principalOutstanding, loanScale, Number::to_nearest);
+
    auto const firstPaymentPrincipal = [&]() {
-        // Compute the unrounded parts for the first payment. Ensure that the
+        // Compute the parts for the first payment. Ensure that the
        // principal payment will actually change the principal.
        auto const paymentComponents = detail::computePaymentComponents(
            asset,
@@ -735,9 +744,9 @@ computeLoanProperties(
            periodicRate,
            paymentsRemaining);

-        // The rounded principal part needs to be large enough to affect the
+        // The unrounded principal part needs to be large enough to affect the
        // principal. What to do if not is left to the caller
-        return paymentComponents.roundedPrincipal;
+        return paymentComponents.rawPrincipal;
    }();

    auto const interestOwedToVault = valueMinusFee(
@@ -1086,6 +1095,10 @@ loanMakePayment(
            periodicRate,
            paymentRemainingProxy),
        serviceFee};
+    XRPL_ASSERT_PARTS(
+        periodic.roundedPrincipal > 0,
+        "ripple::loanMakePayment",
+        "regular payment pays principal");

    // -------------------------------------------------------------
    // late payment handling
@@ -1193,6 +1206,10 @@ loanMakePayment(
                periodicRate,
                paymentRemainingProxy),
            periodic.fee};
+        XRPL_ASSERT_PARTS(
+            nextPayment.roundedPrincipal > 0,
+            "ripple::loanMakePayment",
+            "additional payment pays principal");
        XRPL_ASSERT(
            nextPayment.rawInterest <= periodic.rawInterest,
            "ripple::loanMakePayment : decreasing interest");
--- a/src/xrpld/app/tx/detail/LoanSet.cpp
+++ b/src/xrpld/app/tx/detail/LoanSet.cpp
@@ -179,6 +179,21 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx)
    return normalCost + (signerCount * baseFee);
 }

+std::vector<OptionaledField<STNumber>> const&
+LoanSet::getValueFields()
+{
+    static std::vector<OptionaledField<STNumber>> const valueFields{
+        ~sfPrincipalRequested,
+        ~sfLoanOriginationFee,
+        ~sfLoanServiceFee,
+        ~sfLatePaymentFee,
+        ~sfClosePaymentFee
+        // Overpayment fee is really a rate. Don't check it here.
+    };
+
+    return valueFields;
+}
+
 TER
 LoanSet::preclaim(PreclaimContext const& ctx)
 {
@@ -221,6 +236,24 @@ LoanSet::preclaim(PreclaimContext const& ctx)
    Asset const asset = vault->at(sfAsset);
    auto const vaultPseudo = vault->at(sfAccount);

+    // Check that relevant values can be represented as the vault asset type.
+    // This check is almost duplicated in doApply, but that check is done after
+    // the overall loan scale is known. This is mostly only relevant for
+    // integral (non-IOU) types
+    {
+        for (auto const& field : getValueFields())
+        {
+            if (auto const value = tx[field];
+                value && STAmount{asset, *value} != *value)
+            {
+                JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value
+                                   << ") can not be represented as a(n) "
+                                   << to_string(asset) << ".";
+                return tecPRECISION_LOSS;
+            }
+        }
+    }
+
    if (auto const ter = canAddHolding(ctx.view, asset))
        return ter;

@@ -310,43 +343,52 @@ LoanSet::doApply()
        paymentTotal,
        TenthBips32{brokerSle->at(sfManagementFeeRate)});

+    // Check that relevant values won't lose precision. This is mostly only
+    // relevant for IOU assets.
+    {
+        for (auto const& field : getValueFields())
+        {
+            if (auto const value = tx[field];
+                value && !isRounded(vaultAsset, *value, properties.loanScale))
+            {
+                JLOG(j_.warn())
+                    << field.f->getName() << " (" << *value
+                    << ") has too much precision. Total loan value is "
+                    << properties.totalValueOutstanding << " with a scale of "
+                    << properties.loanScale;
+                return tecPRECISION_LOSS;
+            }
+        }
+    }
+
    // Guard 1: if there is no computed total interest over the life of the loan
    // for a non-zero interest rate, we cannot properly amortize the loan
    if (interestRate > TenthBips32{0} &&
        (properties.totalValueOutstanding - principalRequested) <= 0)
    {
-        // Unless this is a zero-interst loan, there must be some interest due
+        // Unless this is a zero-interest loan, there must be some interest due
        // on the loan, even if it's (measurable) dust
        JLOG(j_.warn()) << "Loan with " << interestRate
                        << "% interest has no interest due";
        return tecPRECISION_LOSS;
    }
-
-    // Guard 2: if the rounded periodic payment is large enough that the loan
-    // can't be amortized in the specified number of payments, raise an error
-    if (auto const computedPayments = roundToAsset(
-            vaultAsset,
-            properties.totalValueOutstanding /
-                roundToAsset(
-                    vaultAsset,
-                    properties.periodicPayment,
-                    properties.loanScale,
-                    Number::upward),
-            properties.loanScale,
-            Number::upward);
-        computedPayments < paymentTotal)
+    // Guard 1a: If there is any interest computed over the life of the loan,
+    // for a zero interest rate, something went sideways.
+    if (interestRate == TenthBips32{0} &&
+        (properties.totalValueOutstanding - principalRequested) > 0)
    {
-        JLOG(j_.warn()) << "Loan Periodic payment rounding will complete the "
-                           "loan in less than the specified number of payments";
-        return tecPRECISION_LOSS;
+        // LCOV_EXCL_START
+        JLOG(j_.warn()) << "Loan with 0% interest has interest due";
+        return tecINTERNAL;
+        // LCOV_EXCL_STOP
    }

-    // Guard 3: if the principal portion of the first periodic payment is too
+    // Guard 2: if the principal portion of the first periodic payment is too
    // small to be accurately represented with the given rounding mode, raise an
    // error
    if (properties.firstPaymentPrincipal <= 0)
    {
-        // Check that some reference principal is paid each period. Since
+        // Check that some true (unrounded) principal is paid each period. Since
        // the first payment pays the least principal, if it's good, they'll
        // all be good. Note that the outstanding principal is rounded, and
        // may not change right away.
@@ -354,6 +396,36 @@ LoanSet::doApply()
        return tecPRECISION_LOSS;
    }

+    // Guard 3: If the periodic payment is so small that it can't even be
+    // rounded to a representable value, then the loan can't be paid. Also,
+    // avoids dividing by 0.
+    auto const roundedPayment = roundPeriodicPayment(
+        vaultAsset, properties.periodicPayment, properties.loanScale);
+    if (roundedPayment == Number{})
+    {
+        JLOG(j_.warn()) << "Loan Periodic payment ("
+                        << properties.periodicPayment << ") rounds to 0. ";
+        return tecPRECISION_LOSS;
+    }
+
+    // Guard 4: if the rounded periodic payment is large enough that the loan
+    // can't be amortized in the specified number of payments, raise an error
+    if (auto const computedPayments = roundToAsset(
+            vaultAsset,
+            properties.totalValueOutstanding / roundedPayment,
+            properties.loanScale,
+            Number::upward);
+        computedPayments < paymentTotal)
+    {
+        JLOG(j_.warn())
+            << "Loan Periodic payment (" << properties.periodicPayment
+            << ") rounding (" << roundedPayment
+            << ") will complete the "
+               "loan in less than the specified number of payments ("
+            << computedPayments << " < " << paymentTotal << ")";
+        return tecPRECISION_LOSS;
+    }
+
    // Check that the other computed values are valid
    if (properties.interestOwedToVault < 0 ||
        properties.totalValueOutstanding <= 0 ||
@@ -366,30 +438,6 @@ LoanSet::doApply()
        // LCOV_EXCL_STOP
    }

-    // Check that relevant values won't lose precision
-    {
-        static std::map<std::string, OptionaledField<STNumber>> const
-            valueFields{
-                {"Principal Requested", ~sfPrincipalRequested},
-                {"Origination fee", ~sfLoanOriginationFee},
-                {"Service fee", ~sfLoanServiceFee},
-                {"Late Payment fee", ~sfLatePaymentFee},
-                {"Close Payment fee", ~sfClosePaymentFee}
-                // Overpayment fee is really a rate. Don't include it.
-            };
-        for (auto const& [name, field] : valueFields)
-        {
-            if (auto const value = tx[field];
-                value && !isRounded(vaultAsset, *value, properties.loanScale))
-            {
-                JLOG(j_.warn())
-                    << name << " has too much precision. Total loan value is "
-                    << properties.totalValueOutstanding << " with a scale of "
-                    << properties.loanScale;
-                return tecPRECISION_LOSS;
-            }
-        }
-    }
    auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{});

    auto const loanAssetsToBorrower = principalRequested - originationFee;
--- a/src/xrpld/app/tx/detail/LoanSet.h
+++ b/src/xrpld/app/tx/detail/LoanSet.h
@@ -48,6 +48,9 @@ public:
    static XRPAmount
    calculateBaseFee(ReadView const& view, STTx const& tx);

+    static std::vector<OptionaledField<STNumber>> const&
+    getValueFields();
+
    static TER
    preclaim(PreclaimContext const& ctx);