Conversation
|
This PR needs to be updated to match the latest Dynamic MPT spec and xrpld changes. |
|
/ai-review |
![ripple-code-reviewer[bot]](https://avatars.githubusercontent.com/u/3165523?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-tested implementation of DynamicMPT (XLS-94) flag decoding and UI surfacing. Two things worth a second look: the TX_FLAGS map for MPTokenIssuanceSet drops the old 0x1000/0x2000 bit mappings entirely rather than keeping them alongside the renamed ones, and the new 'Mutable' pill logic in Settings.tsx wires up immutableFlag for six of the seven capability flags but skips canConfidentialAmount even though a corresponding lsifMPTCanHoldConfidentialBalance bit exists in the immutable-flags map.
There was a problem hiding this comment.
Well-structured addition of DynamicMPT (XLS-94) support: new ImmutableFlags decoding on MPTokenIssuanceCreate/MPTokenIssuanceSet/MPTokenIssuance, and a 'Mutable' badge UI in the MPT Settings panel. I cross-checked all the new bit-flag maps (MPT_IMMUTABLE_FLAGS in transactionUtils.ts, MPT_ISSUANCE_IMMUTABLE_FLAGS in rippled/lib/utils.ts, and the immutableFlag wiring in Settings.tsx) against each other and against the new test fixtures/mock data — the bit values and flag names are internally consistent, and the added/updated unit tests match the implemented badge-visibility logic (capability enable-only, badge hidden once enabled or once locked, field rows for Metadata/TransferFee hidden once locked). No correctness, security, or resource-handling bugs found in the changed lines. Two low-confidence/low-value items worth a quick look below.
There was a problem hiding this comment.
Solid, well-tested implementation of DynamicMPT (XLS-94) decoding and UI. Main finding: the ImmutableFlags bit-to-name mapping is defined twice with parallel but independently-maintained tables (tif* names in transactionUtils.ts and lsif* names in rippled/lib/utils.ts), which is a duplication risk if rippled ever changes/adds a bit value. No correctness or security issues found in the changed lines otherwise.
There was a problem hiding this comment.
The core DynamicMPT decoding/UI logic (immutable flags on MPTokenIssuanceCreate/Set, Settings page badges) looks correct and is well covered by tests. The one thing worth fixing before merge is that the ImmutableFlags bit-to-name tables are duplicated verbatim (same bit values, different name prefixes) across two files instead of being derived from a single source, which the comments themselves acknowledge ("Same bit values as the ledger lsif* map"). Everything else (badge logic, parser wiring, i18n key usage) is consistent with existing patterns.
There was a problem hiding this comment.
Solid feature addition: decodes MutableFlags/ImmutableFlags on MPTokenIssuanceCreate/Set and the MPTokenIssuance ledger object, and adds a clear Mutable/Immutable UI on the MPT Settings panel. Bit-value mappings for the new capability flags were cross-checked against the Settings component's flag→immutableFlag wiring and all line up correctly (no copy-paste mismatch found). The one thing worth tightening before merge is that the tif*/lsif* immutable-flag bit constants are defined twice in two different files with identical values — a future spec tweak to one map without the other would silently desync ledger-object decoding from transaction decoding.
| "resolved": "https://registry.npmjs.org/xrpl/-/xrpl-4.6.0.tgz", | ||
| "integrity": "sha512-0nXZfqDHRJ6bsDv1WtA9MdCYalMtXuxVa9mtLdqT3xypRKf2LwT5DbuGL/kHcVfuqk3B+ly+SFARlrnX+LHtRQ==", | ||
| "version": "5.1.0", | ||
| "resolved": "https://artifactory.ops.ripple.com/artifactory/api/npm/ripple-npm/xrpl/-/xrpl-5.1.0.tgz", |
There was a problem hiding this comment.
Artifactory URLs on xrpl deps block external installs. Regenerate package-lock.json against public npm registry:
Ensure .npmrc points to https://registry.npmjs.org, then:
rm -rf node_modules package-lock.json && npm install
Or confirm https://artifactory.ops.ripple.com is publicly reachable before merging.
There was a problem hiding this comment.
This MR wires up DynamicMPT (XLS-94) decoding and UI across the MPTokenIssuanceCreate/Set transactions and the MPT ledger object/Settings page. The logic is consistent and well tested (bit values, badge visibility rules, and translations line up correctly across the files I checked). Two things worth a second look before merging: the ImmutableFlags bit-to-name map is now duplicated verbatim in two separate files, and one capability's enabled-flag name doesn't match its immutable-lock-flag name — worth a quick sanity check against the XLS-94 spec/rippled source to make sure that's intentional and not a copy-paste slip.
| label={t('immutable_flags')} | ||
| className="flag" | ||
| data-testid="mpt-mutable-flags" | ||
| > | ||
| {immutableFlags.map((flag) => ( |
There was a problem hiding this comment.
Why is the label is for immutable_flags but the data-testid is for mpt-mutable-flags?
| <SimpleRow | ||
| label={t('immutable_flags')} | ||
| className="flag" | ||
| data-testid="mpt-mutable-flags" |
There was a problem hiding this comment.
Same issue mentioned earlier: the label says "immutable flags" but the data-setid is "mpt-mutable-flags"?
There was a problem hiding this comment.
The diff adds decoding/display for DynamicMPT (XLS-94) ImmutableFlags/MutableFlags across MPTokenIssuanceCreate, MPTokenIssuanceSet, and the MPT Settings page. The flag-bit maps for tif*/lsif* are consistent with each other and with the test mock data, and the new Settings pill logic (Enabled / Disabled+Mutable / Disabled+Immutable) matches the documented enable-only semantics. Two minor issues worth a look: a type-safety risk from dropping the custom encryption-key type extension in MPTokenIssuanceSet's Simple view, and duplicated immutable-flag bit maps across two files that could drift out of sync over time. The large xrpl/package-lock bump appears justified since changed files directly consume the updated xrpl types (MPTokenIssuanceCreate/Set) needed for the new fields.
There was a problem hiding this comment.
Solid feature addition wiring up DynamicMPT's MutableFlags/ImmutableFlags decoding and the new mutable/immutable pill UI in Settings.tsx. Two things worth a second look before merge: a naming asymmetry in the new capability→immutableFlag mapping that breaks the otherwise consistent lsf→lsif pattern (possible typo/wrong constant), and a major xrpl dependency version bump that should be confirmed as intentional/required for the new fields rather than an incidental jump.
There was a problem hiding this comment.
This is a well-structured, thoroughly tested implementation of DynamicMPT (XLS-94) support. I traced the new bit-flag maps (MPT_IMMUTABLE_FLAGS / MPT_ISSUANCE_IMMUTABLE_FLAGS / the new tfMPTSet* capability flags) across transactionUtils.ts, rippled/lib/utils.ts, the MPTokenIssuanceCreate/Set parsers and Simple views, and the Settings component, and the bit values are internally consistent (the ledger lsif*/tx tif* maps intentionally mirror the lsf*/Flags bit positions 1:1, while the Set tfMPTSet* capability flags occupy a separate, non-colliding range in the Flags field). Test coverage for the new Settings pill logic (enabled/disabled/mutable/immutable combinations) and the new Simple-view rows is comprehensive and matches the implementation. The xrpl package major-version bump is directly required by this change (new ImmutableFlags/MutableFlags typings) so it's in-scope, not a stray dependency change. I did not find any correctness, security, or consistency issues in the changed lines that clear the bar for flagging — nothing here looks like a bug likely to bite users in production.
There was a problem hiding this comment.
Well-structured feature addition for DynamicMPT (XLS-94). I traced all the new bit-flag maps (MPT_IMMUTABLE_FLAGS in transactionUtils.ts, MPT_ISSUANCE_IMMUTABLE_FLAGS in rippled/lib/utils.ts, and the tfMPTSetCan* capability flags in TX_FLAGS.MPTokenIssuanceSet) against each other and against the new mock-data fixtures — bit values are internally consistent and the test assertions (e.g. 196610 = tifMPTCanLock|tifMPTMetadata|tifMPTTransferFee, 16 = tifMPTCanTrade) check out correctly. The xrpl package bump to ^5.1.0 (and its transitive lockfile churn) is directly required by this change since MPTokenIssuanceSet/MPTokenIssuanceCreate types now need ImmutableFlags/TransferFee/MPTokenMetadata fields, so it's in-scope and not flagged per the dependency-drift rule. The Settings.tsx pill logic (Enabled vs Disabled+Mutable/Immutable) matches the enable-only capability semantics described in the PR, and relative import paths for the newly wired utils/transactionUtils modules resolve correctly. No correctness, security, or resource-leak issues found in the changed lines; the showcase script's hardcoded genesis secret is the well-known public rippled standalone-mode credential, not a real secret.
3 participants
High Level Overview of Change
MutableFlagsfield onMPTokenIssuanceCreateand theMPTokenIssuanceledger object, plus the newMutableFlags / MPTokenMetadata / TransferFeefields onMPTokenIssuanceSet.Mutablepill on capabilities the issuer can still enabled, hidden once a capability is enabled (capabilities are enable-only in the merged scheme), plusMutablerows forMetadataandTransfer Fee.Context of Change
Spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0094-dynamic-MPT
Corresponding merged rippled PRs:
#5705 – Support DynamicMPT XLS-94d (core implementation)
#5797 – Rename mutable flags for DynamicMPT XLS-94d
#5820 – Rename flags for DynamicMPT
#6834 – Fix DynamicMPT for Attackathon
#7439 – Remove clear mutable flags for DynamicMPT XLS-94
Type of Change
Before / After
MPTokenIssuanceCreate — with ImmutableFlags
MPTokenIssuanceSet — enable CanEscrow + CanTrade
MPTokenIssuanceSet — update metadata + TransferFee + lock via ImmutableFlags
MPT page with new Settings