Support NFTokenMintOffer #2875
Support NFTokenMintOffer #2875
Conversation
WalkthroughThis pull request introduces support for the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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.
@achowdhry-ripple Here are the additional test cases to verify error handling:
it(
'should fail with zero Amount',
async function () {
const tx: NFTokenMint = {
TransactionType: 'NFTokenMint',
Account: testContext.wallet.address,
URI: convertStringToHex('https://www.google.com'),
NFTokenTaxon: 0,
Amount: '0',
Destination: destinationWallet.address,
}
await assert.isRejected(
testTransaction(testContext.client, tx, testContext.wallet),
/Amount must be greater than zero/,
)
},
TIMEOUT,
)
it(
'should fail with expired timestamp',
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), // 1 minute ago
Destination: destinationWallet.address,
}
await assert.isRejected(
testTransaction(testContext.client, tx, testContext.wallet),
/Expiration time must be in the future/,
)
},
TIMEOUT,
)
it(
'should fail with invalid destination address',
async function () {
const tx: NFTokenMint = {
TransactionType: 'NFTokenMint',
Account: testContext.wallet.address,
URI: convertStringToHex('https://www.google.com'),
NFTokenTaxon: 0,
Amount: xrpToDrops(1),
Destination: 'invalid_address',
}
await assert.isRejected(
testTransaction(testContext.client, tx, testContext.wallet),
/invalid address/i,
)
},
TIMEOUT,
)These test cases verify that the transaction fails appropriately when:
- Amount is set to zero
- Expiration is set to a past timestamp
- Destination address is invalid
Would you like me to add any other test cases or modify these?
There was a problem hiding this comment.
these cases would have been covered in the rippled cpp implementation. Usually, client libraries forward these transactions to the rippled node.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
There was a problem hiding this comment.
line 169 seems unnecessary as well
There was a problem hiding this comment.
Expiration and Destination can only be set if Amount is present, but they are still optional fields.
There was a problem hiding this comment.
What happens if a developer specifies Destination field but not the Amount field?
There was a problem hiding this comment.
it will be temMALFORMED
There was a problem hiding this comment.
Yes, but that error is returned from the rippled node. I feel we can catch that error inside client-libraries itself.
However, this is just a suggestion. I like the PR even without this change.
|
@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 |
| validateOptionalField(tx, 'Amount', isAmount) | ||
| validateOptionalField(tx, 'Expiration', isNumber) | ||
| validateOptionalField(tx, 'Destination', isAccount) |
There was a problem hiding this comment.
nit: move these to the top so we validate all fields before any other logic that might use them
There was a problem hiding this comment.
It is probably better for developers to check for cases where values should not exist before checking for values.
(In other words, you may have seen an error message (invalid field) and fixed the value, but you shouldn't have set that field in the first place.)
But I have no objection to making the change. What do you think?
There was a problem hiding this comment.
More of a code cleanup to stay consistent with the repo, and makes the logic a bit easier to follow. I don't feel strongly about this though so ultimately up to you
Co-authored-by: achowdhry-ripple <achowdhry@ripple.com>
Co-authored-by: achowdhry-ripple <achowdhry@ripple.com>
| const txResponse = await testContext.client.request(txRequest) | ||
|
|
||
| assert.equal( | ||
| (txResponse.result.meta as TransactionMetadata).TransactionResult, |
There was a problem hiding this comment.
This fix to the tx model ought to resolve your typing issues here:
meta_blob?: Version extends typeof RIPPLED_API_V2 ? string : never
meta?: Version extends typeof RIPPLED_API_V2
? TransactionMetadata<T>
: TransactionMetadata<T> | string
There was a problem hiding this comment.
@mvadari This test uses rippled APIv2 (default version, if api_version field is not specified). Is there a need to multiplex between the APIv1 and APIv2 types? How is this useful?
There was a problem hiding this comment.
The tx model is slightly incorrect - the casting shouldn't be necessary, and this change should fix that.
There was a problem hiding this comment.
@tequdev if you'd like to address this comment, we can wait.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/HISTORY.md (1)
13-15: Great addition for NFTokenMintOffer (XLS-52)
The new entry clearly communicates the added support, which aligns with the PR objective. However, note that there are now two separate "### Added" sections under the "Unreleased" heading. To maintain a clean and consistent history—and to address the MD024 warning from static analysis—it’s recommended to merge duplicate "Added" headings. This would consolidate related additions in one section, making the changelog easier to read.For example, you could combine the entries as follows:
## Unreleased -### Added -* Adds utility function `convertTxFlagsToNumber` - -### Changed -* Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber instead - -### Added -* Support for `NFTokenMintOffer` (XLS-52) +### Added +* Adds utility function `convertTxFlagsToNumber` +* Support for `NFTokenMintOffer` (XLS-52) + +### Changed +* Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber insteadThis change not only resolves the duplicate heading issue flagged by static analysis but also improves overall readability.
🧰 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 (3)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(3 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/test/models/NFTokenMint.test.ts
- packages/xrpl/src/models/transactions/NFTokenMint.ts
🧰 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)
|
@tequdev Can we merge this PR? Are you happy with it? |
yes |
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