From 973a1059594266c8bbb1a6160b181c30e37b3368 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 22 Nov 2025 15:28:27 -0500 Subject: [PATCH] Review feedback: test coverage, round debt total - Add some test cases to improve coverage, and exclude some lines from coverage. - Rounds the Broker.DebtTotal to the Vault scale any time it is modified. This should reduce rounding errors. - Ensure that no debt is left for the Broker after the last loan is deleted. This ensures that any accumulated rounding errors are resolved. --- src/test/app/LoanBroker_test.cpp | 7 +++++- src/test/app/Loan_test.cpp | 6 +---- src/xrpld/app/misc/LendingHelpers.h | 25 ++++++++++++++++++++ src/xrpld/app/tx/detail/LoanBrokerDelete.cpp | 24 +++++++++++++++++++ src/xrpld/app/tx/detail/LoanBrokerSet.cpp | 6 ++--- src/xrpld/app/tx/detail/LoanDelete.cpp | 20 ++++++++++++++++ src/xrpld/app/tx/detail/LoanManage.cpp | 23 +++++++----------- src/xrpld/app/tx/detail/LoanPay.cpp | 6 ++--- src/xrpld/app/tx/detail/LoanSet.cpp | 5 ++-- 9 files changed, 93 insertions(+), 29 deletions(-) diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index a73621aa85..ca64b5e0d6 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -673,11 +673,16 @@ class LoanBroker_test : public beast::unit_test::suite coverRateMinimum(maxCoverRate), coverRateLiquidation(maxCoverRate), ter(tecNO_PERMISSION)); - // Cover: too big + // CoverMinimum: too big env(set(evan, vault.vaultID), coverRateMinimum(maxCoverRate + 1), coverRateLiquidation(maxCoverRate + 1), ter(temINVALID)); + // CoverLiquidation: too big + env(set(evan, vault.vaultID), + coverRateMinimum(maxCoverRate / 2), + coverRateLiquidation(maxCoverRate + 1), + ter(temINVALID)); // Cover: zero min, non-zero liquidation - implicit and // explicit zero values. env(set(evan, vault.vaultID), diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 584f8d6890..97078f8d95 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -141,11 +141,7 @@ protected: using namespace jtx; auto const vaultSle = env.le(keylet::vault(vaultID)); - if (!vaultSle) - // This function is not important enough to return an optional. - // Return an impossibly small number - return STAmount::cMinOffset - 1; - return vaultSle->at(sfAssetsTotal).exponent(); + return getVaultScale(vaultSle); } }; diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index 28a8ad0829..89ac0fb041 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -102,6 +102,31 @@ struct LoanState } }; +// Some values get re-rounded to the vault scale any time they are adjusted. In +// addition, they are prevented from ever going below zero. This helps avoid +// accumulated rounding errors and leftover dust amounts. +template +void +adjustImpreciseNumber( + NumberProxy value, + Number const& adjustment, + Asset const& asset, + int vaultScale) +{ + value = roundToAsset(asset, value + adjustment, vaultScale); + + if (*value < beast::zero) + value = 0; +} + +inline int +getVaultScale(SLE::const_ref vaultSle) +{ + if (!vaultSle) + return Number::minExponent - 1; // LCOV_EXCL_LINE + return vaultSle->at(sfAssetsTotal).exponent(); +} + TER checkLoanGuards( Asset const& vaultAsset, diff --git a/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp b/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp index bae806b45e..d3c4092d21 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerDelete.cpp @@ -43,6 +43,30 @@ LoanBrokerDelete::preclaim(PreclaimContext const& ctx) JLOG(ctx.j.warn()) << "LoanBrokerDelete: Owner count is " << ownerCount; return tecHAS_OBLIGATIONS; } + if (auto const debtTotal = sleBroker->at(sfDebtTotal); + debtTotal != beast::zero) + { + // Any remaining debt should have been wiped out by the last Loan + // Delete. This check is purely defensive. + auto const vault = + ctx.view.read(keylet::vault(sleBroker->at(sfVaultID))); + if (!vault) + return tefINTERNAL; // LCOV_EXCL_LINE + auto const asset = vault->at(sfAsset); + auto const scale = getVaultScale(vault); + + auto const rounded = + roundToAsset(asset, debtTotal, scale, Number::towards_zero); + + if (rounded != beast::zero) + { + // LCOV_EXCL_START + JLOG(ctx.j.warn()) << "LoanBrokerDelete: Debt total is " + << debtTotal << ", which rounds to " << rounded; + return tecHAS_OBLIGATIONS; + // LCOV_EXCL_START + } + } return tesSUCCESS; } diff --git a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp index 0e89cc6533..196d86d091 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp @@ -144,9 +144,9 @@ LoanBrokerSet::doApply() std::make_shared(keylet::loanbroker(account_, sequence)); if (auto const ter = dirLink(view, account_, broker)) - return ter; + return ter; // LCOV_EXCL_LINE if (auto const ter = dirLink(view, vaultPseudoID, broker, sfVaultNode)) - return ter; + return ter; // LCOV_EXCL_LINE adjustOwnerCount(view, owner, 2, j_); auto const ownerCount = owner->at(sfOwnerCount); @@ -156,7 +156,7 @@ LoanBrokerSet::doApply() auto maybePseudo = createPseudoAccount(view, broker->key(), sfLoanBrokerID); if (!maybePseudo) - return maybePseudo.error(); + return maybePseudo.error(); // LCOV_EXCL_LINE auto& pseudo = *maybePseudo; auto pseudoId = pseudo->at(sfAccount); diff --git a/src/xrpld/app/tx/detail/LoanDelete.cpp b/src/xrpld/app/tx/detail/LoanDelete.cpp index 084de2cf3f..87ff4d594b 100644 --- a/src/xrpld/app/tx/detail/LoanDelete.cpp +++ b/src/xrpld/app/tx/detail/LoanDelete.cpp @@ -101,7 +101,27 @@ LoanDelete::doApply() view.erase(loanSle); // Decrement the LoanBroker's owner count. + // The broker's owner count is solely for the number of outstanding loans, + // and is distinct from the broker's pseudo-account's owner count adjustOwnerCount(view, brokerSle, -1, j_); + // If there are no loans left, then any remaining debt must be forgiven, + // because there is no other way to pay it back. + if (brokerSle->at(sfOwnerCount) == 0) + { + auto debtTotalProxy = brokerSle->at(sfDebtTotal); + if (*debtTotalProxy != beast::zero) + { + XRPL_ASSERT_PARTS( + roundToAsset( + vaultSle->at(sfAsset), + debtTotalProxy, + getVaultScale(vaultSle), + Number::towards_zero) == beast::zero, + "ripple::LoanDelete::doApply", + "last loan, remaining debt rounds to zero"); + debtTotalProxy = 0; + } + } // Decrement the borrower's owner count adjustOwnerCount(view, borrowerSle, -1, j_); diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index 73754404e5..4ba8808816 100644 --- a/src/xrpld/app/tx/detail/LoanManage.cpp +++ b/src/xrpld/app/tx/detail/LoanManage.cpp @@ -171,16 +171,16 @@ LoanManage::defaultLoan( // Update the Vault object: + // 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); + { // Decrease the Total Value of the Vault: auto vaultTotalProxy = vaultSle->at(sfAssetsTotal); auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable); - // 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 = vaultTotalProxy.value().exponent(); - if (vaultTotalProxy < vaultDefaultAmount) { // LCOV_EXCL_START @@ -246,16 +246,11 @@ LoanManage::defaultLoan( // Update the LoanBroker object: { + auto const asset = *vaultSle->at(sfAsset); + // Decrease the Debt of the LoanBroker: - if (brokerDebtTotalProxy < totalDefaultAmount) - { - // LCOV_EXCL_START - JLOG(j.warn()) - << "LoanBroker debt total is less than the default amount"; - return tefBAD_LEDGER; - // LCOV_EXCL_STOP - } - brokerDebtTotalProxy -= totalDefaultAmount; + adjustImpreciseNumber( + brokerDebtTotalProxy, -totalDefaultAmount, asset, vaultScale); // Decrease the First-Loss Capital Cover Available: auto coverAvailableProxy = brokerSle->at(sfCoverAvailable); if (coverAvailableProxy < defaultCovered) diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 58fb2739d6..11f23848ab 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -410,10 +410,8 @@ LoanPay::doApply() // Despite our best efforts, it's possible for rounding errors to accumulate // in the loan broker's debt total. This is because the broker may have more // that one loan with significantly different scales. - if (totalPaidToVaultForDebt >= debtTotalProxy) - debtTotalProxy = 0; - else - debtTotalProxy -= totalPaidToVaultForDebt; + adjustImpreciseNumber( + debtTotalProxy, -totalPaidToVaultForDebt, asset, vaultScale); //------------------------------------------------------ // Vault object state changes diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 2f174ff0c8..1da06851a5 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -354,7 +354,7 @@ LoanSet::doApply() auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable); auto vaultTotalProxy = vaultSle->at(sfAssetsTotal); - auto const vaultScale = vaultTotalProxy.value().exponent(); + auto const vaultScale = getVaultScale(vaultSle); if (vaultAvailableProxy < principalRequested) { JLOG(j_.warn()) @@ -574,7 +574,8 @@ LoanSet::doApply() view.update(vaultSle); // Update the balances in the loan broker - brokerSle->at(sfDebtTotal) += newDebtDelta; + adjustImpreciseNumber( + brokerSle->at(sfDebtTotal), newDebtDelta, vaultAsset, vaultScale); // The broker's owner count is solely for the number of outstanding loans, // and is distinct from the broker's pseudo-account's owner count adjustOwnerCount(view, brokerSle, 1, j_);