Conversation
…est models are implemented; TODO -- investigate tests for path_finding requests
|
""" WalkthroughThis update introduces PermissionedDEX (XLS-81d) support across the XRPL codebase. It adds domain-related fields and validation logic to offer, payment, and pathfinding models, updates configuration and changelogs, and provides comprehensive integration and unit tests to ensure correct PermissionedDEX behavior and validation. Changes
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
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (5)
packages/xrpl/HISTORY.md (1)
7-9: Link to the standard for easier navigationReaders usually jump from the changelog straight to the spec. Consider in-lining a link:
-* Support for `PermissionedDEX` (XLS-81d) +* Support for [`PermissionedDEX` (XLS-81d)](https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0081-permissioned-dex)packages/xrpl/src/models/ledger/DirectoryNode.ts (1)
49-50: Document the expected format ofDomainID
DomainIDis a ledger object ID (64-char hex). Stating that up-front helps downstream tooling and prevents accidental submission of non-canonical strings.- /** The domain that the offer directory is a part of. */ - DomainID?: string + /** + * Hex-encoded object ID of the PermissionedDomain that owns this directory + * (64-character, uppercase hexadecimal). + */ + DomainID?: stringpackages/xrpl/src/models/methods/subscribe.ts (1)
43-48: Use a branded type (or re-export) fordomainto avoid free-form stringsEverywhere else the identifier is called
DomainIDand is a 64-char hex.
Exposing it here as a plainstringsilently accepts invalid values and shifts validation cost to runtime.Minimal change:
- domain?: string + /** 64-char hex ID of a PermissionedDomain object. */ + domain?: DomainIDwhere
export type DomainID = string(or an opaque branded type) can live insrc/models/common.tsand be reused by all request/ledger interfaces.This keeps compile-time safety aligned across the PermissionedDEX surface.
packages/xrpl/test/models/offerCreate.test.ts (1)
86-86: Fix typo in test name.The test name contains a typo:
verfiesshould beverifies.Apply this diff to fix the typo:
- it(`verfies valid offerCreate within permissioned domain`, function () { + it(`verifies valid offerCreate within permissioned domain`, function () {packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (1)
179-183: Address the TODO comment about missing Owner field.The commented assertion indicates that the
DirectoryNodeobject is missing theOwnerfield. This should be tracked and resolved to ensure complete validation of the ledger object properties.Would you like me to create an issue to track adding the
Ownerfield to theDirectoryNodeinterface?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.ci-config/rippled.cfg(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/DirectoryNode.ts(1 hunks)packages/xrpl/src/models/ledger/Offer.ts(2 hunks)packages/xrpl/src/models/methods/bookOffers.ts(1 hunks)packages/xrpl/src/models/methods/pathFind.ts(2 hunks)packages/xrpl/src/models/methods/ripplePathFind.ts(1 hunks)packages/xrpl/src/models/methods/subscribe.ts(1 hunks)packages/xrpl/src/models/transactions/offerCreate.ts(5 hunks)packages/xrpl/src/models/transactions/payment.ts(3 hunks)packages/xrpl/src/models/utils/flags.ts(2 hunks)packages/xrpl/test/integration/transactions/permissionedDEX.test.ts(1 hunks)packages/xrpl/test/models/offerCreate.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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: ckeshava
PR: XRPLF/xrpl.js#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.
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.
.ci-config/rippled.cfg (3)
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: ckeshava
PR: XRPLF/xrpl.js#2812
File: packages/xrpl/test/integration/transactions/payment.test.ts:41-43
Timestamp: 2024-10-30T01:05:33.583Z
Learning: In tests involving fee calculations for XRPL, avoid using default fee amounts. If unable to retrieve fee values from the latest validated ledger, throw an error instead of defaulting to a specific amount.
packages/xrpl/src/models/ledger/DirectoryNode.ts (1)
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.
packages/xrpl/src/models/utils/flags.ts (7)
Learnt from: mvadari
PR: XRPLF/xrpl.js#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`).
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: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
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/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: 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.
packages/xrpl/src/models/methods/ripplePathFind.ts (3)
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: 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#2829
File: packages/xrpl/src/models/ledger/Credential.ts:36-36
Timestamp: 2024-12-09T07:06:47.258Z
Learning: In TypeScript interfaces, ensure that fields documented as optional are marked with the `?` operator to denote optionality.
packages/xrpl/src/models/methods/bookOffers.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/ledger/Credential.ts:36-36
Timestamp: 2024-12-09T07:06:47.258Z
Learning: In TypeScript interfaces, ensure that fields documented as optional are marked with the `?` operator to denote optionality.
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.
packages/xrpl/src/models/methods/subscribe.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/ledger/Credential.ts:36-36
Timestamp: 2024-12-09T07:06:47.258Z
Learning: In TypeScript interfaces, ensure that fields documented as optional are marked with the `?` operator to denote optionality.
packages/xrpl/test/models/offerCreate.test.ts (9)
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: 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: shawnxie999
PR: XRPLF/xrpl.js#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: 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: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
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
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: 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.
packages/xrpl/HISTORY.md (3)
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: 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.
packages/xrpl/src/models/methods/pathFind.ts (3)
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: 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#2829
File: packages/xrpl/src/models/ledger/Credential.ts:36-36
Timestamp: 2024-12-09T07:06:47.258Z
Learning: In TypeScript interfaces, ensure that fields documented as optional are marked with the `?` operator to denote optionality.
packages/xrpl/src/models/transactions/payment.ts (7)
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: 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: 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: mvadari
PR: XRPLF/xrpl.js#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`).
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
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/ledger/Offer.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/ledger/Credential.ts:36-36
Timestamp: 2024-12-09T07:06:47.258Z
Learning: In TypeScript interfaces, ensure that fields documented as optional are marked with the `?` operator to denote optionality.
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (9)
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: 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#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: 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: shawnxie999
PR: XRPLF/xrpl.js#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: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#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: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
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 (8)
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: 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: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
Learnt from: mvadari
PR: XRPLF/xrpl.js#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`).
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#2829
File: packages/xrpl/src/models/ledger/Credential.ts:36-36
Timestamp: 2024-12-09T07:06:47.258Z
Learning: In TypeScript interfaces, ensure that fields documented as optional are marked with the `?` operator to denote optionality.
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
🧬 Code Graph Analysis (4)
packages/xrpl/src/models/utils/flags.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
BaseTransaction(318-393)
packages/xrpl/test/models/offerCreate.test.ts (2)
weback.test.config.js (1)
assert(2-2)packages/xrpl/src/models/transactions/offerCreate.ts (1)
validateOfferCreate(151-192)
packages/xrpl/src/models/transactions/payment.ts (2)
packages/xrpl/src/models/transactions/offerCreate.ts (1)
validateDomainID(20-34)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/offerCreate.ts (2)
packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/transactions/index.ts (1)
OfferCreateFlags(84-84)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: browser (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
🔇 Additional comments (11)
.ci-config/rippled.cfg (1)
190-194: Ensure the runtime rippled binary actually supportsPermissionedDEXListing the amendment here is harmless only when the node binary already contains the XLS-81d implementation.
If the CI image is older, the daemon will refuse to start with “unknown amendment” and all PermissionedDEX tests will fail.Please double-check that the Docker image/tag used by the integration tests is built from a commit that includes the amendment, or pin it to the nightly containing PR #5161 before merging.
packages/xrpl/src/models/methods/ripplePathFind.ts (1)
42-46: LGTM! Clean implementation of domain filtering for path finding.The optional
domainfield is properly typed, documented, and follows TypeScript conventions. The documentation clearly explains the filtering behavior when the field is provided.packages/xrpl/src/models/utils/flags.ts (2)
12-12: LGTM! Appropriate type generalization for broader transaction support.Adding
BaseTransactionimport to support the updated function parameter type.
102-102: LGTM! Safe type generalization from Transaction to BaseTransaction.This change safely broadens the function to accept
BaseTransactioninstead ofTransaction. Since the function only usesFlagsandTransactionTypeproperties (both present inBaseTransaction), this generalization supports the new domain-specific transaction types without breaking existing functionality.packages/xrpl/src/models/methods/bookOffers.ts (1)
43-49: LGTM! Comprehensive domain filtering implementation for book offers.The optional
domainfield is well-implemented with clear documentation that explains both the filtering behavior when present and the exclusion behavior when omitted. The implementation follows TypeScript best practices.packages/xrpl/test/models/offerCreate.test.ts (1)
87-104: LGTM! Appropriate test coverage for DomainID validation.The test correctly validates that
OfferCreatetransactions withDomainIDpass validation. The test structure follows the existing pattern and uses appropriate assertions. Based on the retrieved learnings, focusing on happy path validation in the JS SDK is the right approach since complex error cases are handled in the rippled C++ implementation.packages/xrpl/src/models/methods/pathFind.ts (2)
34-38: LGTM! Consistent domain filtering implementation for path finding requests.The optional
domainfield is properly implemented with clear documentation explaining the filtering behavior. This aligns with similar additions in other method interfaces.
107-111: LGTM! Appropriate domain identification in path finding responses.The optional
domainfield in the response properly identifies which domain the orderbook pertains to. The documentation is clear and the field is correctly typed and marked as optional.packages/xrpl/src/models/transactions/payment.ts (1)
19-19: LGTM! Good reuse of validation logic.The addition of
DomainIDto the Payment transaction is well-implemented:
- Proper import and reuse of
validateDomainIDfunction for consistency- Clear documentation explaining when and how
DomainIDshould be used- Correct optional field marking and validation placement
Also applies to: 162-173, 212-214
packages/xrpl/src/models/ledger/Offer.ts (1)
5-14: Well-structured additions for PermissionedDEX support.The ledger model updates are clean and consistent:
- Good encapsulation with the
Bookinterface- Proper use of optional fields for domain-specific properties
- Clear documentation and consistent flag naming
Also applies to: 49-56, 62-63
packages/xrpl/src/models/transactions/offerCreate.ts (1)
11-34: Clean validation utility function.The
validateDomainIDfunction is well-implemented with clear validation logic and appropriate null/undefined handling. Good use of a constant for the domain ID length requirement.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (2)
52-209: Consider extracting setup logic into helper functions for better maintainability.The
beforeAllsetup is comprehensive but quite lengthy (157 lines). Consider extracting related setup steps into helper functions to improve readability and reusability.For example, extract credential setup:
async function setupCredentials(client: Client, issuer: Wallet, subjects: Wallet[]) { for (const subject of subjects) { await testTransaction(client, { TransactionType: 'CredentialCreate', Subject: subject.classicAddress, Account: issuer.classicAddress, CredentialType: stringToHex('Passport'), } as CredentialCreate, issuer) await testTransaction(client, { TransactionType: 'CredentialAccept', Account: subject.classicAddress, Issuer: issuer.classicAddress, CredentialType: stringToHex('Passport'), } as CredentialAccept, subject) } }Similar helper functions could be created for trust line setup and funding.
291-315: Enhance subscription test to validate actual functionality.The current subscription test only validates that
domainis an acceptable parameter but doesn't test the actual subscription functionality. The empty result validation is minimal.Consider adding a more comprehensive subscription test:
it('Validate subscription stream receives domain-filtered offers', async () => { const subscribePromise = new Promise((resolve) => { testContext.client.on('transaction', (tx) => { if (tx.transaction.TransactionType === 'OfferCreate' && tx.transaction.DomainID) { resolve(tx) } }) }) await testContext.client.request({ command: 'subscribe', books: [{ taker_gets: { currency: 'XRP' }, taker_pays: { currency: 'USD', issuer: testContext.wallet.classicAddress }, taker: wallet1.classicAddress, domain: pd_ledger_object.index, }], }) // Create a new offer to trigger the subscription // ... then validate the subscription response })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.ci-config/rippled.cfg(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/offerCreate.ts(5 hunks)packages/xrpl/src/models/transactions/payment.ts(3 hunks)packages/xrpl/src/models/utils/flags.ts(2 hunks)packages/xrpl/test/integration/transactions/permissionedDEX.test.ts(1 hunks)packages/xrpl/test/models/offerCreate.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .ci-config/rippled.cfg
- packages/xrpl/HISTORY.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/xrpl/src/models/utils/flags.ts
- packages/xrpl/src/models/transactions/payment.ts
- packages/xrpl/test/models/offerCreate.test.ts
- packages/xrpl/src/models/transactions/offerCreate.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: ckeshava
PR: XRPLF/xrpl.js#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.
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.
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (9)
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: 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#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: 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: shawnxie999
PR: XRPLF/xrpl.js#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: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#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: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
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
⏰ 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). (5)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: browser (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (20.x)
🔇 Additional comments (4)
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (4)
1-40: LGTM - Clean imports and setup structure.The imports are well-organized and include all necessary types and utilities for the PermissionedDEX integration tests.
213-289: LGTM - Comprehensive ledger object validation.The tests properly validate the Offer and DirectoryNode ledger objects, checking domain IDs, account ownership, and additional books. The book_offers method test correctly validates domain-scoped filtering.
317-350: LGTM - Proper ripple_path_find domain scoping test.The test correctly validates that the
ripple_path_findmethod accepts domain parameters and returns appropriate results for pathfinding within the permissioned domain.
352-386: Run Integration Tests from Monorepo RootThe
jest: not founderrors above are expected when runningnpm testinsidepackages/xrpl—the integration suite is wired up at the repo root. Please verify the “Crossing a PermissionedDEX Offer” test by running from the project root:npm run test:integration -- --testNamePattern="Crossing a PermissionedDEX Offer"Once you’ve confirmed it passes, the offer-crossing logic looks correct:
- wallet1’s hybrid offer (1000 XRP ↔ 10 USD)
- wallet2’s domain-specific counter-offer (10 USD ↔ 1000 XRP)
- Both use the same
DomainIDIf the test still fails, please check:
- Both wallets have their domain credentials accepted
- The domain is configured to allow hybrid and domain-specific offers
- The offer amounts are exactly complementary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (4)
49-217: Consider refactoring the extensive beforeAll setup for better maintainability.The beforeAll setup is quite comprehensive (169 lines) and handles multiple distinct setup phases. While the setup is correct, it could benefit from being broken down into smaller, focused helper functions to improve readability and maintainability.
Consider extracting logical groups into helper functions:
// Helper functions that could be extracted async function setupCredentials(client: Client, issuer: Wallet, subjects: Wallet[]) { // Lines 66-88: Credential creation logic } async function setupPermissionedDomain(client: Client, issuer: Wallet) { // Lines 90-106: PermissionedDomain creation } async function setupTrustLinesAndFunding(client: Client, issuer: Wallet, wallets: Wallet[]) { // Lines 139-198: Trust line and funding logic } async function createHybridOffer(client: Client, wallet: Wallet, domainID: string) { // Lines 200-216: Hybrid offer creation }This would make the beforeAll more readable and the setup logic reusable for other tests.
267-297: Enhance book_offers validation with additional assertions.The test correctly validates the basic structure of the book_offers response, but could benefit from additional assertions to ensure the domain filtering is working correctly.
Consider adding assertions to validate that the offer is correctly filtered by domain:
// Additional assertions to strengthen the test assert.equal(response.result.offers[0].Account, wallet1.classicAddress) assert.equal(response.result.offers[0].LedgerEntryType, 'Offer') assert.isTrue(response.result.offers[0].Flags & OfferCreateFlags.tfHybrid)
325-359: Enhance offer crossing validation beyond empty object checks.The test validates that offers are consumed after crossing, but could benefit from additional assertions to verify that the crossing actually occurred correctly (e.g., checking account balances).
Consider adding balance validations to ensure the crossing executed properly:
// After the crossing transaction, validate balances changed correctly const wallet1_balance = await testContext.client.request({ command: 'account_lines', account: wallet1.classicAddress, ledger_index: 'validated' }) const wallet2_balance = await testContext.client.request({ command: 'account_lines', account: wallet2.classicAddress, ledger_index: 'validated' }) // Validate that USD balances changed as expected from the crossing // wallet1 should have received 1000 XRP drops and given 10 USD // wallet2 should have given 1000 XRP drops and received 10 USDThis would provide stronger validation that the crossing actually occurred correctly rather than just checking empty offer objects.
329-341: Verify the offer crossing logic aligns with PermissionedDEX requirements.The crossing offer doesn't use the
tfHybridflag, which appears intentional since it's crossing an existing hybrid offer within the domain. However, it would be helpful to add a comment explaining this design decision.Consider adding a comment to clarify the crossing behavior:
// wallet2 "crosses" the offer within the domain // Note: The crossing offer doesn't need tfHybrid flag since it's consuming // an existing hybrid offer within the specified domain const offerCrossTx: OfferCreate = { // ... existing code }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: ckeshava
PR: XRPLF/xrpl.js#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.
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.
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (8)
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: 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#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: 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: shawnxie999
PR: XRPLF/xrpl.js#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: XRPLF/xrpl.js#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.
Learnt from: mvadari
PR: XRPLF/xrpl.js#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: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
⏰ 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). (7)
- GitHub Check: browser (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (2)
221-240: Validate Offer ledger object properties are comprehensive.The test correctly validates the basic properties of the Offer ledger object. The assertions for
LedgerEntryType,DomainID, andAccountare appropriate, and checking for the presence ofAdditionalBooksis important for hybrid offers.
299-323: Subscription test validation is appropriate for the scope.The test correctly validates that the domain parameter is accepted by the subscribe command. The comment explaining why the response content validation is skipped is helpful and aligns with the principle of not duplicating server behavior tests.
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts
Outdated
Show resolved
Hide resolved
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts
Outdated
Show resolved
Hide resolved
khancode
left a comment
khancode
left a comment
There was a problem hiding this comment.
One small comment to address then LGTM!
| }) | ||
|
|
||
| if ( | ||
| tx.DomainID === undefined && |
There was a problem hiding this comment.
| tx.DomainID === undefined && | |
| tx.DomainID != null && |
It's better to use this which covers both null and undefined values.
There was a problem hiding this comment.
Hello Omar, I'm not sure if the != null condition is equivalent to === undefined. I incorporated your suggestion, however it causes these unit test failures: https://github.com/XRPLF/xrpl.js/actions/runs/16324764374/job/46111811992?pr=3032#step:8:551
There was a problem hiding this comment.
As shown here in the Node.js interpreter, '' != null // true. An empty tx.DomainID is not evaluated truthy, in comparison with null.
I haven't looked at the internals of Javascript to be able to show the specification. I suspect it has to do with type-fiddling in == operator versus the type+value comparison in === operator.
I'll not accept your suggestion for this PR. If you come across any disparity in my observations, let me know. I can create a fix PR.
There was a problem hiding this comment.
@ckeshava oh, my mistake. I meant to write == null. This will cover both undefined and null values. Otherwise null === undefined resolves to false which is invalid.
There was a problem hiding this comment.
null === undefined resolves to false because of the difference in the operands' types. However, why is that pertinent to our circumstance?
Do you have a unit-test in mind which could slip through the cracks? I don't understand where null adds more value to the validation process, over using undefined.
There was a problem hiding this comment.
The xrpl.js client library appears to have a higher number of === undefined (49 results) as compared against === null (4 results).
I prefer the === over the == for the more stringent comparison.
There was a problem hiding this comment.
@mvadari says who?
Agreed with @khancode,
== nullis best practice - can we fix this?
As per MDN docs, undefined == null // true. I can update the code to use == (which performs implicit type conversion). This should placate your concerns.
However, I'm not convinced that validating against null is better than undefined.
There was a problem hiding this comment.
The xrpl.js client library appears to have a higher number of
=== undefined(49 results) as compared against=== null(4 results).
There are 107 results for == null. Most of the === undefined usages will be removed in #2895.
I prefer the
===over the==for the more stringent comparison.
In this case, the less stringent requirements are better, because it'll cover additional cases, like null and NaN.
There was a problem hiding this comment.
Alright, I have added the == null check in this fix-PR.
As per my understanding of the docs, both operand == null and operand == undefined perform identical functionality. Since the type of the operand is irrelevant, both of them evaluate to false. I don't see why you would prefer null over undefined.
There was a problem hiding this comment.
It's just the norm to use == null instead of == undefined. My guess is that it's because it's clearer
Co-authored-by: Omar Khan <khancodegt@gmail.com>
This reverts commit 819ab25.
|
Thank you all the reviewers for your feedback! |
| * @returns true if the domainID is a valid 64-character string, false otherwise | ||
| */ | ||
| export function isDomainID(domainID: unknown): domainID is string { | ||
| return isString(domainID) && domainID.length === _DOMAIN_ID_LENGTH |
There was a problem hiding this comment.
We should add a hexadecimal check here too.
There was a problem hiding this comment.
I have addressed your concern with code-rabbit: #3032 (comment)
This PR implements support for the [Permissioned DEX (XLS-81D)](https://github.com/XRPLF/XRPL-Standards/blob/56ba122be9f127c940fd37d433faaa2de91a4eb6/XLS-0081d-permissioned-dex/README.md). The rippled C++ PR can be [found here](https://github.com/XRPLF/rippled/pull/5404/files). This is [the relevant PR](XRPLF/xrpl.js#3032) for xrpl.js client library.
This PR implements support for the [Permissioned DEX (XLS-81D)](https://github.com/XRPLF/XRPL-Standards/blob/56ba122be9f127c940fd37d433faaa2de91a4eb6/XLS-0081d-permissioned-dex/README.md). The rippled C++ PR can be [found here](https://github.com/XRPLF/rippled/pull/5404/files). This is [the relevant PR](XRPLF/xrpl.js#3032) for xrpl.js client library.
This PR implements support for the [Permissioned DEX (XLS-81D)](https://github.com/XRPLF/XRPL-Standards/blob/56ba122be9f127c940fd37d433faaa2de91a4eb6/XLS-0081d-permissioned-dex/README.md). The rippled C++ PR can be [found here](https://github.com/XRPLF/rippled/pull/5404/files). This is [the relevant PR](XRPLF/xrpl.js#3032) for xrpl.js client library.
This PR implements support for the [Permissioned DEX (XLS-81D)](https://github.com/XRPLF/XRPL-Standards/blob/56ba122be9f127c940fd37d433faaa2de91a4eb6/XLS-0081d-permissioned-dex/README.md). The rippled C++ PR can be [found here](https://github.com/XRPLF/rippled/pull/5404/files). This is [the relevant PR](XRPLF/xrpl.js#3032) for xrpl.js client library.
5 participants
High Level Overview of Change
This PR implements support for the specification defined here.
The rippled C++ PR can be found here.
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
Appropriate tests have been included.