Add warnings for MPTokenMetadata - XLS-89d#3041
Conversation
|
""" WalkthroughThis update adds detailed validation logic and runtime warnings for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 { Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ 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
🧪 Generate unit tests
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
|
Patel-Raj11
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/xrpl/test/fixtures/transactions/mptokenMetadata.json (1)
217-218: Invalid JSON – stray line breaks the fixtureThe solitary
218after the closing bracket makes the file unparsable JSON.
Any consumer doingJSON.parse(the test runner, tools, IDEs) will throw.} -] -218 +]
🧹 Nitpick comments (2)
packages/xrpl/HISTORY.md (1)
13-13: Minor wording nitConsider “Add warning messages for
MPTokenIssuanceCreate…” – the preposition “for” reads better than “to” in this context.-* Add warning messages to `MPTokenIssuanceCreate` transaction as per ... +* Add warning messages for `MPTokenIssuanceCreate` transaction as per ...packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
241-377: Consider refactoring for better maintainability.The validation function is quite long (137 lines). Consider breaking it into smaller, focused functions for better readability and maintainability.
Example structure:
parseMetadata(input: string): unknown- Handle JSON parsingvalidateMetadataStructure(obj: Record<string, unknown>): boolean- Validate object structurevalidateMetadataContent(metadata: MPTokenMetadata): void- Validate content rulesThis would make the code more modular and easier to test individually.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(4 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadata.json(1 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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: 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: 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: 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: 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#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/HISTORY.md (5)
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#3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
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#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.
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/transactions/index.ts (5)
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/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-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.
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.
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (13)
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: 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#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#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: 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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.970Z
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: 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: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-01-16T04:26:36.757Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (17)
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: 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#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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.970Z
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: 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: ckeshava
PR: XRPLF/xrpl.js#3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
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: 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#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: 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: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-01-16T04:26:36.757Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
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: 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#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#2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).
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/test/fixtures/transactions/mptokenMetadata.json (4)
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/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#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: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
🧬 Code Graph Analysis (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
isString(100-102)
⏰ 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). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: browser (22.x)
🔇 Additional comments (9)
packages/xrpl/test/fixtures/transactions/mptokenMetadata.json (1)
63-66: Key name typo:warningsMessagesAcross all cases the property is spelled
warningsMessages.
If the validation harness expectswarningMessages(singular + plural) or a different key, every test will falsely fail. Double-check the expected schema.No code change suggested here—verify the contract first.
Also applies to: 77-80, 91-94, 105-108, 119-122, 132-135, 153-156, 170-173, 199-205
packages/xrpl/src/models/transactions/index.ts (1)
58-63: LGTM – new exports correctly wired
MPTokenMetadataandMPTokenMetadataUrlare now available to callers; location and style match existing export patterns.packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (3)
5-5: ****
28-35: Good alignment with XLS-89d standard.The updated MPTokenMetadata structure properly demonstrates the expected format with all required fields.
143-174: Well-structured test suite for metadata warnings.The test implementation correctly uses Jest spying to verify console warnings are emitted for various metadata compliance scenarios. The dynamic test generation from fixture data is a good practice for comprehensive coverage.
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (4)
1-2: ****
20-47: Constants correctly implement XLS-89d validation rules.The validation constants properly define the required fields, asset classifications, and ticker format according to the standard.
237-237: Appropriate placement of warning logic.The metadata warning function is correctly called after all validation checks.
382-396: Clean helper function implementation.The URL structure validation helper is well-implemented with proper null checks and type validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (2)
304-314: Improve validation strictness for additional_info object type.The current validation for
additional_infoallows any object type, which may be too permissive. Consider checking for plain objects only to avoid unexpected behavior with Date, RegExp, or other object types.if ( obj.additional_info != null && !( isString(obj.additional_info) || - (typeof obj.additional_info === 'object' && - !Array.isArray(obj.additional_info)) + (typeof obj.additional_info === 'object' && + !Array.isArray(obj.additional_info) && + obj.additional_info.constructor === Object) ) ) {
250-377: Consider refactoring to improve maintainability.The logWarningsForMPTMetadata function is quite long and complex. Consider extracting validation logic into smaller, focused functions to improve readability and maintainability.
For example, you could extract field validation:
function validateRequiredFields(obj: Record<string, unknown>): string[] { return MPT_META_REQUIRED_FIELDS.filter(field => !isString(obj[field])) } function validateOptionalFields(obj: Record<string, unknown>): string[] { const warnings: string[] = [] if (obj.desc != null && !isString(obj.desc)) { warnings.push('desc must be a string.') } // ... other optional field validations return warnings }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: 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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.970Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: shawnxie999
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#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#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: 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: 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: 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/MPTokenIssuanceCreate.ts (21)
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: 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: 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: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.970Z
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#3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
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: 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: 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/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: mvadari
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).
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: 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: 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: 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/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
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: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-01-16T04:26:36.757Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
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
🧬 Code Graph Analysis (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
isString(100-102)
⏰ 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: unit (20.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (5)
1-2: Import statement and ESLint directive look good.The hexToString import is properly utilized for decoding hex-encoded metadata, and the max-lines disable is justified given the extensive validation logic.
20-47: Constants are well-defined and align with XLS-89d standard.The TICKER_REGEX, required fields, and asset class constants provide clear validation boundaries. The regex pattern correctly enforces uppercase alphanumeric characters with a 6-character limit.
102-124: Interface definitions are comprehensive and correctly structured.Both MPTokenMetadata and MPTokenMetadataUrl interfaces properly define the expected structure. The optional fields are correctly marked with
?operator.
237-237: Integration of metadata validation is properly placed.The call to logWarningsForMPTMetadata is correctly positioned after all other validation checks, ensuring warnings are only logged for otherwise valid transactions.
382-396: Helper function is well-implemented.The isValidMPTokenMetadataUrlStructure function correctly validates the required structure for URL objects with proper type checking.
ckeshava
left a comment
ckeshava
left a comment
There was a problem hiding this comment.
Hello Raj, this is good work.
- What is the decision on the
MPTIssuanceCreatetransactions whose meta-data exceed the expected size i.e. 1024 bytes? In this version of the PR, rippled will reject them with an appropriate error-code. Is that the expected behavior?
Here is a verbatim snippet from the final design choices for the XLS-89 spec:
If the metadata is too large, developers will receive a pre-submission warning before anything is sent to rippled
- This solution is repetitive. You need to implement this solution in all the client libraries. Despite all this effort, if a user submits a transaction directly to a rippled node, they are not supplied with any warnings at all. If companies/organizations use non-SDK pipelines, they will not be notified of the XLS-89d structure. Do you have any thoughts on this issue?
For reference, here is the associated validation in rippled.
-
Users can set the MPTokenMetadata in the
VaultCreatetransaction (Single-Asset-Vault amendment) too. Are you planning to add this validation into that transaction too? -
I'm concerned about the number of console warnings that will be printed due to this PR -- Can a user choose to opt out of adhering to the XLS-89D standard? This comment from @sappenin is helpful in detecting if users are trying to adhere to XLS-89.
-
Can you include all of this validation inside an exportable method? A method with the one-of-the-below signature is helpful.
Otherwise, this logic will need to be repeated across explorer and indexers in the XRPL ecosystem. If users can import this method, that will be very helpful.
validateMPTokenMetadata(metadata: unknown): metadata is MPTokenMetadata { ... }
isMetadata_XLS_89_compatible(metadata: unknown): boolean { ... }
isMPTMetadataDiscoverable(metadata: unknown): boolean { ... }
|
@ckeshava Thanks for the detailed feedback:
Yes, it's expected behaviour for rippled to reject it. Client library will just warn the user.
I brought this same point to Julian and it was concluded that if the project/company is really serious about their project, they will take the warnings seriously if they are using our SDKs. If they are directly submitting the transactions to rippled, then we can't do much and I don't have any other solution in mind but to have rippled enforce such metadata structure and error out if the transaction does not adhere to it. Since MPTokensV1 is out for voting, I think we can have another amendment to enforce such metadata structure.
Thanks for highlighting this. I overlooked it. To me it makes sense to give warning messages here as well.
Yes, there would be many warnings for
I will move these validations to a common place now since |
Where have you included the warning in the xrpl-py client library, appropos the size of the meta-data field?
Okay, let me know once the
Hmm apart from implementing this inside rippled and David Fuelling's suggestion, I don't have any other ideas.
Are console warnings the best way to inform the user? If the |
This will be implemented once, xrpl.js review progresses beyond the fact of deciding whether console warnings are the right thing to do, in the interest of development effort.
David Fuelling's suggestion is marked as resolved and is up for future versions of the meta data spec. This PR pertains to XLS-89d. We can revisit this implementation once the spec is updated with that suggestion in future. Let me know if you think otherwise.
For now, to me it seems console warnings is the best way forward. xrpl.org should be updated for even better communication. Developer might or might not view the console warnings. Let me know once you have a better suggestion for this, I'll resume implementation after that, in the interest of development effort. |
My bad, I am referring to metada-sized based warnings in this PR? Where are such validations?
Yes, I understand; I'm only pointing out that his suggestion -- adding a
I agree that xrpl.org should be updated with the XLS-89d standard. Additionally, I recommend using the Typedoc/JSDoc-strings in the I don't have any other objections pertaining to this implementation approach. Please proceed. |
| ticker: string | ||
| name: string | ||
| desc?: string | ||
| icon: string | ||
| asset_class: string | ||
| asset_subclass?: string | ||
| issuer_name: string | ||
| urls?: MPTokenMetadataUrl[] | ||
| additional_info?: string |
There was a problem hiding this comment.
Can you move optional fields after required ones?
| * While adherence to the XLS-89d format is not mandatory, non-compliant metadata | ||
| * may not be discoverable by ecosystem tools such as explorers and indexers. | ||
| */ | ||
| MPTokenMetadata?: string | null |
There was a problem hiding this comment.
| MPTokenMetadata?: string | null | |
| MPTokenMetadata?: string |
I know you didn't make this change but there shouldn't be | null since it's an optional field.
| if (!isHex(input) || input.length / 2 > MAX_MPT_META_BYTE_LENGTH) { | ||
| validationMessages.push( | ||
| `MPTokenMetadata must be in hex format and max ${MAX_MPT_META_BYTE_LENGTH} bytes.`, | ||
| ) | ||
| return { validationMessages, isValid: false } | ||
| } |
There was a problem hiding this comment.
I would separate these error messages so it's cleaner.
| isValid: boolean | ||
| validationMessages: string[] |
There was a problem hiding this comment.
| isValid: boolean | |
| validationMessages: string[] | |
| validationMessages?: string[] |
We can infer validity by checking if validationMessages is returned.
| !( | ||
| jsonMetaData != null && | ||
| typeof jsonMetaData === 'object' && | ||
| !Array.isArray(jsonMetaData) | ||
| ) |
There was a problem hiding this comment.
| !( | |
| jsonMetaData != null && | |
| typeof jsonMetaData === 'object' && | |
| !Array.isArray(jsonMetaData) | |
| ) | |
| jsonMetaData == null || | |
| typeof jsonMetaData !== 'object' || | |
| Array.isArray(jsonMetaData) |
Simplified and easy to read.
| obj.additional_info != null && | ||
| !( | ||
| isString(obj.additional_info) || | ||
| (typeof obj.additional_info === 'object' && | ||
| !Array.isArray(obj.additional_info)) | ||
| ) |
There was a problem hiding this comment.
| obj.additional_info != null && | |
| !( | |
| isString(obj.additional_info) || | |
| (typeof obj.additional_info === 'object' && | |
| !Array.isArray(obj.additional_info)) | |
| ) | |
| obj.additional_info != null && | |
| !isString(obj.additional_info) && | |
| ( | |
| typeof obj.additional_info !== 'object' || | |
| Array.isArray(obj.additional_info) | |
| ) |
Nested negation is difficult to read.
| if ( | ||
| obj.urls != null && | ||
| (!Array.isArray(obj.urls) || | ||
| !obj.urls.every(isValidMPTokenMetadataUrlStructure)) | ||
| ) { | ||
| validationMessages.push( | ||
| `urls field is not properly structured as per the XLS-89d standard.`, | ||
| ) | ||
| return { validationMessages, isValid: false } | ||
| } |
There was a problem hiding this comment.
| if ( | |
| obj.urls != null && | |
| (!Array.isArray(obj.urls) || | |
| !obj.urls.every(isValidMPTokenMetadataUrlStructure)) | |
| ) { | |
| validationMessages.push( | |
| `urls field is not properly structured as per the XLS-89d standard.`, | |
| ) | |
| return { validationMessages, isValid: false } | |
| } | |
| if (obj.urls != null) { | |
| if (!Array.isArray(obj.urls)) { | |
| validationMessages.push('urls must be an array as per XLS-89d.'); | |
| return { validationMessages, isValid: false }; | |
| } | |
| if (!obj.urls.every(isValidMPTokenMetadataUrlStructure)) { | |
| validationMessages.push('One or more urls are not structured per XLS-89d.'); | |
| return { validationMessages, isValid: false }; | |
| } | |
| } | |
Separate error messages here too.
There was a problem hiding this comment.
@khancode Added your suggestions in 756a81a.
Also, updated the validation function to check for field count as well, so that users are warned if they add other fields that are not mentioned in the spec. It's not an issue to have such fields present since tools can ignore them, but good to have warnings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xrpl/test/fixtures/transactions/mptokenMetadata.json (1)
229-229: Consider making the JSON error test more robustThe error message includes the JavaScript engine's specific error text which might vary across different environments.
Consider using a regex or partial string match in the test to make it more robust across different JavaScript engines, or just check that the message starts with "MPTokenMetadata is not properly formatted as JSON".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/xrpl/src/models/common/index.ts(1 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(4 hunks)packages/xrpl/src/models/transactions/common.ts(4 hunks)packages/xrpl/src/models/transactions/vaultCreate.ts(5 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadata.json(1 hunks)packages/xrpl/test/models/utils.test.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/xrpl/test/models/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/xrpl/src/models/common/index.ts
- packages/xrpl/src/models/transactions/vaultCreate.ts
- packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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#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: 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#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: shawnxie999
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: 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: 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: 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/test/fixtures/transactions/mptokenMetadata.json (6)
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/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., as any) is preferred over strict typing as it better represents real-world scenarios where the validate method needs to handle arbitrary JSON/map/dictionary input from users.
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: 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.
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: 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.
packages/xrpl/src/models/transactions/common.ts (22)
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: 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: 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: ckeshava
PR: #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: ckeshava
PR: #3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like number vs string) in ledger objects such as LockedAmount vs OutstandingAmount in MPTokenIssuance are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
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: 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: 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: 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.
Learnt from: mvadari
PR: #2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:32-33
Timestamp: 2025-01-17T09:27:33.335Z
Learning: Use Array.isArray() instead of instanceof Array for reliable array type checking in JavaScript, as instanceof Array fails for cross-realm arrays (e.g., from iframes) and arrays from different JavaScript contexts.
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: 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).
Learnt from: ckeshava
PR: #2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: mvadari
PR: #2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., credentialType), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., CredentialType).
Learnt from: mvadari
PR: #2826
File: packages/xrpl/tools/generateModels.js:36-36
Timestamp: 2024-11-18T23:29:19.882Z
Learning: In the file packages/xrpl/tools/generateModels.js, it's acceptable for the script to crash when the regex doesn't match as expected.
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: 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: 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: achowdhry-ripple
PR: #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.
🧬 Code Graph Analysis (1)
packages/xrpl/src/models/transactions/common.ts (3)
packages/xrpl/src/models/transactions/index.ts (1)
validateMPTokenMetadata(1-1)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(225-235)
⏰ 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). (9)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: integration (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/xrpl/src/models/transactions/common.ts (4)
2-2: LGTM!The imports and constant are correctly added to support MPTokenMetadata validation.
Also applies to: 15-15, 26-26
31-68: Well-structured constants for XLS-89d validation!The constants properly define the validation rules according to the XLS-89d standard. The warning header message is clear and informative.
849-896: Comprehensive content validation!The content validation properly enforces all XLS-89d requirements including ticker format, HTTPS URLs, asset classifications, and conditional subclass requirements.
903-917: Clean URL structure validation!The helper function correctly validates the URL object structure according to XLS-89d requirements.
packages/xrpl/test/fixtures/transactions/mptokenMetadata.json (1)
1-426: Excellent test coverage!The test fixture comprehensively covers all validation scenarios including:
- Valid and invalid metadata structures
- All required and optional fields
- Format constraints (ticker, URLs, asset classifications)
- Size and field count limits
- Edge cases and error conditions
- Multiple simultaneous validation errors
This provides thorough test coverage for the XLS-89d validation implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
744-898: Consider refactoring for improved maintainability.The validation function is comprehensive and functionally correct, but its length (150+ lines) makes it challenging to maintain. Consider extracting validation logic into smaller, focused helper functions:
validateMPTokenMetadataStructure(obj)- for structural validation (lines 792-842)validateMPTokenMetadataContent(metadata)- for content validation (lines 847-896)validateMPTokenMetadataBasics(input)- for hex/size validation (lines 753-763)This would improve readability, testability, and maintainability while preserving the current functionality.
Example refactor structure:
export function validateMPTokenMetadata(input: string): string[] { const validationMessages: string[] = [] // Basic validation const basicErrors = validateMPTokenMetadataBasics(input) if (basicErrors.length > 0) { return basicErrors } // JSON parsing and object validation let jsonMetaData: unknown try { jsonMetaData = JSON.parse(hexToString(input)) } catch (err) { return [`MPTokenMetadata is not properly formatted as JSON - ${String(err)}`] } if (!isValidJSONObject(jsonMetaData)) { return ['MPTokenMetadata is not properly formatted as per XLS-89d.'] } const obj = jsonMetaData as Record<string, unknown> // Structural validation validationMessages.push(...validateMPTokenMetadataStructure(obj)) if (validationMessages.length > 0) { return validationMessages } // Content validation validationMessages.push(...validateMPTokenMetadataContent(obj as MPTokenMetadata)) return validationMessages }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/common.ts(4 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadata.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/fixtures/transactions/mptokenMetadata.json
🧰 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#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: 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#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: shawnxie999
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: 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: 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: 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/common.ts (26)
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: 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: 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: ckeshava
PR: #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: ckeshava
PR: #3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like number vs string) in ledger objects such as LockedAmount vs OutstandingAmount in MPTokenIssuance are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
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: 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: #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: 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.
Learnt from: mvadari
PR: #2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Learnt from: ckeshava
PR: #2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:32-33
Timestamp: 2025-01-17T09:27:33.335Z
Learning: Use Array.isArray() instead of instanceof Array for reliable array type checking in JavaScript, as instanceof Array fails for cross-realm arrays (e.g., from iframes) and arrays from different JavaScript contexts.
Learnt from: mvadari
PR: #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: ckeshava
PR: #2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., as any) is preferred over strict typing as it better represents real-world scenarios where the validate method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: mvadari
PR: #2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., credentialType), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., CredentialType).
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).
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: 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/src/sugar/autofill.ts:374-408
Timestamp: 2025-04-16T15:55:59.970Z
Learning: Code that is moved without substantial changes in a PR should not be the target of refactoring suggestions as it would be out of scope for the PR's primary purpose.
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: #2826
File: packages/xrpl/tools/generateModels.js:25-25
Timestamp: 2025-01-13T22:38:40.085Z
Learning: When parsing rippled source files (e.g., macro files, cpp files) in tools/scripts, extensive error handling for regex matches is not needed as any malformed content would be caught by rippled's C++ compiler and unit tests during the build process.
Learnt from: mvadari
PR: #2826
File: packages/xrpl/tools/generateModels.js:36-36
Timestamp: 2024-11-18T23:29:19.882Z
Learning: In the file packages/xrpl/tools/generateModels.js, it's acceptable for the script to crash when the regex doesn't match as expected.
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: 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: achowdhry-ripple
PR: #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.
🧬 Code Graph Analysis (1)
packages/xrpl/src/models/transactions/common.ts (3)
packages/xrpl/src/models/transactions/index.ts (1)
validateMPTokenMetadata(1-1)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(225-235)
⏰ 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: Analyze (javascript)
- GitHub Check: browser (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
- GitHub Check: build-and-lint (22.x)
🔇 Additional comments (4)
packages/xrpl/src/models/transactions/common.ts (4)
2-2: LGTM! Import addition is correct.The
hexToStringimport is necessary for the new metadata validation functionality.
15-15: LGTM! Constants are well-defined and follow XLS-89d specification.The constants are properly organized, follow consistent naming conventions, and provide clear validation rules. The warning header effectively communicates the purpose and impact of non-compliance.
Also applies to: 26-26, 31-69
902-916: LGTM! Helper function is well-implemented.The
isValidMPTokenMetadataUrlStructurefunction properly validates the URL object structure, including type checking and field count validation. The implementation follows established patterns in the codebase.
854-856: URL validation approach is appropriate for this context.The validation checks for HTTPS prefix rather than full URL validity, which is the right approach here since:
- It avoids network calls during validation
- It provides immediate feedback on protocol requirements
- Full URL validation would be expensive and potentially unreliable
This maintains performance while ensuring compliance with the security requirement of HTTPS URLs.
Also applies to: 892-895
khancode
left a comment
khancode
left a comment
There was a problem hiding this comment.
Left a comment but LGTM.
| validationMessages.push(`url should be a valid https url.`) | ||
| } | ||
|
|
||
| return validationMessages |
There was a problem hiding this comment.
| return validationMessages | |
| return validationMessages.length > 0 ? validationMessages : null |
When there are no errors, I prefer returning null rather than returning an empty array to signal no errors. Then the calling function would only have to do a truthy check if (validationMessages) rather than if (validationMessages.length > 0).
There was a problem hiding this comment.
I would also rename it to warningMessages so it's more explicit.
There was a problem hiding this comment.
I think in general an empty list is better design than a function returning an optional list. The consumers of the functions are guaranteed to get a list as opposed to null or a list. This avoids mistakes at consumer end of iterating, accessing elements of a list that is not even present. In xrpl4j, this convention is followed for simplicity. (i.e. Empty array/list is as good as no array/list).
| * Validates if MPTokenMetadata adheres to XLS-89d standard. | ||
| * | ||
| * @param input - Hex encoded MPTokenMetadata. | ||
| * @returns Validation error messages if MPTokenMetadata does not adheres to XLS-89d standard. |
There was a problem hiding this comment.
| * @returns Validation error messages if MPTokenMetadata does not adheres to XLS-89d standard. | |
| * @returns Validation warning messages if MPTokenMetadata does not adheres to XLS-89d standard. |
These are warning messages rather than errors right?
There was a problem hiding this comment.
This function just returns validation messages. Consumers decide whether to throw error or warnings. Updated to depict this.
…mpt-metadata-warning
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
858-860: Consider more robust URL validation.The current validation only checks if URLs start with "https://". Consider using a more comprehensive URL validation to ensure the URLs are properly formatted.
- if (!mptMPTokenMetadata.icon.startsWith('https://')) { - validationMessages.push(`icon should be a valid https url.`) + try { + const iconUrl = new URL(mptMPTokenMetadata.icon) + if (iconUrl.protocol !== 'https:') { + validationMessages.push(`icon should be a valid https url.`) + } + } catch { + validationMessages.push(`icon should be a valid https url.`) }Similarly for the urls array validation:
- !mptMPTokenMetadata.urls.every((ele) => ele.url.startsWith('https://')) + !mptMPTokenMetadata.urls.every((ele) => { + try { + const url = new URL(ele.url) + return url.protocol === 'https:' + } catch { + return false + } + })Also applies to: 895-899
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/common.ts(4 hunks)
⏰ 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: Analyze (javascript)
- GitHub Check: integration (22.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: browser (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
🔇 Additional comments (5)
packages/xrpl/src/models/transactions/common.ts (5)
2-2: LGTM!The new imports are appropriate for the MPTokenMetadata validation functionality.
Also applies to: 15-15
26-69: Well-structured constants for XLS-89d validation.The constants are properly organized and follow the specification requirements. The warning header message clearly explains the purpose and implications of non-compliance.
755-779: Robust validation implementation with proper error handling.The validation logic is well-structured with clear separation between format, structure, and content validation. The error handling for JSON parsing includes the error details which is helpful for debugging.
906-920: Clean helper function for URL structure validation.The function properly validates the URL object structure with appropriate null checks and field count validation.
797-803: Good field count validation for strict compliance.The field count check ensures metadata adheres to the expected structure and provides helpful feedback about the actual vs expected count.
3 participants
High Level Overview of Change
This PR adds console warnings to
MPTokenIssuanceCreatetransaction if theMPTokenMetadatadoes not follow XLS-89d standard.Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
Added unit tests.