Add canonical "scale" computation to Number by ximinez · Pull Request #6246 · XRPLF/rippled

ximinez · 2026-01-21T01:05:27Z

High Level Overview of Change

Introduces a member function to Number for scale computations, so we can be sure it's done the same everywhere.

Context of Change

Requires a template for STAmount and Asset.
Update tests and computeMinScale from Add rounding to Vault invariants #6217 to use scale.
Convert a few other places to use "scale" correctly.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Tests (you added tests for code that already exists, or your new feature included in this PR)

- Requires a template for STAmount and Asset. - Update tests and computeMinScale from #6217 to use scale. - Convert a few other places to use "scale" correctly.

Tapanito · 2026-01-21T12:46:14Z

I don’t believe this approach addresses the underlying issue. After tracing the execution and considering the edge cases, there are three main reasons why this fix falls short.

In InvariantCheck.cpp::3441, the rounding logic effectively does nothing because of how STAmount handles normalization.

Trace Example:

minScale: -12
destinationDelta: 4999.5099009901 (Mantissa: ...0100000, Exp: -15)
vaultPseudoDeltaAssets: -4999.50990099 (Mantissa: ...0000000, Exp: -15)

When we call roundToAsset, the following occurs:

STAmount const ret{asset, value}; 
// Value converts from (Exp: -15) to (Exp: -12)
// Example: 4999509900990000000, -15 -> 4999509900990000, -12

if (ret.integral()) return ret;

return roundToScale(ret, scale); 
// We are passing an Exp -12 value to be rounded to scale -12.
// No rounding occurs, but normalization returns the original Exp -15 representation.

Even though we passed a minScale of -12, the returned values still have an exponent of -15. The internal representation is fighting the rounding logic.

As you rightfully pointed out in my PR, we cannot blindly round to the smallest scale:

Input 1: 1.000000 (Scale 0)
Input 2: 1.234567 (Scale 6)
Result: Both round to 1.

This creates an artificial equality that shouldn't exist. Rounding to the lowest common scale is too destructive for invariant checks.

Instead of trying to force the exponents to match via rounding, we should implement an epsilon difference tolerance. This acknowledges that while the numbers are represented differently in memory, they are "equal enough" within a defined precision threshold.

Tapanito · 2026-01-21T12:47:05Z

@ximinez, I added the unit test to my branch, then locally merged by branch into yours causing the invariant violation unit-test to fail.

ximinez · 2026-01-21T17:45:23Z

I don’t believe this approach addresses the underlying issue. After tracing the execution and considering the edge cases, there are three main reasons why this fix falls short.

In InvariantCheck.cpp::3441, the rounding logic effectively does nothing because of how STAmount handles normalization.

Trace Example:
* **minScale:** `-12`

* **destinationDelta:** `4999.5099009901` (Mantissa: `...0100000`, Exp: `-15`)

* **vaultPseudoDeltaAssets:** `-4999.50990099` (Mantissa: `...0000000`, Exp: `-15`)

Ah! You're taking the scale of the deltas. You need to take the scale of the original values - the before and afters that were used to compute the deltas. Those tell you how many digits of the resulting delta are actually valid for comparison.

The problem is that in the current implementation, you don't know all of the relevant values until well after you've computed all the deltas.

Right now, deltas_ is std::unordered_map<uint256, Number> deltas_ = {};. You probably need to have it be std::unordered_map<uint256, DeltaInfo> deltas_ = {}; Where DeltaInfo is a struct containing the Number and an int representing the scale of the original values.

… into ximinez/number-scale * XRPLF/tapanito/lending-vault-invariant: flyby change removing unused includes addreses review comments adds invariant test

… into ximinez/number-scale * XRPLF/tapanito/lending-vault-invariant: refactors vault invariant to use relative distance Limit reply size on `TMGetObjectByHash` queries (6110) ci: remove 'master' branch as a trigger (6234) Improve ledger_entry lookups for fee, amendments, NUNL, and hashes (5644)

Tapanito · 2026-01-22T12:08:50Z

Hi @ximinez , VaultClawback unit tests are failing with on some instances.

2026-01-22T12:05:08.6691439Z xrpld: /__w/rippled/rippled/build/modules/xrpl.libxrpl.basics/xrpl/basics/Number.h:764: std::pair<T, int> xrpl::Number::normalizeToRange(T, T) const [T = unsigned long]: Assertion `("xrpl::Number::normalizeToRange" " : " "Number is non-negative for unsigned range.") && (!negative)' failed.

Tapanito · 2026-01-22T12:16:59Z

    if (sign != 0)
-        deltas_[key] = balanceDelta * sign;
+    {
+        balanceDelta.delta *= sign;


Since we're initializing the DeltaInfo with a scale that is outside of a valid range, we should add an assertion here to ensure that scale was initialized.

Since we're initializing the DeltaInfo with a scale that is outside of a valid range, we should add an assertion here to ensure that scale was initialized.

I might change scale to optional.

Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com>

…cale

ximinez · 2026-01-29T19:12:27Z

Commits 5f8acbc through 7c499aa are from #6275.

Tapanito · 2026-02-04T13:03:04Z

+    auto const max = std::max_element(
+        numbers.begin(),
+        numbers.end(),
+        [](auto const& a, auto const& b) -> bool { return a.scale < b.scale; });


Since scale is optional, don't we need to check that it's set?

Since scale is optional, don't we need to check that it's set?

No, because an unseated optional compares as less than any seated optional, regardless of the seated value.

Tapanito · 2026-02-04T13:17:49Z

Since this PR contains commits from #6275, which is a draft, and the tapanito/lending-vault-invariants branch is destined to develop, I cannot merge it.

What is the plan for #6275? Will it be merged to develop, or should it be merged to the https://github.com/XRPLF/rippled/tree/tapanito/lending-fix-amendment branch, which will be the base branch for all changes for the lending fix amendment?

codecov · 2026-02-06T01:33:08Z

Codecov Report

❌ Patch coverage is 92.36641% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.4%. Comparing base (f8d441b) to head (882f9e3).
⚠️ Report is 1 commits behind head on tapanito/lending-vault-invariant.

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/InvariantCheck.cpp	92.1%	10 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           tapanito/lending-vault-invariant   #6246   +/-   ##
================================================================
  Coverage                              79.4%   79.4%           
================================================================
  Files                                   839     839           
  Lines                                 71633   71704   +71     
  Branches                               8231    8234    +3     
================================================================
+ Hits                                  56872   56929   +57     
- Misses                                14761   14775   +14

Files with missing lines	Coverage Δ
include/xrpl/basics/Number.h	`98.8% <100.0%> (+<0.1%)`	⬆️
include/xrpl/protocol/STAmount.h	`96.0% <ø> (ø)`
src/xrpld/app/misc/LendingHelpers.h	`94.7% <100.0%> (-0.3%)`	⬇️
src/xrpld/app/tx/detail/InvariantCheck.h	`100.0% <ø> (ø)`
...rc/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp	`96.5% <100.0%> (ø)`
src/xrpld/app/tx/detail/InvariantCheck.cpp	`92.2% <92.1%> (-0.1%)`	⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ximinez requested review from a team and Tapanito January 21, 2026 01:05

Add canonical "scale" computation to Number

7ab9709

- Requires a template for STAmount and Asset. - Update tests and computeMinScale from #6217 to use scale. - Convert a few other places to use "scale" correctly.

ximinez force-pushed the ximinez/number-scale branch from 3bff525 to 7ab9709 Compare January 21, 2026 01:06

ximinez mentioned this pull request Jan 21, 2026

Add rounding to Vault invariants #6217

Merged

13 tasks

ximinez added 3 commits January 21, 2026 12:48

Merge remote-tracking branch 'XRPLF/tapanito/lending-vault-invariant'…

e5646e4

… into ximinez/number-scale * XRPLF/tapanito/lending-vault-invariant: flyby change removing unused includes addreses review comments adds invariant test

ValidVault tracks scale of original operands alongside deltas

040bf34

Tapanito reviewed Jan 22, 2026

View reviewed changes

Comment thread src/xrpld/app/tx/detail/InvariantCheck.cpp Outdated

Tapanito approved these changes Jan 22, 2026

View reviewed changes

Tapanito self-requested a review January 22, 2026 12:08

Tapanito reviewed Jan 22, 2026

View reviewed changes

ximinez and others added 2 commits January 22, 2026 18:27

Update src/xrpld/app/tx/detail/InvariantCheck.cpp

cd4f915

Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com>

Change ValidVault::DeltaInfo::scale to an optional

accbe73

Tapanito self-requested a review January 26, 2026 18:59

Merge branch 'tapanito/lending-vault-invariant' into ximinez/number-s…

47bfd15

…cale

Tapanito reviewed Feb 4, 2026

View reviewed changes

ximinez added 2 commits February 5, 2026 20:38

Change computeMinScale to use a vector instead of an initializer_list

06bc372

Fix compile errors

882f9e3

ximinez force-pushed the ximinez/number-scale branch from f911603 to 882f9e3 Compare February 6, 2026 01:55

Tapanito merged commit 7085aa3 into tapanito/lending-vault-invariant Feb 10, 2026
25 of 27 checks passed

ximinez mentioned this pull request Feb 18, 2026

Number scale helper function #6384

Merged

1 task

ximinez deleted the ximinez/number-scale branch February 18, 2026 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add canonical "scale" computation to Number#6246

Add canonical "scale" computation to Number#6246
Tapanito merged 9 commits into
tapanito/lending-vault-invariantfrom
ximinez/number-scale

ximinez commented Jan 21, 2026

Uh oh!

Tapanito commented Jan 21, 2026 •

edited

Loading

Uh oh!

Tapanito commented Jan 21, 2026

Uh oh!

ximinez commented Jan 21, 2026

Uh oh!

Uh oh!

Tapanito commented Jan 22, 2026

Uh oh!

Tapanito Jan 22, 2026

Uh oh!

ximinez Jan 22, 2026

Uh oh!

ximinez commented Jan 29, 2026

Uh oh!

Tapanito Feb 4, 2026

Uh oh!

ximinez Feb 5, 2026

Uh oh!

Tapanito commented Feb 4, 2026

Uh oh!

codecov Bot commented Feb 6, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ximinez commented Jan 21, 2026

High Level Overview of Change

Context of Change

Type of Change

Uh oh!

Tapanito commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tapanito commented Jan 21, 2026

Uh oh!

ximinez commented Jan 21, 2026

Uh oh!

Uh oh!

Tapanito commented Jan 22, 2026

Uh oh!

Tapanito Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

ximinez Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

ximinez commented Jan 29, 2026

Uh oh!

Tapanito Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

ximinez Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Tapanito commented Feb 4, 2026

Uh oh!

codecov Bot commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Tapanito commented Jan 21, 2026 •

edited

Loading

codecov Bot commented Feb 6, 2026 •

edited

Loading