Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XLS-46: DynamicNFT #5048
XLS-46: DynamicNFT #5048
Changes from 18 commits
598a0e3
d851678
e30addc
0848a98
624e11e
4cbde96
afaa675
f6418aa
44790ab
ef6ba44
b9becc4
8fd988d
68a51e1
750adda
4b3480c
f4ab8c6
47106cc
4ca5105
579dc25
cae9b62
23ecb99
2eb7559
e9dd06c
c89f41d
1b2cf6d
d52b659
90aeef9
4eea716
321459d
b41b98a
fc81bcc
0f02668
fa732bf
16541e2
a76809a
e3f6542
29e7d4d
727ad8f
d26ba5a
a6aaa28
7154534
13156dd
f6f8084
ca09114
7390b1d
f7311ba
ccbed88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec (XRPLF/XRPL-Standards#130 August 18, 2023 version) specifies that the
sfURI
field is required. Here it is listed assoeOPTIONAL
. I think thatsoeOPTIONAL
is a good choice, because that matches the behavior ofNFTokenMint
. Perhaps the spec needs to be updated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you pointed it out, as of Jun 29 the
sfURI
field has been updated to an optional field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any use case for removing the URI from an existing NFT? Is there any difference from burning the NFT compared to an NFT without a URI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a use case may not be common, but it is not impossible to say that there is no use case.
Since the NFTokenMint transaction can be executed with the URI field unspecified, it is reasonable that the NFTokenModify transaction can change the URI field to unspecified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a flag for removing the URI from the NFT, like with
NFTokenMinter
, instead of just using its absence?