Fix STNumber serialization and add counterparty signing helper functions #3191
Conversation
WalkthroughSTNumber normalization/serialization logic changed (64-bit mantissa bounds and rendering); Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ripple-binary-codec/src/enums/definitions.json (1)
733-742: UpdateLoan.tsto usePreviousPaymentDueDateinstead ofPreviousPaymentDate.The field was renamed in
definitions.json(line 734) but the TypeScript model atpackages/xrpl/src/models/ledger/Loan.ts(line 108) still references the old name. Update the field name and consider whether the JSDoc comment should also be clarified to match the new semantic meaning ("when the previous payment was due" vs. "when it was made").
🤖 Fix all issues with AI agents
In `@packages/ripple-binary-codec/src/enums/definitions.json`:
- Around line 3645-3660: Update the failing test that still expects the removed
terNO_DELEGATE_PERMISSION code: in delegateSet.test.ts replace any
assertion/expectation using the string 'terNO_DELEGATE_PERMISSION' (or its
numeric -85) with the new error code 'tecNO_DELEGATE_PERMISSION' (value 198) so
the test checks for the updated code name/value returned by the transaction;
search for references in the test (the expect/assert block around the
delegateSet result) and swap the identifier accordingly.
In `@packages/ripple-binary-codec/src/types/st-number.ts`:
- Around line 98-125: The normalize() flow can still produce an exponent >
MAX_EXPONENT for huge inputs (e.g. "1e40000"); after the existing
normalization/shrink/grow loops (but before returning or doing int64-specific
shrinking), add an explicit upper-bound check that if exponent > MAX_EXPONENT
the function throws an Error (similar to the existing 'Mantissa and exponent are
too large' case). Ensure you reference the MAX_EXPONENT constant and perform
this guard in normalize() so no out-of-range exponent is returned or serialized.
🧹 Nitpick comments (1)
packages/ripple-binary-codec/test/st-number.test.ts (1)
27-31: Rename the duplicate test title for clearer output.
Two tests share the exact name “-ve normal value”, which makes failures harder to attribute. Consider a more specific label for one of them.Also applies to: 87-91
| "terADDRESS_COLLISION": -86, | ||
| "terFUNDS_SPENT": -98, | ||
| "terINSUF_FEE_B": -97, | ||
| "terLAST": -91, | ||
| "terNO_ACCOUNT": -96, | ||
| "terNO_AMM": -87, | ||
| "terNO_AUTH": -95, | ||
| "terNO_DELEGATE_PERMISSION": -85, | ||
| "terNO_LINE": -94, | ||
| "terNO_RIPPLE": -90, | ||
| "terOWNERS": -93, | ||
| "terPRE_SEQ": -92, | ||
| "terPRE_TICKET": -88, | ||
| "terQUEUED": -89, | ||
| "terRETRY": -99, | ||
|
|
||
| "tesSUCCESS": 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any references to the removed transaction result
rg -n 'terNO_DELEGATE_PERMISSION' --type=ts --type=jsRepository: XRPLF/xrpl.js
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Verify the tecNO_DELEGATE_PERMISSION location and value
rg -n 'tecNO_DELEGATE_PERMISSION' packages/ripple-binary-codec/src/enums/definitions.json -B 2 -A 2Repository: XRPLF/xrpl.js
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check the full context around line 3645-3660 to confirm the state
sed -n '3640,3665p' packages/ripple-binary-codec/src/enums/definitions.jsonRepository: XRPLF/xrpl.js
Length of output: 705
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG or any documentation about this change
find . -name "CHANGELOG*" -o -name "HISTORY*" -o -name "CHANGES*" | head -5Repository: XRPLF/xrpl.js
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Get more context around the delegateSet.test.ts references
sed -n '85,105p' packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Check if the test file has it enabled or disabled
rg -n 'describe|it\(' packages/xrpl/test/integration/transactions/delegateSet.test.ts | head -20Repository: XRPLF/xrpl.js
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check the git history of definitions.json to see when terNO_DELEGATE_PERMISSION was removed
git log --oneline -n 20 packages/ripple-binary-codec/src/enums/definitions.jsonRepository: XRPLF/xrpl.js
Length of output: 80
🏁 Script executed:
#!/bin/bash
# Check HISTORY.md for the ripple-binary-codec for context on this removal
rg -n 'terNO_DELEGATE_PERMISSION|DELEGATE_PERMISSION' packages/ripple-binary-codec/HISTORY.md -B 2 -A 2Repository: XRPLF/xrpl.js
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check when delegateSet.test.ts was added/modified
git log --oneline -n 20 packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 80
🏁 Script executed:
#!/bin/bash
# Check the full delegateSet.test.ts file to understand the test structure
wc -l packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Get the complete delegateSet.test.ts file
cat -n packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 3985
🏁 Script executed:
#!/bin/bash
# Search git history for commits mentioning DelegateSet or terNO_DELEGATE_PERMISSION
git log --all --oneline --grep='DelegateSet\|DELEGATE_PERMISSION' | head -20Repository: XRPLF/xrpl.js
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Get details about the commit that changed terNO_DELEGATE_PERMISSION to tecNO_DELEGATE_PERMISSION
git show a85a4507 --statRepository: XRPLF/xrpl.js
Length of output: 1039
🏁 Script executed:
#!/bin/bash
# Get the full diff for that commit
git show a85a4507Repository: XRPLF/xrpl.js
Length of output: 11181
🏁 Script executed:
#!/bin/bash
# Check if commit a85a4507 is in the current branch
git log --oneline --all | grep -i "a85a4507\|tecNO_DELEGATE_PERMISSION"Repository: XRPLF/xrpl.js
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check git log to understand the commit sequence
git log --oneline -n 50 packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 80
🏁 Script executed:
#!/bin/bash
# Get the exact line 97 of delegateSet.test.ts to confirm
sed -n '97p' packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Get full context around line 97
sed -n '91,98p' packages/xrpl/test/integration/transactions/delegateSet.test.ts | cat -ARepository: XRPLF/xrpl.js
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Verify the commit sequence - show what a85a4507 actually changed
git diff a85a4507~1..a85a4507 packages/xrpl/test/integration/transactions/delegateSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Check what the actual HEAD version of the file contains
git show HEAD:packages/xrpl/test/integration/transactions/delegateSet.test.ts | sed -n '91,98p'Repository: XRPLF/xrpl.js
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Check if a85a4507 is an ancestor of HEAD
git merge-base --is-ancestor a85a4507 HEAD && echo "YES, a85a4507 is ancestor of HEAD" || echo "NO, a85a4507 is NOT ancestor of HEAD"Repository: XRPLF/xrpl.js
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Show the current branch position and recent commits
git log --oneline -5Repository: XRPLF/xrpl.js
Length of output: 80
Test code in delegateSet.test.ts still references the removed terNO_DELEGATE_PERMISSION error code and will fail.
The removal of "terNO_DELEGATE_PERMISSION": -85 from definitions.json is correct and aligns with the protocol change documented in PR #3001, which updated the error code from terNO_DELEGATE_PERMISSION to tecNO_DELEGATE_PERMISSION (value 198). However, packages/xrpl/test/integration/transactions/delegateSet.test.ts lines 91-97 still expects the removed ter variant and must be updated to use 'tecNO_DELEGATE_PERMISSION' instead:
Expected change in delegateSet.test.ts
- // Expect transaction to fail with terNO_DELEGATE_PERMISSION error
+ // Expect transaction to fail with tecNO_DELEGATE_PERMISSION error
await testTransaction(
testContext.client,
accountSetTx,
bob,
undefined,
- 'terNO_DELEGATE_PERMISSION',
+ 'tecNO_DELEGATE_PERMISSION',
)🤖 Prompt for AI Agents
In `@packages/ripple-binary-codec/src/enums/definitions.json` around lines 3645 -
3660, Update the failing test that still expects the removed
terNO_DELEGATE_PERMISSION code: in delegateSet.test.ts replace any
assertion/expectation using the string 'terNO_DELEGATE_PERMISSION' (or its
numeric -85) with the new error code 'tecNO_DELEGATE_PERMISSION' (value 198) so
the test checks for the updated code name/value returned by the transaction;
search for references in the test (the expect/assert block around the
delegateSet result) and swap the identifier accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/xrpl/src/Wallet/counterpartySigner.ts`:
- Around line 1-3: The import list includes an unused symbol `decode` which
causes a TS6133 CI failure; remove `decode` from the import statement (leaving
`encode` and `stringify`) in the top of counterpartySigner.ts so the module only
imports used symbols and the TypeScript build error is resolved.
Patel-Raj11
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CONTRIBUTING.md (2)
80-80: Documentation inconsistency with the updated command.Line 80 still describes
-c 'rippled -a'as the bash command, but the actual command on line 67 now includesmkdir -p /var/lib/rippled/db/ &&beforerippled -a. Update the documentation to match.📝 Suggested fix
-* `-c 'rippled -a'` provides the bash command to start `rippled` in standalone mode from the manual entrypoint +* `-c 'mkdir -p /var/lib/rippled/db/ && rippled -a'` creates the database directory and starts `rippled` in standalone mode from the manual entrypoint
109-109: Browser Tests Docker command missing themkdirfix.The Browser Tests section still uses
-c 'rippled -a'without themkdir -p /var/lib/rippled/db/workaround that was added to the Integration Tests command on line 67 and in the CI workflow.📝 Suggested fix
-docker run -p 6006:6006 --rm -it --name rippled_standalone --volume $PWD/.ci-config:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a' +docker run -p 6006:6006 --rm -it --name rippled_standalone --volume $PWD/.ci-config:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'mkdir -p /var/lib/rippled/db/ && rippled -a'
🤖 Fix all issues with AI agents
In `@packages/ripple-binary-codec/test/st-number.test.ts`:
- Around line 168-173: The test description is misleading: the it(...) currently
says "underflow returns zero (value too small)" but the code asserts that
STNumber.from throws an Error('Underflow: value too small to represent'); update
the it(...) title to reflect that it expects an underflow error (for example
"underflow throws error (value too small to represent)") so the test name
matches the asserted behavior; keep the assertion using STNumber.from and the
exact error message 'Underflow: value too small to represent'.
In `@packages/xrpl/src/models/ledger/Loan.ts`:
- Around line 105-108: Update the JSDoc for the PreviousPaymentDueDate field to
reflect the new semantics: change the description from "when the previous
payment was made" to indicate it is the Ripple Epoch timestamp when the previous
payment was due; ensure the comment still mentions the numeric Ripple Epoch
format and optional status to match the field declaration
PreviousPaymentDueDate?: number.
🧹 Nitpick comments (3)
packages/xrpl/src/Wallet/counterpartySigner.ts (1)
176-199: Consider edge case: duplicate signers across transactions.The
validateLoanSetTransactionEquivalencefunction ensures all transactions are equivalent except for theirCounterpartySignature.Signers. However,getTransactionWithAllLoanSetCounterpartySignersusesflatMapto combine all signers without checking for duplicates.If the same signer account signs multiple input transactions, the combined result would contain duplicate entries, which could cause issues during submission.
♻️ Consider deduplicating signers by Account
function getTransactionWithAllLoanSetCounterpartySigners( transactions: LoanSet[], ): LoanSet { // Signers must be sorted in the combined transaction - See compareSigners' documentation for more details - const sortedSigners: Signer[] = transactions + const allSigners = transactions .flatMap((tx) => tx.CounterpartySignature?.Signers ?? []) + + // Deduplicate by Account + const uniqueSigners = Array.from( + new Map(allSigners.map((s) => [s.Signer.Account, s])).values(), + ) + + const sortedSigners: Signer[] = uniqueSigners .sort((signer1, signer2) => compareSigners(signer1.Signer, signer2.Signer))packages/xrpl/test/wallet/counterpartySigner.test.ts (2)
1-1: Remove outdated TODO comment.This comment appears to be a development note that is no longer needed since both tests are now implemented.
Suggested fix
-// Add one test for single signing and one test for multi-signing - import { assert } from 'chai'
14-14: Typo:singedLoanSetshould besignedLoanSet.This typo appears in both test cases (lines 14 and 88).
Suggested fix for the single sign test
- const singedLoanSet = { + const signedLoanSet = {And update all references in this test case accordingly.
| it('underflow returns zero (value too small)', () => { | ||
| // 1e-40000 has exponent -40000, which is less than MIN_EXPONENT (-32768) | ||
| expect(() => { | ||
| STNumber.from('1e-40000') | ||
| }).toThrow(new Error('Underflow: value too small to represent')) | ||
| }) |
There was a problem hiding this comment.
Rename underflow test to match the thrown error.
The test name says “returns zero” but it expects an exception.
✏️ Proposed rename
- it('underflow returns zero (value too small)', () => {
+ it('throws on underflow (value too small)', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('underflow returns zero (value too small)', () => { | |
| // 1e-40000 has exponent -40000, which is less than MIN_EXPONENT (-32768) | |
| expect(() => { | |
| STNumber.from('1e-40000') | |
| }).toThrow(new Error('Underflow: value too small to represent')) | |
| }) | |
| it('throws on underflow (value too small)', () => { | |
| // 1e-40000 has exponent -40000, which is less than MIN_EXPONENT (-32768) | |
| expect(() => { | |
| STNumber.from('1e-40000') | |
| }).toThrow(new Error('Underflow: value too small to represent')) | |
| }) |
🤖 Prompt for AI Agents
In `@packages/ripple-binary-codec/test/st-number.test.ts` around lines 168 - 173,
The test description is misleading: the it(...) currently says "underflow
returns zero (value too small)" but the code asserts that STNumber.from throws
an Error('Underflow: value too small to represent'); update the it(...) title to
reflect that it expects an underflow error (for example "underflow throws error
(value too small to represent)") so the test name matches the asserted behavior;
keep the assertion using STNumber.from and the exact error message 'Underflow:
value too small to represent'.
| /** | ||
| * The timestamp of when the previous payment was made in Ripple Epoch. | ||
| */ | ||
| PreviousPaymentDate?: number | ||
| PreviousPaymentDueDate?: number |
There was a problem hiding this comment.
Update JSDoc to match new field name semantics.
The field was renamed to PreviousPaymentDueDate, but the JSDoc still says "The timestamp of when the previous payment was made." The new name suggests it should describe when the payment was due, not when it was made.
📝 Suggested documentation fix
/**
- * The timestamp of when the previous payment was made in Ripple Epoch.
+ * The timestamp of when the previous payment was due in Ripple Epoch.
*/
PreviousPaymentDueDate?: number🤖 Prompt for AI Agents
In `@packages/xrpl/src/models/ledger/Loan.ts` around lines 105 - 108, Update the
JSDoc for the PreviousPaymentDueDate field to reflect the new semantics: change
the description from "when the previous payment was made" to indicate it is the
Ripple Epoch timestamp when the previous payment was due; ensure the comment
still mentions the numeric Ripple Epoch format and optional status to match the
field declaration PreviousPaymentDueDate?: number.
| const isNegative = mantissa < BigInt(0) | ||
|
|
||
| while (m !== BigInt(0) && m < MIN_MANTISSA && exponent > MIN_EXPONENT) { | ||
| if (m > MAX_INT64) { |
There was a problem hiding this comment.
just want to make sure, m here is already the mantissa with all trailing zeroes stripped, right?
There was a problem hiding this comment.
No, if the user enters the input as 990000000000000. Then m here would be 990000000000000. But, I think it should be changed to the following to provide more room for other digits:
- 99000 -> m = 99, exponent = 3
- 99.99000 -> m = 9999, exponent = -2
- 0.0025 -> m = 25, exponent = -4
- 0.002500 -> m = 25, exponent = -4
- 0.250025 -> m = 250025, exponent = -6
I will make that change and add some tests.
There was a problem hiding this comment.
I removed trailing zeroes from mantissa and added tests that passes (Otherwise would have failed due to m > MAX_INT64) in 9e3c80e
| if (m > MAX_INT64) { | ||
| if (exponent >= MAX_EXPONENT) { | ||
| throw new Error('Exponent overflow: value too large to represent') | ||
| } | ||
| exponent += 1 | ||
| m /= BigInt(10) | ||
| } | ||
|
|
There was a problem hiding this comment.
just want to make sure, do we really need to divide here
exponent += 1
m /= BigInt(10)
Take 99e20 as example. Initially the beginning of function, m is 99 and exponent is 20.
After m is brought to at least MIN_MANTISSA, m becomes 990,000,000,000,000,000 (18 digits) and exponent is 4.
m = 990,000,000,000,000,000 and exponent = 4 is already an acceptable/serializable input. So I don't believe we need to check if it's greater than INT64_MAX and to divide it further
There was a problem hiding this comment.
Actually after bringing m to be at-least MIN_MANTISSA, it's value becomes 9,900,000,000,000,000,000 and that is > INT64_MAX. So we are reducing mantissa by 10 and increasing exponent by 1. I added a test where the binary is taken from standalone rippled instance and json is what we get if we sign the same transaction using xrpl.js - 0e31a83
There was a problem hiding this comment.
sorry my bad, yes 9,900,000,000,000,000,000 would be m, but I believe that is still acceptable as long as it's less than MAX_MANTISSA
There was a problem hiding this comment.
what would happen if we removed this block of code? Would any tests fail?
There was a problem hiding this comment.
Ya, the transaction fails with temMALFORMED if we remove the if block.
There was a problem hiding this comment.
I just tested in rippled. A number with m = 99 and exponent = 20 would turn into m = 9,900,000,000,000,000,000 (19 digits) and exponent = 3 after normalization.
So m can actually be greater than INT64_MAX, as long as the significant digits (99) is less than INT64_MAX
There was a problem hiding this comment.
I'm wondering what removing this if-block would do, since you just pushed a change that removes trailing zeroes from the mantissa
There was a problem hiding this comment.
If we remove the if-block (do not bring back increased manitssa <= INT64_MAX) the transaction fails with temMALFORMED because the transaction that got submitted does not have properly encoded AssetsMaximum because we use writeInt64BE to write the mantissa into bytes and now it's value is beyond INT64_MAX.
const bytes = new Uint8Array(12)
writeInt64BE(bytes, normalizedMantissa, 0)
writeInt32BE(bytes, normalizedExponent, 8)
This is the transaction that essentially gets submitted to rippled and fails.
{
TransactionType: 'VaultCreate',
Sequence: 65,
Fee: '5000000',
SigningPubKey: 'ED8455056CF70CAFB7F569C4319EAD3875295B6B4DA0AD5AA57C4E1A690B0AC3E5',
TxnSignature: '32A5DD46982A7F2FEDD06CE85C95B2B5158FB26787D279FB36DF16C592FB7F576EDAA59A71D392362E255EB108B342D06B1E2010CFEA8198FB93529B6082170E',
Account: 'rHHJLEQ42gJrakkCfNebAkKFiYZPmFw6u3',
AssetsMaximum: '-8546744073709551616e3',
Asset: { currency: 'USD', issuer: 'rGertFz66AA5zJKMijfBZ8UBuWWMjLRbHo' }
}
However, if we truncate the manitssa to 99 (so that significant digits are less than INT64_MAX) and move all the trailing zeroes to exponent to just before encoding it to bytes, the transaction fails at rippled with fails local checks: Invalid signature..
Can you help in confirming that if we need to use writeUInt64BE for encoding so that we can go beyond signed INT64_MAX but stay within MAX_MANTISSA while encoding?
Also, if we keep the current logic (truncating if it goes beyond INT64_MAX) this is the bytes representation 0DBD2FC137A3000000000004 for 99e20 when encoded. Can you check if you get the same representation in rippled when a request is parsed when received using sign and submit mode of submit command?
High Level Overview of Change
Recent Number type refactor in rippled changed the following:
This PR implements above two changes and fixes toJSON (string representation) to match with rippled's behaviour.
Additional changes in PR:
signLoanSetByCounterpartyandcombineLoanSetCounterpartySignershelper functions.Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
Added unit tests that takes expected value from standalone rippled instance.