XLS-46: DynamicNFT #5048

tequdev · 2024-06-18T04:56:37Z

Replace from #4838
Co-Author: @xVet

High Level Overview of Change

Spec: XLS-46d: Dynamic Non Fungible Tokens (dNFTs)

This Amendment adds functionality to update the URI of NFToken objects by adding:

NFTokenModify: Transactor to initiate the altering of the URI
lsfMutable: Flag to be set in a NFTokenMint transaction to allow NFTokenModify to execute successfully on given NFT.

Context of Change

XRPLF/XRPL-Standards#130

Type of Change

[x ] New feature (non-breaking change which adds functionality)
[x ] Tests (you added tests for code that already exists, or your new feature included in this PR)

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

…NFTs-XLS-46

codecov · 2024-06-18T05:26:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (040cd23) to head (ccbed88).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5048   +/-   ##
=======================================
  Coverage     77.9%   77.9%           
=======================================
  Files          783     785    +2     
  Lines        66707   66757   +50     
  Branches      8118    8119    +1     
=======================================
+ Hits         51954   52002   +48     
- Misses       14753   14755    +2

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
include/xrpl/protocol/detail/transactions.macro	`100.0% <100.0%> (ø)`
include/xrpl/protocol/nft.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/NFTokenMint.cpp	`98.0% <100.0%> (+<0.1%)`	⬆️
src/xrpld/app/tx/detail/NFTokenModify.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/NFTokenModify.h	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/NFTokenUtils.cpp	`92.0% <100.0%> (+0.2%)`	⬆️
src/xrpld/app/tx/detail/NFTokenUtils.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/applySteps.cpp	`78.0% <ø> (ø)`

... and 2 files with indirect coverage changes

shawnxie999 · 2024-06-20T13:31:36Z

In other PR, I suggested adding a check so that NFTokenModify would fail if there is any outstanding offers. This is to prevent the issuer from changing the URI and then accept an existing offer.

tequdev · 2024-06-20T13:58:38Z

In other PR, I suggested adding a check so that NFTokenModify would fail if there is any outstanding offers. This is to prevent the issuer from changing the URI and then accept an existing offer.

As mentioned in this comment, wouldn't that make sense since the issuer can change the URI after the user has bought it?
Users need to be aware of the risk to mutable NFTs, just as they need to be aware of the possibility of being burned by the issuer after purchasing a Burnable NFT.

#4838 (comment)

xVet · 2024-06-20T15:19:24Z

Correct, if you own a dynamic NFT then you agree that it can be changed by the issuer. I wouldn't personally put a check on offers, because this might cut into some use cases people come up with.

shawnxie999 · 2024-06-21T17:05:14Z

BTW, the "Context of change" section linked to the XLS20 proposal, I think you meant to link to this one XRPLF/XRPL-Standards#130

tequdev · 2024-06-22T11:08:48Z

BTW, the "Context of change" section linked to the XLS20 proposal, I think you meant to link to this one XRPLF/XRPL-Standards#130

good catch, fixed!

scottschurr

Nice job on this pull request! The unit test coverage is pretty good. I appreciate that you took the time to do that.

I left quite a number of comments, but don't be dismayed by that ...

Most of the comments are personal preferences of mine (although I justify them).
I can be a very picky reviewer.

That said, I think that my comment regarding the nft::updateToken() implementation is pretty important. If you choose not to implement that change I'll want to understand why.

For your convenience I made commits that implement all of the changes I've suggested. If you want, you're welcome to cherry pick them, or you can just use them as examples of the changes I am suggesting. The commits are the top-most three on branch https://github.com/scottschurr/rippled/commits/tequ-featureDynamicNFT/

Suggested approach to NFTokenMintMask: 8aec32c
Suggested change for testNFTokenModify(): 813c302
Suggested changes to NFTokenModify.cpp and NFTokenUtils.*: 1393268

I hope you find those useful.

Thanks for the submission!

include/xrpl/protocol/TxFlags.h

scottschurr · 2024-06-25T01:17:06Z

src/libxrpl/protocol/TxFormats.cpp

+        {
+            {sfNFTokenID, soeREQUIRED},
+            {sfOwner, soeOPTIONAL},
+            {sfURI, soeOPTIONAL},


The spec (XRPLF/XRPL-Standards#130 August 18, 2023 version) specifies that the sfURI field is required. Here it is listed as soeOPTIONAL. I think that soeOPTIONAL is a good choice, because that matches the behavior of NFTokenMint. Perhaps the spec needs to be updated?

Since you pointed it out, as of Jun 29 the sfURI field has been updated to an optional field.

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?

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.

Makes sense. Thanks!

Should there be a flag for removing the URI from the NFT, like with NFTokenMinter, instead of just using its absence?

src/test/app/NFToken_test.cpp

src/xrpld/app/tx/detail/NFTokenModify.cpp

src/xrpld/app/tx/detail/NFTokenUtils.cpp

src/xrpld/app/tx/detail/NFTokenModify.cpp

src/xrpld/app/tx/detail/NFTokenUtils.cpp

Co-authored-by: Mayukha Vadari <[email protected]>

mvadari · 2024-12-20T20:18:10Z

Build error -

src/xrpld/app/tx/detail/NFTokenModify.cpp:72:35: error: use of undeclared identifier 'sle'
   72 |         if (auto const minter = (*sle)[~sfNFTokenMinter]; minter != account)

include/xrpl/protocol/detail/transactions.macro

mvadari · 2025-01-08T18:47:40Z

Good to go from the automation tests. @tequdev please update the branch and ensure that this is ready to merge from your side.
cc @bthomee for visibility

tequdev · 2025-01-09T00:41:48Z

ready to merge👍

mvadari · 2025-01-09T16:06:42Z

@bthomee this is ready to merge whenever you get a chance

This Amendment adds functionality to update the URI of NFToken objects as described in the XLS-46d: Dynamic Non Fungible Tokens (dNFTs) spec.

mvadari · 2025-01-14T19:15:35Z

@tequdev it looks like this code doesn't check for invalid flags - can that be added in a separate PR?

tequdev · 2025-01-15T13:22:52Z

@tequdev it looks like this code doesn't check for invalid flags - can that be added in a separate PR?

Nice catch!

You can check the changes in this PR.
#5246

xVet and others added 17 commits November 27, 2023 16:30

XLS-46 dNFTs PR first

598a0e3

dNFTs initial

d851678

added dnft amendment and placed guards

e30addc

Merge branch 'XRPLF:develop' into dNFTs-XLS-46

0848a98

added featureDNFT in Feature.h

624e11e

Merge branch 'dNFTs-XLS-46' of https://github.com/xVet/rippled into d…

4cbde96

…NFTs-XLS-46

Merge remote-tracking branch 'upstream/develop' into dNFTs-XLS-46

afaa675

dNFTs

f6418aa

rename dNFT to DynamicNFT

44790ab

clang-format

ef6ba44

fix flag test

b9becc4

add standard license/disclaimer text

8fd988d

Merge remote-tracking branch 'upstream/develop' into dNFTs-XLS-46

68a51e1

chore

750adda

fix tests

4b3480c

Merge remote-tracking branch 'upstream/develop' into featureDynamicNFT

f4ab8c6

Merge branch 'develop' into featureDynamicNFT

47106cc

cindyyan317 mentioned this pull request Jun 18, 2024

Support DynamicNFT XLS-46 XRPLF/clio#1471

Closed

Merge branch 'develop' into featureDynamicNFT

4ca5105

seelabs requested review from scottschurr and shawnxie999 June 21, 2024 16:15

scottschurr suggested changes Jun 28, 2024

View reviewed changes

scottschurr added 2 commits June 29, 2024 13:30

[FOLD] An incremental approach to the tfNFTokenMintMask variants

579dc25

[FOLD] Extend testNFTokenModify unit test

cae9b62

tequdev added 3 commits November 12, 2024 15:40

Merge branch 'develop' into featureDynamicNFT

727ad8f

Merge branch 'develop' into featureDynamicNFT

d26ba5a

Merge branch 'develop' into featureDynamicNFT

a6aaa28

mvadari reviewed Dec 12, 2024

View reviewed changes

src/xrpld/app/tx/detail/NFTokenModify.cpp Outdated Show resolved Hide resolved

mvadari reviewed Dec 12, 2024

View reviewed changes

src/xrpld/app/tx/detail/NFTokenModify.cpp Outdated Show resolved Hide resolved

mvadari reviewed Dec 13, 2024

View reviewed changes

src/xrpld/app/tx/detail/NFTokenUtils.cpp Outdated Show resolved Hide resolved

mvadari reviewed Dec 13, 2024

View reviewed changes

src/xrpld/app/tx/detail/NFTokenUtils.cpp Outdated Show resolved Hide resolved

tequdev and others added 3 commits December 15, 2024 17:35

Update src/xrpld/app/tx/detail/NFTokenModify.cpp

7154534

Co-authored-by: Mayukha Vadari <[email protected]>

Fix error handling that shoudn't be possible

13156dd

clang-format

f6f8084

ximinez and others added 2 commits December 20, 2024 15:54

Merge branch 'develop' into featureDynamicNFT

ca09114

fix build error

7390b1d

cindyyan317 reviewed Dec 23, 2024

View reviewed changes

include/xrpl/protocol/detail/transactions.macro Outdated Show resolved Hide resolved

fix tt tag

f7311ba

mvadari added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jan 8, 2025

Merge branch 'develop' into featureDynamicNFT

ccbed88

bthomee approved these changes Jan 9, 2025

View reviewed changes

bthomee merged commit 58af62f into XRPLF:develop Jan 9, 2025
21 checks passed

ximinez pushed a commit to ximinez/rippled that referenced this pull request Jan 13, 2025

XLS-46: DynamicNFT (XRPLF#5048)

641697d

This Amendment adds functionality to update the URI of NFToken objects as described in the XLS-46d: Dynamic Non Fungible Tokens (dNFTs) spec.

tequdev mentioned this pull request Jan 15, 2025

Fix the flag processing of NFTokenModify #5246

Merged

13 tasks

ximinez mentioned this pull request Jan 16, 2025

Proposed 2.4.0-b2 #5249

Closed

5 tasks

ximinez mentioned this pull request Jan 30, 2025

Proposed 2.4.0-b3 #5262

Merged

5 tasks

yinyiqian1 mentioned this pull request Jan 31, 2025

support DynamicNFT XLS-46 XRPLF/xrpl-py#799

Merged

9 tasks

mvadari mentioned this pull request Feb 13, 2025

XLS-46: Dynamic Non Fungible Tokens (dNFTs) #4838

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XLS-46: DynamicNFT #5048

XLS-46: DynamicNFT #5048

tequdev commented Jun 18, 2024 •

edited by intelliot

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

shawnxie999 commented Jun 20, 2024

tequdev commented Jun 20, 2024

xVet commented Jun 20, 2024

shawnxie999 commented Jun 21, 2024

tequdev commented Jun 22, 2024

scottschurr left a comment

scottschurr Jun 25, 2024

tequdev Jun 29, 2024

shawnxie999 Jul 3, 2024 •

edited

Loading

tequdev Jul 4, 2024

shawnxie999 Jul 4, 2024

mvadari Jul 5, 2024

mvadari commented Dec 20, 2024

mvadari commented Jan 8, 2025 •

edited

Loading

tequdev commented Jan 9, 2025

mvadari commented Jan 9, 2025

mvadari commented Jan 14, 2025

tequdev commented Jan 15, 2025

XLS-46: DynamicNFT #5048

XLS-46: DynamicNFT #5048

Conversation

tequdev commented Jun 18, 2024 • edited by intelliot Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

shawnxie999 commented Jun 20, 2024

tequdev commented Jun 20, 2024

xVet commented Jun 20, 2024

shawnxie999 commented Jun 21, 2024

tequdev commented Jun 22, 2024

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Jun 25, 2024

Choose a reason for hiding this comment

tequdev Jun 29, 2024

Choose a reason for hiding this comment

shawnxie999 Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

tequdev Jul 4, 2024

Choose a reason for hiding this comment

shawnxie999 Jul 4, 2024

Choose a reason for hiding this comment

mvadari Jul 5, 2024

Choose a reason for hiding this comment

mvadari commented Dec 20, 2024

mvadari commented Jan 8, 2025 • edited Loading

tequdev commented Jan 9, 2025

mvadari commented Jan 9, 2025

mvadari commented Jan 14, 2025

tequdev commented Jan 15, 2025

tequdev commented Jun 18, 2024 •

edited by intelliot

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

shawnxie999 Jul 3, 2024 •

edited

Loading

mvadari commented Jan 8, 2025 •

edited

Loading