Conversation
mvadari
requested review from
Patel-Raj11 and
Copilot
and removed request for
Patel-Raj11
WalkthroughAdds integer-only guards to UInt8/16/32/64 factory methods so decimal numbers are rejected, updates HISTORY.md, adds unit tests for decimal rejection and valid integers, and changes a Lending Protocol integration test LoanSet InterestRate from 0.1 to 0. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (4)
packages/ripple-binary-codec/src/types/uint-16.ts (1)
32-40: Integer validation correctly implemented.The addition of
Number.isInteger(val)properly enforces integer-only inputs for UInt16. This correctly rejects decimals, NaN, and Infinity values.💡 Optional: Consider more descriptive error message
For improved developer experience, the error message could specify that only integers are accepted:
- throw new Error('Can not construct UInt16 with given value') + if (typeof val === 'number') { + throw new Error('Can not construct UInt16: value must be an integer') + } + throw new Error('Can not construct UInt16 with given value')This would help developers quickly understand why their decimal value was rejected.
packages/ripple-binary-codec/src/types/uint-8.ts (1)
31-39: Integer validation correctly implemented.The
Number.isInteger(val)check properly enforces integer-only inputs for UInt8, consistent with the other UInt type implementations.💡 Optional: Consider more descriptive error message
For consistency and improved developer experience, consider enhancing the error message similar to the suggestion for UInt16:
- throw new Error('Cannot construct UInt8 from given value') + if (typeof val === 'number') { + throw new Error('Cannot construct UInt8: value must be an integer') + } + throw new Error('Cannot construct UInt8 from given value')packages/ripple-binary-codec/src/types/uint-64.ts (1)
56-95: Integer validation correctly implemented.The
Number.isInteger(val)check is properly added only to the numeric path, while string and bigint handling paths remain unchanged. This ensures decimals are rejected for numeric inputs while preserving existing functionality for other input types.💡 Optional: Consider more descriptive error message
For consistency with other UInt types, consider enhancing the error message:
- throw new Error('Cannot construct UInt64 from given value') + if (typeof val === 'number') { + throw new Error('Cannot construct UInt64: value must be an integer') + } + throw new Error('Cannot construct UInt64 from given value')packages/ripple-binary-codec/src/types/uint-32.ts (1)
40-46: Integer validation correctly implemented.The
Number.isInteger(val)check properly enforces integer-only inputs for UInt32, completing the consistent implementation across all UInt types (8, 16, 32, 64).💡 Optional: Consider more descriptive error message
For consistency across all UInt types, consider enhancing the error message:
- throw new Error('Cannot construct UInt32 from given value') + if (typeof val === 'number') { + throw new Error('Cannot construct UInt32: value must be an integer') + } + throw new Error('Cannot construct UInt32 from given value')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/ripple-binary-codec/HISTORY.mdpackages/ripple-binary-codec/src/types/uint-16.tspackages/ripple-binary-codec/src/types/uint-32.tspackages/ripple-binary-codec/src/types/uint-64.tspackages/ripple-binary-codec/src/types/uint-8.tspackages/xrpl/test/integration/transactions/lendingProtocol.test.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
📚 Learning: 2024-12-06T19:25:15.376Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Applied to files:
packages/xrpl/test/integration/transactions/lendingProtocol.test.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/integration/transactions/lendingProtocol.test.ts
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/test/integration/transactions/lendingProtocol.test.ts
🔇 Additional comments (2)
packages/xrpl/test/integration/transactions/lendingProtocol.test.ts (1)
189-189: No action required—InterestRate field correctly uses decimalnumbertype and validation allows 0.The InterestRate field is typed as
number(supporting decimals), not a UInt. The validation in loanSet.ts correctly allows values from 0 to MAX_INTEREST_RATE inclusive, so settingInterestRate: 0is valid and appropriate for this test, which focuses on the loan creation workflow and multi-signing flow rather than interest rate calculations.packages/ripple-binary-codec/HISTORY.md (1)
5-6: Changelog entry is clear and well-formatted.The new entry appropriately documents the behavioral change with clear, concise language that follows existing conventions in the file. End users will understand that decimals are no longer accepted for UInt fields.
There was a problem hiding this comment.
Pull request overview
This PR adds integer validation to UInt types (UInt8, UInt16, UInt32, UInt64) in the binary codec to ensure that decimal numbers are rejected, addressing a bug where decimal values could be passed where only integers are expected.
- Added
Number.isInteger()checks to all four UInt type classes before accepting number values - Fixed a test that incorrectly passed a decimal value (0.1) for the
InterestRatefield, which is a UInt32 type - Updated HISTORY.md to document the fix
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ripple-binary-codec/src/types/uint-8.ts | Added integer check to reject decimal numbers in UInt8.from() |
| packages/ripple-binary-codec/src/types/uint-16.ts | Added integer check to reject decimal numbers in UInt16.from() |
| packages/ripple-binary-codec/src/types/uint-32.ts | Added integer check to reject decimal numbers in UInt32.from() |
| packages/ripple-binary-codec/src/types/uint-64.ts | Added integer check to reject decimal numbers in UInt64.from() |
| packages/xrpl/test/integration/transactions/lendingProtocol.test.ts | Changed InterestRate from invalid decimal (0.1) to valid integer (0) |
| packages/ripple-binary-codec/HISTORY.md | Added changelog entry documenting the fix |
Comments suppressed due to low confidence (4)
packages/ripple-binary-codec/src/types/uint-16.ts:40
- The error message "Can not construct UInt16 with given value" is not specific enough for users who pass decimal values. Consider providing a more informative error message that explicitly mentions that only integer values are accepted. Additionally, note the inconsistent spelling "Can not" versus "Cannot" used in other UInt classes - consider using "Cannot" for consistency.
if (typeof val === 'number' && Number.isInteger(val)) {
UInt16.checkUintRange(val, 0, 0xffff)
const buf = new Uint8Array(UInt16.width)
writeUInt16BE(buf, val, 0)
return new UInt16(buf)
}
throw new Error('Can not construct UInt16 with given value')
packages/ripple-binary-codec/src/types/uint-32.ts:46
- The error message "Cannot construct UInt32 from given value" is not specific enough for users who pass decimal values. Consider providing a more informative error message that explicitly mentions that only integer values are accepted. For example: "Cannot construct UInt32 from given value: value must be an integer".
if (typeof val === 'number' && Number.isInteger(val)) {
UInt32.checkUintRange(val, 0, 0xffffffff)
writeUInt32BE(buf, val, 0)
return new UInt32(buf)
}
throw new Error('Cannot construct UInt32 from given value')
packages/ripple-binary-codec/src/types/uint-64.ts:95
- The error message "Cannot construct UInt64 from given value" is not specific enough for users who pass decimal values. Consider providing a more informative error message that explicitly mentions that only integer values are accepted. For example: "Cannot construct UInt64 from given value: value must be an integer".
if (typeof val === 'number' && Number.isInteger(val)) {
if (val < 0) {
throw new Error('value must be an unsigned integer')
}
const number = BigInt(val)
const intBuf = [new Uint8Array(4), new Uint8Array(4)]
writeUInt32BE(intBuf[0], Number(number >> BigInt(32)), 0)
writeUInt32BE(intBuf[1], Number(number & BigInt(mask)), 0)
return new UInt64(concat(intBuf))
}
if (typeof val === 'string') {
if (isBase10(fieldName)) {
if (!BASE10_REGEX.test(val)) {
throw new Error(`${fieldName} ${val} is not a valid base 10 string`)
}
val = BigInt(val).toString(16) as T
}
if (typeof val === 'string' && !HEX_REGEX.test(val)) {
throw new Error(`${val} is not a valid hex-string`)
}
const strBuf = (val as string).padStart(16, '0')
buf = hexToBytes(strBuf)
return new UInt64(buf)
}
if (typeof val === 'bigint') {
const intBuf = [new Uint8Array(4), new Uint8Array(4)]
writeUInt32BE(intBuf[0], Number(Number(val >> BigInt(32))), 0)
writeUInt32BE(intBuf[1], Number(val & BigInt(mask)), 0)
return new UInt64(concat(intBuf))
}
throw new Error('Cannot construct UInt64 from given value')
packages/ripple-binary-codec/src/types/uint-8.ts:39
- The error message "Cannot construct UInt8 from given value" is not specific enough for users who pass decimal values. Consider providing a more informative error message that explicitly mentions that only integer values are accepted. For example: "Cannot construct UInt8 from given value: value must be an integer".
if (typeof val === 'number' && Number.isInteger(val)) {
UInt8.checkUintRange(val, 0, 0xff)
const buf = new Uint8Array(UInt8.width)
writeUInt8(buf, val, 0)
return new UInt8(buf)
}
throw new Error('Cannot construct UInt8 from given value')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ripple-binary-codec/test/uint.test.ts (1)
226-250: Error message wording inconsistency across UInt types.The UInt16 error message uses "Can not construct ... with given value" while UInt8, UInt32, and UInt64 use "Cannot construct ... from given value". While the tests correctly validate the current implementation, consider standardizing the error messages in the source files for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ripple-binary-codec/test/uint.test.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/ripple-binary-codec/test/uint.test.ts
📚 Learning: 2024-12-06T19:25:15.376Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Applied to files:
packages/ripple-binary-codec/test/uint.test.ts
🧬 Code graph analysis (1)
packages/ripple-binary-codec/test/uint.test.ts (5)
packages/ripple-binary-codec/src/types/uint-8.ts (1)
UInt8(52-52)packages/ripple-binary-codec/src/types/index.ts (4)
UInt8(63-63)UInt16(64-64)UInt32(65-65)UInt64(66-66)packages/ripple-binary-codec/src/types/uint-16.ts (1)
UInt16(53-53)packages/ripple-binary-codec/src/types/uint-32.ts (1)
UInt32(59-59)packages/ripple-binary-codec/src/types/uint-64.ts (1)
UInt64(136-136)
🔇 Additional comments (4)
packages/ripple-binary-codec/test/uint.test.ts (4)
1-1: LGTM!Import expanded correctly to include all four UInt types needed for the new validation tests.
200-224: LGTM!Good coverage of decimal rejection cases and valid integer boundaries (0-255) for UInt8.
252-276: LGTM!Proper decimal rejection tests and valid integer boundaries (0-4294967295) for UInt32.
278-302: LGTM!Good use of BigInt for testing large UInt64 values. The decimal rejection tests properly mirror the pattern established for other types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ripple-binary-codec/test/uint.test.ts (1)
229-229: Consider standardizing the error message format.The UInt16 error message uses "Can not" (two words) while UInt8, UInt32, and UInt64 use "Cannot" (one word). For API consistency, consider updating the UInt16 implementation to match the format used by the other types.
Current:
- UInt16:
'Can not construct UInt16 from given value'Others:
- UInt8/32/64:
'Cannot construct UInt* from given value'Note: The test correctly reflects the current implementation; the inconsistency is in the underlying type implementation files (packages/ripple-binary-codec/src/types/uint-16.ts).
Also applies to: 235-235, 241-241
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ripple-binary-codec/src/types/uint-16.tspackages/ripple-binary-codec/test/uint.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ripple-binary-codec/src/types/uint-16.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/ripple-binary-codec/test/uint.test.ts
📚 Learning: 2024-12-06T19:25:15.376Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Applied to files:
packages/ripple-binary-codec/test/uint.test.ts
📚 Learning: 2025-01-08T02:12:28.489Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Applied to files:
packages/ripple-binary-codec/test/uint.test.ts
🧬 Code graph analysis (1)
packages/ripple-binary-codec/test/uint.test.ts (5)
packages/ripple-binary-codec/src/types/uint-8.ts (1)
UInt8(52-52)packages/ripple-binary-codec/src/types/index.ts (4)
UInt8(63-63)UInt16(64-64)UInt32(65-65)UInt64(66-66)packages/ripple-binary-codec/src/types/uint-16.ts (1)
UInt16(53-53)packages/ripple-binary-codec/src/types/uint-32.ts (1)
UInt32(59-59)packages/ripple-binary-codec/src/types/uint-64.ts (1)
UInt64(136-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: unit (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: browser (24.x)
- GitHub Check: build-and-lint (24.x)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/ripple-binary-codec/test/uint.test.ts (2)
1-1: LGTM!The import additions for UInt16 and UInt32 are necessary for the new decimal validation tests.
199-303: Excellent test coverage for decimal validation!The test suite is comprehensive and well-structured, covering:
- All four UInt types (UInt8, UInt16, UInt32, UInt64)
- Multiple decimal scenarios (positive, negative, small)
- Valid integer acceptance including boundary values
- Clear, maintainable test organization
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Ready to go, waiting on CI to be fixed |
mvadari
dismissed stale reviews from achowdhry-ripple and Patel-Raj11
via
ac2e890
High Level Overview of Change
This PR ensures that all
numbers passed into the binary codec for aUIntfield is an integer, not a decimal.Context of Change
Found while debugging a test failure in #2895
Type of Change
Did you update HISTORY.md?
Test Plan
CI passes. Fixed a test.