support DynamicNFT XLS-46 #799
Conversation
…er-entry-api Add include delete to ledger entry api
WalkthroughThis pull request introduces significant updates to the XRP Ledger Python library, focusing on non-fungible token (NFT) functionality. The changes include adding a new Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 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 (12)
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: 2
🧹 Nitpick comments (4)
xrpl/models/transactions/nftoken_modify.py (1)
26-30: Improve documentation for nftoken_id field.The documentation states this field is required but doesn't specify the format requirements for the TokenID.
""" -Identifies the TokenID of the NFToken object that the -offer references. This field is required. +Identifies the TokenID of the NFToken object to modify. +Must be a 64-character hexadecimal string representing +a 256-bit value. This field is required. """xrpl/models/transactions/nftoken_mint.py (1)
49-52: Enhance documentation for TF_MUTABLE flag.The current documentation could be more detailed about the implications and limitations of setting this flag.
""" -If set, indicates that this NFT's URI can be modified. +If set, indicates that this NFT's URI can be modified using the NFTokenModify +transaction. This allows for dynamic NFTs whose metadata can be updated after +minting. Once set, this flag cannot be unset. """tests/unit/models/transactions/test_nftoken_modify.py (1)
46-55: Add assertions for object properties in valid test case.The test_valid method should verify that all properties are set correctly.
def test_valid(self): obj = NFTokenModify( account=_ACCOUNT, owner=_ACCOUNT, sequence=_SEQUENCE, fee=_FEE, uri=_URI, nftoken_id=_NFTOKEN_ID, ) self.assertTrue(obj.is_valid()) + self.assertEqual(obj.account, _ACCOUNT) + self.assertEqual(obj.owner, _ACCOUNT) + self.assertEqual(obj.uri, _URI) + self.assertEqual(obj.nftoken_id, _NFTOKEN_ID)tests/unit/models/transactions/test_better_transaction_flags.py (1)
90-90: LGTM! Consider adding dedicated test cases.The TF_MUTABLE flag is properly integrated into the existing test. Consider adding dedicated test cases to verify:
- NFToken modification behavior when minted without TF_MUTABLE
- NFToken modification behavior when minted with TF_MUTABLE
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)tests/unit/models/transactions/test_better_transaction_flags.py(1 hunks)tests/unit/models/transactions/test_nftoken_modify.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(2 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/nftoken_mint.py(2 hunks)xrpl/models/transactions/nftoken_modify.py(1 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
🔇 Additional comments (5)
xrpl/models/transactions/nftoken_modify.py (1)
20-24: Verify handling of TF_MUTABLE flag.The class documentation mentions that URI modification is only allowed if
tfMutableis set to true, but there's no validation in the implementation to check this flag.xrpl/models/transactions/types/transaction_type.py (1)
39-39: LGTM!The new transaction type is correctly added and follows the alphabetical ordering convention.
xrpl/models/transactions/__init__.py (1)
70-70: LGTM! Clean and consistent changes.The NFTokenModify import and export are properly placed, maintaining the file's organization.
Also applies to: 165-165
CHANGELOG.md (1)
46-46: LGTM! Clear and well-documented change.The changelog entry properly documents the addition of the NFTokenModify transaction and TF_MUTABLE flag.
xrpl/core/binarycodec/definitions/definitions.json (1)
3088-3088: LGTM! Verify transaction type number uniqueness.The NFTokenModify transaction type is properly added with value 61.
Run this script to verify the transaction type number is unique and follows the sequence:
✅ Verification successful
Transaction type number 61 for NFTokenModify is unique and valid
The verification confirms that the transaction type number 61 is unique across all transaction types in the definitions file. While it doesn't follow the sequential pattern of other NFToken-related transactions (25-29), this is not a requirement as long as uniqueness is maintained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify NFTokenModify transaction type number is unique # and follows the sequence of other NFToken-related transaction types echo "Checking NFToken-related transaction types..." jq '.TRANSACTION_TYPES | to_entries | .[] | select(.key | startswith("NFToken")) | "\(.key): \(.value)"' xrpl/core/binarycodec/definitions/definitions.json echo -e "\nChecking for duplicate transaction type values..." jq '.TRANSACTION_TYPES | to_entries | group_by(.value) | .[] | select(length > 1) | map(.key)' xrpl/core/binarycodec/definitions/definitions.jsonLength of output: 652
| def _get_uri_error(self: Self) -> Optional[str]: | ||
| if not self.uri: | ||
| return "URI must not be empty string" | ||
| elif len(self.uri) > _MAX_URI_LENGTH: | ||
| return f"Must not be longer than {_MAX_URI_LENGTH} characters" | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for hex-encoded URI format.
The URI documentation states it must be hex-encoded, but the _get_uri_error method only validates length and emptiness. Consider adding validation for hex format.
def _get_uri_error(self: Self) -> Optional[str]:
if not self.uri:
return "URI must not be empty string"
elif len(self.uri) > _MAX_URI_LENGTH:
return f"Must not be longer than {_MAX_URI_LENGTH} characters"
+ elif not all(c in '0123456789ABCDEFabcdef' for c in self.uri):
+ return "URI must be hex-encoded"
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_uri_error(self: Self) -> Optional[str]: | |
| if not self.uri: | |
| return "URI must not be empty string" | |
| elif len(self.uri) > _MAX_URI_LENGTH: | |
| return f"Must not be longer than {_MAX_URI_LENGTH} characters" | |
| return None | |
| def _get_uri_error(self: Self) -> Optional[str]: | |
| if not self.uri: | |
| return "URI must not be empty string" | |
| elif len(self.uri) > _MAX_URI_LENGTH: | |
| return f"Must not be longer than {_MAX_URI_LENGTH} characters" | |
| elif not all(c in '0123456789ABCDEFabcdef' for c in self.uri): | |
| return "URI must be hex-encoded" | |
| return None |
| class TestNFTokenModify(TestCase): | ||
| def test_nftoken_miss(self): | ||
| with self.assertRaises(XRPLModelException): | ||
| NFTokenModify( | ||
| account=_ACCOUNT, | ||
| owner=_ACCOUNT, | ||
| sequence=_SEQUENCE, | ||
| fee=_FEE, | ||
| uri=_ACCOUNT, | ||
| ) | ||
|
|
||
| def test_uri_empty(self): | ||
| with self.assertRaises(XRPLModelException): | ||
| NFTokenModify( | ||
| account=_ACCOUNT, | ||
| owner=_ACCOUNT, | ||
| sequence=_SEQUENCE, | ||
| fee=_FEE, | ||
| nftoken_id=_NFTOKEN_ID, | ||
| uri="", | ||
| ) | ||
|
|
||
| def test_uri_too_long(self): | ||
| with self.assertRaises(XRPLModelException): | ||
| NFTokenModify( | ||
| account=_ACCOUNT, | ||
| owner=_ACCOUNT, | ||
| sequence=_SEQUENCE, | ||
| fee=_FEE, | ||
| nftoken_id=_NFTOKEN_ID, | ||
| uri=_ACCOUNT * 1000, | ||
| ) | ||
|
|
||
| def test_valid(self): | ||
| obj = NFTokenModify( | ||
| account=_ACCOUNT, | ||
| owner=_ACCOUNT, | ||
| sequence=_SEQUENCE, | ||
| fee=_FEE, | ||
| uri=_URI, | ||
| nftoken_id=_NFTOKEN_ID, | ||
| ) | ||
| self.assertTrue(obj.is_valid()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more test cases for comprehensive coverage.
The current test suite should be expanded to include:
- Validation of hex-encoded URI format
- Validation of NFTokenID format
- Integration test with TF_MUTABLE flag
+ def test_uri_not_hex(self):
+ with self.assertRaises(XRPLModelException):
+ NFTokenModify(
+ account=_ACCOUNT,
+ owner=_ACCOUNT,
+ sequence=_SEQUENCE,
+ fee=_FEE,
+ nftoken_id=_NFTOKEN_ID,
+ uri="not-hex-encoded",
+ )
+
+ def test_invalid_nftoken_id_format(self):
+ with self.assertRaises(XRPLModelException):
+ NFTokenModify(
+ account=_ACCOUNT,
+ owner=_ACCOUNT,
+ sequence=_SEQUENCE,
+ fee=_FEE,
+ nftoken_id="invalid",
+ uri=_URI,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestNFTokenModify(TestCase): | |
| def test_nftoken_miss(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| uri=_ACCOUNT, | |
| ) | |
| def test_uri_empty(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| nftoken_id=_NFTOKEN_ID, | |
| uri="", | |
| ) | |
| def test_uri_too_long(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| nftoken_id=_NFTOKEN_ID, | |
| uri=_ACCOUNT * 1000, | |
| ) | |
| def test_valid(self): | |
| obj = NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| uri=_URI, | |
| nftoken_id=_NFTOKEN_ID, | |
| ) | |
| self.assertTrue(obj.is_valid()) | |
| class TestNFTokenModify(TestCase): | |
| def test_nftoken_miss(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| uri=_ACCOUNT, | |
| ) | |
| def test_uri_empty(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| nftoken_id=_NFTOKEN_ID, | |
| uri="", | |
| ) | |
| def test_uri_too_long(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| nftoken_id=_NFTOKEN_ID, | |
| uri=_ACCOUNT * 1000, | |
| ) | |
| def test_uri_not_hex(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| nftoken_id=_NFTOKEN_ID, | |
| uri="not-hex-encoded", | |
| ) | |
| def test_invalid_nftoken_id_format(self): | |
| with self.assertRaises(XRPLModelException): | |
| NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| nftoken_id="invalid", | |
| uri=_URI, | |
| ) | |
| def test_valid(self): | |
| obj = NFTokenModify( | |
| account=_ACCOUNT, | |
| owner=_ACCOUNT, | |
| sequence=_SEQUENCE, | |
| fee=_FEE, | |
| uri=_URI, | |
| nftoken_id=_NFTOKEN_ID, | |
| ) | |
| self.assertTrue(obj.is_valid()) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
xrpl/models/transactions/nftoken_modify.py (2)
1-2: Enhance module documentation to clarify dynamic NFT support.The current docstring could be more descriptive about the purpose and use cases of dynamic NFTs, including when and why to use the NFTokenModify transaction.
-"""Model for NFTokenModify transaction type.""" +""" +Model for NFTokenModify transaction type. + +This transaction type enables modification of mutable NFTs on the XRP Ledger, +allowing updates to an NFT's URI after minting. This is particularly useful for +dynamic NFTs where the metadata or content needs to change over time, such as +evolving artwork, updated game assets, or real-world asset status changes. + +Note: This transaction can only modify NFTs that were minted with the TF_MUTABLE +flag enabled. +"""
38-47: Add example URI usage in documentation.The URI documentation would benefit from concrete examples of different URI formats and how to hex-encode them.
""" URI that points to the data and/or metadata associated with the NFT. This field need not be an HTTP or HTTPS URL; it could be an IPFS URI, a magnet link, immediate data encoded as an RFC2379 "data" URL, or even an opaque issuer-specific encoding. The URI is not checked for validity. This field must be hex-encoded. You can use `xrpl.utils.str_to_hex` to convert a UTF-8 string to hex. + + Examples: + >>> from xrpl.utils import str_to_hex + >>> # HTTP URL + >>> str_to_hex("https://example.com/metadata.json") + >>> # IPFS URI + >>> str_to_hex("ipfs://QmXw5eJZo...") + >>> # Data URL + >>> str_to_hex("data:application/json,{\"key\":\"value\"}") """tests/unit/models/transactions/test_nftoken_modify.py (4)
6-10: Enhance test data with more descriptive constants.Add more test constants to cover edge cases and invalid formats.
_ACCOUNT = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ" _SEQUENCE = 19048 _FEE = "0.00001" -_URI = "ABC" +_VALID_HEX_URI = "414243" # Hex for "ABC" +_INVALID_HEX_URI = "XYZ" _NFTOKEN_ID = "00090032B5F762798A53D543A014CAF8B297CFF8F2F937E844B17C9E00000003" +_INVALID_NFTOKEN_ID = "INVALID"
14-41: Add test cases for additional validation scenarios.The current test coverage could be expanded to include more edge cases.
+ def test_invalid_account(self): + """Test that an invalid account raises an exception.""" + with self.assertRaises(XRPLModelException) as error: + NFTokenModify( + account="invalid", + sequence=_SEQUENCE, + fee=_FEE, + nftoken_id=_NFTOKEN_ID, + uri=_VALID_HEX_URI, + ) + + def test_none_uri(self): + """Test that None URI is accepted (optional field).""" + obj = NFTokenModify( + account=_ACCOUNT, + sequence=_SEQUENCE, + fee=_FEE, + nftoken_id=_NFTOKEN_ID, + ) + self.assertTrue(obj.is_valid())
43-71: Add tests for real-world URI scenarios.Include tests for common URI formats used in NFTs.
+ def test_common_uri_formats(self): + """Test various common URI formats used in NFTs.""" + from xrpl.utils import str_to_hex + + test_cases = [ + "https://example.com/metadata.json", + "ipfs://QmXw5eJZo...", + "data:application/json,{\"key\":\"value\"}", + ] + + for uri in test_cases: + with self.subTest(uri=uri): + obj = NFTokenModify( + account=_ACCOUNT, + sequence=_SEQUENCE, + fee=_FEE, + nftoken_id=_NFTOKEN_ID, + uri=str_to_hex(uri), + ) + self.assertTrue(obj.is_valid())
73-82: Enhance valid case testing.Add more assertions to verify the object's state and serialization.
def test_valid(self): + """Test valid NFTokenModify instance with comprehensive assertions.""" obj = NFTokenModify( account=_ACCOUNT, owner=_ACCOUNT, sequence=_SEQUENCE, fee=_FEE, - uri=_URI, + uri=_VALID_HEX_URI, nftoken_id=_NFTOKEN_ID, ) self.assertTrue(obj.is_valid()) + # Verify object state + self.assertEqual(obj.account, _ACCOUNT) + self.assertEqual(obj.owner, _ACCOUNT) + self.assertEqual(obj.uri, _VALID_HEX_URI) + self.assertEqual(obj.nftoken_id, _NFTOKEN_ID) + # Verify serialization + tx_dict = obj.to_dict() + self.assertEqual(tx_dict["TransactionType"], "NFTokenModify") + self.assertEqual(tx_dict["URI"], _VALID_HEX_URI)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/transactions/test_nftoken_modify.py(1 hunks)xrpl/models/transactions/nftoken_modify.py(1 hunks)
🔇 Additional comments (2)
xrpl/models/transactions/nftoken_modify.py (2)
18-24: LGTM! Well-structured class definition.The class follows best practices with proper use of decorators for immutability and keyword-only initialization.
54-72: LGTM! Comprehensive validation implementation.The validation logic properly checks for empty URIs, maximum length, and hex encoding.
| nftoken_id: str = REQUIRED # type: ignore | ||
| """ | ||
| Identifies the TokenID of the NFToken object that the | ||
| offer references. This field is required. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add NFTokenID format validation.
The nftoken_id field should validate that it matches the XRP Ledger's NFTokenID format to prevent runtime errors.
+_NFTOKEN_ID_REGEX = re.compile(r'^[0-9A-F]{64}$', re.IGNORECASE)
+
@require_kwargs_on_init
@dataclass(frozen=True, **KW_ONLY_DATACLASS)
class NFTokenModify(Transaction):
# ...
def _get_errors(self: Self) -> Dict[str, str]:
return {
key: value
for key, value in {
**super()._get_errors(),
"uri": self._get_uri_error(),
+ "nftoken_id": self._get_nftoken_id_error(),
}.items()
if value is not None
}
+ def _get_nftoken_id_error(self: Self) -> Optional[str]:
+ if not _NFTOKEN_ID_REGEX.match(self.nftoken_id):
+ return "NFTokenID must be a 64-character hexadecimal string"
+ return NoneCommittable suggestion skipped: line range outside the PR's diff.
|
We should add integration test(s) for the new transaction as well using v2.4.0-b3 |
We don't have any integration tests for all the NFT transactions. Can we have another task for that? |
Hmm not sure if there's a reason for for that, but sounds good to me. |
CHANGELOG.md
Outdated
| @@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Add `nfts_by_issuer` clio-only API definition | |||
| - Included `ctid` field in the `tx` request. | |||
| - `from_xrpl` method accepts input dictionary keys exclusively in the proper XRPL format. | |||
| - Add `NFTokenModify` transaction and add `TF_MUTABLE` flag in `NFTokenMintFlag` (XLS-46) | |||
There was a problem hiding this comment.
| - Add `NFTokenModify` transaction and add `TF_MUTABLE` flag in `NFTokenMintFlag` (XLS-46) | |
| - Support for DynamicNFT amendment (XLS-46) |
Since all the NFT related integration tests are missing, currently we don't have bandwidth to do that. We have task in backlog to track it and will work on it in the future. https://ripplelabs.atlassian.net/browse/DEFI-201 |
High Level Overview of Change
related to PR: XRPLF/rippled#5048
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan