Support NFTokenMintOffer #2875
Support NFTokenMintOffer #2875
Conversation
WalkthroughThis pull request introduces support for XLS-52 (NFTokenMintOffer) in the xrpl.js library. The changes primarily focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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?
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