fix: add MPTCurrency type instead of MPTAmount for Issues
#822
Conversation
WalkthroughThis pull request updates terminology and functionality related to Multi-Precision Types (MPTs) across the codebase. It replaces references to Changes
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR changes the support for Issue objects from using the MPTAmount type to a new MPTCurrency type, ensuring that Issue no longer expects a value field. Key changes include updating import statements and type guards, modifying conversion methods and error checks, and adjusting test cases to align with the new type.
- Changed import and conversion logic in xchain_claim.py to correctly derive a Currency via to_currency()
- Introduced the new MPTCurrency type and integrated it into the currency union and binary codec
- Updated tests and changelog entries to support the switch to MPTCurrency
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| xrpl/models/transactions/xchain_claim.py | Updated imports and conversion to use to_currency() instead of direct attribute access |
| xrpl/models/currencies/mpt_currency.py | New type implementation for MPTCurrency |
| xrpl/models/currencies/currency.py | Updated currency union to include MPTCurrency |
| xrpl/models/currencies/init.py | Adjusted all and module docstring for new MPTCurrency |
| xrpl/models/amounts/mpt_amount.py | Added to_currency() method to create a corresponding MPTCurrency |
| xrpl/models/amounts/amount.py | Updated type guard and get_amount_value to cover MPT amounts |
| xrpl/core/binarycodec/types/issue.py | Updated dict model check and error message to refer to MPTCurrency instead of MPTAmount |
| xrpl/constants.py | Added new constant HEX_MPTID_REGEX |
| tests/unit/core/binarycodec/types/test_issue.py | Modified test inputs and adjusted comments reflecting the removal of the value field |
| CHANGELOG.md | Updated changelog to reflect changes to Issue support for MPTCurrency |
Comments suppressed due to low confidence (1)
xrpl/core/binarycodec/types/issue.py:83
- The comment refers to 'MPTIssue', which may be unclear. Consider updating it to 'MPTCurrency' for consistency with the new type.
# Check if it's an MPTIssue by checking mpt_issuance_id byte size
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
xrpl/models/currencies/currency.py (1)
1-5: Update the file docstring to mention MPTsThe docstring still mentions only two kinds of money (XRP and issued currencies), but the implementation now includes three kinds with the addition of MPTs. This should be updated to maintain consistency with the implementation and the docstring in
__init__.py.""" -The XRP Ledger has two kinds of money: XRP, and issued -currencies. Both types have high precision, although their +The XRP Ledger has three kinds of money: XRP, issued +currencies, and MPTs. All types have high precision, although their formats are different. """xrpl/models/currencies/mpt_currency.py (1)
51-63: Consider updating the docstring for accuracy.The docstring on line 53 refers to "MPTCurrencyAmount" but the method actually returns an "MPTAmount". This inconsistency could be confusing to developers.
- """ - Converts an MPTCurrency to an MPTCurrencyAmount. - - Args: - value: The amount of MPTs in the MPTAmount. - - Returns: - An MPTAmount that represents the MPT and the provided value. - """ + """ + Converts an MPTCurrency to an MPTAmount. + + Args: + value: The amount of MPTs in the MPTAmount. + + Returns: + An MPTAmount that represents the MPT and the provided value. + """xrpl/models/transactions/xchain_claim.py (1)
82-89: Consider refactoring the conditional branches.The conditional branches for
is_issued_currencyandis_mptperform the same operation and could be combined for better readability.if is_xrp(amount): currency: Currency = XRP() - elif is_issued_currency(amount): - currency = amount.to_currency() - elif is_mpt(amount): - currency = amount.to_currency() + elif is_issued_currency(amount) or is_mpt(amount): + currency = amount.to_currency() else: errors["amount"] = "Currency can't be derived."🧰 Tools
🪛 Ruff (0.8.2)
84-87: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)tests/unit/core/binarycodec/types/test_issue.py(0 hunks)xrpl/constants.py(1 hunks)xrpl/core/binarycodec/types/issue.py(3 hunks)xrpl/models/amounts/amount.py(4 hunks)xrpl/models/amounts/mpt_amount.py(2 hunks)xrpl/models/currencies/__init__.py(1 hunks)xrpl/models/currencies/currency.py(1 hunks)xrpl/models/currencies/mpt_currency.py(1 hunks)xrpl/models/transactions/clawback.py(1 hunks)xrpl/models/transactions/xchain_claim.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/core/binarycodec/types/test_issue.py
🧰 Additional context used
🧬 Code Definitions (6)
xrpl/models/currencies/currency.py (3)
xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)xrpl/models/currencies/xrp.py (1)
XRP(26-91)xrpl/models/currencies/issued_currency.py (1)
IssuedCurrency(31-76)
xrpl/models/amounts/mpt_amount.py (1)
xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)
xrpl/models/transactions/clawback.py (2)
xrpl/models/amounts/amount.py (2)
is_issued_currency(31-42)is_mpt(45-56)xrpl/core/binarycodec/types/amount.py (1)
is_mpt(439-447)
xrpl/models/currencies/__init__.py (3)
xrpl/models/currencies/issued_currency.py (1)
IssuedCurrency(31-76)xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)xrpl/models/currencies/xrp.py (1)
XRP(26-91)
xrpl/core/binarycodec/types/issue.py (1)
xrpl/models/currencies/mpt_currency.py (1)
MPTCurrency(28-63)
xrpl/models/currencies/mpt_currency.py (2)
xrpl/models/transactions/xchain_claim.py (1)
_get_errors(76-99)xrpl/models/amounts/mpt_amount.py (1)
MPTAmount(18-51)
🪛 Ruff (0.8.2)
xrpl/models/transactions/xchain_claim.py
84-87: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (17)
xrpl/constants.py (1)
45-46: LGTM: Added new validation regexThe addition of
HEX_MPTID_REGEXis appropriate for validating 48-character hexadecimal MPT issuance IDs.CHANGELOG.md (1)
11-11: LGTM: Correct terminology in CHANGELOGThe update properly reflects the change from
MPTAmounttoMPTCurrencysupport inIssue, which aligns with the PR objective.xrpl/models/currencies/__init__.py (2)
2-3: LGTM: Updated documentationAppropriately updated the docstring to include MPTs as the third type of money in the XRP Ledger.
8-8: LGTM: Added MPTCurrency import and exportCorrectly imported and added
MPTCurrencyto the__all__list to make it part of the public API.Also applies to: 14-14
xrpl/models/currencies/currency.py (1)
10-10: LGTM: Updated Currency type aliasProperly updated the
Currencytype alias to include the newMPTCurrencyclass.Also applies to: 13-13
xrpl/models/amounts/amount.py (3)
7-9: Great addition of TypeGuard for improved type safetyThe introduction of
TypeGuardfromtyping_extensionsis an excellent enhancement. This will allow static type checkers to understand that when the type check functions returnTrue, the amount can be safely treated as the specific type indicated.
17-17: Well-implemented TypeGuard return type annotationsChanging the return types from
booltoTypeGuardis a significant improvement for static type checking. This allows the type checker to properly narrow down the type of theamountparameter after these functions are used in conditional statements, making the code more type-safe without changing the runtime behavior.Also applies to: 31-31, 45-45
69-75: Improved error handling and code clarityThe updated
get_amount_valueimplementation is more robust with its explicit error handling for unexpected amount types. The code is also cleaner now that it leverages the TypeGuard functionality to directly access the value attribute after type confirmation.xrpl/models/amounts/mpt_amount.py (2)
11-11: Appropriate import for new conversion functionalityThe addition of the MPTCurrency import is necessary to support the new conversion method.
44-51: Well-designed bidirectional conversion between MPTAmount and MPTCurrencyThe new
to_currencymethod creates a nice symmetrical API with the existingto_amountmethod in theMPTCurrencyclass. This makes it easy to convert between these related types as needed, which aligns well with the PR objective of usingMPTCurrencyinstead ofMPTAmountforIssues.xrpl/models/transactions/clawback.py (1)
53-53: Improved conditional logic structureChanging the
ifstatements toelifstatements creates a more efficient and logical structure since these conditions are mutually exclusive. This ensures only one branch of code executes and improves the readability of the error checking logic.Also applies to: 58-58
xrpl/core/binarycodec/types/issue.py (4)
17-17: Correct replacement of MPTAmount with MPTCurrencyThis import change aligns with the PR objective to use
MPTCurrencyinstead ofMPTAmountforIssues, as theIssuedoesn't contain avaluefield.
57-59: Appropriate update to use MPTCurrencyModelThe condition correctly uses
MPTCurrencyModel.is_dict_of_modelto check if the input is a dictionary representation of an MPTCurrency. This is consistent with the other model checks in this method.
62-63: Updated error message for consistencyThe error message now correctly mentions
MPTCurrencyinstead ofMPTAmount, maintaining consistency with the type changes made in this PR.
83-83: Updated comment for clarityThe comment now refers to
MPTIssueinstead ofMPTAmount, which better describes what's being checked and is consistent with the changes made in this PR.xrpl/models/currencies/mpt_currency.py (1)
1-64: Implementation looks correct and well-structured.This new
MPTCurrencyclass correctly implements a currency representation without a value field, which addresses the PR objective of replacingMPTAmountforIssuesupport. The class is properly decorated withrequire_kwargs_on_initand made immutable withfrozen=True. The validation ofmpt_issuance_idagainstHEX_MPTID_REGEXensures proper formatting.xrpl/models/transactions/xchain_claim.py (1)
10-11: Type safety improvement looks good.The changes properly add
Currencytype annotation and import, which aligns with the PR objective of replacingMPTAmountwithMPTCurrency. Removing the type ignore comments indicates that the underlying type issue has been fixed.Also applies to: 83-83, 85-89
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/models/currencies/test_mpt_currency.py (3)
29-35: Incorrect comment for the hex format test.The comment mentions "+" not being allowed, but the test is actually checking for "X" characters.
- # the "+" is not allowed in a currency format" + # non-hexadecimal characters are not allowed in an MPT issuance IDAlso, consider using a more explicit test case by modifying the valid
_MPTIDwith a non-hex character rather than constructing a new string.
36-42: Fix variable naming to follow Python conventions.Variable names should follow snake_case convention for consistency with Python standards.
- MPT_currency = MPTCurrency(mpt_issuance_id=_MPTID) - MPT_currency_amount = MPT_currency.to_amount(amount) + mpt_currency = MPTCurrency(mpt_issuance_id=_MPTID) + mpt_currency_amount = mpt_currency.to_amount(amount)Also, consider adding tests for edge cases like using integers or other numeric types as the amount value.
Could you verify if the
to_amountmethod handles different numeric types correctly? This would improve test coverage.
9-43: Consider adding docstrings to test methods.Adding docstrings to test methods would improve code readability and make it easier for developers to understand the purpose of each test.
For example:
def test_correct_mptid_format(self): """Test that a correctly formatted MPT issuance ID is accepted.""" # Existing code
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/models/currencies/test_mpt_currency.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.13)
🔇 Additional comments (4)
tests/unit/models/currencies/test_mpt_currency.py (4)
1-7: LGTM on imports and constants setup.The imports are appropriate for the test cases being conducted and the constant
_MPTIDprovides a good test case sample.
9-14: Appropriate test for valid MPT ID format.This test correctly verifies that the
MPTCurrencyclass accepts a valid MPT issuance ID format.
16-20: Good test for case insensitivity.This test ensures that lowercase hexadecimal characters are also accepted for MPT IDs, which is important for robustness.
22-28: Well-structured length validation tests.Good practice using separate assertions for the too short and too long cases. This ensures each case is properly tested independently.
| :meta private: | ||
| """ | ||
|
|
||
| HEX_MPTID_REGEX: Final[Pattern[str]] = re.compile(r"^[0-9A-Fa-f]{48}$") |
There was a problem hiding this comment.
I was under the belief that mpt_issuance_id is represented by 192-bits, as explained here: https://github.com/XRPLF/rippled/blob/6cf37c4abef80fb535d1d7577a548665e139f635/include/xrpl/protocol/MPTIssue.h#L28
However, the size of mpt_issuance_id is much larger than 192 bits in the examples. It's four times larger in the RPC outputs.
- Where can I find the cpp implementation of this
MPTCurrencytype? - Which transactions use the
MPTCurrencytype? I'd like to test the correctness of serialization/de-serialization of theMPTCurrencytype.
There was a problem hiding this comment.
192 bits is 24 bytes, which is a hex string of length 48.
MPTCurrencyis equivalent toMPTIssuein rippled.- Not sure there are any. The only two features that use
STIssuetypes are XChainBridge and AMM and I don't think either of them support MPTs right now (though support for AMM is being added in XLS-82d in Add MPT support to DEX rippled#5285).
There was a problem hiding this comment.
- Can we retain the same name
MPTIssuein the client libraries. Is there a reason you preferMPTCurrency? - I wish there were a way to test the correctness of this serialization. Unless we communicate a serialized
MPTIssuevalue to rippled, we can not validate the correctness of the client library binary codec.
As a new feature, if rippled could expose an echo(serialized_input) => deserialized_output -like of utility command, that would have been helpful. I'm sure there are other SFields which haven't been used in transactions yet.
192 bits is 24 bytes, which is a hex string of length 48.
Hmm yes, you are right. I had mis-understood the calculation.
There was a problem hiding this comment.
1.MPTCurrency aligns more with what we have in the client library and makes more sense in English. I don't have a strong opinion on this, though.
2. rippled can serialize transactions via the sign and submit fields locally. You could try with the XLS-82d branch above.
There was a problem hiding this comment.
Alright, I would slightly prefer the name MPTIssue over MPTCurrency, it aligns well with the documentation too.
The MPT amendment has been merged into the rippled codebase, I should be able to use the develop branch to test the sign, submit commands right?
rippled can serialize transactions via the sign and submit fields locally
Hmm, this will help me with serialization, but not the other way around i.e. I'm trying to understand how I can deserialize a blob of MPTIssue and verify that the xrpl-py library does it correctly?
3 participants
High Level Overview of Change
This PR switches all
Issuesupport to use a new type,MPTCurrency, instead ofMPTAmount(sinceIssuedoesn't have avaluefield). It also enhances some helper functions by changing them to beTypeGuards.This PR will need to make it into the next release to avoid breaking change issues.
Context of Change
#809 mistakenly added support for
MPTAmounttoIssue, when really it should be a newMPTCurrencytype.Type of Change
Did you update CHANGELOG.md?
Test Plan
CI passes.
Future Tasks
Update xrpl.js accordingly (XRPLF/xrpl.js#2919)