Add encode_mptoken_metadata and decode_mptoken_metadata utility functions #880
Conversation
WalkthroughAdds XLS-89 MPTokenMetadata TypedDicts and a new utils module providing encode/decode/validate helpers, removes prior validation from models.utils, lazy-loads validation in transaction models, and updates fixtures and tests to the URIs/compact-key schema. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (app/tests)
participant Encode as encode_mptoken_metadata()
participant Shorten as _shorten_keys
participant JSON as Deterministic JSON
participant Hex as Hex Encoder
Note over Encode,Shorten: encode path (new)
Caller->>Encode: pass MPTokenMetadata (long form)
Encode->>Shorten: validate & map long→short (uris recursive)
Shorten->>JSON: deterministic sort & stringify
JSON->>Hex: bytes → uppercase hex
Hex->>Caller: return hex string
sequenceDiagram
autonumber
participant Caller as Caller (app/tests/ledger)
participant Decode as decode_mptoken_metadata()
participant HexParse as Hex → JSON parser
participant Expand as _expand_keys
participant Output as MPTokenMetadata (long form)
Note over Decode,Expand: decode path (new)
Caller->>Decode: pass hex string
Decode->>HexParse: validate hex, parse JSON
HexParse->>Expand: map short→long keys (uris recursive)
Expand->>Output: return normalized MPTokenMetadata
Output->>Caller: normalized dict/object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
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 (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 (1)
tests/unit/models/transactions/test_mptoken_issuance_create.py (1)
126-131: Consider updating the test name.The warning message assertion is correct and validates the XLS-89 requirement for
asset_subclasswhenasset_classis "rwa". However, the test nametest_tx_emits_warning_for_missing_icon_metadatano longer accurately describes what's being tested.Consider renaming to something like:
-def test_tx_emits_warning_for_missing_icon_metadata(self): +def test_tx_emits_warning_for_missing_asset_subclass(self):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)tests/unit/models/mptoken-metadata-encode-decode-fixtures.json(1 hunks)tests/unit/models/mptoken-metadata-validation-fixtures.json(22 hunks)tests/unit/models/test_utils.py(3 hunks)tests/unit/models/transactions/test_mptoken_issuance_create.py(4 hunks)tests/unit/models/transactions/test_vault_create.py(2 hunks)xrpl/models/__init__.py(2 hunks)xrpl/models/mptoken_metadata.py(1 hunks)xrpl/models/transactions/mptoken_issuance_create.py(1 hunks)xrpl/models/transactions/vault_create.py(1 hunks)xrpl/models/utils.py(0 hunks)xrpl/utils/__init__.py(2 hunks)xrpl/utils/mptoken_metadata.py(1 hunks)
💤 Files with no reviewable changes (1)
- xrpl/models/utils.py
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-06-04T22:17:47.164Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Applied to files:
xrpl/models/mptoken_metadata.pyxrpl/models/__init__.pyxrpl/utils/mptoken_metadata.pytests/unit/models/transactions/test_mptoken_issuance_create.pytests/unit/models/test_utils.pyxrpl/models/transactions/mptoken_issuance_create.py
📚 Learning: 2025-02-11T21:11:19.326Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, the UINT192 type from the MPT amendment is translated to UInt192, following the same pattern as other non-hash UINT types.
Applied to files:
xrpl/models/mptoken_metadata.py
📚 Learning: 2024-10-17T17:45:46.828Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Applied to files:
xrpl/utils/__init__.py
📚 Learning: 2024-10-30T20:26:32.925Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Applied to files:
tests/unit/models/transactions/test_mptoken_issuance_create.pyxrpl/models/transactions/vault_create.pyxrpl/models/transactions/mptoken_issuance_create.pytests/unit/models/transactions/test_vault_create.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
tests/unit/models/transactions/test_mptoken_issuance_create.pyxrpl/models/transactions/mptoken_issuance_create.py
📚 Learning: 2025-09-25T21:33:12.379Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 870
File: tests/unit/core/binarycodec/types/test_issue.py:126-149
Timestamp: 2025-09-25T21:33:12.379Z
Learning: In XRPL Binary Format for MPT currencies, the sequence data is stored and transmitted in little-endian format on the wire, but interpreted in big-endian format for JSON representation. The current test in test_from_raw_hex_mpt correctly uses little-endian sequence bytes for serialization and converts to big-endian for the expected JSON mpt_issuance_id.
Applied to files:
tests/unit/models/transactions/test_mptoken_issuance_create.py
📚 Learning: 2024-10-30T20:36:02.813Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
Applied to files:
tests/unit/models/transactions/test_mptoken_issuance_create.pyxrpl/models/transactions/mptoken_issuance_create.py
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
Learning: The xrpl-py codebase contains many string-typed numeric fields in transaction models (like loan_origination_fee, maximum_amount, signature_reward, debt_maximum, etc.) and consistently uses direct int() calls for validation without try-catch blocks, allowing ValueError exceptions to bubble up naturally for clear error communication.
Applied to files:
tests/unit/models/transactions/test_mptoken_issuance_create.pytests/unit/models/test_utils.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
tests/unit/models/transactions/test_mptoken_issuance_create.pytests/unit/models/test_utils.pyxrpl/models/transactions/vault_create.pyxrpl/models/transactions/mptoken_issuance_create.py
📚 Learning: 2024-10-30T20:32:03.246Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_accept.py:27-39
Timestamp: 2024-10-30T20:32:03.246Z
Learning: In the `xrpl` codebase, when defining required fields in dataclasses (e.g., `account: str = REQUIRED`), it's necessary to include `# type: ignore` to prevent `mypy` errors.
Applied to files:
tests/unit/models/test_utils.py
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
Learning: In xrpl-py transaction models, numeric string fields are validated using direct int() calls without try-catch blocks in _get_errors methods. This is the established pattern across the codebase for consistency and performance reasons, and ValueError exceptions provide clear error communication to users.
Applied to files:
tests/unit/models/test_utils.py
📚 Learning: 2025-09-15T15:54:25.836Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.836Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Applied to files:
tests/unit/models/test_utils.py
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
Learning: In xrpl-py, numeric string fields in transaction models typically use direct int() calls for validation without try-catch blocks. The team prefers to let ValueError exceptions bubble up naturally rather than wrapping them in try-catch for field-level error aggregation, prioritizing code simplicity and performance.
Applied to files:
tests/unit/models/test_utils.py
📚 Learning: 2024-11-01T18:53:01.394Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Applied to files:
tests/unit/models/test_utils.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/models/transactions/vault_create.pyxrpl/models/transactions/mptoken_issuance_create.py
🧬 Code graph analysis (7)
xrpl/models/__init__.py (1)
xrpl/models/mptoken_metadata.py (2)
MPTokenMetadata(50-162)MPTokenMetadataUri(6-47)
xrpl/utils/__init__.py (1)
xrpl/utils/mptoken_metadata.py (3)
decode_mptoken_metadata(219-270)encode_mptoken_metadata(292-336)validate_mptoken_metadata(339-397)
xrpl/utils/mptoken_metadata.py (2)
xrpl/models/mptoken_metadata.py (1)
MPTokenMetadata(50-162)xrpl/utils/str_conversions.py (2)
hex_to_str(19-31)str_to_hex(4-16)
tests/unit/models/transactions/test_mptoken_issuance_create.py (1)
xrpl/utils/mptoken_metadata.py (1)
encode_mptoken_metadata(292-336)
tests/unit/models/test_utils.py (2)
xrpl/models/utils.py (1)
_is_kw_only_attr_defined_in_dataclass(108-124)xrpl/utils/mptoken_metadata.py (2)
decode_mptoken_metadata(219-270)encode_mptoken_metadata(292-336)
xrpl/models/transactions/vault_create.py (1)
xrpl/utils/mptoken_metadata.py (1)
validate_mptoken_metadata(339-397)
xrpl/models/transactions/mptoken_issuance_create.py (1)
xrpl/utils/mptoken_metadata.py (1)
validate_mptoken_metadata(339-397)
🪛 Ruff (0.14.2)
xrpl/utils/mptoken_metadata.py
243-243: Avoid specifying long messages outside the exception class
(TRY003)
248-250: Avoid specifying long messages outside the exception class
(TRY003)
253-253: Prefer TypeError exception for invalid type
(TRY004)
253-253: Avoid specifying long messages outside the exception class
(TRY003)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
369-369: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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.14)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.12)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (15)
xrpl/utils/__init__.py (2)
5-9: LGTM!The imports are clean and follow the existing module pattern. The three MPToken metadata utilities are now accessible via the public
xrpl.utilsAPI.
43-45: LGTM!The public API additions correctly expose the new MPToken metadata utilities.
xrpl/models/transactions/vault_create.py (1)
129-131: LGTM!The lazy import pattern appropriately resolves circular dependency concerns while maintaining the validation functionality. The import is only triggered when
mptoken_metadatais present, minimizing overhead.CHANGELOG.md (2)
10-13: LGTM!The changelog entry clearly documents the new encoding/decoding utilities and references the XLS-89 standard.
17-18: Breaking changes documented with clear rationale.The changelog appropriately flags the breaking changes to
MPTokenMetadatatype andvalidate_mptoken_metadatafunction. The rationale for releasing as a patch version (XLS-89 still evolving) is clearly stated.tests/unit/models/transactions/test_vault_create.py (2)
91-97: LGTM!The test data correctly reflects the new XLS-89 metadata structure. The
urisarray intentionally omits theurifield to trigger validation warnings, which aligns with the test's purpose.
115-123: LGTM!The warning message assertion correctly validates the new XLS-89 compliance requirements for the
urisfield structure.xrpl/models/transactions/mptoken_issuance_create.py (1)
133-135: LGTM!The lazy import pattern is consistently applied across transaction models and appropriately resolves circular dependency concerns.
tests/unit/models/test_utils.py (3)
9-14: LGTM!The import reorganization improves readability, and the new imports appropriately support the MPToken metadata encoding/decoding tests.
79-79: LGTM!The fixture file reference is appropriately updated to reflect the new naming convention.
101-121: LGTM!The new test class properly validates the encode/decode roundtrip for MPToken metadata. The test structure follows existing patterns and provides good coverage using fixture-driven testing with
subTestfor isolation.tests/unit/models/transactions/test_mptoken_issuance_create.py (2)
8-8: LGTM!Excellent refactoring to use
encode_mptoken_metadatainstead of manual hex encoding. This ensures consistency and leverages the dedicated utility for proper metadata encoding.Also applies to: 30-30
108-111: LGTM!The test data correctly reflects XLS-89 requirements. Setting
asset_subclasstoNonewhileasset_classis "rwa" appropriately triggers the validation warning that the test expects.xrpl/models/__init__.py (2)
8-8: LGTM!The import correctly brings in the new MPToken metadata types from the dedicated module.
23-24: LGTM!The public API additions correctly expose the new MPToken metadata types, making them accessible via
xrpl.models.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
xrpl/utils/mptoken_metadata.py (4)
12-173: Validation logic looks solid; past isinstance issue resolved.The validation functions correctly check for both long and compact forms and enforce field constraints. The previous critical issue with
isinstance(..., Dict)has been fixed—all type checks now correctly use the built-indicttype.Optional: Pre-compile regex for ticker validation.
The regex pattern on line 16 is compiled on every
_validate_ticker()call. For better performance, consider compiling it once at module level:+_TICKER_PATTERN = re.compile(r"^[A-Z0-9]{1,6}$") + def _validate_ticker(obj: Dict[str, Any]) -> List[str]: if "ticker" in obj and "t" in obj: return ["ticker/t: both long and compact forms present. expected only one."] value = obj.get("ticker") if "ticker" in obj else obj.get("t") - if not isinstance(value, str) or not re.compile(r"^[A-Z0-9]{1,6}$").match(value): + if not isinstance(value, str) or not _TICKER_PATTERN.match(value): return [ "ticker/t: should have uppercase letters (A-Z) and digits (0-9) " "only. Max 6 characters recommended." ] return []
257-270: Optional: Clarify redundant handling of both long and compact forms.Lines 257–262 expand nested objects in
uris, and lines 263–268 do the same forus. After_expand_keys(line 255), compact keys should already be expanded to long form, sousshould not exist unless the input had bothusanduris(which is invalid per the spec but not rejected by the decoder).This defensive handling is acceptable for a lenient decoder, but consider adding a comment explaining why both are processed, or explicitly choosing to prefer one form and discard the other for clarity.
318-330: Optional: Clarify redundant handling of both long and compact forms.Similar to the decoder, this code handles both
urisandusafter shortening (lines 318–323 and 325–330). After_shorten_keys, long-form keys should already be converted to compact, so only one form should exist unless the input had both (which is invalid but not rejected by the encoder).Consider adding a comment or explicitly preferring one form for clarity.
355-362: Optional: Use integer division for clarity.Line 359 uses float division
/followed byint(). For integer arithmetic, use the//operator directly.Apply this diff:
validation_messages.append( ( "MPTokenMetadata must be max " - f"{int(MAX_MPTOKEN_METADATA_LENGTH / 2)} bytes." + f"{MAX_MPTOKEN_METADATA_LENGTH // 2} bytes." ) )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
xrpl/utils/mptoken_metadata.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-04T22:17:47.164Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Applied to files:
xrpl/utils/mptoken_metadata.py
🧬 Code graph analysis (1)
xrpl/utils/mptoken_metadata.py (2)
xrpl/models/mptoken_metadata.py (1)
MPTokenMetadata(50-162)xrpl/utils/str_conversions.py (2)
hex_to_str(19-31)str_to_hex(4-16)
🪛 Ruff (0.14.2)
xrpl/utils/mptoken_metadata.py
243-243: Avoid specifying long messages outside the exception class
(TRY003)
248-250: Avoid specifying long messages outside the exception class
(TRY003)
253-253: Prefer TypeError exception for invalid type
(TRY004)
253-253: Avoid specifying long messages outside the exception class
(TRY003)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
369-369: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
|
Integration tests are failing |
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 (5)
CHANGELOG.md(1 hunks)tests/integration/transactions/test_mptoken_issuance_create.py(2 hunks)xrpl/models/transactions/mptoken_issuance_create.py(2 hunks)xrpl/models/transactions/vault_create.py(2 hunks)xrpl/utils/mptoken_metadata.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- xrpl/models/transactions/vault_create.py
- xrpl/models/transactions/mptoken_issuance_create.py
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2024-10-30T20:26:32.925Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Applied to files:
tests/integration/transactions/test_mptoken_issuance_create.py
📚 Learning: 2025-06-04T22:17:47.164Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Applied to files:
tests/integration/transactions/test_mptoken_issuance_create.py
📚 Learning: 2024-11-04T19:41:04.808Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Applied to files:
tests/integration/transactions/test_mptoken_issuance_create.py
📚 Learning: 2024-12-12T00:48:38.042Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-10-30T20:34:35.451Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2025-09-15T15:54:25.836Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.836Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-12-11T01:47:28.074Z
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-10-30T20:36:02.813Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
Learning: The xrpl-py codebase contains many string-typed numeric fields in transaction models (like loan_origination_fee, maximum_amount, signature_reward, debt_maximum, etc.) and consistently uses direct int() calls for validation without try-catch blocks, allowing ValueError exceptions to bubble up naturally for clear error communication.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-11-01T20:24:17.202Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-10-30T20:44:27.753Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-11-01T18:53:01.394Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Applied to files:
xrpl/utils/mptoken_metadata.py
📚 Learning: 2024-12-10T20:35:09.762Z
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Applied to files:
xrpl/utils/mptoken_metadata.py
🧬 Code graph analysis (2)
tests/integration/transactions/test_mptoken_issuance_create.py (3)
xrpl/models/mptoken_metadata.py (2)
MPTokenMetadata(50-162)MPTokenMetadataUri(6-47)xrpl/models/transactions/mptoken_issuance_create.py (1)
MPTokenIssuanceCreate(56-143)xrpl/utils/mptoken_metadata.py (2)
decode_mptoken_metadata(219-270)encode_mptoken_metadata(292-336)
xrpl/utils/mptoken_metadata.py (2)
xrpl/models/mptoken_metadata.py (1)
MPTokenMetadata(50-162)xrpl/utils/str_conversions.py (2)
hex_to_str(19-31)str_to_hex(4-16)
🪛 GitHub Actions: Integration test
tests/integration/transactions/test_mptoken_issuance_create.py
[error] 58-62: Test failure: KeyError: 'MPTokenMetadata' in test_mptoken_issuance_create.py during integration tests (mp token issuance creation).
🪛 Ruff (0.14.2)
xrpl/utils/mptoken_metadata.py
243-243: Avoid specifying long messages outside the exception class
(TRY003)
248-250: Avoid specifying long messages outside the exception class
(TRY003)
249-249: Use explicit conversion flag
Replace with conversion flag
(RUF010)
253-253: Avoid specifying long messages outside the exception class
(TRY003)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
369-369: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (6)
tests/integration/transactions/test_mptoken_issuance_create.py (2)
16-30: LGTM! Metadata structure aligns with XLS-89.The MPTokenMetadata instance correctly uses the fields defined in the XLS-89 specification, with proper URI structure.
32-37: LGTM! Encoding and transaction creation are correct.The metadata is properly encoded and passed to the transaction.
xrpl/utils/mptoken_metadata.py (4)
12-173: LGTM! Validator functions are well-structured.All field validators consistently check for duplicate long/compact forms and validate against XLS-89 requirements.
219-270: LGTM! Decoding logic is correct.The function properly validates hex format, parses JSON, expands compact keys to long form, and handles edge cases. Previous exception handling issues have been addressed.
292-336: LGTM! Encoding logic is deterministic and correct.The function properly validates input type, shortens keys to compact form, and produces deterministic hex-encoded JSON using sorted keys and compact separators.
339-397: LGTM! Validation logic is comprehensive.The function validates hex format, length, JSON structure, field count, and runs all field-specific validators. Error handling is appropriate.
| mptoken_obj = next( | ||
| ( | ||
| obj | ||
| for obj in account_objects_response.result["account_objects"] | ||
| if obj["MPTokenMetadata"] == encoded_metadata | ||
| ), | ||
| None, | ||
| ) | ||
| self.assertIsNotNone(mptoken_obj) | ||
| self.assertEqual( | ||
| decode_mptoken_metadata(mptoken_obj["MPTokenMetadata"]), | ||
| metadata, | ||
| ) |
There was a problem hiding this comment.
Fix KeyError by using safe dictionary access.
The integration test is failing because obj["MPTokenMetadata"] raises a KeyError when the field doesn't exist. Not all MPTokenIssuance objects necessarily have the MPTokenMetadata field (e.g., issuances created without metadata).
Apply this diff to use safe dictionary access:
mptoken_obj = next(
(
obj
for obj in account_objects_response.result["account_objects"]
- if obj["MPTokenMetadata"] == encoded_metadata
+ if obj.get("MPTokenMetadata") == encoded_metadata
),
None,
)🧰 Tools
🪛 GitHub Actions: Integration test
[error] 58-62: Test failure: KeyError: 'MPTokenMetadata' in test_mptoken_issuance_create.py during integration tests (mp token issuance creation).
🤖 Prompt for AI Agents
In tests/integration/transactions/test_mptoken_issuance_create.py around lines
58 to 70, the generator and subsequent access use obj["MPTokenMetadata"] which
can raise KeyError; change to use safe dict access (obj.get("MPTokenMetadata"))
in the generator expression to find the matching object, keep the
self.assertIsNotNone check, and when decoding use obj.get("MPTokenMetadata") (or
pass the found value) rather than indexing, so no KeyError occurs if the field
is absent.
There was a problem hiding this comment.
@Patel-Raj11, thank you for fixing the KeyError issue! Using safe dictionary access with .get() will prevent the test from failing when MPTokenIssuance objects don't have the MPTokenMetadata field. 🎉
🧠 Learnings used
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
Learning: The xrpl-py codebase contains many string-typed numeric fields in transaction models (like loan_origination_fee, maximum_amount, signature_reward, debt_maximum, etc.) and consistently uses direct int() calls for validation without try-catch blocks, allowing ValueError exceptions to bubble up naturally for clear error communication.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_set.py:158-181
Timestamp: 2025-09-15T15:54:25.836Z
Learning: In the xrpl-py codebase, validation logic for transaction fields is primarily handled by rippled (the C++ XRPL implementation) rather than being duplicated in the Python models. Python models should focus on basic structural validation rather than comprehensive field validation that rippled already performs.
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 (1)
tests/integration/transactions/test_mptoken_issuance_create.py (1)
50-52: Consider adding assertions for required fields to improve error clarity.While these fields should always be present for successful transactions, using explicit assertions or safe dictionary access would provide clearer error messages if the ledger response structure differs from expectations.
Example with explicit assertions:
tx_hash = response.result["tx_json"]["hash"] tx_res = await client.request(Tx(transaction=tx_hash)) +self.assertIn("meta", tx_res.result, "Transaction response missing meta field") +self.assertIn("mpt_issuance_id", tx_res.result["meta"], "Transaction meta missing mpt_issuance_id") mpt_issuance_id = tx_res.result["meta"]["mpt_issuance_id"]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/transactions/test_mptoken_issuance_create.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
📚 Learning: 2025-06-04T22:17:47.164Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Applied to files:
tests/integration/transactions/test_mptoken_issuance_create.py
📚 Learning: 2024-11-04T19:41:04.808Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Applied to files:
tests/integration/transactions/test_mptoken_issuance_create.py
🧬 Code graph analysis (1)
tests/integration/transactions/test_mptoken_issuance_create.py (6)
xrpl/models/mptoken_metadata.py (2)
MPTokenMetadata(50-162)MPTokenMetadataUri(6-47)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 (1)
MPTokenIssuanceCreate(56-143)xrpl/utils/mptoken_metadata.py (2)
decode_mptoken_metadata(219-270)encode_mptoken_metadata(292-336)xrpl/wallet/main.py (2)
classic_address(34-41)address(24-29)
⏰ 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.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.12)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
tests/integration/transactions/test_mptoken_issuance_create.py (4)
7-11: LGTM! Imports are appropriate for the metadata functionality.The added imports correctly bring in the MPTokenMetadata types and encoding/decoding utilities needed for this test.
17-31: LGTM! Metadata structure follows XLS-89 standard.The test metadata correctly demonstrates the MPTokenMetadata structure with appropriate fields and a nested URI object.
33-39: LGTM! Metadata encoding and transaction creation are correct.The metadata is properly encoded to hex format before being passed to the transaction, which aligns with the
mptoken_metadatafield requirements.
58-65: Verify whethermpt_issuance_idis guaranteed to be present in AccountObjects responses.The suggestion to use safe dictionary access is defensively sound, but the field's presence cannot be confirmed from the codebase. While the pattern of direct bracket access is consistent across multiple test files and suggests the field is reliably present, the XRP Ledger specification does not explicitly document
mpt_issuance_idas a standard MPT_ISSUANCE ledger object field. Additionally, using.get()would change the error-handling behavior: instead of raisingKeyErrorimmediately, a missing field would cause the filter to silently fail and returnNone, which would then fail theassertIsNotNoneassertion downstream.Confirm that the XRP Ledger API guarantees
mpt_issuance_idis always present in MPT_ISSUANCE account objects before applying this change, or accept the modified error semantics if adopting the defensive approach.
| self.assertIsNotNone(mptoken_issuance_obj) | ||
| self.assertEqual( | ||
| decode_mptoken_metadata(mptoken_issuance_obj["MPTokenMetadata"]), | ||
| metadata, | ||
| ) |
There was a problem hiding this comment.
Add explicit assertion for MPTokenMetadata field before accessing it.
The test directly accesses mptoken_issuance_obj["MPTokenMetadata"] on line 69, which will raise a KeyError if the field is missing. While the field should be present since metadata was included in the transaction, an explicit assertion provides a clearer error message when debugging unexpected ledger behavior.
Apply this diff to add an explicit assertion:
self.assertIsNotNone(mptoken_issuance_obj)
+self.assertIn(
+ "MPTokenMetadata",
+ mptoken_issuance_obj,
+ "MPTokenMetadata field missing from issuance object"
+)
self.assertEqual(
decode_mptoken_metadata(mptoken_issuance_obj["MPTokenMetadata"]),
metadata,
)Alternatively, use safe dictionary access:
self.assertIsNotNone(mptoken_issuance_obj)
+metadata_hex = mptoken_issuance_obj.get("MPTokenMetadata")
+self.assertIsNotNone(metadata_hex, "MPTokenMetadata field missing from issuance object")
self.assertEqual(
- decode_mptoken_metadata(mptoken_issuance_obj["MPTokenMetadata"]),
+ decode_mptoken_metadata(metadata_hex),
metadata,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.assertIsNotNone(mptoken_issuance_obj) | |
| self.assertEqual( | |
| decode_mptoken_metadata(mptoken_issuance_obj["MPTokenMetadata"]), | |
| metadata, | |
| ) | |
| self.assertIsNotNone(mptoken_issuance_obj) | |
| self.assertIn( | |
| "MPTokenMetadata", | |
| mptoken_issuance_obj, | |
| "MPTokenMetadata field missing from issuance object" | |
| ) | |
| self.assertEqual( | |
| decode_mptoken_metadata(mptoken_issuance_obj["MPTokenMetadata"]), | |
| metadata, | |
| ) |
🤖 Prompt for AI Agents
In tests/integration/transactions/test_mptoken_issuance_create.py around lines
67 to 71, the test accesses mptoken_issuance_obj["MPTokenMetadata"] directly
which will raise KeyError if the key is missing; add an explicit assertion that
the "MPTokenMetadata" key exists (e.g., assertIn("MPTokenMetadata",
mptoken_issuance_obj) or
assertIsNotNone(mptoken_issuance_obj.get("MPTokenMetadata"))) before decoding,
then use the existing decode_mptoken_metadata call so the failure message is
clear and not a raw KeyError.
There was a problem hiding this comment.
Fetched object using mpt_issuance_id from validated ledger. There is no way MPTokenMetadata won't exists.
There was a problem hiding this comment.
@Patel-Raj11, you're absolutely right. Since the object is retrieved using the mpt_issuance_id from a validated ledger transaction that successfully included metadata, the MPTokenMetadata field should indeed be present. If it's missing, that would indicate a fundamental issue with the ledger behavior itself rather than a test concern. Your current test flow is appropriate for this integration test scenario.
🧠 Learnings used
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: khancode
Repo: XRPLF/xrpl-py PR: 732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
| return output | ||
|
|
||
|
|
||
| def encode_mptoken_metadata(mptoken_metadata: MPTokenMetadata) -> str: |
There was a problem hiding this comment.
From a documentation/communication perspective, are we going to add the encode/decode logic for the MPTMetadata in the XRPL Binary Format docs page? https://xrpl.org/docs/references/protocol/binary-format
I understand that this logic is explained in the XLS specification document. However, it needs to be presented in a more readable form to the future reader as a part of the documentation.
cc @kuan121
There was a problem hiding this comment.
I understand that this logic is explained in the XLS specification document.
It's also explained in the library docs. I'm not sure if we need to add it to the Binary Format docs. What's the criteria?
High Level Overview of Change
Add
encode_mptoken_metadataanddecode_mptoken_metadatautility functions to encode and decode metadata and perform long <-> compact conversions along the way.Adjust
MPTokenMetadatatype andvalidate_mptoken_metadatafunction to reflect changes being made to XLS-89Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added unit tests