XLS-46: DynamicNFT

[tequdev]

Replace from https://github.com/XRPLF/rippled/pull/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

https://github.com/XRPLF/XRPL-Standards/discussions/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)

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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%)	:arrow_up:
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%)	:arrow_up:
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]

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]

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.

https://github.com/XRPLF/rippled/pull/4838#issuecomment-1994709096

[xVet]

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]

BTW, the "Context of change" section linked to the XLS20 proposal, I think you meant to link to this one https://github.com/XRPLF/XRPL-Standards/discussions/130

[tequdev]

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!

[tequdev]

@scottschurr Thanks for the review. I fully agree with your suggestions and have cherry-picked all three commits.

[shawnxie999]

I suggest holding off on merging this PR until the Clio counterpart is also approved. While this isn't standard practice, the NFT functionality is in a unique position where its Clio implementation is necessary for it to work correctly. IMO ideally we should make sure the feature works on both ends before merging both into develop.

[tequdev]

I suggest holding off on merging this PR until the Clio counterpart is also approved. While this isn't standard practice, the NFT functionality is in a unique position where its Clio implementation is necessary for it to work correctly. IMO ideally we should make sure the feature works on both ends before merging both into develop.

The PR has been inactive for about a month... If it is not included in the next release (2.3.0), we will have to wait a few more months.

[cindyyan317]

I suggest holding off on merging this PR until the Clio counterpart is also approved. While this isn't standard practice, the NFT functionality is in a unique position where its Clio implementation is necessary for it to work correctly. IMO ideally we should make sure the feature works on both ends before merging both into develop.

The PR has been inactive for about a month... If it is not included in the next release (2.3.0), we will have to wait a few more months.

Hi @tequdev , The Clio counterpart has been finished. After this PR is merged to rippled, Clio will update the XRPL library and do the final test.

[mvadari]

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)

[mvadari]

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]

ready to merge👍

[mvadari]

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

[mvadari]

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

[tequdev]

@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