Add encodeMPTokenMetadata and decodeMPTokenMetadata utility functions#3117
Add encodeMPTokenMetadata and decodeMPTokenMetadata utility functions#3117Patel-Raj11 merged 15 commits intomainfrom
Conversation
WalkthroughReplaces MPTokenMetadata shape with XLS-89 long-form (adds URIs), moves and reimplements validation/encoding/decoding into a new utils module, exposes encode/decode/validate via models index, updates fixtures/tests, adjusts imports, updates HISTORY, and adds a stable-json dependency. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
packages/xrpl/HISTORY.md (1)
7-9: Move breaking changes out of “Fixed”; clarify change notes.Placing breaking changes under “Fixed” is confusing. Suggest a “Changed” section with explicit breaking notes and the URL→URI type rename.
Apply this diff in Unreleased:
## Unreleased -### Added -* Add `encodeMPTokenMetadata` and `decodeMPTokenMetadata` helper functions to encode and decode MPTokenMetadata as per XLS-89 standard. +### Added +* Add `encodeMPTokenMetadata` and `decodeMPTokenMetadata` helper functions to encode/decode MPTokenMetadata per XLS-89. -### Fixed -* Fix incorrect type checking in `validateVaultCreate` that prevented vault creation with MPT as an asset. -* [Breaking change] Fix `MPTokenMetadata` type to adhere to the XLS-89 standard. Since XLS-89 is still in a forming state and undergoing changes, this breaking change is being released as a bug fix via patch version bump. -* [Breaking change] Fix `validateMPTokenMetadata` to correctly validate MPTokenMetadata as per XLS-89 standard. Since XLS-89 is still in a forming state and undergoing changes, this breaking change is being released as a bug fix via patch version bump. +### Changed +* [Breaking] Align `MPTokenMetadata` with XLS-89 long-form fields and replace `MPTokenMetadataUrl` with `MPTokenMetadataUri` (and `urls` → `uris`). See XRPL-Standards PR #390. +* [Breaking] Update `validateMPTokenMetadata` to XLS-89 semantics. + +### Fixed +* Fix incorrect type checking in `validateVaultCreate` that prevented vault creation with MPT as an asset.Also applies to: 12-13
packages/xrpl/test/models/utils.test.ts (1)
629-634: Make the JSON-parse error assertion robust.Node’s
SyntaxErrortext can vary. Match on a stable prefix with a regex.Apply this diff:
- assert.throws( - () => decodeMPTokenMetadata('464F4F'), - `MPTokenMetadata is not properly formatted as JSON - SyntaxError: Unexpected token 'F', "FOO" is not valid JSON`, - ) + assert.throws( + () => decodeMPTokenMetadata('464F4F'), + /MPTokenMetadata is not properly formatted as JSON -/, + )packages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.json (2)
225-229: Typo in test name ("unkown").Rename to "unknown" for clarity.
- "testName": "with unkown null fields", + "testName": "with unknown null fields",
237-273: Consider canonical expectation (uris only) after decode.If decode is meant to return long-form, prefer asserting only
uris(mergeusintouris) to avoid dual arrays in outputs. Update once decode is canonicalized.packages/xrpl/src/models/common/index.ts (1)
231-236: Align ticker doc with validator.Validator enforces ≤6 chars; doc says “recommended.” Either relax validation or update doc to “Max 6 characters.”
- * Uppercase letters (A-Z) and digits (0-9) only. Max 6 characters recommended. + * Uppercase letters (A-Z) and digits (0-9) only. Max 6 characters.packages/xrpl/src/models/transactions/common.ts (7)
32-45: Freeze field mapping tables.Prevent accidental mutation at runtime for deterministic behavior.
-const MPT_META_URI_FIELDS = [ +const MPT_META_URI_FIELDS = Object.freeze([ { long: 'uri', compact: 'u' }, { long: 'category', compact: 'c' }, { long: 'title', compact: 't' }, -] +]) as const
47-284: Canonicalize and simplify validation helpers.Two points:
- Using
thisinside object methods relies on call-site; ok here, but fragile if refactored. Consider capturinglong/compactvia closure.- Ticker message says “recommended” but enforcement is strict (≤6). Align wording or enforcement.
286-289: Unused constant.MPT_META_WARNING_HEADER is declared but never used. Remove or prepend it to validation output consistently.
1002-1039: Resolve long/compact key conflicts during shortening.Currently, if both forms exist, both are emitted, producing non-canonical JSON and larger payloads. Prefer compact on encode.
- // Both long and compact forms are present - if ( - input[mapping.long] !== undefined && - input[mapping.compact] !== undefined - ) { - output[key] = value - continue - } + // If both long and compact exist, prefer compact in encoded form + if (input[mapping.long] !== undefined && input[mapping.compact] !== undefined) { + output[mapping.compact] = input[mapping.compact] + continue + }
1049-1084: Encode: normalize top-level URIs to compact key (us) and de-dup.Ensure only one top-level array is emitted; merge if both provided, then drop
uris.- input = shortenKeys(input, MPT_META_ALL_FIELDS) + input = shortenKeys(input, MPT_META_ALL_FIELDS) - if (Array.isArray(input.uris)) { + // Normalize top-level: prefer compact 'us' during encode + if (Array.isArray(input.uris) && Array.isArray(input.us)) { + input.us = [...input.us, ...input.uris] + delete (input as Record<string, unknown>).uris + } else if (Array.isArray(input.uris)) { + input.us = input.uris + delete (input as Record<string, unknown>).uris + } + + if (Array.isArray(input.us)) { + input.us = input.us.map((uri: Record<string, unknown>): Record<string, unknown> => { + if (isRecord(uri)) return shortenKeys(uri, MPT_META_URI_FIELDS) + return uri + }) + } - - if (Array.isArray(input.us)) { - input.us = input.us.map( - (uri: Record<string, unknown>): Record<string, unknown> => { - if (isRecord(uri)) { - return shortenKeys(uri, MPT_META_URI_FIELDS) - } - return uri - }, - ) - }
1094-1123: Resolve long/compact key conflicts during expansion.If both forms exist, prefer long in decoded output for consistency with the public interface.
- // Both long and compact forms are present - if ( - input[mapping.long] !== undefined && - input[mapping.compact] !== undefined - ) { - output[key] = value - continue - } + // If both long and compact exist, prefer long in decoded form + if (input[mapping.long] !== undefined && input[mapping.compact] !== undefined) { + output[mapping.long] = input[mapping.long] + continue + }
1188-1239: Validation flow is solid; minor enhancement only.Consider collecting multiple errors before returning early (e.g., after size or JSON parse) to aid users, but current short-circuiting is acceptable.
packages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.json (1)
306-311: Stabilize JSON parse error assertion.Exact engine error strings vary by Node version. Prefer asserting prefix or pattern instead of the full message to avoid brittle tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/common/index.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(3 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.json(1 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.json(22 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(2 hunks)packages/xrpl/test/models/utils.test.ts(4 hunks)packages/xrpl/test/models/vaultCreate.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-06T18:44:55.095Z
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.
Applied to files:
packages/xrpl/test/models/utils.test.tspackages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
🧬 Code graph analysis (4)
packages/xrpl/test/models/utils.test.ts (2)
packages/xrpl/src/models/transactions/common.ts (2)
encodeMPTokenMetadata(1049-1084)decodeMPTokenMetadata(1133-1180)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(229-300)
packages/xrpl/test/models/vaultCreate.test.ts (1)
packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(229-300)
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(229-300)
packages/xrpl/src/models/transactions/common.ts (3)
packages/xrpl/src/models/transactions/index.ts (3)
encodeMPTokenMetadata(5-5)decodeMPTokenMetadata(6-6)validateMPTokenMetadata(4-4)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(229-300)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)
⏰ 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). (10)
- GitHub Check: unit (24.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
167-169: Warning path/message update looks correct.Using an invalid
urisshape to exercise the validator and asserting the newuris/usmessage is appropriate.Also applies to: 177-180
packages/xrpl/test/models/vaultCreate.test.ts (1)
132-132: Icon warning assertion aligns with new validator.Omitting
iconand asserting- icon/i: should be a non-empty string.matches the updated rules.Also applies to: 144-146
packages/xrpl/src/models/transactions/index.ts (1)
1-7: Re-exports look good.
encodeMPTokenMetadata/decodeMPTokenMetadataare correctly exposed alongside existing exports.packages/xrpl/test/models/utils.test.ts (1)
592-592: Use chai’sassert.deepEqual(notdeepStrictEqual).
assert.deepStrictEqualisn’t part of chai’s assert API and will throw. Useassert.deepEqual.Apply this diff:
- assert.deepStrictEqual(validationMessages, testCase.validationMessages) + assert.deepEqual(validationMessages, testCase.validationMessages) @@ - assert.deepStrictEqual(decoded, testCase.expectedLongForm) + assert.deepEqual(decoded, testCase.expectedLongForm)Also applies to: 610-611
⛔ Skipped due to learnings
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.packages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.json (1)
12-23: Fixtures line up with XLS-89 validations.URIs structure and short-field mappings look correct and comprehensive across cases.
Also applies to: 116-127, 324-341, 353-372, 383-395, 406-422, 433-506
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
217-259: Consider validating long/compact exclusivity within URI objects.The validation checks that each URI object has exactly 3 fields (line 240), but doesn't explicitly verify that individual URI objects don't mix long and compact forms for the same field (e.g., having both
uriandu).For strict XLS-89 compliance, consider adding validation to ensure each URI object doesn't contain both forms of the same field:
for (const uriObj of value) { if ( !isRecord(uriObj) || Object.keys(uriObj).length !== MPT_META_URI_FIELDS.length ) { messages.push( `${this.long}/${this.compact}: should be an array of objects each with uri/u, category/c, and title/t properties.`, ) continue } + + // Check exclusivity of long/compact forms in URI object + for (const field of MPT_META_URI_FIELDS) { + if (uriObj[field.long] != null && uriObj[field.compact] != null) { + messages.push( + `${this.long}/${this.compact}: URI object has both ${field.long} and ${field.compact}. expected only one.`, + ) + } + } const uri = uriObj.uri ?? uriObj.u const category = uriObj.category ?? uriObj.c const title = uriObj.title ?? uriObj.t
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/common.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
🧬 Code graph analysis (1)
packages/xrpl/src/models/transactions/common.ts (3)
packages/xrpl/src/models/transactions/index.ts (3)
encodeMPTokenMetadata(5-5)decodeMPTokenMetadata(6-6)validateMPTokenMetadata(4-4)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(229-300)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)
⏰ 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: integration (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/xrpl/src/models/transactions/common.ts (6)
2-3: LGTM!The
no-continuedisable is appropriate for the validation loops, and thestringToHeximport is correctly added for the encoding functionality.
1002-1039: LGTM!The
shortenKeyshelper correctly implements the preservation design: it only converts to compact form when a single form is present, and preserves both forms if both exist. Unknown fields are passed through unchanged.
1041-1085: LGTM!The encoding function correctly:
- Validates input structure
- Shortens keys at both top-level and nested URI objects
- Handles both
urisandusarrays when present- Produces uppercase hex output
Based on learnings: This preserves the original structure, only converting forms when a single form is present.
1087-1124: LGTM!The
expandKeyshelper is the correct inverse ofshortenKeys, expanding to long-form names while preserving both forms when both exist in the input.
1126-1182: LGTM!The decoding function correctly:
- Validates hex format and parses JSON with proper error handling
- Expands keys at both top-level and nested URI objects
- Handles both
urisandusarrays when presentBased on learnings: This intentionally preserves both compact (
us) and long (uris) forms if both exist in the input, without merging or modifying user data. XLS-89 compliance is checked separately byvalidateMPTokenMetadata.
1184-1242: LGTM!The validation function provides comprehensive XLS-89 compliance checking:
- Progressive validation with early returns on critical errors
- Proper field count validation
- Delegates to per-field validation rules defined in
MPT_META_ALL_FIELDSThe separation of concerns between validation (strict XLS-89 compliance) and encode/decode (permissive transformation) is well-designed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/xrpl/src/models/transactions/common.ts (7)
33-46: Make URI field mappings immutable.Freeze and type as readonly to avoid accidental mutation and improve inference.
-const MPT_META_URI_FIELDS = [ +const MPT_META_URI_FIELDS = Object.freeze([ { long: 'uri', compact: 'u', }, { long: 'category', compact: 'c', }, { long: 'title', compact: 't', }, -] +]) as ReadonlyArray<{ long: string; compact: string }>
50-66: Align validation message with enforced rule.Regex enforces max 6 chars, but message says “Max 6 characters recommended.” Remove “recommended” to avoid confusion.
- `${this.long}/${this.compact}: should have uppercase letters (A-Z) and digits (0-9) only. Max 6 characters recommended.`, + `${this.long}/${this.compact}: should have uppercase letters (A-Z) and digits (0-9) only. Max 6 characters.`,
1011-1040: Pre-index mappings to reduce per-key linear scans.Small map here, but pre-indexing improves clarity and avoids O(n) finds per property.
function shortenKeys( input: Record<string, unknown>, mappings: Array<{ long: string; compact: string }>, ): Record<string, unknown> { - const output: Record<string, unknown> = {} - - for (const [key, value] of Object.entries(input)) { - const mapping = mappings.find( - ({ long, compact }) => long === key || compact === key, - ) + const output: Record<string, unknown> = {} + const map = new Map<string, { long: string; compact: string }>() + for (const m of mappings) { + map.set(m.long, m) + map.set(m.compact, m) + } + + for (const [key, value] of Object.entries(input)) { + const mapping = map.get(key) // Extra keys stays there if (mapping === undefined) { output[key] = value continue }
1100-1129: Mirror the mapping pre-indexing in expandKeys.Keep implementations symmetric and O(1) lookup for readability and consistency.
function expandKeys( input: Record<string, unknown>, mappings: Array<{ long: string; compact: string }>, ): Record<string, unknown> { - const output: Record<string, unknown> = {} - - for (const [key, value] of Object.entries(input)) { - const mapping = mappings.find( - ({ long, compact }) => long === key || compact === key, - ) + const output: Record<string, unknown> = {} + const map = new Map<string, { long: string; compact: string }>() + for (const m of mappings) { + map.set(m.long, m) + map.set(m.compact, m) + } + for (const [key, value] of Object.entries(input)) { + const mapping = map.get(key) // Extra keys stays there if (mapping === undefined) { output[key] = value continue }
287-291: Grammar nit in warning header.Use plural without apostrophe.
export const MPT_META_WARNING_HEADER = - 'MPTokenMetadata is not properly formatted as JSON as per the XLS-89 standard. ' + - "While adherence to this standard is not mandatory, such non-compliant MPToken's might not be discoverable " + + 'MPTokenMetadata is not properly formatted as JSON as per the XLS-89 standard. ' + + 'While adherence to this standard is not mandatory, such non-compliant MPTokens might not be discoverable ' + 'by Explorers and Indexers in the XRPL ecosystem.'
1131-1139: Clarify decode behavior in JSDoc (preserves both forms).Docs say “with long field names,” but implementation intentionally preserves compact fields when both exist.
/** - * Decodes hex encoded MPTokenMetadata to a JSON object with long field names. - * Converts compact field names back to their long form equivalents. + * Decodes hex-encoded MPTokenMetadata to a JSON object. + * Converts compact field names to their long-form equivalents when only one form is present. + * If both long and compact forms exist (e.g., `uris` and `us`), both are preserved. * * @param input - Hex encoded MPTokenMetadata. - * @returns Decoded MPTokenMetadata object with long field names. + * @returns Decoded MPTokenMetadata object (may contain both long and compact aliases if present in input). * @throws Error if input is not valid hex or cannot be parsed as JSON. * @category Utilities */ export function decodeMPTokenMetadata(input: string): MPTokenMetadata { @@ - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Required here as output is now properly formatted + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Return type is long-form, but extra compact aliases may be present by design return output as unknown as MPTokenMetadataBased on learnings
Also applies to: 1185-1187
1232-1239: Replace brittle top‑level field count check with explicit unknown‑key detection.Counting keys misses cases (e.g., fewer long fields + extra unknowns = false negative) and double‑reports when both forms exist. Check for unknown keys instead.
- if (Object.keys(jsonMetaData).length > MPT_META_ALL_FIELDS.length) { - validationMessages.push( - `MPTokenMetadata must not contain more than ${MPT_META_ALL_FIELDS.length} top-level fields (found ${ - Object.keys(jsonMetaData).length - }).`, - ) - } + // Disallow unknown top-level keys (beyond long/compact aliases defined by XLS-89) + const allowedTopLevel = new Set( + MPT_META_ALL_FIELDS.flatMap((f) => [f.long, f.compact]), + ) + for (const k of Object.keys(jsonMetaData)) { + if (!allowedTopLevel.has(k)) { + validationMessages.push(`Unknown top-level field: ${k}.`) + } + }packages/xrpl/package.json (1)
33-35: esModuleInterop verified; pinning remains a recommended best practice.Verification confirms:
esModuleInterop: trueis enabled in both base tsconfig.json and packages/xrpl/tsconfig.json, soimport stableStringify from 'fast-json-stable-stringify'resolves correctly.- Only one installation of
fast-json-stable-stringify(v2.1.0) exists in node_modules;json-stable-stringify-without-jsonifyis an unrelated dependency used by eslint.The suggested change from
^2.1.0to2.1.0(pinning) remains valid for stricter reproducibility and avoiding unexpected minor-version behavior shifts, though the caret range is not problematic at present.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
packages/xrpl/package.json(1 hunks)packages/xrpl/src/models/transactions/common.ts(3 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
🧬 Code graph analysis (1)
packages/xrpl/src/models/transactions/common.ts (3)
packages/xrpl/src/models/transactions/index.ts (3)
encodeMPTokenMetadata(7-7)decodeMPTokenMetadata(8-8)validateMPTokenMetadata(6-6)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(229-300)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)
⏰ 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). (10)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (24.x)
- GitHub Check: build-and-lint (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
1055-1090: Verification complete:isHex()correctly handles both uppercase and lowercase hex.The regex
/^[0-9A-Fa-f]+$/uexplicitly accepts both cases (A-Fanda-f), so the decode path will robustly handle the uppercase hex output fromencodeMPTokenMetadata(). Deterministic encoding with stable key order and uppercasing is properly implemented.
|
|
||
| // To validate MPTokenMetadata as per XLS-89d | ||
| const TICKER_REGEX = /^[A-Z0-9]{1,6}$/u | ||
| const MPT_META_URI_FIELDS = [ |
There was a problem hiding this comment.
Is it possible and safe to move all MPT related code into a separate file? It's bad to maintainability to keep everything in a common.ts file.
There was a problem hiding this comment.
Actually, I wanted to move these three utility functions into utils directory. However, doing so was causing cyclic dependency when validateMPTokenMetadata function gets used in MPTIssuanceCreate and VaultCreate transactions. Spent couple of hours to fix those but no luck and it would require a good sized refactoring.
For now I have moved them out from common.ts into ./utils/mptokenMetadata.ts in 8113c9f
I think the top level exports still remains the same, it's safe to move these functions around.
| * Decodes hex encoded MPTokenMetadata to a JSON object with long field names. | ||
| * Converts compact field names back to their long form equivalents. |
There was a problem hiding this comment.
How about
- Decodes hex-encoded MPTokenMetadata into a JSON object.
- Converts compact field names to their corresponding long-form equivalents.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/common/index.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-06T18:44:55.095Z
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.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-06-26T17:25:36.429Z
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.
Applied to files:
packages/xrpl/src/models/common/index.ts
📚 Learning: 2024-12-05T16:48:12.951Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-08T13:08:52.688Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-02-12T23:30:40.622Z
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
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-07-10T22:04:59.994Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-08T02:11:21.997Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-06T19:25:15.376Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-07-11T21:22:07.809Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-04-16T15:22:45.633Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
🧬 Code graph analysis (1)
packages/xrpl/src/models/transactions/common.ts (3)
packages/xrpl/src/models/transactions/index.ts (3)
encodeMPTokenMetadata(7-7)decodeMPTokenMetadata(8-8)validateMPTokenMetadata(6-6)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(227-298)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)
⏰ 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: integration (22.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (9)
packages/xrpl/src/models/common/index.ts (2)
222-298: LGTM!The MPTokenMetadata interface is well-documented with clear examples for each field. The field types are appropriate, and the optional fields are correctly marked. The structure aligns with the XLS-89 standard as described in the PR objectives.
300-328: LGTM!The MPTokenMetadataUri interface is well-defined with clear documentation and examples. The required fields (uri, category, title) are appropriately typed as strings, and the category field documentation clearly lists the allowed values.
packages/xrpl/src/models/transactions/common.ts (7)
1-46: LGTM!The updated imports and MPT_META_URI_FIELDS constant are appropriate for the new encode/decode functionality. The field mappings are clear and consistent with the XLS-89 standard.
1003-1040: LGTM!The
shortenKeysfunction correctly implements the conversion from long-form to compact field names while preserving the original structure when both forms are present. The handling of unmapped keys ensures flexibility for users who may include additional fields.Based on learnings.
1042-1090: LGTM!The
encodeMPTokenMetadatafunction correctly encodes the metadata to a hex string. It properly handles both long-form (uris) and compact-form (us) arrays, shortens nested URI objects, and usesstableStringifyfor deterministic encoding.Based on learnings.
1092-1129: LGTM!The
expandKeysfunction correctly implements the reverse operation ofshortenKeys, expanding compact field names to their long-form equivalents while preserving the original structure when both forms are present.Based on learnings.
1131-1187: LGTM!The
decodeMPTokenMetadatafunction correctly decodes hex-encoded metadata back to a JSON object. It properly handles both long-form (uris) and compact-form (us) arrays, expands nested URI objects, and includes comprehensive error handling with clear error messages.Based on learnings.
1189-1247: LGTM!The
validateMPTokenMetadatafunction provides comprehensive validation of the metadata against the XLS-89 standard. It checks hex format, byte length, JSON structure, field count, and delegates to per-field validators for detailed validation. The function appropriately returns all validation messages to help users identify issues.
287-290: LGTM!The updated warning header correctly references the XLS-89 standard and clearly explains the implications of non-compliance for discoverability in the XRPL ecosystem.
| { | ||
| long: 'uris', | ||
| compact: 'us', | ||
| required: false, | ||
| validate(obj: Record<string, unknown>): string[] { | ||
| if (obj[this.long] != null && obj[this.compact] != null) { | ||
| return [ | ||
| `${this.long}/${this.compact}: both long and compact forms present. expected only one.`, | ||
| ] | ||
| } | ||
|
|
||
| if (obj[this.long] === undefined && obj[this.compact] === undefined) { | ||
| return [] | ||
| } | ||
| const value = obj[this.long] ?? obj[this.compact] | ||
| if (!Array.isArray(value) || value.length === 0) { | ||
| return [`${this.long}/${this.compact}: should be a non-empty array.`] | ||
| } | ||
|
|
||
| const messages: string[] = [] | ||
| for (const uriObj of value) { | ||
| if ( | ||
| !isRecord(uriObj) || | ||
| Object.keys(uriObj).length !== MPT_META_URI_FIELDS.length | ||
| ) { | ||
| messages.push( | ||
| `${this.long}/${this.compact}: should be an array of objects each with uri/u, category/c, and title/t properties.`, | ||
| ) | ||
| continue | ||
| } | ||
|
|
||
| const uri = uriObj.uri ?? uriObj.u | ||
| const category = uriObj.category ?? uriObj.c | ||
| const title = uriObj.title ?? uriObj.t | ||
| if (!isString(uri) || !isString(category) || !isString(title)) { | ||
| messages.push( | ||
| `${this.long}/${this.compact}: should be an array of objects each with uri/u, category/c, and title/t properties.`, | ||
| ) | ||
| } | ||
| } | ||
| return messages | ||
| }, |
There was a problem hiding this comment.
URI validation doesn't check for both long and compact forms.
The uris validation checks that each URI object has exactly MPT_META_URI_FIELDS.length (3) keys at line 241, but it doesn't validate that both long-form (e.g., uri) and compact-form (e.g., u) aren't present simultaneously within the same URI object. If a URI object contains both uri and u, it would have more than 3 keys and would be caught by the length check. However, the validation at lines 249-252 uses ?? fallback logic that tolerates both forms being present, which is inconsistent with the parent-level field validation that explicitly rejects both forms.
Consider adding explicit validation for URI objects to check that both forms aren't present, similar to the parent-level field validation pattern used for other fields.
Apply this approach:
for (const uriObj of value) {
+ // Check for both long and compact forms in the same URI object
+ for (const uriField of MPT_META_URI_FIELDS) {
+ if (uriObj[uriField.long] != null && uriObj[uriField.compact] != null) {
+ messages.push(
+ `${this.long}/${this.compact}: URI object cannot have both ${uriField.long} and ${uriField.compact} fields.`,
+ )
+ break
+ }
+ }
+
if (
!isRecord(uriObj) ||
Object.keys(uriObj).length !== MPT_META_URI_FIELDS.length
) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/xrpl/src/models/transactions/common.ts around lines 218 to 259, the
per-URI-object validation currently uses fallback operators that tolerate both
long and compact forms (e.g., uri and u) being present; update the loop so each
URI object explicitly rejects simultaneous long+compact keys for each field
(uri/u, category/c, title/t), pushing an error message when both forms exist,
and only then use a single selected value to validate type and non-empty array
constraints; keep the existing error messages and structure but add explicit
checks before assigning uri/category/title to ensure consistency with the
parent-level validation.
There was a problem hiding this comment.
@Patel-Raj11, thank you for addressing this! I'm glad the URI validation now properly checks for both long and compact forms within each URI object. 🎉
🧠 Learnings used
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
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#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.768Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
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#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: achowdhry-ripple
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-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#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: 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/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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/xrpl/src/models/utils/mptokenMetadata.ts (3)
33-52: Simplify regex pattern.The ticker validation regex uses the unicode flag
ufor a purely ASCII pattern. While not incorrect, the unicode flag is unnecessary here and adds minimal overhead.Consider simplifying:
- if (!isString(value) || !/^[A-Z0-9]{1,6}$/u.test(value)) { + if (!isString(value) || !/^[A-Z0-9]{1,6}$/.test(value)) {
350-370: Consider extracting duplicate URI shortening logic.The same URI shortening logic is duplicated for both
input.urisandinput.usarrays.Consider extracting the common logic:
+ const shortenUriArray = (uris: unknown): unknown => { + if (!Array.isArray(uris)) return uris + return uris.map((uri: Record<string, unknown>): Record<string, unknown> => { + if (isRecord(uri)) { + return shortenKeys(uri, MPT_META_URI_FIELDS) + } + return uri + }) + } + - if (Array.isArray(input.uris)) { - input.uris = input.uris.map( - (uri: Record<string, unknown>): Record<string, unknown> => { - if (isRecord(uri)) { - return shortenKeys(uri, MPT_META_URI_FIELDS) - } - return uri - }, - ) - } - - if (Array.isArray(input.us)) { - input.us = input.us.map( - (uri: Record<string, unknown>): Record<string, unknown> => { - if (isRecord(uri)) { - return shortenKeys(uri, MPT_META_URI_FIELDS) - } - return uri - }, - ) - } + input.uris = shortenUriArray(input.uris) + input.us = shortenUriArray(input.us)
445-465: Consider extracting duplicate URI expansion logic.Similar to encodeMPTokenMetadata, the URI expansion logic is duplicated for both
output.urisandoutput.usarrays.Consider extracting the common logic:
+ const expandUriArray = (uris: unknown): unknown => { + if (!Array.isArray(uris)) return uris + return uris.map((uri: Record<string, unknown>): Record<string, unknown> => { + if (isRecord(uri)) { + return expandKeys(uri, MPT_META_URI_FIELDS) + } + return uri + }) + } + - if (Array.isArray(output.uris)) { - output.uris = output.uris.map( - (uri: Record<string, unknown>): Record<string, unknown> => { - if (isRecord(uri)) { - return expandKeys(uri, MPT_META_URI_FIELDS) - } - return uri - }, - ) - } - - if (Array.isArray(output.us)) { - output.us = output.us.map( - (uri: Record<string, unknown>): Record<string, unknown> => { - if (isRecord(uri)) { - return expandKeys(uri, MPT_META_URI_FIELDS) - } - return uri - }, - ) - } + output.uris = expandUriArray(output.uris) + output.us = expandUriArray(output.us)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/xrpl/src/models/index.ts(1 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(1 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/src/models/transactions/vaultCreate.ts(1 hunks)packages/xrpl/src/models/utils/mptokenMetadata.ts(1 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.json(1 hunks)packages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.json(22 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(3 hunks)packages/xrpl/test/models/utils.test.ts(4 hunks)packages/xrpl/test/models/vaultCreate.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/test/models/vaultCreate.test.ts
- packages/xrpl/src/models/transactions/index.ts
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.768Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Applied to files:
packages/xrpl/src/models/utils/mptokenMetadata.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/index.tspackages/xrpl/test/models/utils.test.tspackages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.jsonpackages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.jsonpackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/vaultCreate.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2025-10-28T14:16:24.567Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.567Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Applied to files:
packages/xrpl/src/models/utils/mptokenMetadata.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/index.tspackages/xrpl/test/models/utils.test.tspackages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.jsonpackages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.jsonpackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/vaultCreate.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2024-12-06T18:44:55.095Z
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.
Applied to files:
packages/xrpl/src/models/utils/mptokenMetadata.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/index.tspackages/xrpl/test/models/utils.test.tspackages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.jsonpackages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.jsonpackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/vaultCreate.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2025-10-30T12:09:55.768Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.768Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
Applied to files:
packages/xrpl/src/models/utils/mptokenMetadata.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.jsonpackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/vaultCreate.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2025-09-16T05:00:20.420Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.420Z
Learning: In XRPL's MPTokenIssuanceCreate flags, the tfMPTCanMutate* flags intentionally use the same bit values as their corresponding base flags (e.g., tfMPTCanMutateCanLock = 0x00000002 matches tfMPTCanLock = 0x00000002) because they operate in different contexts - base flags go in the Flags field while mutable flags go in the MutableFlags field. This design avoids ambiguity since the flags are applied to separate bit fields.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2024-12-05T16:48:12.951Z
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.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2024-12-06T19:25:15.376Z
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.
Applied to files:
packages/xrpl/test/models/utils.test.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2025-01-31T17:46:25.375Z
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.
Applied to files:
packages/xrpl/test/models/utils.test.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-02-12T23:28:55.377Z
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.
Applied to files:
packages/xrpl/test/models/utils.test.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2025-01-16T04:26:36.757Z
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.
Applied to files:
packages/xrpl/test/models/utils.test.ts
📚 Learning: 2025-07-10T22:04:59.994Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-02-12T23:30:40.622Z
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
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-08T02:11:21.997Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-08T02:12:28.489Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
PR: XRPLF/xrpl.js#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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-07-11T21:22:07.809Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-08T02:08:00.476Z
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.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-06-26T17:25:36.429Z
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.
Applied to files:
packages/xrpl/src/models/transactions/vaultCreate.ts
📚 Learning: 2024-10-02T15:47:02.491Z
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.
Applied to files:
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
🧬 Code graph analysis (3)
packages/xrpl/src/models/utils/mptokenMetadata.ts (3)
packages/xrpl/src/models/transactions/common.ts (2)
isString(106-108)isRecord(96-98)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(227-298)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)
packages/xrpl/test/models/utils.test.ts (2)
packages/xrpl/src/models/utils/mptokenMetadata.ts (2)
encodeMPTokenMetadata(338-373)decodeMPTokenMetadata(423-469)packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(227-298)
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(227-298)
⏰ 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 (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (18)
packages/xrpl/src/models/transactions/common.ts (1)
3-3: Consolidated HEX regex import looks good.Pulling
HEX_REGEXstraight from@xrplf/isomorphic/utilskeeps the validator in sync with our shared utility source. Nice cleanup.packages/xrpl/src/models/transactions/vaultCreate.ts (1)
4-8: Importing from the new MP token metadata utilities is spot on.Updating the source of these helpers to
../utils/mptokenMetadatamatches the refactor and keeps VaultCreate wired to the canonical encode/validate path.packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
3-7: Good reuse of the centralized metadata helpers.Switching the constants and validator over to the utils module keeps MPTokenIssuanceCreate aligned with the shared encoding/validation logic.
packages/xrpl/src/models/index.ts (1)
17-20: Thanks for exporting the new helpers.Surfacing
validateMPTokenMetadata,decodeMPTokenMetadata, andencodeMPTokenMetadatafrom the models index makes the new utilities immediately accessible to consumers.packages/xrpl/test/fixtures/transactions/mptokenMetadataEncodeDecodeData.json (1)
1-298: Fixture coverage is comprehensive.These cases exercise long/short keys, mixed forms, and passthrough fields, which should keep the encode/decode utilities honest.
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (2)
4-8: LGTM!The import reorganization correctly reflects the new module structure where MPTokenMetadata utilities have been extracted into a dedicated utils module.
160-185: LGTM!The test correctly validates warning messages for malformed URI data, using intentional type assertions to test runtime validation behavior.
packages/xrpl/test/models/utils.test.ts (4)
17-17: LGTM!The imports are well-organized, separating utility functions from test fixtures and maintaining clear module boundaries.
Also applies to: 30-36
582-597: LGTM!The data-driven validation test suite correctly handles both string and object inputs, using deep equality checks for comprehensive validation message verification.
599-615: LGTM!The encode/decode round-trip tests properly verify both directions of transformation and validate the expected long-form output structure.
631-636: Verify error message format is consistent across environments.The test asserts an exact error message that includes JavaScript engine-specific SyntaxError text. Different JS engines (Node.js versions, browsers) may format JSON.parse errors differently, potentially causing this test to fail in some environments.
Consider using a more flexible assertion:
- it('throws error for invalid JSON underneath hex', function () { - assert.throws( - () => decodeMPTokenMetadata('464F4F'), - `MPTokenMetadata is not properly formatted as JSON - SyntaxError: Unexpected token 'F', "FOO" is not valid JSON`, - ) - }) + it('throws error for invalid JSON underneath hex', function () { + assert.throws( + () => decodeMPTokenMetadata('464F4F'), + /MPTokenMetadata is not properly formatted as JSON/, + ) + })packages/xrpl/src/models/utils/mptokenMetadata.ts (5)
1-16: LGTM!The module setup correctly uses stable JSON stringification for deterministic encoding, and the warning message appropriately explains the impact of non-compliance with XLS-89.
208-258: LGTM!The URI validation correctly enforces strict XLS-89 compliance by checking exact field counts, detecting mixed long/compact forms, and validating string types for all required properties.
294-323: LGTM!The shortenKeys function correctly preserves the original structure when conflicts exist (both long and compact forms present), aligning with the design documented in learnings to avoid modifying user data.
Based on learnings
383-412: LGTM!The expandKeys function correctly mirrors shortenKeys, providing the symmetric reverse operation while maintaining the same conflict preservation semantics.
478-529: LGTM!The validation function is well-structured with layered checks, early returns to prevent cascading errors, and comprehensive field-level validation that collects all messages rather than failing fast.
packages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.json (2)
34-106: LGTM!The new test cases comprehensively cover short-form, long-form, and mixed-form field names, as well as properly testing conflict detection when both forms are present.
537-561: LGTM!The null values test case provides comprehensive edge-case coverage, ensuring all field validations properly handle null inputs and report appropriate error messages.
packages/xrpl/test/fixtures/transactions/mptokenMetadataValidationData.json
Show resolved
Hide resolved
packages/xrpl/HISTORY.md
Outdated
|
|
||
| ### Fixed | ||
| * Fix incorrect type checking in `validateVaultCreate` that prevented vault creation with MPT as an asset. | ||
| * [Breaking change] Fix `MPTokenMetadata` type to adhere to the XLS-89 standard. Since XLS-89 is still in a forming state and undergoing changes, this breaking change is being released as a bug fix via patch version bump. |
There was a problem hiding this comment.
Sorry I'm a bit confused here. Why this is a breaking change even though validateMPTokenMetadata only raise a warning (so actually no code is broken)? Also why a breaking change is included in a patch version update, not major?
There was a problem hiding this comment.
Breaking change is in MPTokenMetadata type. validateMPTokenMetadata returns messages, so for the same input, if it does not return the previously expected output, its considered as breaking change.
More context here for why - https://ripple-enterprise.slack.com/archives/C027E6YK5GS/p1761334584264839
There was a problem hiding this comment.
Got it, may be we should explain a bit here in terms of what's impact this has to users to be considered as breaking change to avoid confusion
| * @throws Error if input is not a JSON object. | ||
| * @category Utilities | ||
| */ | ||
| export function encodeMPTokenMetadata( |
There was a problem hiding this comment.
Should we suggest using this function to encode metadata in the docstring of the field itself so users are aware?
There was a problem hiding this comment.
Or maybe we can add a few integration tests for the MPT transactions to showcase how this is done
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/HISTORY.md (1)
13-14: Consider clarifying the user impact of these breaking changes.Following up on the past review discussion: The breaking changes section could be more explicit about what will actually break for users. Consider expanding the explanation to mention:
- TypeScript users: Existing code using
MPTokenMetadatatype will see type errors due to the updated interface structure (e.g., changed field names/structure per XLS-89)- JavaScript users: Code relying on specific
validateMPTokenMetadataoutput messages will need updates as the validation messages have changed- Both: Any code parsing or constructing metadata objects needs to align with the new XLS-89 structure
This would help users quickly understand if they need to take action when upgrading.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(2 hunks)packages/xrpl/src/models/transactions/vaultCreate.ts(2 hunks)packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/transactions/vaultCreate.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.784Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
📚 Learning: 2025-10-28T14:16:24.585Z
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Applied to files:
packages/xrpl/HISTORY.mdpackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2025-10-28T14:16:24.585Z
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Applied to files:
packages/xrpl/HISTORY.mdpackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2024-12-05T16:48:12.951Z
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Applied to files:
packages/xrpl/HISTORY.mdpackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
📚 Learning: 2025-10-30T12:09:55.784Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.784Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
Applied to files:
packages/xrpl/HISTORY.mdpackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2025-06-26T17:25:36.429Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js 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.
Applied to files:
packages/xrpl/HISTORY.md
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2025-09-16T05:00:20.420Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.420Z
Learning: In XRPL's MPTokenIssuanceCreate flags, the tfMPTCanMutate* flags intentionally use the same bit values as their corresponding base flags (e.g., tfMPTCanMutateCanLock = 0x00000002 matches tfMPTCanLock = 0x00000002) because they operate in different contexts - base flags go in the Flags field while mutable flags go in the MutableFlags field. This design avoids ambiguity since the flags are applied to separate bit fields.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2024-12-06T19:25:15.376Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Applied to files:
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2024-10-08T16:29:11.194Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js 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.
Applied to files:
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2025-01-16T04:26:36.757Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 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.
Applied to files:
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
📚 Learning: 2025-01-16T04:11:37.316Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 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.
Applied to files:
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts
🧬 Code graph analysis (1)
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts (2)
packages/xrpl/src/models/common/index.ts (1)
MPTokenMetadata(227-298)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
MPTokenIssuanceCreate(84-124)
⏰ 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: semgrep-cloud-platform/scan
🔇 Additional comments (7)
packages/xrpl/test/integration/transactions/mptokenIssuanceCreate.test.ts (4)
3-8: LGTM!The imports correctly bring in the new metadata encoding/decoding utilities and types needed for the test.
31-58: LGTM!The test metadata object is comprehensive and well-structured, exercising all the XLS-89 fields including optional ones. The T-Bill example provides realistic test data.
65-65: LGTM!Correctly uses
encodeMPTokenMetadatato convert the metadata object to the blob format required by the transaction.
86-92: Good round-trip verification!The test correctly verifies that encoding and decoding preserves the metadata structure. Using
deepStrictEqualensures all fields are preserved exactly.The
@ts-expect-errorfor the account_objects type is acceptable here per established patterns in the codebase.packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (3)
3-7: LGTM!The import reorganization to source validation utilities from the dedicated
mptokenMetadatamodule improves code organization.
114-120: LGTM!The documentation clearly guides developers on using the metadata utilities and references the XLS-89 standard. The note about ecosystem tool discoverability for non-compliant metadata is helpful.
147-154: Validation approach looks correct.The two-tier validation strategy is sound:
- Lines 147-154: Hard validation for hex format and size ensures the transaction won't be rejected by rippled
- Lines 192-204: XLS-89 compliance validation emits warnings without failing the transaction
This allows users to submit metadata that isn't XLS-89 compliant (with warnings) while preventing format/size issues that would cause transaction failure.
Based on learnings: This separation between format validation (errors) and standard compliance (warnings) is intentional.
Also applies to: 192-204
3 participants
High Level Overview of Change
MPTokenMetadatatype andvalidateMPTokenMetadatafunction to reflect changes being made to XLS-89Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
Added and updated unit tests