Conversation
Co-authored-by: Ed Hennis <ed@ripple.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6632 +/- ##
=======================================
Coverage 81.4% 81.5%
=======================================
Files 998 998
Lines 74443 74519 +76
Branches 7563 7558 -5
=======================================
+ Hits 60632 60703 +71
- Misses 13811 13816 +5
🚀 New features to boost your workflow:
|
Replace std::optional<int> with plain int, using std::numeric_limits<int>::min() as a sentinel to detect uninitialized state. The optional was unnecessary since every code path that stores a DeltaInfo into deltas_ always sets scale.
There was a problem hiding this comment.
Five issues flagged inline: assert-disabled sentinel propagation, zero-balance trust-line scale edge case, rounding tolerance masking financial discrepancies, dead asset parameter in computeMinScale, and potential round-up at the cover-availability security boundary.
Review by Claude Opus 4.6 · Prompt: V12
…nused asset parameter The function returns the maximum (coarsest) exponent, not the minimum scale. Rename to computeCoarsestScale to accurately reflect behavior. Also remove the unused Asset parameter from the signature.
|
As I was reading this PR, I kept thinking if we can get away from repeated allocation of @ximinez Have you already tried the epsilon based comparison approach? I ran some scenarios on this with AI and asked it to check if the accuracy holds. This is a short summary of my suggestion: When considering using Performance:Each Robustness at rounding boundaries:The a = 5,050,000,000,000,000,499 × 10⁻¹⁷ //→ rounds down to ...000 × 10⁻¹⁴
b = 5,050,000,000,000,000,501 × 10⁻¹⁷ //→ rounds up to ...001 × 10⁻¹⁴
roundToAsset: round(a) ≠ round(b) //→ false invariant violation
epsilon: |a - b| = 2×10⁻¹⁷ < 10⁻¹⁴ //→ correctly equalThis is rare (requires digit 16 at a rounding tie + opposite-direction noise), but interest accrual arithmetic could produce it. One tradeoff — the > comparison: The single magnitude check vaultDeltaAssets > txAmount (deposit must not exceed tx amount) would become Summary:Across all 14 rounded comparisons in finalize (8 equality, 5 sign, 1 magnitude), epsilon is equivalent or more robust for 13, with a negligible theoretical tradeoff on 1. It's also cheaper to compute. |
pratikmankawde
left a comment
There was a problem hiding this comment.
Added a comment about a slightly diff., but performant, approach on implementing this.
Summary
Cherry-picked from: #6217
NumberandSTAmountDeltaInfostruct that tracks both the delta value and its scale (exponent), and acomputeMinScalehelper that finds the coarsest scale across all values being comparedscale()utility function inSTAmount.hthat returns the STAmount exponent for a givenNumber/AssetpaircomputeMinScaleand an end-to-end Loan test (testLoanCoverWithdrawAfterInterest) that exercises the rounding fix through a multi-step lending scenarioEdit:
The PR contains one more additional commit: Replace std::optional with plain int, using std::numeric_limits::min() as a sentinel to detect uninitialized state. The optional was unnecessary since every code path that stores a DeltaInfo into deltas_ always sets scale.
Context
When vault invariants compare balance changes across different ledger objects (vault pseudo-account, trust lines, MPTs), the values may have been computed via different arithmetic paths, resulting in
Numbervalues that are mathematically equal but differ at insignificant digits. Previously, invariant checks compared these values directly, which could cause spurious invariant failures — particularly in lending scenarios where interest accrual introduces small rounding discrepancies.The fix rounds all compared values to the coarsest (least precise) scale present among the operands before performing equality/inequality checks, ensuring that insignificant digit differences don't trigger false failures.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)