Support NFTokenMintOffer #2875
Support NFTokenMintOffer #2875
Conversation
WalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (4)
packages/xrpl/test/models/NFTokenMint.test.ts (4)
113-129: Consider adding edge cases for Expiration validation.While the test correctly validates that Amount is required when Expiration is present, consider adding test cases for:
- Invalid Expiration values (negative numbers, zero)
- Expiration values in the past
- Maximum allowed Expiration value
131-147: Add test cases for invalid Destination formats.While the test correctly validates that Amount is required when Destination is present, consider adding test cases for:
- Invalid Destination address format
- Destination same as Account
- Destination same as Issuer
149-163: Enhance test coverage for field combinations.While the test correctly validates a transaction with all new fields, consider adding test cases for:
- Different Amount formats (drops vs XRP)
- Various combinations of optional fields:
- Amount only
- Amount + Destination only
- Amount + Expiration only
- Edge cases when all fields are present:
- Minimum/maximum values
- Different currency codes for Amount
112-163: Improve organization of XLS-52 related tests.Consider grouping the new XLS-52 related tests in a nested describe block for better organization and documentation:
describe('XLS-52 NFTokenMintOffer', () => { // Group the three new test cases here })Also, consider adding JSDoc comments to document the XLS-52 requirements and validation rules.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(3 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (3)
packages/xrpl/src/models/transactions/NFTokenMint.ts (2)
105-124: Interface additions look good!The new fields are well documented with clear descriptions:
- Amount: Optional amount for the token
- Expiration: Optional expiration time in seconds since Ripple Epoch
- Destination: Optional account restriction
164-174: Validation logic is correctThe validation properly enforces that Amount must be present when either Expiration or Destination is specified.
packages/xrpl/HISTORY.md (1)
7-9: Changelog entry is appropriateThe addition of XLS-52 support is properly documented under Unreleased Changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/xrpl/test/integration/transactions/nftokenMint.test.ts (2)
103-111: Consider extracting test constants.The test uses hardcoded values that could be extracted as constants for better maintainability:
xrpToDrops(1)for Amount1000 * 60 * 60 * 24for expiration duration+ const TEST_AMOUNT_XRP = 1 + const ONE_DAY_IN_SECONDS = 24 * 60 * 60 const tx: NFTokenMint = { // ... - Amount: xrpToDrops(1), - Expiration: unixTimeToRippleTime(Date.now() + 1000 * 60 * 60 * 24), + Amount: xrpToDrops(TEST_AMOUNT_XRP), + Expiration: unixTimeToRippleTime(Date.now() + ONE_DAY_IN_SECONDS * 1000), // ... }
144-154: Enhance offer verification.The test only verifies the existence of the offer but not its details. Consider adding assertions for:
- Amount matches the specified value
- Destination matches the specified address
- Expiration matches the specified time
const existsOffer = sellOffers.result.offers.some( - (value) => value.nft_offer_index === nftokenOfferID, + (value) => + value.nft_offer_index === nftokenOfferID && + value.amount === xrpToDrops(TEST_AMOUNT_XRP) && + value.destination === destinationWallet.address && + value.expiration === tx.Expiration )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/integration/transactions/nftokenMint.test.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: snippets (18.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (2)
packages/xrpl/test/integration/transactions/nftokenMint.test.ts (2)
9-11: LGTM! Imports are well-organized.The new imports are properly grouped with other imports from the same source and are essential for the new NFTokenMintOffer functionality.
27-27: LGTM! Test setup is well-structured.Good practice to:
- Use a typed wallet variable
- Generate a fresh funded wallet for each test
- Initialize in
beforeEachAlso applies to: 31-31
| it( | ||
| 'test with Amount', | ||
| async function () { | ||
| const tx: NFTokenMint = { | ||
| TransactionType: 'NFTokenMint', | ||
| Account: testContext.wallet.address, | ||
| URI: convertStringToHex('https://www.google.com'), | ||
| NFTokenTaxon: 0, | ||
| Amount: xrpToDrops(1), | ||
| Expiration: unixTimeToRippleTime(Date.now() + 1000 * 60 * 60 * 24), | ||
| Destination: destinationWallet.address, | ||
| } | ||
| const response = await testTransaction( | ||
| testContext.client, | ||
| tx, | ||
| testContext.wallet, | ||
| ) | ||
| assert.equal(response.type, 'response') | ||
|
|
||
| const txRequest: TxRequest = { | ||
| command: 'tx', | ||
| transaction: hashSignedTx(response.result.tx_blob), | ||
| } | ||
| const txResponse = await testContext.client.request(txRequest) | ||
|
|
||
| assert.equal( | ||
| (txResponse.result.meta as TransactionMetadata).TransactionResult, | ||
| 'tesSUCCESS', | ||
| ) | ||
|
|
||
| const nftokenID = | ||
| getNFTokenID( | ||
| txResponse.result.meta as TransactionMetadata<NFTokenMint>, | ||
| ) ?? 'undefined' | ||
|
|
||
| const nftokenOfferID = ( | ||
| txResponse.result.meta as TransactionMetadata<NFTokenMint> | ||
| ).offer_id | ||
|
|
||
| const sellOffers = await testContext.client.request({ | ||
| command: 'nft_sell_offers', | ||
| nft_id: nftokenID, | ||
| }) | ||
|
|
||
| const existsOffer = sellOffers.result.offers.some( | ||
| (value) => value.nft_offer_index === nftokenOfferID, | ||
| ) | ||
|
|
||
| assert.isTrue( | ||
| existsOffer, | ||
| `Expected to exist an offer for NFT with NFTokenID ${nftokenID} but did not find it. | ||
| \n\nHere's what was returned from 'nft_sell_offers': ${JSON.stringify( | ||
| sellOffers, | ||
| )}`, | ||
| ) | ||
| }, | ||
| TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add negative test cases.
The test suite would benefit from additional test cases that verify error handling:
- Invalid Amount (zero or negative)
- Expired timestamp
- Invalid destination address
Would you like me to help generate these negative test cases?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/xrpl/HISTORY.md (1)
13-15: Ensure Consistent and Concise Heading StructureThe new "### Added" section reporting "* Support for XLS-52 (NFTokenMintOffer)" clearly documents the new feature. However, note that there is an earlier "### Added" heading (line 7) in the "Unreleased" section. To avoid duplicate headings—which may trigger markdownlint warnings (MD024)—consider merging these entries under a single "### Added" heading. This will improve readability and consistency of the release history.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Multiple headings with the same content
null(MD024, no-duplicate-heading)
packages/xrpl/src/models/transactions/NFTokenMint.ts (2)
136-137: Enhance the comment for offer_id field.The comment could be more specific about when the offer_id is present.
- // if Amount is present + // Present when Amount is specified, representing the ID of the NFTokenMintOffer offer_id?: string
169-179: Consider adding more robust validations.While the current validations are good, consider adding these additional checks:
- Validate that Expiration is a non-negative number (since it represents seconds since Ripple Epoch)
- If applicable, validate that Amount is non-negative
Example implementation:
validateOptionalField(tx, 'Amount', isAmount) - validateOptionalField(tx, 'Expiration', isNumber) + validateOptionalField(tx, 'Expiration', (value: unknown): boolean => { + return isNumber(value) && (value as number) >= 0 + }) validateOptionalField(tx, 'Destination', isAccount)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/xrpl/HISTORY.md
13-13: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: unit (22.x)
- 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: build-and-lint (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/xrpl/src/models/transactions/NFTokenMint.ts (2)
2-2: LGTM! Import changes are appropriate.The new imports are correctly added to support the validation of new fields.
Also applies to: 10-11
110-129: LGTM! Interface changes are well-documented and properly typed.The new optional fields (Amount, Expiration, Destination) are appropriately added with comprehensive JSDoc comments explaining their purpose and constraints.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/HISTORY.md (1)
13-15: Merge duplicate "### Added" headings for clarity and markdown compliance
The new entry for NFTokenMintOffer is introduced as a separate "### Added" section, but there is already an "### Added" block earlier. This duplicate heading can trigger markdownlint MD024 and may confuse readers. Consider merging the two "Added" sections—for example, by appending the bullet for XLS-52 support to the first "### Added" list or by renaming one of them to differentiate the groups.Suggested diff to merge the sections (assuming merging into the first "### Added"):
@@ In the Unreleased section: -### Added -* Adds utility function `convertTxFlagsToNumber` - -### Changed -* Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber instead - -### Added -* Support for XLS-52 (NFTokenMintOffer) +### Added +* Adds utility function `convertTxFlagsToNumber` +* Support for XLS-52 (NFTokenMintOffer) + +### Changed +* Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber insteadFeel free to adjust the grouping if the intent was to separate distinct types of additions.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Multiple headings with the same content
null(MD024, no-duplicate-heading)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/HISTORY.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/xrpl/HISTORY.md
13-13: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: unit (18.x)
- GitHub Check: build-and-lint (18.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: browser (18.x)
| @@ -140,4 +165,16 @@ export function validateNFTokenMint(tx: Record<string, unknown>): void { | |||
| if (tx.NFTokenTaxon == null) { | |||
| throw new ValidationError('NFTokenMint: missing field NFTokenTaxon') | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
| if (!((tx.Amount == null && tx.Expiration == null && tx.Destination == null) || (tx.Amount != null && tx.Expiration != null && tx.Destination != null))) { | |
| if (tx.Expiration != null || tx.Destination != null) { | |
| throw new ValidationError( | |
| 'NFTokenMint: All three fields Amount, Expiration, Destination must be present', | |
| ) | |
| } | |
| } |
We will need all three fields to be present (or) absent to make logical sense, isn't it? Isn't this a more comprehensive check?
I'm guessing this is already checked in rippled, so perhaps there is no need for a rigorous check here.
|
@tequdev if you would like to fix the integration test, please check this file: https://github.com/XRPLF/xrpl.js/pull/2874/files#diff-aed00401282930e11f525e026ee0128d91284d95586aaee7c741f9ba0c8d409e The |
High Level Overview of Change
Support NFTokenMintOffer
Context of Change
add fields to NFTokenMint transaction
Type of Change
Did you update HISTORY.md?
Test Plan
add unit test