XLS-46: DynamicNFT #5048
XLS-46: DynamicNFT #5048
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5048 +/- ##
=========================================
- Coverage 76.2% 76.1% -0.0%
=========================================
Files 760 762 +2
Lines 61568 61620 +52
Branches 8126 8147 +21
=========================================
+ Hits 46909 46923 +14
- Misses 14659 14697 +38
... and 10 files with indirect coverage changes |
|
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? |
|
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. |
|
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! |
There was a problem hiding this comment.
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!
| { | ||
| {sfNFTokenID, soeREQUIRED}, | ||
| {sfOwner, soeOPTIONAL}, | ||
| {sfURI, soeOPTIONAL}, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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.
Should there be a flag for removing the URI from the NFT, like with NFTokenMinter, instead of just using its absence?
|
@scottschurr |
| ApplyView& view, | ||
| AccountID const& owner, | ||
| uint256 const& nftokenID, | ||
| std::optional<ripple::Slice> const& uri) |
There was a problem hiding this comment.
@scottschurr may I ask why is Slice type used here, and not Buffer?
Also another related question, in STBlob, it usesBuffer to store value_, but why value_type have type Slice and not Buffer? In STBlob:
class STBlob : public STBase, public CountedObject<STBlob>
{
Buffer value_;
public:
using value_type = Slice;
Thanks!
There was a problem hiding this comment.
@scottschurr may I ask why is Slice type used here, and not Buffer?
Sure. The simple answer is that I was looking for the most obvious type to pass in. The only call in the code base (currently) to nft::changeTokenURI looks like this:
return nft::changeTokenURI(view(), owner, nftokenID, ctx_.tx[~sfURI]);
The awkward question is "What type does ctx_.tx[~sfURI] return?" I could dig through a bunch of templates in STObject and try to figure that out. But it's really easy to get lost in there. So in situations like this I usually rely on my compiler to tell me.
I picked a type that I'm pretty sure is not constructible or assignable from whatever ctx_.tx[~sfURI] returns. I don't remember the specific type I picked this time, but I often pick int*. Then I tell the compiler that I want to assign ctx_.tx[~sfURI] to an int*. It might look like this:
[[maybe_unused]] int* junk = ctx_.tx[~sfURI]; // !!!! DEBUG !!!!
Then I compile hoping to get a useful error message out. I use clang, so I this case I got:
/Users/scott/Development/rippled/src/xrpld/app/tx/detail/NFTokenModify.cpp:87:27: error: no viable conversion from 'std::optional<std::decay_t<typename STBlob::value_type>>' (aka 'optional<ripple::Slice>') to 'int *'
[[maybe_unused]] int* junk = ctx_.tx[~sfURI];
Way off at the end of the error message it tells me "no viable conversion from ... (aka optional<ripple::Slice>) to int *.
So the error message told me that, in order to avoid conversions, I needed to pass an optional<ripple::Slice> to changeTokenURI(). Once I had that information I removed the test code from the code base.
A Slice is really cheap to copy, so it often makes sense to pass a Slice by value. But I'm all the way up to an optional<ripple::Slice> which is not quite so obvious about pass-by-value or pass-by-const-ref. I was conservative and picked pass-by-const-ref.
Also another related question, in
STBlob, it usesBufferto storevalue_, but whyvalue_typehave typeSliceand notBuffer?
That's a great question, and it's all about efficiency. If we look inside Buffer it holds onto the buffer contents like this:
std::unique_ptr<std::uint8_t[]> p_;
It's not a shared_ptr, it's a unique_ptr. So the Buffer holds a unique_ptr to the only copy of the data. If you're not acquainted with unique_ptr, it's worth getting to know. Here's what CppReference has to say: https://en.cppreference.com/w/cpp/memory/unique_ptr
As CppReference notes, a unique_ptr is neither copy constructible or copy assignable. So we can't copy the Buffer. The way it holds its data does not allow us to do so. We have to get information about the contents of the Buffer some other way.
That way is called a Slice. If you look inside a Slice, you'll see that it only holds two pieces of data:
private:
std::uint8_t const* data_ = nullptr;
std::size_t size_ = 0;
We have a pointer to the const data (so we can't mess with the data inside the Buffer) and a length. If this code had been written with C++20, we'd probably be using the ranges library. A Slice looks very much like a ranges::contiguous_range. But this was done using C++11.
So Slice gives us a way to pass around information about the contents of the Buffer without actually copying Buffer or moving its contents around.
Why go to all this effort instead of putting the data inside of a shared_ptr and passing around shared_ptrs? We gain both efficiency and safety.
Efficiency
A shared_ptr requires atomic operations, which are compute expensive. If you copy a shared_ptr you have to increment its atomic count. And every time a shared_ptr is destroyed its atomic count is decremented. The unique_ptr has none of that overhead.
Safety
A shared_ptr is risky in two ways.
-
First is that if you have multiple
shared_ptrs to non-constdata, then anybody with one of thoseshared_ptrcan modify the contents of theBuffer. That leads to debugging mayhem if anyone doesn't follow rules closely. That's why aSliceholds astd::uint8_t const* data_. Thatconstkeeps anyone holding aSlicefrom messing with the contents of theBuffer. -
The other problem with
shared_ptris that the contents of theshared_ptrhang around until the last copy of thatshared_ptris destroyed. It is often difficult to know who has responsibility for destroying the contents of ashared_ptr. But aunique_ptrhas none of that ambiguity.
With unique_ptr the lifetime is obvious. Whoever holds the unique_ptr has responsibility for the lifetime. If the lifetime needs to exceed the lifetime of the holding object, then the unique_ptr must be moved, so the originally holding object no longer has the contents of the unique_ptr.
That's a very long winded answer. I hope it made some sense.
There was a problem hiding this comment.
Really appreciate the detailed response. Thanks Scott! ❤️
|
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... |
Hi @tequdev , |
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 URIlsfMutable: 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
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)