squashed: Expand Number to support full integer range as of 9526a30 #6192
Merged
ximinez merged 42 commits intorelease-3.1from Jan 13, 2026
Merged
squashed: Expand Number to support full integer range as of 9526a30 #6192ximinez merged 42 commits intorelease-3.1from
ximinez merged 42 commits intorelease-3.1from
Conversation
- Originally defined as uint64_t, but the testIssuerLoan() test called it with a negative number, causing an overflow to a very large number that in some circumstances could be silently cast back to an int64_t, but might not be. I believe this is UB, and we don't want to rely on that.
- Adds additional unit tests to cover math calculations. - Removes unused methods.
- add nodiscard to unimpairLoan, and check result in LoanPay - add a check to verify that issuer exists - improve LoanManage error code for dust amounts
- In overpayment results, the management fee was being calculated twice: once as part of the value change, and as part of the fees paid. Exclude it from the value change.
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
- 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.
- Adds loan state to LoanProperties. - Cleans up computeLoanProperties. - Fixes missing management fee from overpayment.
* Replace accountHolds with accountSpendable when checking for account funds in VaultDeposit and LoanBrokerCoverDeposit
- Updates or fixes a couple of things I noticed while reviewing changes to the spec. - Rename sfPreviousPaymentDate to sfPreviousPaymentDueDate. - Make the vault asset cap check added in #6124 a little more robust: 1. Check in preflight if the vault is _already_ over the limit. 2. Prevent overflow when checking with the loan value. (Subtract instead of adding, in case the values are near maxint. Both return the same result. Also add a unit test so each case is covered.
- Fixes LoanManage tfBAD_LEDGER case by capping the amount of FLC to use to cover a loss at the amount of cover available. - Check if the Vault pseudo-account is frozen in LoanBrokerSet
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-3.1 #6192 +/- ##
============================================
Coverage 79.3% 79.4%
============================================
Files 837 839 +2
Lines 71444 71716 +272
Branches 8245 8255 +10
============================================
+ Hits 56665 56913 +248
- Misses 14779 14803 +24
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…6117) - Check the trust line limit is not exceeded for a withdraw to a third party Destination account.
- Remove redundant "else" because of early return. - Add a concept "Integral64" to use in the template parameter for normalizeToRange. - Add documentation to STTakesAsset, associateAsset, and add explanation for how `STNumber` uses it. - Assert that `numberToJson` is always outside of a Transaction context. - Remove copy of `InitialFibSeqPct` and the assert in `Number::operator*=` that prompted me to add it. - Add amendment gating to `STAmount::operator=`
- Adds a mechanism for the vault owner to burn user shares when the vault is stuck. If the Vault has 0 AssetsAvailable and Total, the owner may submit a VaultClawback to reclaim the worthless fees, and thus allow the Vault to be deleted. The Amount must be left off (unless the owner is the asset issuer), specified as 0 Shares, or specified as the number of Shares held.
- Directly access member variables instead of "external view" mantissa() and exponent() in computations. - Remove some overly restrictive asserts. - Update the testLoanBrokerSetDebtMaximum test to use an MPT asset (otherwise, large limits get truncated), and more operationally safe math.
…nez/lending-3.1 * mywork/ximinez/vault-fix-3.1: VaultClawback: Burn shares of an empty vault (6120) fix: Inner batch transactions never have valid signatures (6069) fix: Reorder Batch Preflight Errors (6176)
…z/lending-3.1-number * mywork/ximinez/lending-3.1: Fix test failures from interaction between 6120 and 6133 VaultClawback: Burn shares of an empty vault (6120) fix: Inner batch transactions never have valid signatures (6069) fix: Reorder Batch Preflight Errors (6176)
ximinez
force-pushed
the
ximinez/lending-3.1-number
branch
from
9108a4c to
fce4cbe
Compare
- The previous fix removed the guarantee that addition mantissas were less than maxRep. Without that guaranee, and with large enough mantissas, the total can be greater than max uint64_t, causing an overflow. - Please please please let this be the last bug!
ximinez
force-pushed
the
ximinez/lending-3.1-number
branch
from
abd8d68 to
f5a64b2
Compare
gregtatcam
approved these changes
Jan 12, 2026
shawnxie999
reviewed
Jan 12, 2026
Comment on lines
306
to
587
| if (xm < 0) | ||
| { | ||
| xm = -xm; | ||
| xn = -1; | ||
| } | ||
| auto ym = y.mantissa(); | ||
| auto ye = y.exponent(); | ||
| int yn = 1; | ||
| if (ym < 0) | ||
| { | ||
| ym = -ym; | ||
| yn = -1; | ||
| } | ||
| "xrpl::Number::operator+=(Number) : is normal"); | ||
| // *n = negative | ||
| // *s = sign | ||
| // *m = mantissa | ||
| // *e = exponent | ||
|
|
||
| // Need to use uint128_t, because large mantissas can overflow when added | ||
| // together. | ||
| bool xn = negative_; | ||
| uint128_t xm = mantissa_; | ||
| auto xe = exponent_; | ||
|
|
||
| bool yn = y.negative_; | ||
| uint128_t ym = y.mantissa_; | ||
| auto ye = y.exponent_; |
Collaborator
There was a problem hiding this comment.
trying to understand the change. The conversions when the number is negative( xm = -xm; and ym = -ym;) are removed. Wouldn't that cause problem when one number is negative and the other is positive? (also isn't this a breaking change?)
Collaborator
There was a problem hiding this comment.
It's internal representation now so it's always positive.
Collaborator
There was a problem hiding this comment.
right, but should it be using internal rep if the two numbers have different signs?
Collaborator
There was a problem hiding this comment.
I'm not following. Why?
Collaborator
Author
There was a problem hiding this comment.
trying to understand the change. The conversions when the number is negative(
xm = -xm;andym = -ym;) are removed. Wouldn't that cause problem when one number is negative and the other is positive? (also isn't this a breaking change?)
Good question. It's safe because in both cases xn and yn are used to keep track of the sign of the original mantissas. The difference is that now I can grab them directly from the member variables instead of having to test, e.g. if (xm < 0).
So remember, before this refactoring, mantissa_ was an int64_t and mantissa() returned it unaltered.
- If
xis non-negative, thenxn == 1andxm == mantissa_and thusxm >= 0. - If
xis negative, thenxn == -1andxm == -mantissa_and thusxm > 0.
After the change, mantissa_ will never be negative. xn changes from an int to a bool.
- If
xis non-negative, thenxn == false, andxm == mantissa_, andxm >= 0. - If
xis negative, thenxn == true, and stillxm == mantissa_, andxm >= 0.
During the computation in both cases, xn may be altered depending on how x relates to y, and at the end when the result is being put back together, the old code did mantissa_ = xn * xm, and the new code just assigns xn to negative_ and xm to mantissa_.
Collaborator
There was a problem hiding this comment.
Thanks for the explanation! I looked through the function again and it makes sense
shawnxie999
approved these changes
Jan 12, 2026
Comment on lines
306
to
587
| if (xm < 0) | ||
| { | ||
| xm = -xm; | ||
| xn = -1; | ||
| } | ||
| auto ym = y.mantissa(); | ||
| auto ye = y.exponent(); | ||
| int yn = 1; | ||
| if (ym < 0) | ||
| { | ||
| ym = -ym; | ||
| yn = -1; | ||
| } | ||
| "xrpl::Number::operator+=(Number) : is normal"); | ||
| // *n = negative | ||
| // *s = sign | ||
| // *m = mantissa | ||
| // *e = exponent | ||
|
|
||
| // Need to use uint128_t, because large mantissas can overflow when added | ||
| // together. | ||
| bool xn = negative_; | ||
| uint128_t xm = mantissa_; | ||
| auto xe = exponent_; | ||
|
|
||
| bool yn = y.negative_; | ||
| uint128_t ym = y.mantissa_; | ||
| auto ye = y.exponent_; |
Collaborator
There was a problem hiding this comment.
Thanks for the explanation! I looked through the function again and it makes sense
- Reduce code duplication in LoanBrokerDelete - Reorder "canWithdraw" parameters to put "view" first - Combine accountSpendable into accountHolds - Avoid copies by taking a reference for the claw amount - Return function results directly - Fix typo for "parseLoan" in ledger_entry RPC - Improve some comments and unused variables - No need for late payment components lambda - Add explanatory comment for computeLoanProperties - Add comment linking computeRawLoanState to spec - Fix typo: TrueTotalPrincipalOutstanding - Fix missed ripple -> xrpl update - Remove unnecessary "else"s. - Clean up std::visit in accountHolds.
…/lending-3.1-number * XRPLF/ximinez/lending-3.1: Address review feedback from Lending Protocol re-review (6161)
…-3.1-number * XRPLF/release-3.1: Improve and fix bugs in Lending Protocol - XLS-66 (6156)
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
ximinez
force-pushed
the
ximinez/lending-3.1-number
branch
2 times, most recently
from
7b7075a to
e32455d
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
#6025 (
ximinez/lending-number-simple) rebased onto #6156 (ximinez/lending-3.1).Changes after the one noted will need to be manually synced across the two branches.