introduce hex validation for domainID in Payment, OfferCreate txn #3050
Conversation
|
""" WalkthroughThe code updates the Changes
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (8)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/offerCreate.ts(1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#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
PR: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
packages/xrpl/src/models/transactions/offerCreate.ts (11)
Learnt from: ckeshava
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: ckeshava
PR: #3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: achowdhry-ripple
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
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.
Learnt from: mvadari
PR: #2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In generateModels.js, the regex used to match SubmittableTransaction in transaction.ts is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: #2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In generateModels.js, the regex used to match SubmittableTransaction in transaction.ts is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: #2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
- Object.keys() comparison with expected set
- TypeScript interfaces with strict object literal checks
- Object sanitization by filtering to allowed keys only
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: shawnxie999
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.
Learnt from: ckeshava
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.
Learnt from: mvadari
PR: #2801
File: packages/xrpl/src/Wallet/batchSigner.ts:0-0
Timestamp: 2025-04-16T15:55:50.121Z
Learning: When using encodeForSigningBatch for Batch transactions in the XRPL.js library, the field names should use camelCase (flags, txIDs) even though the transaction object properties themselves use PascalCase (Flags, TxIDs).
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#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
PR: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
packages/xrpl/src/models/transactions/offerCreate.ts (11)
Learnt from: ckeshava
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: ckeshava
PR: #3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: achowdhry-ripple
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
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.
Learnt from: mvadari
PR: #2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In generateModels.js, the regex used to match SubmittableTransaction in transaction.ts is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: #2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In generateModels.js, the regex used to match SubmittableTransaction in transaction.ts is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: #2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
- Object.keys() comparison with expected set
- TypeScript interfaces with strict object literal checks
- Object sanitization by filtering to allowed keys only
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: shawnxie999
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.
Learnt from: ckeshava
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.
Learnt from: mvadari
PR: #2801
File: packages/xrpl/src/Wallet/batchSigner.ts:0-0
Timestamp: 2025-04-16T15:55:50.121Z
Learning: When using encodeForSigningBatch for Batch transactions in the XRPL.js library, the field names should use camelCase (flags, txIDs) even though the transaction object properties themselves use PascalCase (Flags, TxIDs).
| if ( | ||
| tx.DomainID === undefined && | ||
| tx.DomainID == null && | ||
| hasFlag(tx, OfferCreateFlags.tfHybrid, 'tfHybrid') | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
null is now silently accepted but the type still says string
Changing the condition to tx.DomainID == null allows transactions where DomainID is explicitly null to pass the “present” test.
However, the interface still declares DomainID?: string, so null is not a legal value at the type-level.
Either:
- DomainID?: string
+ DomainID?: string | nullor keep the original strict check (=== undefined) so that only the absence of the field, not an explicit null, is allowed.
Pick one for consistency between runtime behaviour and compile-time guarantees.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/xrpl/src/models/transactions/offerCreate.ts around lines 160 to 163,
the condition uses `tx.DomainID == null` which allows `null` values at runtime,
but the type definition only allows `string` or `undefined`. To fix this
inconsistency, either change the condition back to `tx.DomainID === undefined`
to only allow absence of the field, or update the type definition to include
`null` as a valid value for `DomainID`. Choose one approach to keep runtime
checks consistent with the type system.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
These checks are for JS users, not TS users - these validations are mostly unnecessary for TS users, since TS will do 90% of them. So it doesn't really matter if it perfectly matches the TS types.
There was a problem hiding this comment.
I think using == undefined would server both the purpose of correct type at type-level in TS and loose runtime checks when actual JS code executes.
There was a problem hiding this comment.
== null is the norm over == undefined
There was a problem hiding this comment.
I don't have a strong opinion over null or undefined. Both seem equivalent in this context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/snippets/src/permissionedDEX.ts (1)
241-244: Add newline at end of file.The cleanup and function execution are correct, but the file should end with a newline for consistency with coding standards.
} void PermDEXExamples() +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/snippets/src/permissionedDEX.ts(1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#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
PR: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#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.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:06:50.307Z
Learning: Domain IDs in XRPL are sha512Half values used as ledger entry indices and are always represented as 64-character uppercase hexadecimal strings (0-9, A-F), following the standard XRPL hash format for ledger object identification.
packages/xrpl/snippets/src/permissionedDEX.ts (8)
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
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.
Learnt from: shawnxie999
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.
Learnt from: shawnxie999
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.
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/src/models/ledger/PermissionedDomain.ts:3-8
Timestamp: 2025-01-08T02:08:00.476Z
Learning: In xrpl.js, the Credential interface must maintain a nested structure with a Credential property containing Issuer and CredentialType fields to mirror the structure defined in the rippled codebase.
Learnt from: shawnxie999
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.
Learnt from: mvadari
PR: #2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using as any type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Learnt from: ckeshava
PR: #2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#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
PR: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#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.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:06:50.307Z
Learning: Domain IDs in XRPL are sha512Half values used as ledger entry indices and are always represented as 64-character uppercase hexadecimal strings (0-9, A-F), following the standard XRPL hash format for ledger object identification.
packages/xrpl/snippets/src/permissionedDEX.ts (8)
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
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.
Learnt from: shawnxie999
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.
Learnt from: shawnxie999
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.
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/src/models/ledger/PermissionedDomain.ts:3-8
Timestamp: 2025-01-08T02:08:00.476Z
Learning: In xrpl.js, the Credential interface must maintain a nested structure with a Credential property containing Issuer and CredentialType fields to mirror the structure defined in the rippled codebase.
Learnt from: shawnxie999
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.
Learnt from: mvadari
PR: #2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using as any type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Learnt from: ckeshava
PR: #2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
⏰ 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). (8)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: integration (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
packages/xrpl/snippets/src/permissionedDEX.ts (9)
18-31: LGTM! Well-structured wallet setup.The function declaration and wallet creation follow proper async/await patterns and include appropriate usage context for the devnet funding.
33-43: LGTM! Proper rippling setup for IOU trading.The AccountSet transaction correctly enables default rippling on the issuer wallet, which is essential for IOU offer crossing. The comment clearly explains the necessity.
45-95: LGTM! Complete credential workflow demonstration.The credential creation and acceptance workflow is properly implemented, showing the full process for both wallets. The consistent pattern and appropriate type assertions make this a good example.
97-114: LGTM! Correct permissioned domain configuration.The PermissionedDomainSet transaction properly configures the domain with the accepted credential type. The nested credential structure matches the expected format from the rippled codebase.
116-137: LGTM! Demonstrates proper domain usage in payments.The domain retrieval and payment transaction correctly show how to use the domainID field. This aligns with the PR objectives for hex validation of domainID in Payment transactions.
139-168: LGTM! Proper trust line setup for IOU trading.The trust line establishment for both wallets is correctly implemented with appropriate limit amounts, enabling USD IOU transactions.
170-201: LGTM! Adequate IOU funding for demonstration.Both wallets are properly funded with USD IOUs from the issuer, providing sufficient balance for the upcoming offer creation and trading.
203-238: LGTM! Demonstrates domain-restricted offer creation correctly.The offer creation within the permissioned domain properly uses the tfHybrid flag and DomainID field. The complementary offers (XRP↔USD) provide a complete trading example within the domain, aligning with the PR's OfferCreate validation updates.
13-13: Import path verified: The relative import toPermissionedDomainresolves correctly frompackages/xrpl/snippets/src/permissionedDEX.tstopackages/xrpl/src/models/ledger/PermissionedDomain.ts. No changes needed.
|
@khancode Does the PR look good to you? Let me know if you would like to include anything else. |
| */ | ||
| export function isDomainID(domainID: unknown): domainID is string { | ||
| return isString(domainID) && domainID.length === _DOMAIN_ID_LENGTH | ||
| return isString(domainID) && /^[0-9a-fA-F]{64}$/u.test(domainID) |
There was a problem hiding this comment.
Use _DOMAIN_ID_LENGTH with isHex().
|
Thanks for the reviews @Patel-Raj11 @khancode |
High Level Overview of Change
This PR aims to address any pending concerns about the PermissionedDEX implementation in the xrpl.js client library.
Type of Change
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release
Did you update HISTORY.md?
Test Plan
Tests exist for the code-paths that are exercised in this PR.