Support for Dynamic MPTs (XLS-94D) #3081
Conversation
WalkthroughAdds DynamicMPT to CI config, a new serialized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (5)
packages/xrpl/HISTORY.md (1)
7-9: Standardize XLS casing and add spec linkUse the conventional lowercase “d” and link the spec for consistency with prior entries.
-* Support for `Dynamic MPT` (XLS-94D) +* Support for `Dynamic MPT` ([XLS-94d](https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0094-dynamic-MPT))packages/xrpl/src/models/ledger/MPTokenIssuance.ts (2)
18-59: Separate flag maps for Flags vs MutableFlags to avoid misuseCombining base flags and “can‑mutate” booleans in one interface invites accidental mixing. Define a distinct interface for MutableFlags and use clearer naming in call sites.
-export interface MPTokenIssuanceFlagsInterface { +export interface MPTokenIssuanceFlagsInterface { lsfMPTLocked?: boolean lsfMPTCanLock?: boolean lsfMPTRequireAuth?: boolean lsfMPTCanEscrow?: boolean lsfMPTCanTrade?: boolean lsfMPTCanTransfer?: boolean lsfMPTCanClawback?: boolean - /** - * Indicates flag lsfMPTCanLock can be changed - */ - lsfMPTCanMutateCanLock?: boolean +} + +export interface MPTokenIssuanceMutableFlagsInterface { + /** Indicates flag lsfMPTCanLock can be changed */ + lsfMPTCanMutateCanLock?: boolean /** Indicates flag lsfMPTRequireAuth can be changed */ lsfMPTCanMutateRequireAuth?: boolean /** Indicates flag lsfMPTCanEscrow can be changed */ lsfMPTCanMutateCanEscrow?: boolean /** Indicates flag lsfMPTCanTrade can be changed */ lsfMPTCanMutateCanTrade?: boolean /** Indicates flag lsfMPTCanTransfer can be changed */ lsfMPTCanMutateCanTransfer?: boolean /** Indicates flag lsfMPTCanClawback can be changed */ lsfMPTCanMutateCanClawback?: boolean /** Allows field MPTokenMetadata to be modified */ lsfMPTCanMutateMetadata?: boolean /** Allows field TransferFee to be modified */ lsfMPTCanMutateTransferFee?: boolean }
61-102: Split enums for clarity: Flags vs MutableFlags share bit positionsReusing the same bit values for two different bitmaps is correct but confusing in a single enum. Create a second enum (MPTokenIssuanceMutableFlags) for the MutableFlags field and leave MPTokenIssuanceFlags for Flags only.
-export enum MPTokenIssuanceFlags { +export enum MPTokenIssuanceFlags { lsfMPTLocked = 0x00000001, lsfMPTCanLock = 0x00000002, lsfMPTRequireAuth = 0x00000004, lsfMPTCanEscrow = 0x00000008, lsfMPTCanTrade = 0x00000010, lsfMPTCanTransfer = 0x00000020, lsfMPTCanClawback = 0x00000040, - - /** - * Indicates flag lsfMPTCanLock can be changed - */ - lsfMPTCanMutateCanLock = 0x00000002, - /** Indicates flag lsfMPTRequireAuth can be changed */ - lsfMPTCanMutateRequireAuth = 0x00000004, - /** Indicates flag lsfMPTCanEscrow can be changed */ - lsfMPTCanMutateCanEscrow = 0x00000008, - /** Indicates flag lsfMPTCanTrade can be changed */ - lsfMPTCanMutateCanTrade = 0x00000010, - /** Indicates flag lsfMPTCanTransfer can be changed */ - lsfMPTCanMutateCanTransfer = 0x00000020, - /** Indicates flag lsfMPTCanClawback can be changed */ - lsfMPTCanMutateCanClawback = 0x00000040, - /** Allows field MPTokenMetadata to be modified */ - lsfMPTCanMutateMetadata = 0x00010000, - /** Allows field TransferFee to be modified */ - lsfMPTCanMutateTransferFee = 0x00020000, } + +export enum MPTokenIssuanceMutableFlags { + /** Indicates flag lsfMPTCanLock can be changed */ + lsfMPTCanMutateCanLock = 0x00000002, + /** Indicates flag lsfMPTRequireAuth can be changed */ + lsfMPTCanMutateRequireAuth = 0x00000004, + /** Indicates flag lsfMPTCanEscrow can be changed */ + lsfMPTCanMutateCanEscrow = 0x00000008, + /** Indicates flag lsfMPTCanTrade can be changed */ + lsfMPTCanMutateCanTrade = 0x00000010, + /** Indicates flag lsfMPTCanTransfer can be changed */ + lsfMPTCanMutateCanTransfer = 0x00000020, + /** Indicates flag lsfMPTCanClawback can be changed */ + lsfMPTCanMutateCanClawback = 0x00000040, + /** Allows field MPTokenMetadata to be modified */ + lsfMPTCanMutateMetadata = 0x00010000, + /** Allows field TransferFee to be modified */ + lsfMPTCanMutateTransferFee = 0x00020000, +}packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts (1)
87-88: Arithmetic addition of flags could lead to incorrect valuesUsing arithmetic addition (
+) to combine flags is error-prone and could result in incorrect values if the same flag is accidentally included twice. Bitwise OR should be used for combining flags.Use bitwise OR to combine flags:
- MutableFlags: - MPTokenIssuanceCreateFlags.tfMPTCanMutateTransferFee + - MPTokenIssuanceCreateFlags.tfMPTCanMutateCanTransfer, + MutableFlags: + MPTokenIssuanceCreateFlags.tfMPTCanMutateTransferFee | + MPTokenIssuanceCreateFlags.tfMPTCanMutateCanTransfer,packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
30-77: Documentation style inconsistencyThe new flags use C-style comments (
/* */) while the existing code uses JSDoc style (/** */). This creates inconsistency in the documentation format.Convert to JSDoc style for consistency:
- // The below flags are used in the MutableFlags field - /* - ** Sets the lsfMPTCanLock flag. Enables the token to be locked both individually and globally. - */ + // The below flags are used in the MutableFlags field + /** + * Sets the lsfMPTCanLock flag. Enables the token to be locked both individually and globally. + */ tfMPTSetCanLock = 0x00000001,Apply this pattern to all the new flag documentation comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.ci-config/rippled.cfg(1 hunks)packages/ripple-binary-codec/src/enums/definitions.json(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/LedgerEntry.ts(2 hunks)packages/xrpl/src/models/ledger/MPTokenIssuance.ts(1 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(3 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts(3 hunks)packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts(1 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(1 hunks)packages/xrpl/test/models/MPTokenIssuanceSet.test.ts(1 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/MPTokenIssuanceSet.test.tspackages/xrpl/src/models/ledger/MPTokenIssuance.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.tspackages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts
🧬 Code graph analysis (4)
packages/xrpl/src/models/ledger/LedgerEntry.ts (2)
packages/xrpl/src/models/ledger/MPTokenIssuance.ts (1)
MPTokenIssuance(3-16)packages/xrpl/src/models/ledger/index.ts (1)
MPTokenIssuance(65-65)
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
packages/xrpl/src/models/transactions/index.ts (1)
MPTokenIssuanceSetFlags(65-65)
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
packages/xrpl/src/models/transactions/index.ts (1)
MPTokenIssuanceCreateFlags(59-59)
packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts (4)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
MPTokenIssuanceCreate(100-142)packages/xrpl/test/integration/utils.ts (1)
testTransaction(240-290)packages/xrpl/src/models/transactions/metadata.ts (1)
TransactionMetadata(95-108)packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
MPTokenIssuanceSet(144-160)
⏰ 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 (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (9)
packages/xrpl/src/models/ledger/MPTokenIssuance.ts (1)
14-16: Is MutableFlags always present on-ledger?Interface marks MutableFlags as required. Confirm rippled always emits this field; if not, make it optional to avoid unnecessary ts friction when decoding historical or pre‑amendment data.
- MutableFlags: number + MutableFlags?: numberpackages/xrpl/src/models/ledger/LedgerEntry.ts (3)
13-13: Import looks goodImporting MPTokenIssuance here is appropriate.
50-51: Union extension LGTMIncluding MPTokenIssuance in the LedgerEntry union aligns with the new ledger object.
66-68: No change required — 'mpt_issuance' and 'mptoken' are the canonical rippled filter keys.Confirmed: rippled/XLS docs and the public API mapping list
MPTokenIssuance -> mpt_issuanceandMPToken -> mptoken, and the repo's ledger/ledgerEntry interfaces/tests already use those keys. (github.com).ci-config/rippled.cfg (1)
191-197: Confirm rippled image/tag and that the [features] stanza is honored (one DynamicMPT entry found)Found a single "DynamicMPT" at .ci-config/rippled.cfg:196. The CI rippled image/tag wasn't located by the prior search — verify the rippled image/tag used by CI and that the container applies the config's [features] stanza (not started in a mode that ignores it).
- Locate rippled image references in the repo: rg -n --hidden -S 'image:.rippled' --glob '**/.yml' --glob '/*.yaml' --glob '/Dockerfile' || true
- Inspect the features block and nearby lines: sed -n '184,204p' .ci-config/rippled.cfg && rg -n '^[features]' .ci-config/rippled.cfg || true
- Check CI startup/entrypoint for flags that override/ignore config: rg -n --hidden -S 'rippled.(--conf|--cfg|--standalone|--run)' --glob '**/.yml' --glob '**/Dockerfile' || true
If the rippled build used by CI lacks DynamicMPT, bump the rippled image/tag to a build that includes DynamicMPT or add the amendment ID explicitly to the config.
packages/ripple-binary-codec/src/enums/definitions.json (1)
683-692: MutableFlags (UInt32, nth=53) — unique in definitions.jsonVerified: 51 UInt32 entries; no duplicate nth values; MutableFlags is at FIELDS/68 with nth=53.
Action: verify that rippled's codec maps MutableFlags to field code 53 to avoid signing/encode mismatches.packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
31-31: Test uses potentially incorrect flag valueBased on the flag collision issue identified in the source file, this test might be using an incorrect flag value. If the mutable flags are updated to use distinct values, this test will need to be updated accordingly.
Once the flag values are corrected in the source file, update this test to use the new value for
tfMPTCanMutateTransferFee.packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
48-53: Good test coverage for MutableFlagsThe test properly validates that transactions with
MutableFlagsare accepted by the validator, which aligns with the Dynamic MPT specification.packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts (1)
79-138: Comprehensive integration test for Dynamic MPTThis test effectively validates the Dynamic MPT (XLS-94D) functionality by:
- Creating an MPT with mutable flags
- Setting a transfer fee dynamically
- Clearing the transfer capability
The test flow properly exercises the new mutability features.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
188-209: Bug: TransferFee presence check ignores 0-value caseDocs say “must NOT be present if tfMPTCanTransfer is not set.” Current check uses truthiness, so TransferFee=0 slips through.
Apply this diff:
- if (tx.TransferFee && !isTfMPTCanTransfer) { + if (tx.TransferFee !== undefined && !isTfMPTCanTransfer) { throw new ValidationError( 'MPTokenIssuanceCreate: TransferFee cannot be provided without enabling tfMPTCanTransfer flag', ) }Additionally, consider requiring an integer value:
- validateOptionalField(tx, 'TransferFee', isNumber) + validateOptionalField(tx, 'TransferFee', isNumber) + if (typeof tx.TransferFee === 'number' && !Number.isInteger(tx.TransferFee)) { + throw new ValidationError('MPTokenIssuanceCreate: TransferFee must be an integer') + }
♻️ Duplicate comments (2)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
57-64: Flag value “collisions” are OK here (separate bitfields)These tfMPTCanMutate* values matching the base flags is intentional since Flags vs MutableFlags are distinct fields. No change needed.
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
205-213: Add conflict checks for Set/Clear pairs in MutableFlagsPrevent simultaneously setting and clearing the same property in one tx.
Apply this diff:
if ( typeof tx.MPTokenMetadata === 'string' && (!isHex(tx.MPTokenMetadata) || tx.MPTokenMetadata.length / 2 > MAX_MPT_META_BYTE_LENGTH) ) { throw new ValidationError( `MPTokenIssuanceSet: MPTokenMetadata (hex format) must be non-empty and no more than ${MAX_MPT_META_BYTE_LENGTH} bytes.`, ) } + + if (typeof tx.MutableFlags === 'number') { + const mf = tx.MutableFlags + const pairs: Array<[number, number, string]> = [ + [MPTokenIssuanceSetFlags.tfMPTSetCanLock, MPTokenIssuanceSetFlags.tfMPTClearCanLock, 'CanLock'], + [MPTokenIssuanceSetFlags.tfMPTSetRequireAuth, MPTokenIssuanceSetFlags.tfMPTClearRequireAuth, 'RequireAuth'], + [MPTokenIssuanceSetFlags.tfMPTSetCanEscrow, MPTokenIssuanceSetFlags.tfMPTClearCanEscrow, 'CanEscrow'], + [MPTokenIssuanceSetFlags.tfMPTSetCanTrade, MPTokenIssuanceSetFlags.tfMPTClearCanTrade, 'CanTrade'], + [MPTokenIssuanceSetFlags.tfMPTSetCanTransfer, MPTokenIssuanceSetFlags.tfMPTClearCanTransfer, 'CanTransfer'], + [MPTokenIssuanceSetFlags.tfMPTSetCanClawback, MPTokenIssuanceSetFlags.tfMPTClearCanClawback, 'CanClawback'], + ] + for (const [setBit, clearBit, name] of pairs) { + if ((mf & setBit) !== 0 && (mf & clearBit) !== 0) { + throw new ValidationError( + `MPTokenIssuanceSet: MutableFlags conflict for ${name} (set and clear both specified)`, + ) + } + } + }
🧹 Nitpick comments (3)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
163-164: Tighten validation: require integer for bitmasksMutableFlags is a bitmask; accept only finite integers (e.g., Number.isInteger) and, ideally, constrain to UInt32.
Apply this diff to align with other integer checks:
-import { +import { BaseTransaction, GlobalFlagsInterface, validateBaseTransaction, validateOptionalField, isString, - isNumber, + isNumber, + isInteger, MAX_MPT_META_BYTE_LENGTH, MPT_META_WARNING_HEADER, validateMPTokenMetadata, } from './common' @@ - validateOptionalField(tx, 'MutableFlags', isNumber) + validateOptionalField(tx, 'MutableFlags', isInteger) + if (typeof tx.MutableFlags === 'number') { + if (tx.MutableFlags < 0 || tx.MutableFlags > 0xffffffff) { + throw new ValidationError( + 'MPTokenIssuanceCreate: MutableFlags out of range (must be 0..0xFFFFFFFF)', + ) + } + }packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (2)
176-179: Tighten types: integer-only for MutableFlagsSame as Create: MutableFlags should be finite UInt32.
Apply this diff:
- validateOptionalField(tx, 'MutableFlags', isNumber) + validateOptionalField(tx, 'MutableFlags', isInteger) + if (typeof tx.MutableFlags === 'number') { + if (tx.MutableFlags < 0 || tx.MutableFlags > 0xffffffff) { + throw new ValidationError( + 'MPTokenIssuanceSet: MutableFlags out of range (must be 0..0xFFFFFFFF)', + ) + } + }
197-203: Minor: enforce integer TransferFeeServer expects integral steps; guard against decimals/NaN.
Apply this diff:
if (typeof tx.TransferFee === 'number') { + if (!Number.isFinite(tx.TransferFee) || !Number.isInteger(tx.TransferFee)) { + throw new ValidationError('MPTokenIssuanceSet: TransferFee must be a finite integer') + } if (tx.TransferFee < 0 || tx.TransferFee > MAX_TRANSFER_FEE) { throw new ValidationError( `MPTokenIssuanceSet: TransferFee must be between 0 and ${MAX_TRANSFER_FEE}`, ) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(5 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts(7 hunks)packages/xrpl/test/models/MPTokenIssuanceSet.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/models/MPTokenIssuanceSet.test.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.389Z
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.
📚 Learning: 2025-09-16T05:00:20.389Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.389Z
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/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.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/transactions/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.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/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.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/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
🧬 Code graph analysis (2)
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (4)
packages/xrpl/src/models/transactions/common.ts (4)
validateOptionalField(412-432)isString(141-143)isNumber(151-153)MAX_MPT_META_BYTE_LENGTH(26-26)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
MAX_TRANSFER_FEE(19-19)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/utils/index.ts (1)
isHex(60-62)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
packages/xrpl/src/models/transactions/common.ts (2)
validateOptionalField(412-432)isNumber(151-153)
⏰ 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). (6)
- GitHub Check: browser (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (2)
19-19: Good: centralize the TransferFee limitExporting MAX_TRANSFER_FEE for reuse is the right move and avoids magic numbers across transactions.
140-142: Confirm: Should MPTokenIssuanceCreate accept MutableFlags?Spec-wise, MutableFlags typically belong to the Set transaction. If Create does not support it on-ledger, keeping it in the interface may confuse users or produce invalid tx blobs.
Would you like me to remove MutableFlags from Create, or document explicitly that it’s ignored by rippled on Create?
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (4)
17-18: Good reuse: import MAX_TRANSFER_FEEAvoids duplication and keeps fee limits consistent with Create.
90-141: Potential confusion: booleans for MutableFlags are on Flags interfaceMPTokenIssuanceSetFlagsInterface includes tfMPTSet*/tfMPTClear* booleans (which belong to MutableFlags). Verify the builder splits these into tx.MutableFlags and does not place them under tx.Flags.
If splitting isn’t implemented, I can propose a minimal helper to compute both fields from the interface.
161-164: LGTM: new optional fields are addedPublic surface matches spec additions.
33-81: Clarify separation: these flags must end up in MutableFlags, not FlagsMPTokenIssuanceSetFlagsInterface includes both regular and "Set/Clear" mutable booleans, but I didn't find code that converts those mutable booleans into tx.MutableFlags during serialization; tests set MutableFlags as a numeric value. Verify that callers/serialization (e.g. setTransactionFlagsToNumber / convertTxFlagsToNumber / client.autofill) map the tfMPTSet*/tfMPTClear* booleans into MutableFlags, or require callers to pass MutableFlags numerically.
Relevant locations: packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts, packages/xrpl/src/models/utils/flags.ts (setTransactionFlagsToNumber / convertTxFlagsToNumber / parseTransactionFlags), and tests under packages/xrpl/test/models/MPTokenIssuanceSet.test.ts and packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts.
|
I'm working on fixing the integration and browser tests, however I'd love to get a round of code-reviews. I'm not entirely sure why the integ-tests are not connecting to the local rippled standalone instance. |
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 (1)
.github/workflows/nodejs.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (22.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.github/workflows/nodejs.yml (1)
221-230: DRY & replace presence-only Docker check with health-based waitReplace the step in .github/workflows/nodejs.yml (around lines 221–230) with the health-wait below and extract it to .github/actions/wait-for-rippled; call that action from other jobs (e.g., any browser/integration jobs) to avoid flakes.
- - name: Check if Docker container is running - run: | - docker ps - docker ps -a - docker inspect rippled-service - if ! docker ps | grep -q rippled-service; then - echo "rippled-service Docker container is not running" - exit 1 - fi + - name: Wait for rippled-service to be healthy + run: | + set -Eeuo pipefail + if ! docker ps --filter 'name=^/rippled-service$' --format '{{.Names}}' | grep -qx rippled-service; then + echo "rippled-service container not found." + docker ps -a || true + exit 1 + fi + for i in {1..40}; do + status="$(docker inspect -f '{{.State.Health.Status}}' rippled-service || echo unknown)" + echo "Attempt $i: health=$status" + [ "$status" = "healthy" ] && break || sleep 3 + done + if [ "${status:-unknown}" != "healthy" ]; then + echo "::error::rippled-service failed to become healthy." + docker inspect -f '{{json .State.Health}}' rippled-service || true + docker logs --tail 200 rippled-service || true + exit 1 + fi
| - name: Check if Docker container is running | ||
| run: | | ||
| docker ps | ||
| docker ps -a | ||
| docker inspect rippled-service | ||
| if ! docker ps | grep -q rippled-service; then | ||
| echo "rippled-service Docker container is not running" | ||
| exit 1 | ||
| fi | ||
There was a problem hiding this comment.
🧩 Analysis chain
Wait for rippled to be healthy, not merely “running”.
The grep on docker ps only checks container presence and can pass while health is starting/unhealthy, which matches the author’s symptom (“integration tests not connecting”). Poll the container health and surface logs on failure.
Apply this diff to replace the step with a health-aware wait + diagnostics:
- - name: Check if Docker container is running
- run: |
- docker ps
- docker ps -a
- docker inspect rippled-service
- if ! docker ps | grep -q rippled-service; then
- echo "rippled-service Docker container is not running"
- exit 1
- fi
+ - name: Wait for rippled-service to be healthy
+ run: |
+ set -Eeuo pipefail
+ echo "Checking rippled-service container existence..."
+ if ! docker ps --filter 'name=^/rippled-service$' --format '{{.Names}}' | grep -qx rippled-service; then
+ echo "rippled-service container not found in running containers."
+ docker ps -a || true
+ exit 1
+ fi
+ echo "Waiting for rippled-service (health=healthy)..."
+ for i in {1..40}; do
+ status="$(docker inspect -f '{{.State.Health.Status}}' rippled-service || echo unknown)"
+ echo "Attempt $i: health=$status"
+ if [ "$status" = "healthy" ]; then
+ echo "rippled-service is healthy."
+ break
+ fi
+ sleep 3
+ done
+ if [ "${status:-unknown}" != "healthy" ]; then
+ echo "::error::rippled-service failed to become healthy in time."
+ echo "=== docker ps ==="
+ docker ps -a || true
+ echo "=== inspect (Health) ==="
+ docker inspect -f '{{json .State.Health}}' rippled-service || true
+ echo "=== last 200 logs ==="
+ docker logs --tail 200 rippled-service || true
+ exit 1
+ fiOptional follow-up outside this hunk: make container teardown non-fatal to avoid masking earlier failures.
# change step "Stop docker container"
run: docker stop rippled-service || trueTo verify, re-run the workflow; the step should show “health=healthy” before tests start. If it times out, the emitted logs will pinpoint why rippled isn’t ready.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Searching for workflow file and snippet..."
rg -n --hidden --glob '!node_modules' 'Check if Docker container is running' || true
echo "Check for .github/workflows/nodejs.yml existence..."
if [ -f .github/workflows/nodejs.yml ]; then
echo ".github/workflows/nodejs.yml found; printing lines 140-180:"
nl -ba .github/workflows/nodejs.yml | sed -n '140,180p'
else
echo ".github/workflows/nodejs.yml not found; searching for nodejs.yml and docker inspect usage..."
rg -n --hidden --glob '!node_modules' 'nodejs.yml|docker inspect|rippled-service' || true
fiLength of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='.github/workflows/nodejs.yml'
echo "PWD: $(pwd)"
if [ -f "$FILE" ]; then
echo "$FILE exists; size: $(stat -c%s "$FILE" || true) bytes"
echo "---- lines 150-170 ----"
sed -n '150,170p' "$FILE" || true
echo "---- lines 156-166 ----"
sed -n '156,166p' "$FILE" || true
echo "---- search for keywords in file ----"
if command -v rg >/dev/null 2>&1; then
rg -n 'rippled-service|Check if Docker container is running|Wait for rippled-service' "$FILE" || true
else
grep -nE 'rippled-service|Check if Docker container is running|Wait for rippled-service' "$FILE" || true
fi
else
echo "$FILE not found; searching repo for matches"
if command -v rg >/dev/null 2>&1; then
rg -n 'rippled-service|docker inspect|Check if Docker container is running' || true
else
grep -RIn --exclude-dir=node_modules -E 'rippled-service|docker inspect|Check if Docker container is running' . || true
fi
fiLength of output: 2336
Wait for rippled-service to be healthy, not just running
The current step only checks container presence; it can pass while health is "starting"/"unhealthy", causing integration tests to fail—poll .State.Health and surface logs on timeout.
Location: .github/workflows/nodejs.yml — replace the step at lines 156–166 (duplicate at 221–227).
- - name: Check if Docker container is running
- run: |
- docker ps
- docker ps -a
- docker inspect rippled-service
- if ! docker ps | grep -q rippled-service; then
- echo "rippled-service Docker container is not running"
- exit 1
- fi
+ - name: Wait for rippled-service to be healthy
+ run: |
+ set -Eeuo pipefail
+ echo "Checking rippled-service container existence..."
+ if ! docker ps --filter 'name=^/rippled-service$' --format '{{.Names}}' | grep -qx rippled-service; then
+ echo "rippled-service container not found in running containers."
+ docker ps -a || true
+ exit 1
+ fi
+ echo "Waiting for rippled-service (health=healthy)..."
+ for i in {1..40}; do
+ status="$(docker inspect -f '{{.State.Health.Status}}' rippled-service || echo unknown)"
+ echo "Attempt $i: health=$status"
+ if [ "$status" = "healthy" ]; then
+ echo "rippled-service is healthy."
+ break
+ fi
+ sleep 3
+ done
+ if [ "${status:-unknown}" != "healthy" ]; then
+ echo "::error::rippled-service failed to become healthy in time."
+ echo "=== docker ps ==="
+ docker ps -a || true
+ echo "=== inspect (Health) ==="
+ docker inspect -f '{{json .State.Health}}' rippled-service || true
+ echo "=== last 200 logs ==="
+ docker logs --tail 200 rippled-service || true
+ exit 1
+ fiOptional operational change: make teardown non-fatal to avoid masking earlier failures — change any run: docker stop rippled-service to run: docker stop rippled-service || true (lines ~172 and ~236).
There was a problem hiding this comment.
It is not useful to poll for the existence of the rippled docker container. If the docker container failed to start-up correctly, there is no chance for it to restart in a healthy manner.
There was a problem hiding this comment.
@ckeshava You're absolutely right that polling won't help a failed container restart. I think there was a misunderstanding in my suggestion - I wasn't proposing to restart failed containers, but rather to wait for an already-started container to reach "healthy" status before running tests.
However, since you've identified that as unnecessary, let me suggest a simpler diagnostic approach that might help identify why integration tests aren't connecting:
- - name: Check if Docker container is running
- run: |
- docker ps
- docker ps -a
- docker inspect rippled-service
- if ! docker ps | grep -q rippled-service; then
- echo "rippled-service Docker container is not running"
- exit 1
- fi
+ - name: Check rippled-service connectivity
+ run: |
+ docker ps
+ if ! docker ps | grep -q rippled-service; then
+ echo "rippled-service container not found"
+ docker ps -a
+ exit 1
+ fi
+ echo "Testing rippled connectivity on port 6006..."
+ curl -f http://localhost:6006 || {
+ echo "Failed to connect to rippled on port 6006"
+ docker logs --tail 50 rippled-service
+ exit 1
+ }This approach simply verifies the service is actually accessible on the expected port and shows logs if it's not, which should help diagnose the connection issues without unnecessary waiting.
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: .github/workflows/nodejs.yml:157-168
Timestamp: 2025-09-22T20:30:43.864Z
Learning: ckeshava prefers not to poll for Docker container health status in CI workflows, believing that failed containers won't recover through polling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Why do we need to test the curl command? If the docker ps response does not list the rippled-service container, why do I need other ways to detect the existence of the container?
There was a problem hiding this comment.
@ckeshava You're absolutely right - if docker ps shows the rippled-service container is running, that should be sufficient to confirm it exists and is operational.
The curl test was intended to distinguish between "container exists" and "service is accessible," which could help diagnose cases where the container runs but the rippled process inside has issues or port mapping failed. However, you make a valid point that this adds unnecessary complexity.
If docker ps confirms the container is running, the integration tests should be able to connect. If they still can't connect despite the container being present, that would likely indicate a different issue (network configuration, test setup, etc.) that wouldn't be solved by additional container checks.
Your current approach of simply verifying container presence with docker ps and failing fast if it's not there is the most straightforward solution.
･ﾟ✧*:･ﾟ✧
🐰 ( curious )
∪ ∪
🧠 Learnings used
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: .github/workflows/nodejs.yml:157-168
Timestamp: 2025-09-22T20:30:43.864Z
Learning: ckeshava prefers not to poll for Docker container health status in CI workflows, believing that failed containers won't recover through polling.
| tfMPTCanTransfer?: boolean | ||
| tfMPTCanClawback?: boolean | ||
|
|
||
| tmfMPTCanMutateCanLock?: boolean |
There was a problem hiding this comment.
These flags are only available for MutableFlags field. Should it be separated into its own interface, similar to what we did with SetFlag options in AccountSet?
There was a problem hiding this comment.
- The flags associated with Dynamic MPTs have
tmf...in their prefix. This is used to indicatetransaction-mutable-flag. This difference in names (tf...versustmf...) should help users differentiate between the intent behind the flags. - If we use a separate interface for these flags, it could potentially cause confusion. The xrpl.org documentation (and the rippled cpp code, other programming language clients) organize these flags under the
MPTokenIssuanceCreatetransaction's flags. This change would have been more impactful if it was done at the XLS level, instead of only xrpl.js deviating from the specification.
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 (1)
.github/workflows/nodejs.yml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-09-22T20:30:43.864Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: .github/workflows/nodejs.yml:157-168
Timestamp: 2025-09-22T20:30:43.864Z
Learning: ckeshava prefers not to poll for Docker container health status in CI workflows, believing that failed containers won't recover through polling.
Applied to files:
.github/workflows/nodejs.yml
⏰ 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: unit (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: build-and-lint (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
| - name: Check if Docker container is running | ||
| id: check-docker-container | ||
| run: | | ||
| if ! docker ps | grep -q rippled-service; then | ||
| echo "rippled-service Docker container is not running" | ||
| echo "docker-container-running=false" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "docker-container-running=true" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
Fail fast when rippled-service never launched
Right now, if docker ps doesn’t find rippled-service, the step exits 0 after setting docker-container-running=false. That lets the job go green while completely skipping the integration/browser tests, so we lose coverage exactly when the rippled container failed to start — the situation we most need to catch.
Drop the “false” path and fail the step instead. That keeps the fast‑fail behavior you expect (no polling) and surfaces the container startup error immediately.
- if ! docker ps | grep -q rippled-service; then
- echo "rippled-service Docker container is not running"
- echo "docker-container-running=false" >> $GITHUB_OUTPUT
- else
- echo "docker-container-running=true" >> $GITHUB_OUTPUT
- fi
- docker inspect ${{ steps.run-docker.outputs.docker-container-id }}
+ if ! docker ps --filter 'name=^/rippled-service$' --format '{{.Names}}' | grep -qx rippled-service; then
+ echo "::error::rippled-service Docker container is not running"
+ docker ps -a || true
+ exit 1
+ fi
+ echo "docker-container-running=true" >> $GITHUB_OUTPUT
+ docker inspect ${{ steps.run-docker.outputs.docker-container-id }}Repeat the same fix in the browser job block (Lines 229‑237).
Also applies to: 229-237
🤖 Prompt for AI Agents
In .github/workflows/nodejs.yml around lines 160-168 (and similarly lines
229-237), the check for the rippled-service Docker container currently writes
docker-container-running=false and exits 0 when the container is missing,
allowing the job to continue and silently skip tests; change the false path to
fail the step instead: remove the branch that emits
"docker-container-running=false" and instead exit with non-zero (or use a direct
fail) and emit a descriptive error so the workflow stops immediately when
rippled-service is not running; make the same change in the browser job block at
lines 229-237.
High Level Overview of Change
Specification is located here: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0094-dynamic-MPT
CPP implementation can be found here: XRPLF/rippled#5705
Type of Change
Did you update HISTORY.md?
Test Plan
A few unit and integration tests have been added.