Support for Dynamic MPT (XLS-94d)#877
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds DynamicMPT support across issuance creation, issuance updates, validation, tests, exports, and CI configuration, plus a changelog entry. ChangesDynamicMPT Feature Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xrpl/utils/time_conversions.py (1)
16-18: Fix MAX_XRPL_TIME off-by-one error by setting it to 232 - 1.**XRPL time is a 32-bit unsigned integer counting seconds since the "Ripple Epoch": 2000-01-01 00:00:00 UTC. A 32-bit unsigned integer has a maximum value of 2^32 - 1, not 2^32.
Currently,
MAX_XRPL_TIME = 2**32combined with the> MAX_XRPL_TIMEchecks allows the value 2^32 to pass validation, even though it cannot be represented in a 32-bit unsigned integer. This causes an overflow.Changing
MAX_XRPL_TIMEto2**32 - 1(4,294,967,295) fixes the boundary error while preserving the existing comparison logic. The test file lacks explicit boundary tests for these edge cases—consider adding tests that verify 2^32 - 1 is accepted and 2^32 is rejected.- MAX_XRPL_TIME: Final[int] = 2**32 - """The maximum time that can be expressed on the XRPL""" + MAX_XRPL_TIME: Final[int] = 2**32 - 1 + """The maximum Ripple time offset (in seconds) expressible on the XRPL (2^32 - 1)"""xrpl/core/binarycodec/definitions/definitions.json (1)
3370-3377: Add XChainAddAccountCreateAttestation transaction type code to definitions.jsonXChainAddAccountCreateAttestation is a supported XRPL transaction type, yet it's missing from the TRANSACTION_TYPES mapping in definitions.json. The model class, enum, and tests all exist in the codebase, and codec fixtures contain examples of this transaction type. Without the mapping, binary encoding/decoding will fail for this transaction.
Add to xrpl/core/binarycodec/definitions/definitions.json at line ~3375 (the numeric gap between 45 and 47 indicates code 46):
"XChainAddClaimAttestation": 45, + "XChainAddAccountCreateAttestation": 46, "XChainModifyBridge": 47
🧹 Nitpick comments (10)
CHANGELOG.md (1)
8-8: Nit: unify bracket style for section headers.Headers mix single and double brackets (e.g., “[[Unreleased]]” vs “[4.2.0]”). Consider standardizing for consistency.
tests/unit/models/transactions/test_mptoken_issuance_create.py (1)
131-159: Good coverage for valid, zero, and reserved‑bit cases.Consider two quick additions:
- Assert negative mutable_flags (e.g., -1) is rejected.
- Assert serialization includes the proper XRPL field name (MutableFlags) via
to_dict().Happy to draft these test cases if useful.
xrpl/models/transactions/mptoken_issuance_create.py (1)
183-195: Build the valid mask programmatically to avoid drift.Hard-coding the OR of every enum member is easy to miss during future updates. Derive the mask from the enum at runtime (or define a single module-level constant computed from the enum) to keep it source-of-truth. The suggested refactoring is appropriate:
- valid_mutable_flags = ( - MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_CAN_LOCK.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_REQUIRE_AUTH.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_CAN_ESCROW.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_CAN_TRADE.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_CAN_TRANSFER.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_CAN_CLAWBACK.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_METADATA.value - | MPTokenIssuanceCreateMutableFlag.TMF_MPT_CAN_MUTATE_TRANSFER_FEE.value - ) + valid_mutable_flags = 0 + for f in MPTokenIssuanceCreateMutableFlag: + valid_mutable_flags |= f.valueAlso verified: The binary-codec definition uses "type": "UInt32" for MutableFlags, confirming an unsigned 32-bit type as intended.
tests/unit/models/transactions/test_mptoken_issuance_set.py (2)
52-56: Avoid asserting the entire error dict stringAsserting exact string of a dict is brittle (ordering/formatting). Prefer substring checks or parsing.
Apply:
- self.assertEqual( - error.exception.args[0], - "{'flags': \"flag conflict: both TF_MPT_LOCK and TF_MPT_UNLOCK can't be set" - '"}', - ) + self.assertIn("TF_MPT_LOCK and TF_MPT_UNLOCK", error.exception.args[0])
157-166: Conflict tests are solidClear coverage of set/clear flag conflicts. Consider adding a test for “unknown/reserved bits” once validation is added in the model (see model comment).
Also applies to: 167-176
xrpl/models/transactions/mptoken_issuance_set.py (2)
139-144: Docstring nit“apply to all any accounts” → “apply to all accounts holding MPTs.”
- If omitted, this transaction will apply to all any accounts holding MPTs. + If omitted, this transaction applies to all accounts holding MPTs.
198-206: Flags + dynamic fieldsYou allow flags=0 with dynamic fields. If the intent is “no Flags at all,” consider requiring flags is None. If flags=0 is intentional, add a brief comment to document it.
- if has_dynamic_fields and self.flags is not None: + # flags=0 allowed for backwards-compat; non-zero disallowed + if has_dynamic_fields and self.flags is not None:tests/integration/transactions/test_mptoken_issuance_dynamic_mpt.py (3)
1-1: Spec reference consistencyDocstring says “XLS-94”; PR title uses “XLS-94d”. Align naming to avoid confusion.
-"""Integration tests for DynamicMPT (XLS-94) feature.""" +"""Integration tests for DynamicMPT (XLS-94d) feature."""
22-29: Avoid duplicating ledger flag constants in testsHardcoding LSF_* can drift from source. Prefer importing from a single module (if available) or deriving from the model’s enum to reduce duplication.
50-58: Tx->mpt_issuance_id extractionAccessing meta["mpt_issuance_id"] is central to these tests. If upstream ever renames this, tests will fail. Consider a small helper to fetch it with a clearer error when missing.
Also applies to: 99-105, 156-160, 216-220, 263-267, 340-344, 404-408, 456-459
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/integration/transactions/test_mptoken_issuance_dynamic_mpt.py(1 hunks)tests/unit/models/transactions/test_mptoken_issuance_create.py(2 hunks)tests/unit/models/transactions/test_mptoken_issuance_set.py(2 hunks)xrpl/core/binarycodec/definitions/definitions.json(4 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/mptoken_issuance_create.py(3 hunks)xrpl/models/transactions/mptoken_issuance_set.py(5 hunks)xrpl/utils/time_conversions.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/unit/models/transactions/test_mptoken_issuance_create.py (4)
xrpl/models/transactions/mptoken_issuance_create.py (3)
MPTokenIssuanceCreate(89-208)MPTokenIssuanceCreateFlag(25-37)MPTokenIssuanceCreateMutableFlag(40-69)xrpl/models/base_model.py (1)
is_valid(295-302)tests/unit/models/transactions/test_mptoken_issuance_set.py (1)
test_tx_mutable_flags_zero_fails(148-155)xrpl/models/exceptions.py (1)
XRPLModelException(6-9)
tests/integration/transactions/test_mptoken_issuance_dynamic_mpt.py (9)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (2)
sign_and_reliable_submission_async(209-245)test_async_and_sync(296-400)xrpl/models/requests/account_objects.py (2)
AccountObjects(48-71)AccountObjectType(19-43)xrpl/models/requests/tx.py (1)
Tx(20-87)xrpl/models/transactions/mptoken_issuance_create.py (3)
MPTokenIssuanceCreate(89-208)MPTokenIssuanceCreateFlag(25-37)MPTokenIssuanceCreateMutableFlag(40-69)xrpl/models/transactions/mptoken_issuance_set.py (2)
MPTokenIssuanceSet(127-298)MPTokenIssuanceSetMutableFlag(46-111)xrpl/utils/str_conversions.py (1)
str_to_hex(4-16)xrpl/wallet/main.py (2)
classic_address(34-41)address(24-29)xrpl/models/response.py (1)
is_successful(74-82)
tests/unit/models/transactions/test_mptoken_issuance_set.py (5)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/mptoken_issuance_set.py (3)
MPTokenIssuanceSet(127-298)MPTokenIssuanceSetFlag(26-43)MPTokenIssuanceSetMutableFlag(46-111)xrpl/utils/str_conversions.py (1)
str_to_hex(4-16)xrpl/models/base_model.py (1)
is_valid(295-302)tests/unit/models/transactions/test_mptoken_issuance_create.py (1)
test_tx_mutable_flags_zero_fails(142-148)
xrpl/models/transactions/__init__.py (2)
xrpl/models/transactions/mptoken_issuance_create.py (1)
MPTokenIssuanceCreateMutableFlag(40-69)xrpl/models/transactions/mptoken_issuance_set.py (2)
MPTokenIssuanceSet(127-298)MPTokenIssuanceSetMutableFlag(46-111)
xrpl/models/transactions/mptoken_issuance_set.py (3)
xrpl/models/transactions/transaction.py (2)
Transaction(184-549)has_flag(393-419)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-70)xrpl/models/utils.py (2)
require_kwargs_on_init(317-370)validate_mptoken_metadata(145-275)
⏰ 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: Integration test (3.9)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
.ci-config/rippled.cfg (1)
209-209: Confirm amendment name + rippled version; ensure features apply in CI.Please verify:
- The amendment identifier is exactly “DynamicMPT” upstream, and the CI rippled image includes it (rippled PR #5705 or later).
- If CI runs rippled in standalone, the [features] stanza may be ignored (noted in Lines 91–92). Ensure your harness enables the amendment effectively (e.g., by using a compatible image or alternative enablement).
CHANGELOG.md (1)
10-13: LGTM — clear user‑facing note for XLS‑94d.xrpl/utils/time_conversions.py (1)
26-26: Docstrings correctly updated to 2000‑01‑01T00:00Z Ripple Epoch.Also applies to: 56-56, 79-79, 111-111
xrpl/models/transactions/mptoken_issuance_create.py (2)
40-71: New MutableFlag enum looks good and aligns with field/flag mutability intent.
137-144: Public fieldmutable_flagsaddition is documented and non‑breaking.tests/unit/models/transactions/test_mptoken_issuance_create.py (1)
6-10: Import surface looks right after re‑exporting the new enum.xrpl/models/transactions/__init__.py (1)
53-54: Public export additions look goodNew mutable-flag enums are properly imported and re-exported. No issues spotted.
Also applies to: 60-61, 177-178, 182-183
tests/unit/models/transactions/test_mptoken_issuance_set.py (1)
148-156: Good coverage for zero mutable_flagsCovers the invalid zero case; aligns with model validation.
xrpl/core/binarycodec/definitions/definitions.json (1)
683-692: MutableFlags field additionAdding MutableFlags (UInt32, signing) aligns with the new model fields. LGTM.
tests/integration/transactions/test_mptoken_issuance_dynamic_mpt.py (2)
61-78: Assertions are focused and robustGood use of uppercase hex comparisons and explicit field presence/absence checks. Looks solid.
Also applies to: 112-116, 191-195, 245-247, 373-381, 433-435, 484-485
335-361: End-to-end combo update test is valuableCombining flag updates with transfer fee exercises key interactions. Nice coverage.
…ons for amendment PRs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/pull_request_template.md (1)
44-47: Nice addition; tighten the ask with a spec link and examples.Consider linking the exact XLS-94d section and naming a few typical tem* you expect covered (e.g., temINVALID_FLAG, temBAD_TRANSFER_FEE, temMALFORMED) so authors know the bar and reviewers can verify quickly.
tests/unit/models/transactions/test_mptoken_issuance_set.py (2)
181-246: DRY the set/clear conflict tests with parametrization.You can cover all pairs in one test using subTests to cut repetition and speed up maintenance.
@@ - def test_mutable_flags_set_clear_can_lock_conflict(self): - ... - def test_mutable_flags_set_clear_require_auth_conflict(self): - ... - # (repeated per pair) + def test_mutable_flags_set_clear_conflicts(self): + pairs = [ + (MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_LOCK, + MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_LOCK, + "CAN_LOCK"), + (MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_REQUIRE_AUTH, + MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_REQUIRE_AUTH, + "REQUIRE_AUTH"), + (MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_ESCROW, + MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_ESCROW, + "CAN_ESCROW"), + (MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_TRADE, + MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_TRADE, + "CAN_TRADE"), + (MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_TRANSFER, + MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_TRANSFER, + "CAN_TRANSFER"), + (MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_CLAWBACK, + MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_CLAWBACK, + "CAN_CLAWBACK"), + ] + for set_flag, clear_flag, name in pairs: + with self.subTest(name=name), self.assertRaises(XRPLModelException) as err: + MPTokenIssuanceSet( + account=_ACCOUNT, + mptoken_issuance_id=_TOKEN_ID, + mutable_flags=set_flag | clear_flag, + ) + self.assertIn(f"Cannot set and clear {name}", err.exception.args[0])
13-15: Silence false-positive secret detectors on test constants.Token ID is a fixture, not a credential. Add an inline suppression to avoid CI noise.
-_TOKEN_ID = "000004C463C52827307480341125DA0577DEFC38405B0E3E" +_TOKEN_ID = "000004C463C52827307480341125DA0577DEFC38405B0E3E" # noqa: S105 # gitleaks:allowxrpl/models/transactions/mptoken_issuance_set.py (2)
213-230: Avoid recomputing the valid_mask on every call.Hoist a module‑level constant derived from the Enum to reduce duplication and keep source‑of‑truth centralized.
+from functools import reduce @@ -class MPTokenIssuanceSetMutableFlag(int, Enum): +class MPTokenIssuanceSetMutableFlag(int, Enum): ... +# Bitmask of all valid TMF bits +_VALID_MUTABLE_FLAG_MASK: Final[int] = reduce( + lambda acc, f: acc | f.value, MPTokenIssuanceSetMutableFlag, 0 +) @@ - valid_mask = ( - MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_LOCK.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_LOCK.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_REQUIRE_AUTH.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_REQUIRE_AUTH.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_ESCROW.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_ESCROW.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_TRADE.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_TRADE.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_TRANSFER.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_TRANSFER.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_SET_CAN_CLAWBACK.value - | MPTokenIssuanceSetMutableFlag.TMF_MPT_CLEAR_CAN_CLAWBACK.value - ) - if self.mutable_flags & ~valid_mask: + if self.mutable_flags & ~_VALID_MUTABLE_FLAG_MASK: errors["mutable_flags"] = "mutable_flags contains invalid bits"
145-166: Docs: small nits.
- “apply to all any accounts” → “apply to all accounts”.
- Double‑check flag names in prose (“TMF_MPT_CAN_MUTATE_*”) match the spec’s canonical names.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/pull_request_template.md(1 hunks)tests/unit/models/transactions/test_mptoken_issuance_create.py(2 hunks)tests/unit/models/transactions/test_mptoken_issuance_set.py(3 hunks)xrpl/core/binarycodec/definitions/definitions.json(2 hunks)xrpl/models/transactions/mptoken_issuance_set.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/transactions/test_mptoken_issuance_create.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T18:43:34.412Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#840
File: xrpl/core/binarycodec/definitions/definitions.json:2594-2602
Timestamp: 2025-05-13T18:43:34.412Z
Learning: The `definitions.json` file in XRPL is a generated file and should not be directly edited. Constraints like maximum array lengths are enforced at the model level (e.g., in transaction models) rather than in the binary codec layer.
Applied to files:
xrpl/core/binarycodec/definitions/definitions.json
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Applied to files:
xrpl/core/binarycodec/definitions/definitions.json
🧬 Code graph analysis (2)
tests/unit/models/transactions/test_mptoken_issuance_set.py (4)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/mptoken_issuance_set.py (3)
MPTokenIssuanceSet(127-316)MPTokenIssuanceSetFlag(26-43)MPTokenIssuanceSetMutableFlag(46-111)xrpl/utils/str_conversions.py (1)
str_to_hex(4-16)xrpl/models/base_model.py (1)
is_valid(295-302)
xrpl/models/transactions/mptoken_issuance_set.py (2)
xrpl/models/transactions/transaction.py (1)
has_flag(393-419)xrpl/models/utils.py (2)
require_kwargs_on_init(317-370)validate_mptoken_metadata(145-275)
🪛 Gitleaks (8.28.0)
tests/unit/models/transactions/test_mptoken_issuance_set.py
[high] 14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff (0.14.1)
tests/unit/models/transactions/test_mptoken_issuance_set.py
14-14: Possible hardcoded password assigned to: "_TOKEN_ID"
(S105)
⏰ 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 test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
xrpl/core/binarycodec/definitions/definitions.json (2)
3356-3359: Confirm these Vault entries came from the generator (no semantic changes).*Values appear unchanged; if this churn wasn’t produced by the defs generator, please revert to avoid noisy diffs in a generated file.
Based on learnings
683-692: All verification checks passed. MutableFlags, terADDRESS_COLLISION, and XChainAddAccountCreateAttestation are correctly defined and wired.tests/unit/models/transactions/test_mptoken_issuance_set.py (1)
61-74: Add a test to lock in behavior for flags=0 with dynamic fields.Model allows flags==0 when dynamic fields are present. If spec prefers “no Flags at all” in this mode, assert reject; otherwise, assert accept to prevent regressions.
xrpl/models/transactions/mptoken_issuance_set.py (1)
198-206: Confirm policy: allow Flags==0 with dynamic fields?Current logic rejects non‑zero Flags but accepts Flags==0 when any of mutable_flags/metadata/transfer_fee are set. Verify XLS-94d intent; if the intent is “no Flags in DynamicMPT mode,” reject any provided Flags (even zero). Otherwise, consider normalizing by dropping Flags on serialization.
Resolved conflicts in xrpld.cfg, CHANGELOG.md, definitions.json, mptoken_issuance_set.py, and test_mptoken_issuance_create.py. Adapted MPTokenIssuanceSet to main's refactors: dropped the removed require_kwargs_on_init import and lazy-import validate_mptoken_metadata from xrpl.utils.mptoken_metadata. Updated set/create metadata-warning test assertions to main's XLS-89 message format. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/models/transactions/test_mptoken_issuance_create.py (1)
108-135: 💤 Low valueTest name no longer matches what it asserts.
iconis present (Line 112) andasset_subclassisNone, so the warning being asserted is theasset_subclass-required-for-rwarule, not a missing-icon case. The nametest_tx_emits_warning_for_missing_icon_metadatamisrepresents the coverage and may mislead maintainers into thinking the missing-icon path is exercised.Suggested rename
- def test_tx_emits_warning_for_missing_icon_metadata(self): + def test_tx_emits_warning_for_missing_asset_subclass_metadata(self):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/models/transactions/test_mptoken_issuance_create.py` around lines 108 - 135, Rename the test function to reflect that it checks for the missing asset_subclass warning for rwa metadata rather than a missing icon; e.g., change the function name MPTokenIssuanceCreate test from test_tx_emits_warning_for_missing_icon_metadata to something like test_tx_emits_warning_for_missing_asset_subclass_for_rwa_metadata (update any references or docstring/comment in the test as needed) so the test name matches the asserted behavior in MPTokenIssuanceCreate.is_valid().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/models/transactions/test_mptoken_issuance_create.py`:
- Around line 108-135: Rename the test function to reflect that it checks for
the missing asset_subclass warning for rwa metadata rather than a missing icon;
e.g., change the function name MPTokenIssuanceCreate test from
test_tx_emits_warning_for_missing_icon_metadata to something like
test_tx_emits_warning_for_missing_asset_subclass_for_rwa_metadata (update any
references or docstring/comment in the test as needed) so the test name matches
the asserted behavior in MPTokenIssuanceCreate.is_valid().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88dda422-74a9-458e-adf3-869f821666e6
📒 Files selected for processing (7)
.ci-config/xrpld.cfgCHANGELOG.mdtests/unit/models/transactions/test_mptoken_issuance_create.pytests/unit/models/transactions/test_mptoken_issuance_set.pyxrpl/models/transactions/__init__.pyxrpl/models/transactions/mptoken_issuance_create.pyxrpl/models/transactions/mptoken_issuance_set.py
🚧 Files skipped from review as they are similar to previous changes (3)
- xrpl/models/transactions/init.py
- xrpl/models/transactions/mptoken_issuance_create.py
- xrpl/models/transactions/mptoken_issuance_set.py
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/models/transactions/test_mptoken_issuance_set.py (1)
232-254: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the metadata error text with the actual 1024-byte limit The guard allows exactly 1024 bytes (
> MAX_MPTOKEN_METADATA_LENGTH), so “less than 1024 bytes” is misleading. Change the message to “at most 1024 bytes” (or update the check if a strict< 1024limit is intended).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/models/transactions/test_mptoken_issuance_set.py` around lines 232 - 254, The metadata length validation in MPTokenIssuanceSet is inconsistent with the actual guard and max-size behavior. Update the error text asserted in test_metadata_too_long_fails to match the real limit enforced by mptoken_metadata validation (“at most 1024 bytes” if the current > MAX_MPTOKEN_METADATA_LENGTH check is correct), or adjust the MAX_MPTOKEN_METADATA_LENGTH check in MPTokenIssuanceSet if a strict under-1024-byte rule is intended. Keep test_metadata_at_max_length_valid aligned with the final rule.xrpl/models/transactions/mptoken_issuance_set.py (1)
53-81: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRenumber
MPTokenIssuanceSetMutableFlagto match rippled
TMF_MPT_SET_REQUIRE_AUTH,TMF_MPT_SET_CAN_ESCROW,TMF_MPT_SET_CAN_TRADE,TMF_MPT_SET_CAN_TRANSFER, andTMF_MPT_SET_CAN_CLAWBACKshould be0x00000004,0x00000010,0x00000040,0x00000100, and0x00000400; onlyTMF_MPT_SET_CAN_LOCKis aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xrpl/models/transactions/mptoken_issuance_set.py` around lines 53 - 81, The MPTokenIssuanceSetMutableFlag values are out of sync with rippled, so update the enum in MPTokenIssuanceSetMutableFlag to use the correct bit positions for TMF_MPT_SET_REQUIRE_AUTH, TMF_MPT_SET_CAN_ESCROW, TMF_MPT_SET_CAN_TRADE, TMF_MPT_SET_CAN_TRANSFER, and TMF_MPT_SET_CAN_CLAWBACK while leaving TMF_MPT_SET_CAN_LOCK unchanged. Make the change in the flag definitions themselves so all downstream serialization and validation in mptoken_issuance_set.py use the corrected constants.
🧹 Nitpick comments (2)
xrpl/models/transactions/mptoken_issuance_set.py (2)
203-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant metadata format validation runs even when a blocking error already exists.
When
mptoken_metadatais too long or not valid hex,errors["mptoken_metadata"]is already set at line 208/213, but the code still callsvalidate_mptoken_metadataand may emit awarnings.warnwith an overlapping/duplicate message (validate_mptoken_metadatare-checks hex format and length internally). This produces confusing double signals (a warning plus a blocking validation error for the same underlying problem) and does unnecessary work.♻️ Proposed refactor
if len(self.mptoken_metadata) > 0: if len(self.mptoken_metadata) > MAX_MPTOKEN_METADATA_LENGTH: errors["mptoken_metadata"] = ( "Metadata must be a hex string less than 1024 bytes " "(alternatively, 2048 hex characters)." ) elif not HEX_REGEX.fullmatch(self.mptoken_metadata): errors["mptoken_metadata"] = "Metadata must be a valid hex string" - - # Validate metadata format with warnings - # Lazy import to avoid circular dependency - from xrpl.utils.mptoken_metadata import validate_mptoken_metadata - - validation_messages = validate_mptoken_metadata(self.mptoken_metadata) - if len(validation_messages) > 0: - message = "\n".join( - [MPT_META_WARNING_HEADER] - + [f"- {msg}" for msg in validation_messages] - ) - warnings.warn(message, stacklevel=5) + else: + # Validate metadata format with warnings + # Lazy import to avoid circular dependency + from xrpl.utils.mptoken_metadata import validate_mptoken_metadata + + validation_messages = validate_mptoken_metadata( + self.mptoken_metadata + ) + if len(validation_messages) > 0: + message = "\n".join( + [MPT_META_WARNING_HEADER] + + [f"- {msg}" for msg in validation_messages] + ) + warnings.warn(message, stacklevel=5)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xrpl/models/transactions/mptoken_issuance_set.py` around lines 203 - 226, The metadata validation in mptoken_issuance_set.py is running duplicate checks even after a blocking error is already recorded. In the mptoken_metadata validation block inside the transaction model, skip the validate_mptoken_metadata/warnings.warn path whenever length or HEX_REGEX validation has already populated errors["mptoken_metadata"], and only emit warnings for values that passed the existing hard validation.
196-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_MAX_TRANSFER_FEEconstant.
_MAX_TRANSFER_FEE = 50000is now defined independently here and inmptoken_issuance_create.py. If the two ever drift,MPTokenIssuanceCreateandMPTokenIssuanceSetcould silently enforce different transfer-fee bounds. Consider moving this to a shared location (e.g.xrpl/models/utils.py, alongsideHEX_REGEX/MAX_MPTOKEN_METADATA_LENGTH) and importing it in both files.♻️ Proposed refactor
-_MAX_TRANSFER_FEE = 50000 +from xrpl.models.utils import MAX_MPT_TRANSFER_FEE as _MAX_TRANSFER_FEE(and remove the duplicate definition from
mptoken_issuance_create.py, replacing it with the same shared import)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xrpl/models/transactions/mptoken_issuance_set.py` around lines 196 - 201, The transfer-fee limit is duplicated via _MAX_TRANSFER_FEE in both MPTokenIssuanceCreate and MPTokenIssuanceSet, which can drift over time. Move the constant to a shared module such as xrpl/models/utils.py alongside existing shared validation constants, then import and use that shared value in both mptoken_issuance_create.py and mptoken_issuance_set.py. Remove the local duplicate definition so both classes enforce the same bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/models/transactions/test_mptoken_issuance_set.py`:
- Around line 232-254: The metadata length validation in MPTokenIssuanceSet is
inconsistent with the actual guard and max-size behavior. Update the error text
asserted in test_metadata_too_long_fails to match the real limit enforced by
mptoken_metadata validation (“at most 1024 bytes” if the current >
MAX_MPTOKEN_METADATA_LENGTH check is correct), or adjust the
MAX_MPTOKEN_METADATA_LENGTH check in MPTokenIssuanceSet if a strict
under-1024-byte rule is intended. Keep test_metadata_at_max_length_valid aligned
with the final rule.
In `@xrpl/models/transactions/mptoken_issuance_set.py`:
- Around line 53-81: The MPTokenIssuanceSetMutableFlag values are out of sync
with rippled, so update the enum in MPTokenIssuanceSetMutableFlag to use the
correct bit positions for TMF_MPT_SET_REQUIRE_AUTH, TMF_MPT_SET_CAN_ESCROW,
TMF_MPT_SET_CAN_TRADE, TMF_MPT_SET_CAN_TRANSFER, and TMF_MPT_SET_CAN_CLAWBACK
while leaving TMF_MPT_SET_CAN_LOCK unchanged. Make the change in the flag
definitions themselves so all downstream serialization and validation in
mptoken_issuance_set.py use the corrected constants.
---
Nitpick comments:
In `@xrpl/models/transactions/mptoken_issuance_set.py`:
- Around line 203-226: The metadata validation in mptoken_issuance_set.py is
running duplicate checks even after a blocking error is already recorded. In the
mptoken_metadata validation block inside the transaction model, skip the
validate_mptoken_metadata/warnings.warn path whenever length or HEX_REGEX
validation has already populated errors["mptoken_metadata"], and only emit
warnings for values that passed the existing hard validation.
- Around line 196-201: The transfer-fee limit is duplicated via
_MAX_TRANSFER_FEE in both MPTokenIssuanceCreate and MPTokenIssuanceSet, which
can drift over time. Move the constant to a shared module such as
xrpl/models/utils.py alongside existing shared validation constants, then import
and use that shared value in both mptoken_issuance_create.py and
mptoken_issuance_set.py. Remove the local duplicate definition so both classes
enforce the same bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0afb1c69-9402-435d-b786-6a8fe0e5659b
📒 Files selected for processing (5)
tests/integration/transactions/test_mptoken_issuance_dynamic_mpt.pytests/unit/models/transactions/test_mptoken_issuance_create.pytests/unit/models/transactions/test_mptoken_issuance_set.pyxrpl/models/transactions/mptoken_issuance_create.pyxrpl/models/transactions/mptoken_issuance_set.py
🚧 Files skipped from review as they are similar to previous changes (2)
- xrpl/models/transactions/mptoken_issuance_create.py
- tests/unit/models/transactions/test_mptoken_issuance_create.py
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| # A transaction "mutates the issuance" if it sets any capability flag, | ||
| # updates metadata / transfer_fee, or declares immutability. | ||
| has_capability_flag = any(self.has_flag(f) for f in _CAPABILITY_SET_FLAGS) | ||
| is_mutate = ( |
There was a problem hiding this comment.
⚪ Severity: LOW
The domain_id field (an issuance-level mutation that changes the associated Permissioned Domain) is omitted from the is_mutate check. This allows domain_id to bypass the explicit authorization guards, permitting it to be combined with holder (line 197) or TF_MPT_LOCK/TF_MPT_UNLOCK (line 204) — combinations the code was designed to reject for all issuance-mutating operations.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add self.domain_id is not None to the is_mutate check so that domain_id (an issuance-level mutation) is treated the same as mptoken_metadata, transfer_fee, and immutable_flags. Also update the error messages in the holder and flags guards to mention domain_id.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| is_mutate = ( | |
| is_mutate = ( | |
| has_capability_flag | |
| or self.mptoken_metadata is not None | |
| or self.transfer_fee is not None | |
| or self.immutable_flags is not None | |
| or self.domain_id is not None | |
| ) |
3 participants
High Level Overview of Change
Add support of Dynamic MPT (XLS-94d)
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan