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

xVet · 2023-11-27T21:42:33Z

High Level Overview of Change

This Amendment adds functionality to update the URI of XLS-20 NFTs 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

Changes and spec: XLS-46

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

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)

Future Tasks

Test case for:

NFTokenModify
lsfMutable (new flag for NFTokenMint)

src/ripple/protocol/TxFlags.h

src/ripple/app/tx/impl/NFTokenModify.cpp

src/ripple/protocol/TxFlags.h

src/ripple/protocol/nft.h

…NFTs-XLS-46

xVet · 2023-11-28T02:17:21Z

Tests are for the next commits. Tests, guards and code related to tfTrustLine/fixRemoveNFTokenAutoTrustLine are removed in my draft as well

shawnxie999 · 2023-11-30T15:51:22Z

Also would want to point out that this change would require a Clio ETL update as well, as it introduces a new transaction. Would be happy to help 😄

shawnxie999 · 2023-11-30T21:52:53Z

src/ripple/protocol/TxFlags.h

should also add
constexpr std::uint32_t const tfNFTokenModifyMask = ~tfUniversal;

This is unnecessary - all the other transactions that don't have any flags just use the tfUniversalMask variable instead of creating a new one.

True, unless new flags are looking to be added in the future 🤷

Yes, but a new variable can be added then.

shawnxie999 · 2023-11-30T21:53:42Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+        if (uri.length() == 0 || uri.length() > maxTokenURILength)
+            return temMALFORMED;
+    }
+


need to check

if (txFlags & tfNFTokenModifyMask) return temINVALID_FLAG;

mvadari · 2023-12-01T20:47:09Z

src/ripple/protocol/Feature.h

@@ -352,6 +352,8 @@ extern uint256 const featureXChainBridge;
 extern uint256 const fixDisallowIncomingV1;
 extern uint256 const featureDID;
 extern uint256 const fixFillOrKill;
+extern uint256 const featureDNFT;


nit: I would call this featureDynamicNFT to be a bit more descriptive

mvadari · 2023-12-01T20:47:36Z

src/ripple/protocol/TxFlags.h

 constexpr std::uint32_t const tfTransferable               = 0x00000008;
+constexpr std::uint32_t const tfMutable                    = 0x00000016;


Flags are bitmasks, they should only be one bit and the number is already in hex.

Suggested change

constexpr std::uint32_t const tfMutable = 0x00000016;

constexpr std::uint32_t const tfMutable = 0x00000010;

mvadari · 2023-12-01T20:49:48Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+
+

nit:

Suggested change

mvadari · 2023-12-01T20:50:00Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+
+    return tesSUCCESS;    
+}
+}


nit: files should end with a newline

mvadari · 2023-12-01T20:50:17Z

src/ripple/app/tx/impl/NFTokenModify.h

@@ -0,0 +1,31 @@
+


This needs the standard license/disclaimer text (ditto for NFTokenModify.cpp)

mvadari · 2023-12-01T20:51:34Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+    auto const nftID = ctx.tx[sfNFTokenID];
+    auto const account = ctx.tx[sfAccount];
+
+    if (!account || !nftID)
+       return temMALFORMED; 


This check is unneeded - it'll be checked by TxFormats (it'll also crash when you try to fetch those values from ctx.tx if you don't use the ~ anyways)

mvadari · 2023-12-01T20:52:30Z

src/ripple/app/tx/impl/NFTokenModify.cpp

@@ -0,0 +1,96 @@
+
+#include <ripple/app/tx/impl/NFTokenModify.h>


This new transaction type needs to be added to ripple/protocol/TxFormats.h and ripple/protocol/impl/TxFormats.cpp and ripple/app/tx/impl/applySteps.cpp - otherwise rippled doesn't know it exists

shawnxie999 · 2023-12-04T15:14:17Z

I would suggest add a validation in NFTokenModify, where it returns failure if the NFT still has outstanding buy and sell offers.

Otherwise, I see two problems:

If the owner changes the URI and someone else accepts a current offer around the same time, the buyer may not realize that the URI has been updated, resulting in buying a NFT that they may not have want. I think we would like to avoid this confusion and potential bad act at the protocol layer.
From a technical perspective, the Clio ETL does not handle a edge case very well - if a NFTokenModify and NFTokenAcceptOffer occur at the the same ledger, Clio will result in data loss. This a design with the clio that hasn't considered the possibility where the URI change and a transfer of ownership can happen at the same ledger. Trying to solve this problem is rather messy to do in the Clio end, so I would prefer not go that route.

So I think adding a validation to check for offers, even though it will be some extra work as the owner would need to cancel all the offers, but that's reasonable.

Any thoughts?

xVet · 2023-12-04T16:52:09Z

Great point! I agree it makes totally sense to first check if offers are available and if so, to cancel them all ? Would you say, if offers available then let NFTokenModify fail or rather cancel all offers and then execute? Basically guaranteeing a successful execution either way.

Thanks for the noticing this edge scenario Shawn 🤗

shawnxie999 · 2023-12-04T17:52:52Z

Great point! I agree it makes totally sense to first check if offers are available and if so, to cancel them all ? Would you say, if offers available then let NFTokenModify fail or rather cancel all offers and then execute? Basically guaranteeing a successful execution either way.

Thanks for the noticing this edge scenario Shawn 🤗

Both are possible solutions, I don't really have a strong opinion.

To summarize the two options:

NFTokenModify fails if the NFT still has offers, and does not cancel them. This should be pretty straightforward, all we need is checking if the sell and buy offer directories are empty or not.
NFTokenModify implicitly cancels all the existing offers first, and then changes the URI. However, we need to make sure the the number of existing offers is within a threshold(500), so that the metadata of the transaction won't be too huge. The code to implement this is very similar to NFTokenBurn before the activation of the amendment fixNonFungibleTokensV1_2. In preclaim of NFTokenBurn.cpp, there is a line of code :

    if (!ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
    {
        // If there are too many offers, then burning the token would produce
        // too much metadata.  Disallow burning a token with too many offers.
        return nft::notTooManyOffers(ctx.view, ctx.tx[sfNFTokenID]);
    }

This code will return error if the number of offers exceeds 500, and therefore would require the owner to manually cancel them.

Similarly, NFTokenModify should cancel up to maximum of 500 offers, and then change the URI. But, if the number of offers exceeds 500, then it will simply return an error without changing anything.

Please feel free to make the call, as I think both are acceptable solutions, unless other people think otherwise. 😄

shawnxie999 · 2023-12-04T18:19:45Z

But there is still a concern, what if, in the same ledger, the owner submits a NFTokenModify at the same time someone else creates a buy offer? If that can happen, then the buyer submits a buy offer without knowing the URI changed, and the offer wasn't cancelled because NFTokenModify and NFTokenCreateOffer occured in the same ledger (if NFTokenModify is executed before NFTokenCreateOffer in that ledger)

I'm not sure of this either and I think we may want to double check this edge case in unit test if it's something we would care about.

tequdev · 2023-12-05T04:50:17Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+        return ret;
+
+    auto const nftID = ctx.tx[sfNFTokenID];
+    auto const account = ctx.tx[sfAccount];


As mentioned in the Discussion, sfAccount is a reserved common field for transaction types, it should be sfOwner.

XRPLF/XRPL-Standards#130 (comment)

tequdev · 2023-12-05T04:51:47Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+NotTEC
+NFTokenModify::preflight(PreflightContext const& ctx)
+{
+    if (!ctx.rules.enabled(featureDNFT))


NonFungibleTokensV1_1 must be also enabled

tequdev · 2023-12-05T04:54:22Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+NFTokenModify::doApply()
+{
+    auto const nftokenID = ctx_.tx[sfNFTokenID];
+    auto const account = ctx_.tx[sfAccount];


NFToken must be found from sfOwner.

tequdev · 2023-12-05T05:01:03Z

src/ripple/app/tx/impl/NFTokenModify.cpp

+    if (ctx_.tx.isFieldPresent(sfURI)) {
+        auto newURI = ctx_.tx[sfURI];
+        tokenAndPage->token[sfURI] = newURI; 
+    }


Since the URI is a required field in the Standard, this condition is always true if it is followed.

If the URI can be deleted, the URI field should be an optional field.

I think it would be appropriate to change the URI field in Standard to an optional field.
And overwrite the URI if it exists in the transaction and delete the URI if it does not.

shawnxie999 · 2024-03-13T15:24:34Z

But there is still a concern, what if, in the same ledger, the owner submits a NFTokenModify at the same time someone else creates a buy offer? If that can happen, then the buyer submits a buy offer without knowing the URI changed, and the offer wasn't cancelled because NFTokenModify and NFTokenCreateOffer occured in the same ledger (if NFTokenModify is executed before NFTokenCreateOffer in that ledger)

I'm not sure of this either and I think we may want to double check this edge case in unit test if it's something we would care about.

To resolve this concern. I would propose an update to the spec, such that the owner can only submit a NFTokenModify transaction if the the NFT owner account has set lsfDisallowIncomingNFTokenOffer, to block any incoming offers until the URI has changed. But another concern of this approach is that it would block incoming offers for all NFTs, which may not be desirable.

mvadari · 2024-02-27T21:38:09Z

src/ripple/app/tx/impl/details/NFTokenUtils.h

@@ -97,6 +97,9 @@ deleteTokenOffer(ApplyView& view, std::shared_ptr<SLE> const& offer);
 bool
 compareTokens(uint256 const& a, uint256 const& b);

+TER 
+updateToken(ApplyView& view, AccountID const& owner, STObject&& nft, std::shared_ptr<SLE>&& page);


nit: should this method be called modifyToken to match the transaction name?

mvadari · 2024-02-27T21:39:32Z

src/ripple/app/tx/impl/NFTokenMint.cpp

    std::uint32_t const NFTokenMintMask =
-        ctx.rules.enabled(fixRemoveNFTokenAutoTrustLine) ? tfNFTokenMintMask
-                                                         : tfNFTokenMintOldMask;
+        ctx.rules.enabled(featureDNFT) ? tfNFTokenMintMask
+                                       : tfNFTokenMintOldMask;


The old mask should still also be used based on if fixRemoveNFTokenAutoTrustLine is enabled (this may make the mask a bit complicated, so it might be worth constructing it here instead of having several variables).

dangell7 · 2024-03-13T15:41:36Z

But there is still a concern, what if, in the same ledger, the owner submits a NFTokenModify at the same time someone else creates a buy offer? If that can happen, then the buyer submits a buy offer without knowing the URI changed, and the offer wasn't cancelled because NFTokenModify and NFTokenCreateOffer occured in the same ledger (if NFTokenModify is executed before NFTokenCreateOffer in that ledger)
I'm not sure of this either and I think we may want to double check this edge case in unit test if it's something we would care about.

To resolve this concern. I would propose an update to the spec, such that the owner can only submit a NFTokenModify transaction if the the NFT owner account has set lsfDisallowIncomingNFTokenOffer, to block any incoming offers until the URI has changed. But another concern of this approach is that it would block incoming offers for all NFTs, which may not be desirable.

I think all of that is irrelevant as the issuer can just update the NFToken after the sale. Wether the change happens before the sale or after is irrelevant imo.

Personally I wouldn't do anything with the offers. If clio cant handle this then thats a design flaw on clio and no one should be limiting themselves because clio cant do something.

shawnxie999 · 2024-03-13T16:45:16Z

I think all of that is irrelevant as the issuer can just update the NFToken after the sale. Wether the change happens before the sale or after is irrelevant imo.

Personally I wouldn't do anything with the offers. If clio cant handle this then thats a design flaw on clio and no one should be limiting themselves because clio cant do something.

This actually has nothing to do with clio, but yes it's only a minor detail. If everyone feels good about it, then I don't have any concern.

Dynamic NFT

mvadari · 2025-02-13T18:45:06Z

Closing this in favor of #5048

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

XLS-46 dNFTs PR first

598a0e3

dNFTs initial

d851678

mvadari reviewed Nov 27, 2023

View reviewed changes

src/ripple/protocol/TxFlags.h Outdated Show resolved Hide resolved

mvadari reviewed Nov 27, 2023

View reviewed changes

src/ripple/app/tx/impl/NFTokenModify.cpp Outdated Show resolved Hide resolved

mvadari reviewed Nov 27, 2023

View reviewed changes

src/ripple/protocol/TxFlags.h Outdated Show resolved Hide resolved

mvadari reviewed Nov 27, 2023

View reviewed changes

src/ripple/protocol/nft.h Outdated Show resolved Hide resolved

xVet closed this Nov 27, 2023

xVet reopened this Nov 27, 2023

xVet marked this pull request as draft November 27, 2023 22:43

xVet and others added 4 commits November 28, 2023 02:14

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

shawnxie999 reviewed Nov 30, 2023

View reviewed changes

mvadari reviewed Dec 1, 2023

View reviewed changes

src/ripple/app/tx/impl/NFTokenModify.cpp Outdated

Comment on lines 77 to 78

Copy link

Collaborator

mvadari Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change

mvadari reviewed Dec 1, 2023

View reviewed changes

src/ripple/app/tx/impl/NFTokenModify.cpp Outdated

return tesSUCCESS;

}

}

Copy link

Collaborator

mvadari Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: files should end with a newline

mvadari reviewed Dec 1, 2023

View reviewed changes

tequdev reviewed Dec 5, 2023

View reviewed changes

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

afaa675

tequdev added 5 commits February 16, 2024 17:55

dNFTs

f6418aa

rename dNFT to DynamicNFT

44790ab

clang-format

ef6ba44

fix flag test

b9becc4

add standard license/disclaimer text

8fd988d

mvadari reviewed Mar 13, 2024

View reviewed changes

Merge PR tequdev/dNFTs-XLS-46 - add URI token optional

b9785a0

Dynamic NFT

This was referenced Jun 18, 2024

XLS-46: DynamicNFT tequdev/rippled#8

Open

XLS-46: DynamicNFT #5048

Merged

mvadari closed this Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xVet commented Nov 27, 2023 •

edited

Loading

xVet commented Nov 28, 2023

shawnxie999 commented Nov 30, 2023 •

edited

Loading

shawnxie999 Nov 30, 2023

mvadari Nov 30, 2023

shawnxie999 Nov 30, 2023

mvadari Nov 30, 2023

shawnxie999 Nov 30, 2023

mvadari Dec 1, 2023

mvadari Dec 1, 2023

mvadari Dec 1, 2023

mvadari Dec 1, 2023

mvadari Dec 1, 2023 •

edited

Loading

mvadari Dec 1, 2023 •

edited

Loading

mvadari Dec 1, 2023

shawnxie999 commented Dec 4, 2023

xVet commented Dec 4, 2023 •

edited

Loading

shawnxie999 commented Dec 4, 2023 •

edited

Loading

shawnxie999 commented Dec 4, 2023

tequdev Dec 5, 2023

tequdev Dec 5, 2023

tequdev Dec 5, 2023

tequdev Dec 5, 2023

shawnxie999 commented Mar 13, 2024

mvadari Feb 27, 2024

mvadari Feb 27, 2024

dangell7 commented Mar 13, 2024

shawnxie999 commented Mar 13, 2024

mvadari commented Feb 13, 2025

		constexpr std::uint32_t const tfTransferable = 0x00000008;
		constexpr std::uint32_t const tfMutable = 0x00000016;

		@@ -0,0 +1,96 @@

		#include <ripple/app/tx/impl/NFTokenModify.h>

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

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

Conversation

xVet commented Nov 27, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

Future Tasks

xVet commented Nov 28, 2023

shawnxie999 commented Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

mvadari Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 commented Dec 4, 2023

xVet commented Dec 4, 2023 • edited Loading

shawnxie999 commented Dec 4, 2023 • edited Loading

shawnxie999 commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangell7 commented Mar 13, 2024

shawnxie999 commented Mar 13, 2024

mvadari commented Feb 13, 2025

xVet commented Nov 27, 2023 •

edited

Loading

shawnxie999 commented Nov 30, 2023 •

edited

Loading

mvadari Dec 1, 2023 •

edited

Loading

mvadari Dec 1, 2023 •

edited

Loading

xVet commented Dec 4, 2023 •

edited

Loading

shawnxie999 commented Dec 4, 2023 •

edited

Loading