fix: Include management-fee delta in doOverpayment assertion (#7039)

src/libxrpl/ledger/helpers/LendingHelpers.cpp

@@ -769,9 +769,6 @@ doOverpayment(
         "xrpl::detail::doOverpayment",
         "principal change agrees");
 
-    // I'm not 100% sure the following asserts are correct. If in doubt, and
-    // everything else works, remove any that cause trouble.
-
     JLOG(j.debug()) << "valueChange: " << loanPaymentParts.valueChange
                     << ", totalValue before: " << *totalValueOutstandingProxy
                     << ", totalValue after: " << newRoundedLoanState.valueOutstanding
@@ -783,11 +780,28 @@ doOverpayment(
                     << overpaymentComponents.trackedPrincipalDelta -
             (totalValueOutstandingProxy - newRoundedLoanState.valueOutstanding);
 
+    // The valueChange returned by tryOverpayment satisfies
+    //   valueChange = (newInterestDue - oldInterestDue) + untrackedInterest.
+    // Using the loan-state identity v = p + i + m and the adjacent
+    // `principal change agrees` assertion (dp = oldP - newP), this
+    // rearranges into three independently-computable terms:
+    //
+    //   1. TVO change beyond what principal repayment alone explains:
+    //        newTVO - (oldTVO - dp)
+    //   2. Management fee released by re-amortization (positive when
+    //      mfee decreased; zero when managementFeeRate == 0):
+    //        oldMfee - newMfee
+    //   3. The overpayment's penalty interest part (= untrackedInterest
+    //      for the overpayment path; see computeOverpaymentComponents):
+    //        trackedInterestPart()
+    [[maybe_unused]] Number const tvoChange = newRoundedLoanState.valueOutstanding -
+        (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta);
+    [[maybe_unused]] Number const managementFeeReleased =
+        managementFeeOutstandingProxy - newRoundedLoanState.managementFeeDue;
+    [[maybe_unused]] Number const interestPart = overpaymentComponents.trackedInterestPart();
+
     XRPL_ASSERT_PARTS(
-        loanPaymentParts.valueChange ==
-            newRoundedLoanState.valueOutstanding -
-                (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta) +
-                overpaymentComponents.trackedInterestPart(),
+        loanPaymentParts.valueChange == tvoChange + managementFeeReleased + interestPart,
         "xrpl::detail::doOverpayment",
         "interest paid agrees");
 
@@ -2027,51 +2041,62 @@ loanMakePayment(
         // It shouldn't be possible for the overpayment to be greater than
         // totalValueOutstanding, because that would have been processed as
         // another normal payment. But cap it just in case.
-        Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy);
+        Number const overpaymentRaw =
+            std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy);
 
-        detail::ExtendedPaymentComponents const overpaymentComponents =
-            detail::computeOverpaymentComponents(
-                asset,
-                loanScale,
-                overpayment,
-                overpaymentInterestRate,
-                overpaymentFeeRate,
-                managementFeeRate);
+        bool const fixEnabled = view.rules().enabled(fixCleanup3_2_0);
+        Number const overpayment = fixEnabled
+            ? roundToAsset(asset, overpaymentRaw, loanScale, Number::RoundingMode::Downward)
+            : overpaymentRaw;
 
-        // Don't process an overpayment if the whole amount (or more!)
-        // gets eaten by fees and interest.
-        if (overpaymentComponents.trackedPrincipalDelta > 0)
+        // Post-amendment, the rounded overpayment can be zero; pre-amendment
+        // it's always positive given the surrounding guards.
+        if (!fixEnabled || overpayment > 0)
         {
-            XRPL_ASSERT_PARTS(
-                overpaymentComponents.untrackedInterest >= beast::kZero,
-                "xrpl::loanMakePayment",
-                "overpayment penalty did not reduce value of loan");
-            // Can't just use `periodicPayment` here, because it might
-            // change
-            auto periodicPaymentProxy = loan->at(sfPeriodicPayment);
-            if (auto const overResult = detail::doOverpayment(
-                    view.rules(),
+            detail::ExtendedPaymentComponents const overpaymentComponents =
+                detail::computeOverpaymentComponents(
                     asset,
                     loanScale,
-                    overpaymentComponents,
-                    totalValueOutstandingProxy,
-                    principalOutstandingProxy,
-                    managementFeeOutstandingProxy,
-                    periodicPaymentProxy,
-                    periodicRate,
-                    paymentRemainingProxy,
-                    managementFeeRate,
-                    j))
+                    overpayment,
+                    overpaymentInterestRate,
+                    overpaymentFeeRate,
+                    managementFeeRate);
+
+            // Don't process an overpayment if the whole amount (or more!)
+            // gets eaten by fees and interest.
+            if (overpaymentComponents.trackedPrincipalDelta > 0)
             {
-                totalParts += *overResult;
-            }
-            else if (overResult.error())
-            {
-                // error() will be the TER returned if a payment is not
-                // made. It will only evaluate to true if it's unsuccessful.
-                // Otherwise, tesSUCCESS means nothing was done, so
-                // continue.
-                return Unexpected(overResult.error());
+                XRPL_ASSERT_PARTS(
+                    overpaymentComponents.untrackedInterest >= beast::kZero,
+                    "xrpl::loanMakePayment",
+                    "overpayment penalty did not reduce value of loan");
+                // Can't just use `periodicPayment` here, because it might
+                // change
+                auto periodicPaymentProxy = loan->at(sfPeriodicPayment);
+                if (auto const overResult = detail::doOverpayment(
+                        view.rules(),
+                        asset,
+                        loanScale,
+                        overpaymentComponents,
+                        totalValueOutstandingProxy,
+                        principalOutstandingProxy,
+                        managementFeeOutstandingProxy,
+                        periodicPaymentProxy,
+                        periodicRate,
+                        paymentRemainingProxy,
+                        managementFeeRate,
+                        j))
+                {
+                    totalParts += *overResult;
+                }
+                else if (overResult.error())
+                {
+                    // error() will be the TER returned if a payment is not
+                    // made. It will only evaluate to true if it's unsuccessful.
+                    // Otherwise, tesSUCCESS means nothing was done, so
+                    // continue.
+                    return Unexpected(overResult.error());
+                }
             }
         }
     }

src/test/app/Loan_test.cpp

@@ -169,6 +169,10 @@ protected:
         TenthBips32 coverRateLiquidation = percentageToTenthBips(25);
         std::string data = {};  // NOLINT(readability-redundant-member-init)
         std::uint32_t flags = 0;
+        // If set, the vault is created with this sfScale value. Useful for
+        // tests that need finer loanScale to exercise rounding edge cases.
+        std::optional<std::uint8_t> vaultScale =
+            std::nullopt;  // NOLINT(readability-redundant-member-init)
 
         [[nodiscard]] Number
         maxCoveredLoanValue(Number const& currentDebt) const
@@ -522,6 +526,8 @@ protected:
         auto const coverRateMinValue = params.coverRateMin;
 
         auto [tx, vaultKeylet] = vault.create({.owner = lender, .asset = asset});
+        if (params.vaultScale)
+            tx[sfScale] = *params.vaultScale;
         env(tx);
         env.close();
         BEAST_EXPECT(env.le(vaultKeylet));
@@ -2157,21 +2163,23 @@ protected:
                 // If the loan does not allow overpayments, send a payment that
                 // tries to make an overpayment. Do not include `txFlags`, so we
                 // don't end up duplicating the next test transaction.
-                env(pay(borrower,
-                        loanKeylet.key,
-                        STAmount{broker.asset, state.periodicPayment * Number{15, -1}},
-                        tfLoanOverpayment),
-                    Fee(XRPAmount{baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}),
-                    Ter(tecNO_PERMISSION));
+                //
+                // fixCleanup3_1_3 gates tfLoanOverpayment as a valid flag:
+                // with fix on → preflight passes, apply returns tecNO_PERMISSION;
+                // with fix off → preflight rejects the flag, returns temINVALID_FLAG.
+                bool const hasFix313 = env.current()->rules().enabled(fixCleanup3_1_3);
+                STAmount const overpayAmount{broker.asset, state.periodicPayment * Number{15, -1}};
+                XRPAmount const overpayFee{
+                    baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)};
+                env(pay(borrower, loanKeylet.key, overpayAmount, tfLoanOverpayment),
+                    Fee(overpayFee),
+                    Ter(hasFix313 ? TER{tecNO_PERMISSION} : TER{temINVALID_FLAG}));
 
+                if (hasFix313)
                 {
                     env.disableFeature(fixCleanup3_1_3);
-                    env(pay(borrower,
-                            loanKeylet.key,
-                            STAmount{broker.asset, state.periodicPayment * Number{15, -1}},
-                            tfLoanOverpayment),
-                        Fee(XRPAmount{
-                            baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}),
+                    env(pay(borrower, loanKeylet.key, overpayAmount, tfLoanOverpayment),
+                        Fee(overpayFee),
                         Ter(temINVALID_FLAG));
                     env.enableFeature(fixCleanup3_1_3);
                 }
@@ -7027,7 +7035,7 @@ protected:
 
         auto credType = "credential1";
 
-        pdomain::Credentials const credentials1{{.issuer = issuer, .credType = credType}};
+        pdomain::Credentials const credentials1 = {{.issuer = issuer, .credType = credType}};
         env(pdomain::setTx(issuer, credentials1));
         env.close();
 
@@ -7572,6 +7580,74 @@ protected:
         attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS);
     }
 
+    // An overpayment whose residual amount has more precision than loanScale
+    // fires the isRounded(asset, overpayment, loanScale) assertion in
+    // computeOverpaymentComponents (and a downstream "interest paid agrees"
+    // assertion in doOverpayment). fixCleanup3_2_0 rounds the residual down
+    // to loanScale before passing it in. The pre-amendment path can't be
+    // tested here because the assertion fires in Debug builds and aborts
+    // the test process — see the PR description for context.
+    void
+    testBugOverpayUnroundedAmount()
+    {
+        testcase("bug: computeOverpaymentComponents isRounded assertion");
+
+        using namespace jtx;
+        using namespace loan;
+        Env env(*this, all_);
+
+        Account const issuer{"issuer"};
+        Account const lender{"vaultOwner"};
+        Account const borrower{"borrower"};
+
+        env.fund(XRP(1'000'000), issuer, lender, borrower);
+        env(fset(issuer, asfDefaultRipple));
+        env.close();
+
+        PrettyAsset const iouAsset = issuer["USD"];
+        STAmount const iouLimit{iouAsset.raw(), Number{9'999'999'999'999'999LL}};
+        env(trust(lender, iouLimit));
+        env(trust(borrower, iouLimit));
+        env(pay(issuer, lender, iouAsset(1'000'000)));
+        env(pay(issuer, borrower, iouAsset(1'000'000)));
+        env.close();
+
+        auto const broker = createVaultAndBroker(
+            env,
+            iouAsset,
+            lender,
+            {.vaultDeposit = 100'000,
+             .debtMax = 5000,
+             .managementFeeRate = TenthBips16{1000},
+             .vaultScale = 1});
+
+        auto const sleBroker = env.le(broker.brokerKeylet());
+        if (!BEAST_EXPECT(sleBroker))
+            return;
+        auto const loanSequence = sleBroker->at(sfLoanSequence);
+        auto const loanKeylet = keylet::loan(broker.brokerID, loanSequence);
+
+        using namespace loan;
+        env(set(borrower, broker.brokerID, Number{1000}, tfLoanOverpayment),
+            Sig(sfCounterpartySignature, lender),
+            kInterestRate(TenthBips32{10000}),
+            kPaymentTotal(12),
+            kPaymentInterval(60),
+            kGracePeriod(60),
+            kOverpaymentFee(TenthBips32{1000}),
+            kOverpaymentInterestRate(TenthBips32{1000}),
+            Fee(env.current()->fees().base * 2),
+            Ter(tesSUCCESS));
+        env.close();
+
+        // periodic * 1.5 at 15-sig-digit precision: 125.000154585042. This
+        // has too many digits to round cleanly to loanScale=-10, so the
+        // overpayment residual fails the isRounded check.
+        STAmount const payAmount{iouAsset.raw(), Number{125'000'154'585'042LL, -12}};
+        env(pay(borrower, loanKeylet.key, payAmount), Txflags(tfLoanOverpayment), Ter(tesSUCCESS));
+        env.close();
+    }
+
     // Regression for the dual-rounding fix at coarse (integer-MPT) scale.
     //
     // Loan: P=1, r=50% (50000 tenth-bips), n=3, yearly interval. The
@@ -8280,6 +8356,9 @@ protected:
         testRIPD3901();
         testBorrowerIsBroker();
         testLimitExceeded();
+        testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared();
+        testLendingCanTradeClearedNoImpact();
+        testBugOverpayUnroundedAmount();
 
         for (auto const flags : {0u, tfLoanOverpayment})
             testYieldTheftRounding(flags);
@@ -8295,11 +8374,11 @@ protected:
         testLoanPayLateFullPaymentBypassesPenalties(features);
         testLoanCoverMinimumRoundingExploit(features);
 #endif
-
         // Lifecycle
-        testSelfLoan(features);
-        testLoanSet(features);
         testLifecycle(features);
+        testLoanSet(features);
+        testDosLoanPay(features);
+        testSelfLoan(features);
 
         // Payment paths
         testWithdrawReflectsUnrealizedLoss(features);
@@ -8346,11 +8425,8 @@ public:
     run() override
     {
         runAmendmentIndependent();
-        testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared();
-        testLendingCanTradeClearedNoImpact();
-        testDosLoanPay(all_ | fixCleanup3_1_3);
-        testDosLoanPay(all_ - fixCleanup3_1_3);
-        for (auto const& features : amendmentCombinations({fixCleanup3_2_0, featureMPTokensV2}))
+        for (auto const& features :
+             amendmentCombinations({fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2}))
             runAmendmentSensitive(features);
     }
 };