fix: add MPTAmount support in Issue model #2919
Conversation
WalkthroughThe changes introduce a new section titled "Fixed" in the Changes
Suggested reviewers
Poem
🪧 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
|
![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
🔭 Outside diff range comments (1)
packages/ripple-binary-codec/src/types/issue.ts (1)
83-90:⚠️ Potential issueUpdate the
fromParsermethod to handle MPT amounts.The
fromParsermethod doesn't appear to have been updated to handle MPT amounts. Since you've added support for MPT in other methods, this method should also be updated to ensure consistency when parsing from binary data.Consider adding logic to detect and handle MPT amounts in the parser:
static fromParser(parser: BinaryParser): Issue { const currency = parser.read(20) if (new Currency(currency).toJSON() === 'XRP') { return new Issue(currency) } + // If the currency value matches an MPT issuance format, treat as MPT + if (currency.length === Hash192.width / 2) { + return new Issue(currency) + } const currencyAndIssuer = [currency, parser.read(20)] return new Issue(concat(currencyAndIssuer)) }Please verify this implementation against the xrpl-py repository's approach as mentioned in the PR objectives.
🧹 Nitpick comments (1)
packages/ripple-binary-codec/src/types/issue.ts (1)
1-121: Add tests for MPT amount handling.With the addition of MPT amount support, it would be beneficial to add tests that verify the serialization and deserialization of MPT amounts to ensure the implementation works correctly for all cases.
Would you like me to help draft some test cases for this implementation?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ripple-binary-codec/HISTORY.md(1 hunks)packages/ripple-binary-codec/src/types/issue.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (5)
packages/ripple-binary-codec/HISTORY.md (1)
5-6: Documentation looks good.The added "Fixed" section properly documents the implementation of MPTAmount support in the Issue type, making it clear what change was made in this release.
packages/ripple-binary-codec/src/types/issue.ts (4)
7-7: Appropriate import for MPT support.The
Hash192import is correctly added to handle the MPT issuance ID in the Issue type.
13-16: Interface modifications look good.The changes to the
IssueObjectinterface are appropriate:
- Making
currencyoptional allows for cases where only an MPT issuance ID is present- Adding the optional
mpt_issuance_idproperty supports the new MPT functionalityThese changes align with the PR's objective of adding MPTAmount support.
23-29: Type guard logic properly updated.The
isIssueObjectfunction is correctly updated to handle all three cases:
- XRP case: object has only a currency property
- IOU case: object has both currency and issuer properties
- MPT case: object has only an mpt_issuance_id property
This comprehensive type guard ensures proper validation for all supported Issue object types.
54-72: Implementation of MPT case infrommethod looks good.The static
frommethod has been properly updated to handle the three cases:
- IOU case (when both currency and issuer are present)
- XRP case (when only currency is present)
- MPT case (when mpt_issuance_id is present)
The code structure is clear and the implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/ripple-binary-codec/src/types/issue.ts (1)
92-99: 🛠️ Refactor suggestionUpdate fromParser method to handle MPT Issue types.
The fromParser method hasn't been updated to handle MPT issue types, which could lead to issues when parsing serialized MPT Issues. Consider adding support for MPT Issue types in this method.
static fromParser(parser: BinaryParser): Issue { const currency = parser.read(20) if (new Currency(currency).toJSON() === 'XRP') { return new Issue(currency) } + + // Check if this is an MPT issue (should be exactly Hash192.width bytes) + if (currency.length === Hash192.width) { + return new Issue(currency) + } + const currencyAndIssuer = [currency, parser.read(20)] return new Issue(concat(currencyAndIssuer)) }
♻️ Duplicate comments (1)
packages/ripple-binary-codec/src/types/issue.ts (1)
107-113:⚠️ Potential issueFix method call in toJSON implementation.
There appears to be a syntax error when checking the byte length. The
toBytesshould be called as a method, not accessed as a property.- if (this.toBytes.length === Hash192.width) { + if (this.toBytes().length === Hash192.width) {
🧹 Nitpick comments (3)
packages/ripple-binary-codec/src/types/issue.ts (3)
48-54: Update JSDoc to include MPT issue type.The JSDoc comment for the from method should be updated to include the MPT issue type.
/** * Construct an amount from an IOU or string amount * - * @param value An Amount, object representing an IOU, or a string + * @param value An Amount, object representing an XRP, IOU, or MPT issue, or a string * representing an integer amount * @returns An Amount object */
55-55: Consider implementing stronger type checking in the from method.The current implementation relies on checking properties in a specific order. Consider making the type guard more explicit to handle unexpected combinations of properties.
static from<T extends Issue | IssueObject>(value: T): Issue { if (value instanceof Issue) { return value } if (isIssueObject(value)) { + // Handle each type explicitly rather than relying on property checks + if ('mpt_issuance_id' in value) { + const mptIssuanceIdBytes = Hash192.from( + value.mpt_issuance_id.toString(), + ).toBytes() + return new Issue(mptIssuanceIdBytes) + } else if ('currency' in value) { + const currency = Currency.from(value.currency.toString()).toBytes() + + if ('issuer' in value) { + const issuer = AccountID.from(value.issuer.toString()).toBytes() + return new Issue(concat([currency, issuer])) + } + + return new Issue(currency) + } - if (value.currency) { - const currency = Currency.from(value.currency.toString()).toBytes() - - //IOU case - if (value.issuer) { - const issuer = AccountID.from(value.issuer.toString()).toBytes() - return new Issue(concat([currency, issuer])) - } - - //XRP case - return new Issue(currency) - } - - // MPT case - if (value.mpt_issuance_id) { - const mptIssuanceIdBytes = Hash192.from( - value.mpt_issuance_id.toString(), - ).toBytes() - return new Issue(mptIssuanceIdBytes) - } } throw new Error('Invalid type to construct an Amount') }
83-83: Improve error message specificity.The current error message is generic. Consider providing more specific error messages for different failure scenarios.
- throw new Error('Invalid type to construct an Amount') + throw new Error('Invalid type to construct an Issue: expected Issue instance or IssueObject with valid properties')
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ripple-binary-codec/src/types/issue.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: snippets (20.x)
- GitHub Check: unit (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/ripple-binary-codec/src/types/issue.ts (5)
7-7: LGTM: Adding Hash192 import for MPT support.The import of Hash192 is correctly added to support the new MPTIssue type.
9-19: Well-structured type definitions for Issue types.The interfaces are cleanly structured to represent the three distinct issue types:
- XRPIssue: Contains only currency
- IOUIssue: Contains currency and issuer
- MPTIssue: Contains mpt_issuance_id
This approach provides good type safety and clarity.
23-23: Good union type approach for IssueObject.Converting IssueObject from an interface to a union type is the correct approach to handle the different issue variants.
28-36: Correctly updated type guard for the new union type.The isIssueObject type guard has been properly updated to handle all three issue types. The implementation is clean and follows a logical pattern.
61-81: Well-implemented Issue handling in the from method.The from method has been correctly updated to handle all three issue types with appropriate branching logic.
|
|
||
| it(`test from value mpt`, () => { | ||
| const mptJson = { | ||
| // value: '100', // MPT amounts must be an integer string (no decimal point) |
There was a problem hiding this comment.
@khancode saw value in the python tests, but wasn't sure whether to add it here based on our discussions. Seems like it should be here, but that would require me to change the isMPT logic in Issue to something different than what we discussed
There was a problem hiding this comment.
Can you check if value is included when Issue.from is normally called? If yes, then we should add it.
There was a problem hiding this comment.
Issue.from accepts both types in the input parameter.
static from<T extends Issue | IssueObject>(value: T): Issue
It should work either ways
There was a problem hiding this comment.
@khancode when you say normally called do you mean the implementations for IOU and XRP? there were no tests before so hard to tell but based off of the isIssueObject check it seems like those have no value param. However, in your python implementation there was no value parameter for IOU and XRP either, but you added it for MPT. I'm assuming this should be the same in JS so I should add value as a param
There was a problem hiding this comment.
Yes I meant the implementations normal call flow. Yes, it looks like in PY its design is different than JS. Just need to make sure we make it compatible with JS design.
There was a problem hiding this comment.
Looks like value is excluded by looking at the console.log inside of Issue.from so this looks good 👍
achowdhry-ripple
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/ripple-binary-codec/src/types/issue.ts (3)
28-36: Type guard implementation could be more robustThe current implementation of
isIssueObjectrelies on checking the existence and order of keys, which might be brittle if object properties are reordered. Consider enhancing the type guard to also validate the types of the values:function isIssueObject(arg): arg is IssueObject { const keys = Object.keys(arg).sort() const isXRP = keys.length === 1 && keys[0] === 'currency' const isIOU = keys.length === 2 && keys[0] === 'currency' && keys[1] === 'issuer' const isMPT = keys.length === 1 && keys[0] === 'mpt_issuance_id' + // Additional validation for value types + if (isXRP && typeof arg.currency !== 'string') return false + if (isIOU && (typeof arg.currency !== 'string' || typeof arg.issuer !== 'string')) return false + if (isMPT && typeof arg.mpt_issuance_id !== 'string') return false return isXRP || isIOU || isMPT }
48-54: Update JSDoc comment for the from methodThe JSDoc comment for the
frommethod needs to be updated to reflect the addition of MPT issue support./** * Construct an amount from an IOU or string amount * * @param value An Amount, object representing an IOU, or a string * representing an integer amount - * @returns An Issue object + * @param value An Amount, object representing an XRP issue, IOU issue, MPT issue, or a string + * representing an integer amount + * @returns An Issue object representing XRP, IOU, or MPT */
75-81: Add validation for MPT issuance ID formatCurrently, the code doesn't validate the format of the
mpt_issuance_idstring beyond passing it toHash192.from(). Consider adding explicit validation to ensure it meets the expected format.// MPT case if (value.mpt_issuance_id) { + // Validate mpt_issuance_id format (should be 48 characters of hex) + const mptId = value.mpt_issuance_id.toString() + if (!/^[0-9A-Fa-f]{48}$/.test(mptId)) { + throw new Error('Invalid MPT issuance ID format') + } const mptIssuanceIdBytes = Hash192.from( value.mpt_issuance_id.toString(), ).toBytes() return new Issue(mptIssuanceIdBytes) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ripple-binary-codec/src/types/issue.ts(4 hunks)packages/ripple-binary-codec/test/issue.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
🔇 Additional comments (7)
packages/ripple-binary-codec/test/issue.test.ts (2)
29-36: Good implementation of MPT issue test caseThe test case properly verifies that an MPT issue with a
mpt_issuance_idcan be created and correctly serialized back to JSON.There's a commented-out line about
valueproperty with an explanation that "MPT amounts must be an integer string (no decimal point)". This comment provides helpful context about the constraints of MPT amounts.
79-86: Comprehensive error handling testGood job including a test case for invalid input handling. The test properly verifies that the
Issue.frommethod throws an appropriate error message when given invalid input.packages/ripple-binary-codec/src/types/issue.ts (5)
7-7: Good addition of Hash192 importThe addition of the Hash192 import is necessary for handling MPT issuance IDs, which are 192-bit hashes.
9-23: Well-structured type definitions for different issue typesBreaking down the
IssueObjectinto three distinct interfaces (XRPIssue,IOUIssue, andMPTIssue) improves type safety and makes the code more maintainable. The union type definition in line 23 properly combines these interfaces.
55-81: Structured handling of different issue typesThe implementation of the
frommethod properly handles all three types of issues (XRP, IOU, and MPT). The structure is clear with dedicated code paths for each case.
95-106: Proper implementation of MPT parsing with hint parameterThe
fromParsermethod has been updated to handle MPT issues by checking if the hint parameter matchesHash192.width. This approach is efficient and clear.
113-119: Fix method call in toJSON implementationThe issue identified in the previous review regarding
this.toByteshas been correctly fixed tothis.toBytes().The implementation properly handles MPT issues by checking if the byte length matches
Hash192.widthand returning an appropriate object withmpt_issuance_id.
|
|
||
| it(`test from value mpt`, () => { | ||
| const mptJson = { | ||
| // value: '100', // MPT amounts must be an integer string (no decimal point) |
There was a problem hiding this comment.
Can you check if value is included when Issue.from is normally called? If yes, then we should add it.
|
|
||
| it(`test from value mpt`, () => { | ||
| const mptJson = { | ||
| // value: '100', // MPT amounts must be an integer string (no decimal point) |
There was a problem hiding this comment.
Issue.from accepts both types in the input parameter.
static from<T extends Issue | IssueObject>(value: T): Issue
It should work either ways
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ripple-binary-codec/src/types/issue.ts (1)
57-57: Remove console.log statement.Debug logging statements should be removed before merging to production.
- console.log('ISSUE: ', JSON.stringify(value))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ripple-binary-codec/src/types/issue.ts(4 hunks)packages/ripple-binary-codec/test/issue.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ripple-binary-codec/test/issue.test.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/ripple-binary-codec/src/types/issue.ts (7)
packages/ripple-binary-codec/src/types/serialized-type.ts (1) (1)
JsonObject(122-122)packages/xrpl/src/models/common/index.ts (1) (1)
Currency(17-17)packages/ripple-binary-codec/src/types/index.ts (3) (3)
Currency(53-53)AccountID(50-50)Hash192(56-56)packages/ripple-binary-codec/src/types/currency.ts (1) (1)
Currency(140-140)packages/ripple-binary-codec/src/types/account-id.ts (1) (1)
AccountID(86-86)packages/isomorphic/src/utils/shared.ts (1) (1)
concat(5-7)packages/ripple-binary-codec/src/serdes/binary-parser.ts (1) (1)
BinaryParser(228-228)
🪛 ESLint
packages/ripple-binary-codec/src/types/issue.ts
[error] 33-33: Insert ⏎···
(prettier/prettier)
[error] 55-56: Delete ⏎
(prettier/prettier)
🪛 GitHub Actions: Node.js CI
packages/ripple-binary-codec/src/types/issue.ts
[error] 33-33: Insert ⏎··· prettier/prettier
[error] 55-55: Delete ⏎ prettier/prettier
🔇 Additional comments (10)
packages/ripple-binary-codec/src/types/issue.ts (10)
7-7: LGTM: Hash192 import included for MPTAmount support.The added import of
Hash192is necessary to handle the MPT issuance ID data type.
9-19: LGTM: Well-structured interface definitions.The introduction of separate interfaces for different issue types (XRP, IOU, MPT) provides clear type definitions and improves code readability.
22-23: LGTM: Union type properly defines all issue object variants.Converting
IssueObjectfrom an interface to a union type is the right approach to model these distinct cases.
25-27: LGTM: Updated documentation.The documentation now properly describes the purpose of the type guard function.
51-54: LGTM: Updated parameter documentation.The documentation now correctly includes MPTAmount as a possible input type.
63-83: LGTM: Properly handling all issue types.The implementation correctly handles all three issue types (XRP, IOU, and MPT) with appropriate type checking and conversion.
92-97: LGTM: Updated documentation for the fromParser method.The documentation now correctly explains the purpose of the new
hintparameter.
97-108: LGTM: Added MPT support to the fromParser method.The method now handles MPT amounts by using the optional
hintparameter. This approach aligns with the previous discussion in the PR comments.
115-122: LGTM: Fixed toJSON method to handle MPT amounts.The implementation now correctly handles MPT amounts and the previous syntax error has been fixed by changing
this.toBytes.lengthtothis.toBytes().length.
1-138:❓ Verification inconclusive
Verify MPT functionality with comprehensive unit tests.
The changes introduce a new type of Issue (MPT) and modify several methods to handle this new type. Please ensure there are comprehensive unit tests covering all possible edge cases, especially for the newly added MPT support.
🏁 Script executed:
#!/bin/bash # Check for unit tests related to MPT support in the Issue class echo "Searching for unit tests related to MPT in Issue..." fd -e ts -e js "issue.*test" packages/ripple-binary-codec/test --exec grep -l "MPT" {} \; echo "Searching for test cases specifically testing mpt_issuance_id..." fd -e ts -e js "issue.*test" packages/ripple-binary-codec/test --exec grep -l "mpt_issuance_id" {} \; echo "If no files are shown above, it suggests missing test coverage for MPT functionality"Length of output: 724
MPT Test Coverage Confirmation
Unit tests for the new MPT support are present in the Issue test file (packages/ripple-binary-codec/test/issue.test.ts), with markers for both "MPT" and "mpt_issuance_id". Please review these tests to confirm they cover all edge cases for MPT functionality—including, but not limited to, handling valid/invalid mpt_issuance_id values and ensuring the parser logic for a 24-byte MPT amount is robust. If you identify any gaps, additional test cases should be added accordingly.
🧰 Tools
🪛 ESLint
[error] 33-33: Insert
⏎···(prettier/prettier)
[error] 55-56: Delete
⏎(prettier/prettier)
🪛 GitHub Actions: Node.js CI
[error] 33-33: Insert
⏎···prettier/prettier
[error] 55-55: Delete
⏎prettier/prettier
High Level Overview of Change
Add MPTAmount support in the Issue object. Parallel to XRPLF/xrpl-py#809
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan