Introduce NFT support (XLS-20) #4101

scottschurr · 2022-02-15T03:14:52Z

High Level Overview of Change

Support for XLS-20 NFTs on the XRP Ledger.

Context of Change

If integrated, this branch provides native NFT support on the XRP Ledger.

Please see XRPLF/XRPL-Standards#46 for the spec and further details.

Type of Change

New feature on amendment. So enabling the amendment will amendment block older versions of the software.
Tests for NFTs are added.

RichardAH · 2022-02-16T17:15:27Z

Renaming global types (in this case the serialized field types) inside an amendment commit is inadvisable. I realise this amendment introduces new types but renaming existing types inside it breaks any number of other backlogged PRs (including probably mine) which all need to be individually updated to be merge-compatible again.

Instead change the code to continue with the existing naming convention and do the rename in a separate non-amendment PR after backlog has been cleared. That way we can update type names once, "one and done" so to speak, without needlessly multiplying work.

I'm speaking about SF_HASH256-> SF_UINT256 and so on. I'm not saying this rename shouldn't be done, but I am saying it shouldn't be done here.

I think as a rule amendment PR's should not change anything that isn't explicitly and exactly needed for that amendment to function. There should be a clear delineation between writing new functionality into the codebase and refactoring existing functionality for the reasons above (multiplication of work)

scottschurr · 2022-02-25T21:07:42Z

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

+static bool
+compareTokens(uint256 const& a, uint256 const& b)
+{
+    return (a & nft::pageMask) < (b & nft::pageMask);


I think there's a latent bug here. We need every server to produce identically sorted NFTPages. But this comparison only sorts on the low 96-bits. If there are NFTs with identical low 96-bits then the sort order becomes nondeterministic.

So I think this function needs to look more like this:

// The sort of NFTokens needs to be fully deterministic, but the sort // is weird because we sort on the low 96-bits first. But if the low // 96-bits are identical we still need a fully deterministic sort. // So we sort on the low 96-bits first. If those are equal we sort on // the whole thing. int const lowBitsCmp = compare(a & nft::pageMask, b & nft::pageMask); if (lowBitsCmp != 0) return lowBitsCmp < 0; return a < b;

Nice catch. I also like the exposition in the comment which explains the need for this curious construct.

Minor nit, I'd prefer:

if (auto const lc = compare(a & nft::pageMask, b & nft::pageMask); lc != 0) return lc < 0; return a < b;

nbougalis · 2022-02-27T01:59:10Z

Renaming global types (in this case the serialized field types) inside an amendment commit is inadvisable. I realise this amendment introduces new types but renaming existing types inside it breaks any number of other backlogged PRs (including probably mine) which all need to be individually updated to be merge-compatible again.

I agree with you, and in a perfect world this wouldn't have happened. My original set of commits had a separate commit for this, but somewhere along the way I squashed it down into a single commit.

Instead change the code to continue with the existing naming convention and do the rename in a separate non-amendment PR after backlog has been cleared. That way we can update type names once, "one and done" so to speak, without needlessly multiplying work.

I'm speaking about SF_HASH256-> SF_UINT256 and so on. I'm not saying this rename shouldn't be done, but I am saying it shouldn't be done here.

I could unsquash it but I don't know if it's worth the trouble to do this. Rationale: the renaming here doesn't alter the binary protocol; it's a purely mechanical change and any conflicts with other PRs are likely to be very simple to resolve.

I think as a rule amendment PR's should not change anything that isn't explicitly and exactly needed for that amendment to function. There should be a clear delineation between writing new functionality into the codebase and refactoring existing functionality for the reasons above (multiplication of work)

I think that going forward, it's a good rule for contributors and maintainers to adopt: changes to global types should be a standalone commit. I also agree with your rationale about some mechanism to avoid conflict between different PRs that introduce new fields and/or types.

While I don't think we need to do this for this commit, if you feel strongly enough that this should be done here, I'll put in the work to do it. Let me know.

scottschurr · 2022-03-03T02:23:43Z

src/ripple/protocol/Protocol.h

+std::uint16_t constexpr maxTransferFee = 50000;
+
+/** The maximum length of a URI inside an NFT */
+std::size_t constexpr maxTokenURILength = 512;


This 512 limit does not match the spec or the documentation, which sets the limit at 256. This value should be changed to 256.

seelabs

Great job on this! Lots of tests and the code is really well done. I left some comments. The biggest things I'd like to tackle are:

Naming. I'd like to have consistent nomenclature for this new functionality. I prefer nft, not token or NFToken. I'd also prefer to rename the nft functionality that overlaps with existing functionality - like transfer fees and offers.
IOU transfer fees should probably be paid by the seller, not the buyer.
One "typo" bug in "invariants.
The rest is the usual collection of minor issues.

Edit: One more minor comment. I'd rather the code was attributed to whoever wrote it, rather than "XLS-20 developers". git-blame to a specific developer is useful because it lets us direct questions. git-blame to some group is less useful.

seelabs · 2022-03-05T19:24:10Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -1172,6 +1172,7 @@ NetworkOPsImp::processTransaction(
    if ((newFlags & SF_BAD) != 0)
    {
        // cached bad
+        std::cerr << transaction->getID() << ": cached bad!\n";


Use log instead of writting to `cerr

seelabs · 2022-03-05T19:25:40Z

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

+    ReadView const& view,
+    beast::Journal const& j)
+{
+    if (tx.getTxnType() == ttNFTOKEN_MINT && tx.getTxnType() == ttNFTOKEN_BURN)


This will never be true. Should be:

if (tx.getTxnType() != ttNFTOKEN_MINT && tx.getTxnType() != ttNFTOKEN_BURN)

seelabs · 2022-03-05T19:27:55Z

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

+NFTokenAcceptOffer::pay(
+    AccountID const& from,
+    AccountID const& to,
+    STAmount amount)


Pass by const&

seelabs · 2022-03-05T19:29:37Z

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

+    if (amount < beast::zero)
+        return tecINTERNAL;
+
+    return accountSend(view(), from, to, amount, j_);


As written, the destination pays the IOU transfer fee. I don't think this is what we want. In payments, the source account pays the fee. We should probably do the same thing here.

I think this is okay as written, but maybe you'll convince me otherwise. Let me walk through the steps.

The total paid for the NFT always equals or exceeds all fees associated with the transfer -- both the transfer fee and the broker fee.

If there is a broker, the broker is paid their fee first. That is subtracted from the total paid.

If the seller specified a selling price then the remainder, after the broker is paid, must equal or exceed the specified selling price.

The issuer's portion is determined as a percentage of this remainder. We now pay that portion to the issuer. The amount paid to the issuer is then subtracted from the total paid.

At this point, the last bit of the remainder may be below the amount requested by the seller. We consider that to be okay because the seller is paying the transfer fee to the issuer.

These last remaining funds are then transferred to the seller.

So the effect is that the seller is paying the transfer fee, even though the funds did not actually transfer from buyer to seller to issuer. The final effect is that the seller is paying the transfer fee to the issuer.

I'm open to further discussion if this doesn't seem right to you.

seelabs · 2022-03-05T19:33:04Z

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

+    if (!nft)
+        return tecINTERNAL;
+
+    if (auto const ret = nft::removeToken(view(), seller, tokenID);


We do the work of finding the token twice. Maybe findToken can return a hint with the page so removeToken doesn't need to repeat this work.

As it stands right now, nft::findToken() can't do this work, because it takes a ReadView parameter instead of an ApplyView. What I'll try is adding a new free function, nft::findTokenAndPage, that will take an ApplyView and return both the token object and a std::shared_ptr<SLE> to the page. That may help. And it may be useful elsewhere. We'll see.

seelabs · 2022-03-05T20:27:05Z

src/test/app/NFToken_test.cpp

+    }
+};
+
+BEAST_DEFINE_TESTSUITE_PRIO(NFToken, tx, ripple, 3);


A 5000 like file is pretty big. We might want to split this up.

seelabs · 2022-03-05T20:28:40Z

src/test/jtx/impl/Account.cpp

@@ -46,14 +46,25 @@ Account::Account(
 }

 Account
-Account::fromCache(std::string name, KeyType type)
+Account::fromCache(AcctStringType stringType, std::string name, KeyType type)
 {
    auto p = std::make_pair(name, type);  // non-const so it can be moved from


I'll note that this allocates, which isn't great for a function that's meant to be a cache. But not new code, so no change needed. But we may want to clean this up in another PR.

seelabs · 2022-03-05T20:29:42Z

src/test/jtx/token.h

+    std::uint16_t xferFee_;
+
+public:
+    xferFee(std::uint16_t fee) : xferFee_(fee)


explicit (the other constructors in this file should be explicit as well)

seelabs · 2022-03-05T20:30:18Z

src/test/jtx/token.h

+class issuer
+{
+private:
+    std::string issuer_;


I see we're storing string representation of the wrapped object rather then the object itself (here and in the other classes). I'd probably store the object - they are 20-byte object, there's not much cost to copying them and storing them. But it's also fine as-is if you prefer it.

Storing it as a string makes it a little easier to use, since we're just about to use it to build JSON. I'll leave as is. But thanks for the comment.

seelabs · 2022-03-05T20:30:36Z

src/test/jtx/token.h

+    std::string issuer_;
+
+public:
+    issuer(jtx::Account const& issue) : issuer_(issue.human())


scottschurr · 2022-03-07T21:28:55Z

src/ripple/protocol/impl/ErrorCodes.cpp

+    {rpcUNKNOWN_COMMAND,         "unknownCmd",             "Unknown method."},
+    {rpcSENDMAX_MALFORMED,       "sendMaxMalformed",       "SendMax amount malformed."},
+    {rpcOBJECT_NOT_FOUND,        "objectNotFound",         "The requested object was not found."}};
+// clang-format off


Should be clang-format on.

scottschurr · 2022-03-10T19:25:04Z

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

+    static uint256 constexpr accountBits(
+        "ffffffffffffffffffffffffffffffffffffffff000000000000000000000000"sv);


Use nft::pageMask instead of local definition.

scottschurr · 2022-03-14T20:16:33Z

src/ripple/protocol/SField.h

@@ -150,7 +153,8 @@ class SField
        const char* fn,
        int meta = sMD_Default,
        IsSigning signing = IsSigning::yes);
-    explicit SField(private_access_tag_t, int fc);
+    explicit SField(private_access_tag_t, int fc) = delete;
+    explicit SField(private_access_tag_t, int fc, bool);


The bool parameter in this signature is unused. Remove the bool, which takes this signature back to the original signature.

scottschurr · 2022-03-15T19:05:31Z

src/ripple/protocol/TER.h


    temUNCERTAIN,  // An internal intermediate result; should never be returned.
    temUNKNOWN,    // An internal intermediate result; should never be returned.

    temSEQ_AND_TICKET,
+    tefNOT_POSSIBLE,


tefNOT_POSSIBLE was added to the wrong enum!

scottschurr · 2022-03-16T17:23:16Z

@seelabs, thank you very much for the thorough review. I've just added a commit that I believe addresses all of your comments that did not require renames.

Today or tomorrow I'll submit a comment here that can be a starting place for the rename discussion. Renames will be complicated by the fact that we should probably not change API-facing names that are in the spec. That means a number of the names are cast in stone. Non-API facing names should harmonize with the names we can't change. More thoughts to follow.

I'm assuming I should hold off on rebasing until the review is complete.

scottschurr · 2022-03-30T00:38:40Z

I have been looking for a way to address the naming concerns raised by @seelabs. I agree with @seelabs that we have some serious naming inconsistencies and opportunities for developer confusion. I've pushed a couple of commits that address non-SFields, but still have some minor impact on the API. Those commits are:

043afbd ([FOLD] Improve names of and remove unused TER codes) and
1a932bf ([FOLD] Rename jss::tokenid to jss::nft_id for RPC field consistency)

If someone has concerns with those two commits they should be easy to remove.

That leaves changes to SFields to consider. This is the last chance we have to change these names. I think changing these names is the right call. But changing the names now will require editing the spec. It will also cause some heartache for those diligent developers who have been coding against the NFT testnet. We have to weight future self-consistency against current-day pain.

Since I'm less confident that there is appetite for changing the SField names I've pushed those commits in a different branch. If we choose to pick up the changes they will be very easy to cherry-pick. The commits that change SField names are these:

scottschurr@5c6e8a9 ([FOLD] Rename SFields to NFTokenTaxon, MintedNFTokens, and BurnedNFTokens)
scottschurr@8f9167f ([FOLD] Rename SFields to NFTokenOffer and NFTokenOffers)
scottschurr@7b5ecf3 ([FOLD] Rename SFields to NFTokenMinter and NFTokenBrokerFee)
scottschurr@af7c54e ([FOLD] Rename SFields to NFToken and NFTokens) and
scottschurr@8a0fce1 ([FOLD] Rename SFields to NFTokenOfferNode, NFTokenBuyOffer, and NFTokenSellOffer)

I'd appreciate reviewers weighing in on whether or not we should change the SField names now. If not now, then we should assume it will be never.

Thanks.

scottschurr · 2022-04-06T05:13:48Z

All name changes are incorporated. Rebased to 1.9.0-b2 and squashed.

scottschurr requested review from thejohnfreeman, seelabs and ximinez February 15, 2022 03:14

scottschurr assigned thejohnfreeman, seelabs and ximinez Feb 15, 2022

nbougalis mentioned this pull request Feb 17, 2022

sfcodes harmonization for hooks #4089

Closed

7 tasks

scottschurr commented Feb 25, 2022

View reviewed changes

scottschurr commented Mar 3, 2022

View reviewed changes

seelabs reviewed Mar 5, 2022

View reviewed changes

scottschurr commented Mar 7, 2022

View reviewed changes

scottschurr commented Mar 10, 2022

View reviewed changes

scottschurr commented Mar 14, 2022

View reviewed changes

scottschurr commented Mar 15, 2022

View reviewed changes

scottschurr force-pushed the xls20-1.8.5 branch from 702526e to e873088 Compare April 6, 2022 03:12

Introduce NFT support (XLS020)

9b8ebcb

scottschurr force-pushed the xls20-1.8.5 branch from e873088 to 9b8ebcb Compare April 6, 2022 04:24

nbougalis approved these changes Apr 6, 2022

View reviewed changes

manojsdoshi mentioned this pull request Apr 6, 2022

Proposed 1.9.0-b3 #4136

Merged

manojsdoshi closed this in #4136 Apr 6, 2022

intelliot mentioned this pull request May 11, 2022

Introduce fixNFTokenDirV1 amendment: #4155

Closed

3 tasks

scottschurr deleted the xls20-1.8.5 branch May 12, 2022 00:46

intelliot referenced this pull request Jun 8, 2023

Introduce NFT support (XLS020)

70779f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce NFT support (XLS-20) #4101

Introduce NFT support (XLS-20) #4101

scottschurr commented Feb 15, 2022

RichardAH commented Feb 16, 2022 •

edited

Loading

scottschurr Feb 25, 2022

nbougalis Feb 27, 2022

nbougalis commented Feb 27, 2022

scottschurr Mar 3, 2022

seelabs left a comment •

edited

Loading

seelabs Mar 5, 2022

seelabs Mar 5, 2022

seelabs Mar 5, 2022

seelabs Mar 5, 2022

scottschurr Mar 10, 2022

seelabs Mar 5, 2022

scottschurr Mar 10, 2022

seelabs Mar 5, 2022

seelabs Mar 5, 2022

seelabs Mar 5, 2022

seelabs Mar 5, 2022

scottschurr Mar 15, 2022

seelabs Mar 5, 2022

scottschurr Mar 7, 2022

scottschurr Mar 10, 2022

scottschurr Mar 14, 2022

scottschurr Mar 15, 2022

scottschurr commented Mar 16, 2022

scottschurr commented Mar 30, 2022

scottschurr commented Apr 6, 2022

		static uint256 constexpr accountBits(
		"ffffffffffffffffffffffffffffffffffffffff000000000000000000000000"sv);

Introduce NFT support (XLS-20) #4101

Introduce NFT support (XLS-20) #4101

Conversation

scottschurr commented Feb 15, 2022

High Level Overview of Change

Context of Change

Type of Change

RichardAH commented Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis commented Feb 27, 2022

Choose a reason for hiding this comment

seelabs left a comment • 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

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

scottschurr commented Mar 16, 2022

scottschurr commented Mar 30, 2022

scottschurr commented Apr 6, 2022

RichardAH commented Feb 16, 2022 •

edited

Loading

seelabs left a comment •

edited

Loading