fix: rectify STIssue codec (MPTCurrency case only)
#870
Conversation
WalkthroughAdds MPT-aware Issue encoding/decoding: validates 48-hex mpt_issuance_id, constructs/parses 44-byte MPT buffers using a black-holed AccountID, updates to_json for MPT, and removes length_hint from Issue.from_parser. Introduces BLACK_HOLED_ACCOUNT_ID. Adds tests for AccountID special encodings and Issue MPT parsing/validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant Issue as Issue
participant Aid as AccountID
participant U32 as UInt32
rect rgb(245,248,255)
note left of C: from_value path
C->>Issue: from_value({currency, mpt_issuance_id})
Issue->>Issue: Validate mpt_issuance_id length == 48 hex
Issue->>Aid: Resolve issuer (first 20 bytes)
Issue->>Issue: Use BLACK_HOLED_ACCOUNT_ID (20 bytes)
Issue->>U32: Encode sequence (last 4 bytes)
Issue-->>C: 44-byte buffer [currency + BH + seq]
end
rect rgb(245,255,245)
note left of C: from_parser path
C->>Issue: from_parser(parser)
Issue->>Issue: Parse currency
alt currency is XRP
Issue-->>C: currency bytes (no issuer)
else non-XRP
Issue->>Issue: Parse 20-byte issuer
alt issuer == BLACK_HOLED_ACCOUNT_ID
Issue->>U32: Parse 4-byte sequence
Issue-->>C: 44-byte buffer [currency + BH + seq]
else
Issue-->>C: 40-byte buffer [currency + issuer]
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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)
xrpl/core/binarycodec/types/issue.py (1)
70-73: Consider extracting the error message to a constant.While not critical, the long error message could be extracted to improve maintainability.
+ _MPT_ID_LENGTH_ERROR = ( + "Invalid mpt_issuance_id length: expected 48 characters, " + "received {0} characters." + ) + if MPTCurrencyModel.is_dict_of_model(value): if len(value["mpt_issuance_id"]) != 48: raise XRPLBinaryCodecException( - "Invalid mpt_issuance_id length: expected 48 characters, " - f"received {len(value['mpt_issuance_id'])} characters." + cls._MPT_ID_LENGTH_ERROR.format(len(value['mpt_issuance_id'])) )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/unit/core/binarycodec/types/test_account_id.py(1 hunks)tests/unit/core/binarycodec/types/test_issue.py(2 hunks)xrpl/core/binarycodec/types/issue.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mvadari
PR: XRPLF/xrpl-py#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.
📚 Learning: 2025-06-04T22:17:47.164Z
Learnt from: ckeshava
PR: XRPLF/xrpl-py#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/unit/core/binarycodec/types/test_issue.pyxrpl/core/binarycodec/types/issue.py
📚 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/types/issue.py
📚 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, 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/core/binarycodec/types/issue.py
🧬 Code graph analysis (3)
tests/unit/core/binarycodec/types/test_account_id.py (2)
xrpl/core/binarycodec/types/account_id.py (1)
AccountID(27-89)xrpl/core/binarycodec/types/serialized_type.py (1)
to_hex(84-91)
tests/unit/core/binarycodec/types/test_issue.py (4)
xrpl/core/binarycodec/exceptions.py (1)
XRPLBinaryCodecException(6-9)xrpl/core/binarycodec/types/issue.py (4)
Issue(21-150)from_value(39-86)to_json(126-150)from_parser(89-119)xrpl/core/binarycodec/types/serialized_type.py (1)
to_hex(84-91)xrpl/core/binarycodec/binary_wrappers/binary_parser.py (1)
BinaryParser(31-261)
xrpl/core/binarycodec/types/issue.py (6)
xrpl/core/binarycodec/types/hash192.py (1)
Hash192(17-26)xrpl/core/binarycodec/types/serialized_type.py (5)
SerializedType(15-100)from_value(39-42)from_parser(29-35)to_json(64-73)to_hex(84-91)xrpl/core/binarycodec/types/uint32.py (3)
UInt32(18-71)from_value(44-71)from_parser(29-41)xrpl/core/binarycodec/types/account_id.py (3)
AccountID(27-89)from_value(45-80)to_json(82-89)xrpl/core/binarycodec/types/currency.py (3)
from_value(97-120)Currency(64-131)to_json(122-131)xrpl/core/binarycodec/types/uint.py (2)
value(25-32)to_json(82-91)
🪛 Ruff (0.12.2)
xrpl/core/binarycodec/types/issue.py
70-73: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Unit test
xrpl/core/binarycodec/types/issue.py
[error] 122-122: mypy error: Method cannot have explicit self annotation and Self type.
[error] 122-122: mypy error: The erased type of self "xrpl.core.binarycodec.types.issue.Issue" is not a supertype of its class "type[xrpl.core.binarycodec.types.issue.Issue]".
⏰ 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.8)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
tests/unit/core/binarycodec/types/test_account_id.py (2)
26-36: LGTM! Test validates the special ACCOUNT_ONE encoding correctly.The test properly verifies the bidirectional conversion between hex and base58 representations for the special ACCOUNT_ONE account (0x0...01), which is crucial for MPT handling.
37-46: LGTM! Test validates the special ACCOUNT_ZERO encoding correctly.The test properly verifies the bidirectional conversion for ACCOUNT_ZERO, ensuring both special account constants work correctly.
tests/unit/core/binarycodec/types/test_issue.py (3)
43-48: LGTM! Proper validation of MPT issuance ID length.The test correctly ensures that MPT issuance IDs must be exactly 48 hex characters (192 bits), preventing malformed data from being processed.
50-65: LGTM! Binary representation test validates the MPT encoding format.The test correctly verifies that the binary representation follows the expected format:
- First 20 bytes: MPT issuer account
- Next 20 bytes: Black-holed account ID (ACCOUNT_ONE)
- Last 4 bytes: Sequence number
This ensures compliance with the XRPL Binary Format standards.
106-108: API change tofrom_parseris correctly reflected in tests.The removal of
length_hintparameter fromIssue.from_parser()call is consistent with the API change.xrpl/core/binarycodec/types/issue.py (5)
24-26: LGTM! Black-holed account constant is properly defined.The
BLACK_HOLED_ACCOUNT_IDconstant correctly represents the special ACCOUNT_ONE (0x0...01) used as a marker in MPT binary encoding.
62-81: LGTM! MPT serialization logic correctly implements the binary format.The implementation properly:
- Validates the mpt_issuance_id length (48 hex chars = 192 bits)
- Constructs the 44-byte buffer as per XRPL spec: [issuer(20)] + [black-hole(20)] + [sequence(4)]
- References the rippled implementation for clarity
105-119: LGTM! MPT parsing logic correctly handles the binary format.The parsing logic properly:
- Checks for XRP first
- Identifies MPT by checking if issuer equals BLACK_HOLED_ACCOUNT_ID
- Reconstructs the correct buffer structure for MPT (44 bytes)
- Falls back to standard issued currency (40 bytes)
133-142: LGTM! JSON conversion correctly handles MPT format.The method properly identifies MPT buffers by their 44-byte length and correctly reconstructs the mpt_issuance_id by combining the issuer (first 20 bytes/40 hex chars) with the sequence (last 4 bytes/8 hex chars).
89-93: Incorrect —length_hintremains onIssue.from_parser.
xrpl/core/binarycodec/types/issue.py still declareslength_hint: Optional[int] = None(≈line 89); binary_parser passessize_hint/Noneintofrom_parser(xrpl/core/binarycodec/binary_wrappers/binary_parser.py:224,242).Likely an incorrect or invalid review comment.
| @classmethod | ||
| def _print_buffer(self: Self, buffer: bytes) -> None: | ||
| print("DEBUG: Inside Issue._print_buffer(), buffer: ", buffer.hex().upper()) | ||
| print("DEBUG: Inside Issue._print_buffer(), buffer length: ", len(buffer)) |
There was a problem hiding this comment.
Fix the incorrect method signature for _print_buffer.
The method has incorrect type annotation - it should use cls for class methods or no first parameter annotation for instance methods, not self with a class method decorator.
Apply this diff to fix the method signature:
- @classmethod
- def _print_buffer(self: Self, buffer: bytes) -> None:
+ def _print_buffer(self: Self, buffer: bytes) -> None:Alternatively, if this should be a class method:
@classmethod
- def _print_buffer(self: Self, buffer: bytes) -> None:
+ def _print_buffer(cls: Type[Self], buffer: bytes) -> None:Also consider whether this debug method should remain in production code, or if it should be removed before merging.
📝 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.
| @classmethod | |
| def _print_buffer(self: Self, buffer: bytes) -> None: | |
| print("DEBUG: Inside Issue._print_buffer(), buffer: ", buffer.hex().upper()) | |
| print("DEBUG: Inside Issue._print_buffer(), buffer length: ", len(buffer)) | |
| def _print_buffer(self: Self, buffer: bytes) -> None: | |
| print("DEBUG: Inside Issue._print_buffer(), buffer: ", buffer.hex().upper()) | |
| print("DEBUG: Inside Issue._print_buffer(), buffer length: ", len(buffer)) |
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 122-122: mypy error: Method cannot have explicit self annotation and Self type.
[error] 122-122: mypy error: The erased type of self "xrpl.core.binarycodec.types.issue.Issue" is not a supertype of its class "type[xrpl.core.binarycodec.types.issue.Issue]".
🤖 Prompt for AI Agents
In xrpl/core/binarycodec/types/issue.py around lines 121 to 124, the
@classmethod uses an incorrect first parameter named self; change the signature
to use cls (def _print_buffer(cls, buffer: bytes) -> None:) if you intend a
class method, or remove @classmethod and use def _print_buffer(self, buffer:
bytes) -> None: for an instance method; also consider removing or gating the
debug prints (print(...)) before merging so this debug helper is not left in
production code.
High Level Overview of Change
This PR aims to fix this issue: #869
Context of Change
The
MPTCurrencytype is not being serialized correctly. The present serialized version of the field is not compatible with the XRPL Binary Format standards, hence it is un-parsable by rippled.Type of Change
Did you update CHANGELOG.md?
Test Plan
Additional unit tests have been added to the binary-codec module.
The unit tests for the AccountID rippled internal type have been enhanced to identify special accounts (
ACCOUNT_ZEROandACCOUNT_ONE)