fix: Include management-fee delta in doOverpayment assertion - #7039
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7039 +/- ##
=========================================
- Coverage 82.4% 82.4% -0.0%
=========================================
Files 1011 1011
Lines 76330 76338 +8
Branches 7318 7308 -10
=========================================
Hits 62874 62874
- Misses 13456 13464 +8
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
f68fdab to
04c9f27
Compare
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
04c9f27 to
45b4e5b
Compare
45b4e5b to
d61ffa6
Compare
Two related issues on the loan overpayment path triggered spurious assertion failures. 1. computeOverpaymentComponents requires its input amount to be representable at the loan's scale (`isRounded(asset, overpayment, loanScale)`). The residual passed in by loanMakePayment was the raw `amount - totalPaid` (capped at totalValueOutstanding), which could carry more precision than loanScale allows when the borrower pays an amount with extra fractional digits. Round the residual down to loanScale before calling computeOverpaymentComponents. 2. The "interest paid agrees" assertion in doOverpayment was missing the management-fee component released during re-amortization. When a borrower overpays and the loan is re-amortized, the management fee outstanding decreases, but the original assertion formula did not account for this delta. Add `mfeeReleased` (oldMfee - newMfee) to the assertion, and introduce named intermediate variables with a comment explaining the loan-state identity behind the derivation. Add a regression test (testBugOverpayUnroundedAmount) that exercises the overpayment path with a non-zero management fee rate and an unrounded payment amount.
d61ffa6 to
00af7ba
Compare
Wrap the roundToAsset call on the overpayment residual in loanMakePayment with a view.rules().enabled(fixCleanup3_2_0) check so pre-amendment behavior is preserved. Update the testBugOverpayUnrounded Amount comment to note that the pre-amendment path can't be exercised as a regression test because the assertion fires in Debug builds and aborts the test process.
There was a problem hiding this comment.
Pull request overview
Fixes assertion failures in the loan overpayment path by ensuring overpayment residuals respect loanScale precision and by correcting the doOverpayment value/interest identity to include management-fee release during re-amortization. Adds a regression test that reproduces the unrounded residual scenario with a non-zero management fee rate.
Changes:
- Round overpayment residual down to
loanScale(behindfixCleanup3_2_0) before callingcomputeOverpaymentComponents. - Update
doOverpayment’s “interest paid agrees” assertion to includemanagementFeeOutstandingdelta released during re-amortization, with clearer intermediate terms. - Add a regression test covering overpayment with an unrounded payment amount + non-zero management fee rate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/libxrpl/ledger/helpers/LendingHelpers.cpp |
Fixes overpayment residual precision handling and corrects the overpayment value-change assertion to account for management-fee delta. |
src/test/app/Loan_test.cpp |
Adds a regression test that exercises the overpayment path with extra fractional precision and management fees enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
We recently merged a refactor to One-time setupIf you don't already have clang-tidy working in your env, on macOS: brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.Workflow on your branch (before merging develop)1. Grab the new git remote -v # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy2. Reconfigure conan/cmake so 3. Apply renames for the files modified in your PR: git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
| grep -E '\.(cpp|h|hpp|ipp)$' \
| xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called4. Build + test, then commit as a single dedicated commit: cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"5. Now merge develop: git merge upstream/developExtraRun clang-tidy once more after the merge to catch any stragglers introduced from develop's side: run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
d30bc90 to
f94bda2
Compare
Tapanito
added
the
Ready to merge
Two related issues on the loan overpayment path triggered assertion failures.
computeOverpaymentComponents requires its input amount to be representable at the loan's scale (
isRounded(asset, overpayment, loanScale)). The residual passed in by loanMakePayment was the rawamount - totalPaid(capped at totalValueOutstanding), which could carry more precision than loanScale allows when the borrower pays an amount with extra fractional digits. Round the residual down to loanScale before calling computeOverpaymentComponents.The "interest paid agrees" assertion in doOverpayment was missing the management-fee component released during re-amortization. When a borrower overpays and the loan is re-amortized, the management fee outstanding decreases, but the original assertion formula did not account for this delta. Add
mfeeReleased(oldMfee - newMfee) to the assertion, and introduce named intermediate variables with a comment explaining the loan-state identity behind the derivation.Add a regression test (testBugOverpayUnroundedAmount) that exercises the overpayment path with a non-zero management fee rate and an unrounded payment amount.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)