Review feedback from @shawnxie999: even more rounding

- Round the initial total value computation upward, unless there is 0-interest. - Rename getVaultScale to getAssetsTotalScale, and convert one incorrect computation to use it. - Use adjustImpreciseNumber for LossUnrealized. - Add some logging to computeLoanProperties.
2026-07-23 23:20:33 +00:00 · 2025-12-05 21:04:53 -05:00
parent b02b700532
commit af43572ee5
9 changed files with 94 additions and 42 deletions
--- a/src/test/app/Loan_test.cpp
+++ b/src/test/app/Loan_test.cpp
@@ -141,7 +141,7 @@ protected:
            using namespace jtx;

            auto const vaultSle = env.le(keylet::vault(vaultID));
-            return getVaultScale(vaultSle);
+            return getAssetsTotalScale(vaultSle);
        }
    };

@@ -551,12 +551,15 @@ protected:
                       broker.vaultScale(env),
                       state.principalOutstanding.exponent())));
        BEAST_EXPECT(state.paymentInterval == 600);
-        BEAST_EXPECT(
-            state.totalValue ==
-            roundToAsset(
-                broker.asset,
-                state.periodicPayment * state.paymentRemaining,
-                state.loanScale));
+        {
+            NumberRoundModeGuard mg(Number::upward);
+            BEAST_EXPECT(
+                state.totalValue ==
+                roundToAsset(
+                    broker.asset,
+                    state.periodicPayment * state.paymentRemaining,
+                    state.loanScale));
+        }
        BEAST_EXPECT(
            state.managementFeeOutstanding ==
            computeManagementFee(
@@ -697,7 +700,8 @@ protected:
            interval,
            total,
            feeRate,
-            asset(brokerParams.vaultDeposit).number().exponent());
+            asset(brokerParams.vaultDeposit).number().exponent(),
+            env.journal);
        log << "Loan properties:\n"
            << "\tPrincipal: " << principal << std::endl
            << "\tInterest rate: " << interest << std::endl
@@ -1477,7 +1481,8 @@ protected:
            state.paymentInterval,
            state.paymentRemaining,
            broker.params.managementFeeRate,
-            state.loanScale);
+            state.loanScale,
+            env.journal);

        verifyLoanStatus(
            0,
@@ -2448,13 +2453,18 @@ protected:
                // Make all the payments in one transaction
                // service fee is 2
                auto const startingPayments = state.paymentRemaining;
-                auto const rawPayoff = startingPayments *
-                    (state.periodicPayment + broker.asset(2).value());
-                STAmount const payoffAmount{broker.asset, rawPayoff};
-                BEAST_EXPECT(
-                    payoffAmount ==
-                    broker.asset(Number(1024014840139457, -12)));
-                BEAST_EXPECT(payoffAmount > state.principalOutstanding);
+                STAmount const payoffAmount = [&]() {
+                    NumberRoundModeGuard mg(Number::upward);
+                    auto const rawPayoff = startingPayments *
+                        (state.periodicPayment + broker.asset(2).value());
+                    STAmount const payoffAmount{broker.asset, rawPayoff};
+                    BEAST_EXPECTS(
+                        payoffAmount ==
+                            broker.asset(Number(1024014840139457, -12)),
+                        to_string(payoffAmount));
+                    BEAST_EXPECT(payoffAmount > state.principalOutstanding);
+                    return payoffAmount;
+                }();

                singlePayment(
                    loanKeylet,
@@ -4009,7 +4019,7 @@ protected:
        createJson = env.json(createJson, sig(sfCounterpartySignature, lender));
        // Fails in preclaim because principal requested can't be
        // represented as XRP
-        env(createJson, ter(tecPRECISION_LOSS));
+        env(createJson, ter(tecPRECISION_LOSS), THISLINE);
        env.close();

        BEAST_EXPECT(!env.le(keylet));
@@ -4021,7 +4031,7 @@ protected:
        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(createJson, ter(tecPRECISION_LOSS), THISLINE);
        env.close();
    }

@@ -4996,7 +5006,7 @@ protected:
        auto const keylet = keylet::loan(broker.brokerID, loanSequence);

        createJson = env.json(createJson, sig(sfCounterpartySignature, lender));
-        env(createJson, ter(tecPRECISION_LOSS));
+        env(createJson, ter(tecPRECISION_LOSS), THISLINE);
        env.close(startDate);

        auto loanPayTx = env.json(
--- a/src/xrpld/app/misc/LendingHelpers.h
+++ b/src/xrpld/app/misc/LendingHelpers.h
@@ -179,11 +179,12 @@ adjustImpreciseNumber(
 }

 inline int
-getVaultScale(SLE::const_ref vaultSle)
+getAssetsTotalScale(SLE::const_ref vaultSle)
 {
    if (!vaultSle)
        return Number::minExponent - 1;  // LCOV_EXCL_LINE
-    return vaultSle->at(sfAssetsTotal).exponent();
+    return STAmount{vaultSle->at(sfAsset), vaultSle->at(sfAssetsTotal)}
+        .exponent();
 }

 TER
@@ -418,7 +419,8 @@ computeLoanProperties(
    std::uint32_t paymentInterval,
    std::uint32_t paymentsRemaining,
    TenthBips32 managementFeeRate,
-    std::int32_t minimumScale);
+    std::int32_t minimumScale,
+    beast::Journal j);

 bool
 isRounded(Asset const& asset, Number const& value, std::int32_t scale);
--- a/src/xrpld/app/misc/detail/LendingHelpers.cpp
+++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp
@@ -451,7 +451,8 @@ tryOverpayment(
        paymentInterval,
        paymentRemaining,
        managementFeeRate,
-        loanScale);
+        loanScale,
+        j);

    JLOG(j.debug()) << "new periodic payment: "
                    << newLoanProperties.periodicPayment
@@ -1611,7 +1612,8 @@ computeLoanProperties(
    std::uint32_t paymentInterval,
    std::uint32_t paymentsRemaining,
    TenthBips32 managementFeeRate,
-    std::int32_t minimumScale)
+    std::int32_t minimumScale,
+    beast::Journal j)
 {
    auto const periodicRate = loanPeriodicRate(interestRate, paymentInterval);
    XRPL_ASSERT(
@@ -1622,13 +1624,22 @@ computeLoanProperties(
        principalOutstanding, periodicRate, paymentsRemaining);

    auto const [totalValueOutstanding, loanScale] = [&]() {
-        NumberRoundModeGuard mg(Number::to_nearest);
+        // only round up if there should be interest
+        NumberRoundModeGuard mg(
+            periodicRate == 0 ? Number::to_nearest : Number::upward);
        // Use STAmount's internal rounding instead of roundToAsset, because
        // we're going to use this result to determine the scale for all the
        // other rounding.

        // Equation (30) from XLS-66 spec, Section A-2 Equation Glossary
        STAmount amount{asset, periodicPayment * paymentsRemaining};
+        JLOG(j.debug()) << "computeLoanProperties:" << " Principal requested: "
+                        << principalOutstanding
+                        << ". Periodic payment: " << periodicPayment
+                        << ". Payments remaining: " << paymentsRemaining
+                        << ". Raw total value: "
+                        << periodicPayment * paymentsRemaining
+                        << ". Candidate total value: " << amount << std::endl;

        // Base the loan scale on the total value, since that's going to be
        // the biggest number involved (barring unusual parameters for late,
@@ -1643,7 +1654,10 @@ computeLoanProperties(

        // We may need to truncate the total value because of the minimum
        // scale
-        amount = roundToAsset(asset, amount, loanScale, Number::to_nearest);
+        amount = roundToAsset(asset, amount, loanScale);
+
+        JLOG(j.debug()) << "computeLoanProperties: Loan scale:" << loanScale
+                        << ". Actual total value: " << amount << std::endl;

        return std::make_pair(amount, loanScale);
    }();
--- a/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp
+++ b/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp
@@ -56,7 +56,7 @@ LoanBrokerDelete::preclaim(PreclaimContext const& ctx)
        if (!vault)
            return tefINTERNAL;  // LCOV_EXCL_LINE
        auto const asset = vault->at(sfAsset);
-        auto const scale = getVaultScale(vault);
+        auto const scale = getAssetsTotalScale(vault);

        auto const rounded =
            roundToAsset(asset, debtTotal, scale, Number::towards_zero);
--- a/src/xrpld/app/tx/detail/LoanDelete.cpp
+++ b/src/xrpld/app/tx/detail/LoanDelete.cpp
@@ -115,7 +115,7 @@ LoanDelete::doApply()
                roundToAsset(
                    vaultSle->at(sfAsset),
                    debtTotalProxy,
-                    getVaultScale(vaultSle),
+                    getAssetsTotalScale(vaultSle),
                    Number::towards_zero) == beast::zero,
                "ripple::LoanDelete::doApply",
                "last loan, remaining debt rounds to zero");
--- a/src/xrpld/app/tx/detail/LoanManage.cpp
+++ b/src/xrpld/app/tx/detail/LoanManage.cpp
@@ -178,7 +178,7 @@ LoanManage::defaultLoan(
    // The vault may be at a different scale than the loan. Reduce rounding
    // errors during the accounting by rounding some of the values to that
    // scale.
-    auto const vaultScale = getVaultScale(vaultSle);
+    auto const vaultScale = getAssetsTotalScale(vaultSle);

    {
        // Decrease the Total Value of the Vault:
@@ -242,7 +242,11 @@ LoanManage::defaultLoan(
                return tefBAD_LEDGER;
                // LCOV_EXCL_STOP
            }
-            vaultLossUnrealizedProxy -= totalDefaultAmount;
+            adjustImpreciseNumber(
+                vaultLossUnrealizedProxy,
+                -totalDefaultAmount,
+                vaultAsset,
+                vaultScale);
        }
        view.update(vaultSle);
    }
@@ -250,11 +254,9 @@ LoanManage::defaultLoan(
    // Update the LoanBroker object:

    {
-        auto const asset = *vaultSle->at(sfAsset);
-
        // Decrease the Debt of the LoanBroker:
        adjustImpreciseNumber(
-            brokerDebtTotalProxy, -totalDefaultAmount, asset, vaultScale);
+            brokerDebtTotalProxy, -totalDefaultAmount, vaultAsset, vaultScale);
        // Decrease the First-Loss Capital Cover Available:
        auto coverAvailableProxy = brokerSle->at(sfCoverAvailable);
        if (coverAvailableProxy < defaultCovered)
@@ -297,13 +299,20 @@ LoanManage::impairLoan(
    ApplyView& view,
    SLE::ref loanSle,
    SLE::ref vaultSle,
+    Asset const& vaultAsset,
    beast::Journal j)
 {
    Number const lossUnrealized = owedToVault(loanSle);

+    // The vault may be at a different scale than the loan. Reduce rounding
+    // errors during the accounting by rounding some of the values to that
+    // scale.
+    auto const vaultScale = getAssetsTotalScale(vaultSle);
+
    // Update the Vault object(set "paper loss")
    auto vaultLossUnrealizedProxy = vaultSle->at(sfLossUnrealized);
-    vaultLossUnrealizedProxy += lossUnrealized;
+    adjustImpreciseNumber(
+        vaultLossUnrealizedProxy, lossUnrealized, vaultAsset, vaultScale);
    if (vaultLossUnrealizedProxy >
        vaultSle->at(sfAssetsTotal) - vaultSle->at(sfAssetsAvailable))
    {
@@ -334,8 +343,14 @@ LoanManage::unimpairLoan(
    ApplyView& view,
    SLE::ref loanSle,
    SLE::ref vaultSle,
+    Asset const& vaultAsset,
    beast::Journal j)
 {
+    // The vault may be at a different scale than the loan. Reduce rounding
+    // errors during the accounting by rounding some of the values to that
+    // scale.
+    auto const vaultScale = getAssetsTotalScale(vaultSle);
+
    // Update the Vault object(clear "paper loss")
    auto vaultLossUnrealizedProxy = vaultSle->at(sfLossUnrealized);
    Number const lossReversed = owedToVault(loanSle);
@@ -347,7 +362,10 @@ LoanManage::unimpairLoan(
        return tefBAD_LEDGER;
        // LCOV_EXCL_STOP
    }
-    vaultLossUnrealizedProxy -= lossReversed;
+    // Reverse the "paper loss"
+    adjustImpreciseNumber(
+        vaultLossUnrealizedProxy, -lossReversed, vaultAsset, vaultScale);
+
    view.update(vaultSle);

    // Update the Loan object
@@ -403,12 +421,14 @@ LoanManage::doApply()
    }
    else if (tx.isFlag(tfLoanImpair))
    {
-        if (auto const ter = impairLoan(view, loanSle, vaultSle, j_))
+        if (auto const ter =
+                impairLoan(view, loanSle, vaultSle, vaultAsset, j_))
            return ter;
    }
    else if (tx.isFlag(tfLoanUnimpair))
    {
-        if (auto const ter = unimpairLoan(view, loanSle, vaultSle, j_))
+        if (auto const ter =
+                unimpairLoan(view, loanSle, vaultSle, vaultAsset, j_))
            return ter;
    }

--- a/src/xrpld/app/tx/detail/LoanManage.h
+++ b/src/xrpld/app/tx/detail/LoanManage.h
@@ -44,6 +44,7 @@ public:
        ApplyView& view,
        SLE::ref loanSle,
        SLE::ref vaultSle,
+        Asset const& vaultAsset,
        beast::Journal j);

    /** Helper function that might be needed by other transactors
@@ -53,6 +54,7 @@ public:
        ApplyView& view,
        SLE::ref loanSle,
        SLE::ref vaultSle,
+        Asset const& vaultAsset,
        beast::Journal j);

    TER
--- a/src/xrpld/app/tx/detail/LoanPay.cpp
+++ b/src/xrpld/app/tx/detail/LoanPay.cpp
@@ -305,7 +305,7 @@ LoanPay::doApply()
    // change will be discarded.
    if (loanSle->isFlag(lsfLoanImpaired))
    {
-        LoanManage::unimpairLoan(view, loanSle, vaultSle, j_);
+        LoanManage::unimpairLoan(view, loanSle, vaultSle, asset, j_);
    }

    LoanPaymentType const paymentType = [&tx]() {
@@ -379,7 +379,7 @@ LoanPay::doApply()

    // The vault may be at a different scale than the loan. Reduce rounding
    // errors during the payment by rounding some of the values to that scale.
-    auto const vaultScale = assetsTotalProxy.value().exponent();
+    auto const vaultScale = getAssetsTotalScale(vaultSle);

    auto const totalPaidToVaultRaw =
        paymentParts->principalPaid + paymentParts->interestPaid;
--- a/src/xrpld/app/tx/detail/LoanSet.cpp
+++ b/src/xrpld/app/tx/detail/LoanSet.cpp
@@ -383,7 +383,7 @@ LoanSet::doApply()

    auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable);
    auto vaultTotalProxy = vaultSle->at(sfAssetsTotal);
-    auto const vaultScale = getVaultScale(vaultSle);
+    auto const vaultScale = getAssetsTotalScale(vaultSle);
    if (vaultAvailableProxy < principalRequested)
    {
        JLOG(j_.warn())
@@ -404,7 +404,8 @@ LoanSet::doApply()
        paymentInterval,
        paymentTotal,
        TenthBips16{brokerSle->at(sfManagementFeeRate)},
-        vaultScale);
+        vaultScale,
+        j_);

    // Check that relevant values won't lose precision. This is mostly only
    // relevant for IOU assets.
@@ -440,7 +441,10 @@ LoanSet::doApply()
    {
        // LCOV_EXCL_START
        JLOG(j_.warn())
-            << "Computed loan properties are invalid. Does not compute.";
+            << "Computed loan properties are invalid. Does not compute."
+            << " Management fee: " << properties.managementFeeOwedToBroker
+            << ". Total Value: " << properties.totalValueOutstanding
+            << ". PeriodicPayment: " << properties.periodicPayment;
        return tecINTERNAL;
        // LCOV_EXCL_STOP
    }