fix: Parse IOU amounts above INT64_MAX without wrapping - #8190
markneonin wants to merge 1 commit into
Conversation
|
If only the most recent commit is unsigned, you can run:
If multiple commits are unsigned, you can run:
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
8573457 to
9138dae
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core amount-parsing behavior across multiple RPC entrypoints and should receive final human review despite strong test coverage.
Pull request overview
Fixes a JSON/string parsing bug in STAmount where issued-currency (IOU) values whose digit strings produce a mantissa above INT64_MAX could wrap during IOU canonicalization (due to narrowing to int64_t), potentially flipping sign and producing an unrelated amount. The change routes those out-of-range mantissas through Number so sign/mantissa stay separated and STAmount::fromNumber() performs the correct IOU-range normalization/rounding.
Changes:
- Update
amountFromJson()andamountFromString()to detect IOU mantissas aboveINT64_MAXand normalize viaNumberbefore constructingSTAmount. - Add targeted unit tests covering the regression and end-to-end
TrustSetbehavior for values written with trailing zeros. - Document the RPC/API-facing behavior change in
API-CHANGELOG.md.
File summaries
| File | Description |
|---|---|
| src/libxrpl/protocol/STAmount.cpp | Routes large IOU mantissas through Number to prevent wrap/sign-flip during parsing. |
| src/test/protocol/STAmount_test.cpp | Adds coverage for IOU JSON parsing cases around the INT64_MAX boundary and unsigned range. |
| src/test/app/TrustSet_test.cpp | Adds an integration-style test ensuring trailing-zero representations produce correct ledger limits and negative limits are rejected consistently. |
| API-CHANGELOG.md | Notes the parsing bugfix and its observable RPC impact. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
partsFromString() reports an unsigned mantissa, but an IOU stores a signed one, so a digit string above INT64_MAX wrapped into an unrelated number, and a negative value came back positive. A TrustSet limit of "-1000" was rejected with temBAD_LIMIT, while the same number written as "-1000.0000000000000000" was accepted as a limit of 844.6744073709552. Route those values through Number, which keeps the mantissa and the sign apart, and let STAmount::fromNumber() normalize them into the range an IOU can hold. Only JSON parsing changes: transactions deserialized from binary have their mantissa masked to 54 bits and range-checked, so no amendment is needed. Fixes XRPLF#8188
9138dae to
a8e40f8
Compare
High Level Overview of Change
An issued currency
valuewhose digit string exceedsINT64_MAXno longer wraps into an unrelated number. Fixes #8188.LimitAmount.valueon aTrustSet"-1000"temBAD_LIMIT"1000.0000000000000000"temBAD_LIMIT, limit-844.6744073709552tesSUCCESS, limit1000"-1000.0000000000000000"tesSUCCESS, limit844.6744073709552temBAD_LIMIT"18446744073709551615"-11844674407370955e4Context of Change
partsFromString()reports the mantissa as an unsigned 64-bit value with the sign kept apart from it.amountFromJson()passed those parts to theSTAmountconstructor, whose range check only throws forintegral()assets, so for an IOUSTAmount::iou()narrowed the mantissa with astatic_castand then negated it, wrapping the value and flipping its sign. What decided this was the digit string, not the number:"-1000"and"-1000.0000000000000000"are the same value, but only the second one has 19 digits.The fix routes such values through
Number, which keeps the mantissa and the sign apart, and letsSTAmount::fromNumber()normalize them into the range an IOU can hold. Digits beyond the 16 an IOU keeps are rounded away, as they already are for any other over-precise value —"12345678901234567"has always been accepted as1234567890123457e1.Number::Unchecked{}rather thanNormalized{}keeps the result identical under everyMantissaScale.amountFromString()shares the defect and is fixed alongside. A value too large to represent, such as"18446744073709551615e77", now throwsstd::overflow_errorwhere it previously wrapped to a small number.No amendment is needed: only JSON parsing changes. Transactions reaching consensus are deserialized from binary, where the IOU mantissa is masked to 54 bits and range-checked before
canonicalize()runs.STParsedJSONObjectis only constructed fromTransactionSign.cpp,Simulate.cppand the node-localloadLedgerpath. Same reasoning as #5990.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)amountFromJson()andamountFromString()change how they parse a digit string aboveINT64_MAX.API-CHANGELOG.mdis updated under "Bugfixes in 3.4.0".Test Plan
xrpl.protocol.STAmount,parse json (iou): the same numbers written with and without trailing zeros parse to the same amount in both signs, values spanning the unsigned range keep their sign and round to 16 digits, both sides of theINT64_MAXboundary land on the same value, and"18446744073709551616"is still rejected.xrpl.app.TrustSet:"-1500.0000000000000000"is rejected withtemBAD_LIMIT, and a new testcase reads the trust line created by"1500.0000000000000000"back out of the ledger.Every new assertion fails without the fix. Verified with
xrpld --unittest(248 suites, 0 failures),xrpl_tests, andpre-commit.