support DynamicNFT #2726
support DynamicNFT #2726
Conversation
|
FYI the general policy with this repo and xrpl-py is that amendment PRs aren't merged until the corresponding rippled PR is merged (though of course development can happen before). |
|
@CodeRabbit full review |
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThis pull request introduces enhancements to the XRPL library's non-fungible token (NFT) capabilities. Key changes include the addition of a new 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/NFTokenModify.test.ts (1)
14-25: Consider adding more coverage for invalid URIs.The current test suite validates a correct NFTokenModify scenario and missing NFTokenID. However, additional tests for an invalid or empty URI would further strengthen coverage. For instance, verifying that a non-hex or empty string
URIcorrectly triggers a validation error.packages/xrpl/src/models/transactions/NFTokenModify.ts (1)
60-62: Document a recommended approach for removing a URI.It may be helpful to clarify in the docstring that if a user wants to remove or clear a previously set URI, they should set
URItoundefinedor omit it instead of passing an empty string, since an empty string triggers a validation error.packages/xrpl/test/integration/transactions/nftokenModify.test.ts (1)
3-4: Resolve ESLint import/extensions rule.According to static analysis hints, adding file extensions to imports under
../../../dist/npmand../../../dist/npm/srcmay be required. Consider applying a configuration or including.jsin these imports to resolve lint errors.Here's an example of how to fix it:
-import { NFTokenModify } from '../../../dist/npm' -import { NFTokenMintFlags } from '../../../dist/npm/src' +import { NFTokenModify } from '../../../dist/npm/index.js' +import { NFTokenMintFlags } from '../../../dist/npm/src/index.js'🧰 Tools
🪛 eslint
[error] 3-3: Missing file extension for "../../../dist/npm"
(import/extensions)
[error] 4-4: Missing file extension for "../../../dist/npm/src"
(import/extensions)
packages/xrpl/src/models/transactions/NFTokenMint.ts (1)
41-44: Validate bitmask usage for new flag.Adding
tfMutable = 0x00000010seems correct following the prior flag 0x00000008. Ensure that there is no overlap with any other existing flags in the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/ripple-binary-codec/HISTORY.md(1 hunks)packages/ripple-binary-codec/src/enums/definitions.json(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenModify.ts(1 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)packages/xrpl/test/integration/transactions/nftokenModify.test.ts(1 hunks)packages/xrpl/test/models/NFTokenModify.test.ts(1 hunks)
🧰 Additional context used
🪛 eslint
packages/xrpl/test/integration/transactions/nftokenModify.test.ts
[error] 3-3: Missing file extension for "../../../dist/npm"
(import/extensions)
[error] 4-4: Missing file extension for "../../../dist/npm/src"
(import/extensions)
🔇 Additional comments (10)
packages/xrpl/test/models/NFTokenModify.test.ts (1)
27-40: Good negative test coverage.This test correctly verifies that an NFTokenModify transaction missing the
NFTokenIDfield raises aValidationError. This ensures robust handling of incomplete inputs.packages/xrpl/src/models/transactions/index.ts (1)
55-55: Export statement looks good.Exporting
NFTokenModifyandvalidateNFTokenModifyhere ensures they are conveniently available. The ordering is consistent and no immediate issues are observed.packages/xrpl/test/integration/transactions/nftokenModify.test.ts (1)
34-103: Excellent integration test for NFTokenModify.The flow of minting an NFT with the
tfMutableflag, then modifying its URI, and finally verifying the updated URI is comprehensive. The use oftestTransactionand subsequent validation viaaccount_nftsensures end-to-end coverage.packages/xrpl/src/models/transactions/NFTokenMint.ts (1)
58-58: Ensure parity between interface and flag.The new
tfMutable?: booleanproperty correctly corresponds to thetfMutableflag. No further changes needed.packages/xrpl/src/models/transactions/transaction.ts (3)
124-124: Extend transaction coverage.Adding
NFTokenModifyto theSubmittableTransactionunion is consistent with the introduction of a new transaction type.
331-334: Validate newly introduced transaction.Including a
case 'NFTokenModify'to invokevalidateNFTokenModifyensures the transaction is properly validated. This is a necessary and correct addition.
44-44: Confirm import presence.The added import
{ NFTokenModify, validateNFTokenModify }looks good. Verify that the fileNFTokenModify.tsexists and is being exported in the correct format.✅ Verification successful
Import verification successful
The file
NFTokenModify.tsexists and exports bothNFTokenModifyinterface andvalidateNFTokenModifyfunction in the correct format.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm presence of NFTokenModify.ts and export statements fd NFTokenModify.ts rg -A 10 'export function validateNFTokenModify'Length of output: 1153
Script:
#!/bin/bash # Check NFTokenModify export ast-grep --pattern 'export interface NFTokenModify' ast-grep --pattern 'export type NFTokenModify'Length of output: 102
Script:
#!/bin/bash # Check all exports in NFTokenModify.ts rg "export" packages/xrpl/src/models/transactions/NFTokenModify.tsLength of output: 198
packages/ripple-binary-codec/HISTORY.md (1)
9-9: Retain clarity of update history.Documenting the newly added
NFTokenModifytransaction andtfMutableflag under version 2.1.0 is helpful for future reference. Good job keeping HISTORY.md current.packages/ripple-binary-codec/src/enums/definitions.json (1)
2977-2977: Add consistent tooling note.The addition of
"NFTokenModify": 53is correct for the new transaction type. However, note that previous feedback suggests using thexrpl-codec-genscript for these changes. Please confirm this step was followed or consider generating the definitions to maintain consistency.packages/xrpl/HISTORY.md (1)
9-9: LGTM! Changes align with DynamicNFT support.The addition of
NFTokenModifytransaction andtfMutableflag inNFTokenMintis properly documented and consistent with the PR objectives of supporting Dynamic Non-Fungible Tokens (DynamicNFT).
|
Thank you for this :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.ci-config/rippled.cfg (1)
191-191: Add version grouping comment for better maintainability.To maintain consistency with other amendments, consider adding a version group comment above the
DynamicNFTamendment. This helps track which rippled version introduced this feature.fixReducedOffersV2 +# X.X.X Amendments DynamicNFT
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ci-config/rippled.cfg(1 hunks).github/workflows/nodejs.yml(1 hunks)
🔇 Additional comments (3)
.ci-config/rippled.cfg (1)
191-191: Verify the amendment status in rippled.Since this amendment corresponds to rippled PR #5048, we should verify its status before enabling it.
Let's check the rippled PR status:
✅ Verification successful
The DynamicNFT amendment is properly merged in rippled.
The amendment corresponds to PR #5048 which is now merged, confirming it's safe to enable this feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the status of the DynamicNFT amendment PR in rippled gh pr view 5048 --repo XRPLF/rippled --json state,title,urlLength of output: 160
.github/workflows/nodejs.yml (2)
Line range hint
15-236: LGTM! The workflow configuration follows CI/CD best practices.The job configurations demonstrate good practices:
- Comprehensive test matrix across Node.js versions
- Efficient caching strategy
- Proper cleanup handling
- Reasonable timeout limits
7-7: Verify the stability implications of using thedeveloptag.Switching from a specific version (
2.3.0-rc1) todeveloptag could introduce instability in the CI pipeline as the image content can change unexpectedly. While this might be necessary for testing the new DynamicNFT features, consider:
- Adding a comment explaining why the
developtag is required- Creating a tracking issue to revert back to a stable version once the DynamicNFT features are released
Run this script to check the current state of the develop branch and available tags:
✅ Verification successful
Using the
developtag is appropriate but requires monitoringThe verification shows that the Docker image is actively maintained and synchronized with the rippled develop branch. While using a specific version is generally preferred for stability, using the
developtag is acceptable in this context as:
- The Docker image is regularly updated to match the develop branch
- Testing new features like DynamicNFT may require the latest changes
Consider adding a comment in the workflow file explaining this dependency on the develop branch.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the rippled Docker image tags and compare with the rippled repository state # Get available Docker tags echo "Available Docker tags:" curl -s https://registry.hub.docker.com/v2/repositories/rippleci/rippled/tags | jq -r '.results[].name' # Check the rippled repository state echo -e "\nRippled repository state:" gh api repos/XRPLF/rippled/branches/develop --jq '.commit.sha, .commit.commit.message'Length of output: 776
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| ### Added | |||
| * Support for the Price Oracles amendment (XLS-47). | |||
| * Add `NFTokenModify` transaction and add `tfMutable` flag in `NFTokenMint` | |||
There was a problem hiding this comment.
nit: can follow the format on line 13 and indicate XLS-46 instead of adding the specifics here
| @@ -17,6 +17,7 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr | |||
| * New `MPTAmount` type support for `Payment` and `Clawback` transactions | |||
| * `parseTransactionFlags` as a utility function in the xrpl package to streamline transactions flags-to-map conversion | |||
| * Support for XLS-70d (Credentials) | |||
| * Add `NFTokenModify` transaction and add `tfMutable` flag in `NFTokenMint` | |||
There was a problem hiding this comment.
same nit here to reference xls-46 here instead of the specifics
| * convert this field to the proper encoding. | ||
| * | ||
| * This field must not be an empty string. Omit it from the transaction or | ||
| * set to `undefined` value if you do not use it. |
There was a problem hiding this comment.
could this comment reference setting to null for clarity
| const mutableTx: TxRequest = { | ||
| command: 'tx', | ||
| transaction: hashSignedTx(response.result.tx_blob), | ||
| } | ||
| const mutableTxResponse = await testContext.client.request(mutableTx) |
There was a problem hiding this comment.
how come we need a separate tx request here? could we get the nft id from the mint response
There was a problem hiding this comment.
mint response won't provide nftoken id. we have to query it through another request
| () => validate(invalid), | ||
| ValidationError, | ||
| 'NFTokenModify: missing field NFTokenID', | ||
| ) |
There was a problem hiding this comment.
could you add test cases for invalid hex format and empty string check here
There was a problem hiding this comment.
added a separate PR to add the test and resolve comments: #2892
support DynamicNFT XRPLF/rippled#5048
High Level Overview of Change
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan