From af43572ee5a408d2cd92e09685eca59efc4a624d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 5 Dec 2025 21:04:53 -0500 Subject: [PATCH] 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. --- src/test/app/Loan_test.cpp | 48 ++++++++++++-------- src/xrpld/app/misc/LendingHelpers.h | 8 ++-- src/xrpld/app/misc/detail/LendingHelpers.cpp | 22 +++++++-- src/xrpld/app/tx/detail/LoanBrokerDelete.cpp | 2 +- src/xrpld/app/tx/detail/LoanDelete.cpp | 2 +- src/xrpld/app/tx/detail/LoanManage.cpp | 38 ++++++++++++---- src/xrpld/app/tx/detail/LoanManage.h | 2 + src/xrpld/app/tx/detail/LoanPay.cpp | 4 +- src/xrpld/app/tx/detail/LoanSet.cpp | 10 ++-- 9 files changed, 94 insertions(+), 42 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index b2ad47c2b4..769492dd59 100644 --- 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( diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index 559af28a47..64ae1f30e2 100644 --- 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); diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index cb75444a90..2e6b813255 100644 --- 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); }(); diff --git a/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp b/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp index f3dd781bb5..4b88f82484 100644 --- 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); diff --git a/src/xrpld/app/tx/detail/LoanDelete.cpp b/src/xrpld/app/tx/detail/LoanDelete.cpp index 87ff4d594b..fd538f3c08 100644 --- 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"); diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index da86d7b56b..c6c4b731ff 100644 --- 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; } diff --git a/src/xrpld/app/tx/detail/LoanManage.h b/src/xrpld/app/tx/detail/LoanManage.h index dde1023cad..abb76e8182 100644 --- 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 diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 43f19743a7..56e8ada99e 100644 --- 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; diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 838e774cae..b92b51d3b7 100644 --- 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 }