Amendment XLS-35: URIToken — Lightweight first-class NFTs #4456

dangell7 · 2023-03-09T11:07:49Z

High Level Overview of Change

The URIToken Amendment (XLS-35d) provides a lightweight alternative to XLS20 suitable for both main-net and side-chains.

The amendment adds:

A new type of ledger object: ltURI_TOKEN
A new serialized field: URITokenID
Five new transaction types:

URITokenMint
URITokenBurn
URITokenBuy
URITokenCreateSellOffer
URITokenCancelSellOffer

New feature (non-breaking change which adds functionality)

intelliot · 2023-04-24T18:47:45Z

@dangell7 is this ready for review?

intelliot · 2023-04-24T18:48:41Z

note: there are merge conflicts you'd need to resolve

dangell7 · 2023-04-24T22:39:48Z

@dangell7 is this ready for review?

Ready for review

RichardAH · 2023-05-23T17:58:42Z

I am requesting a code review on this. Anyone feel like reviewing it please?

kennyzlei · 2023-05-24T22:50:44Z

I am requesting a code review on this. Anyone feel like reviewing it please?

From the ripple side, @ledhed2222 will be able to help provide code review on this. In terms of timing, I'm predicting he will be able to get to this in two weeks or so

It would be great if we can seek additional help from non-ripple in terms for code review so that we can get more community representation/participation in this process. Would anyone else be able to help review? 🙏

ledhed2222 · 2023-06-13T17:52:33Z

just started taking a look at this :)

shawnxie999 · 2023-06-17T02:24:31Z

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

+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2014 Ripple Labs Inc.


year is 2023

ledhed2222 · 2023-06-21T20:10:21Z

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

+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2017 Ripple Labs Inc.


please use 2023 :)

ledhed2222 · 2023-06-21T20:11:47Z

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

+
+            auto const uri = ctx.tx.getFieldVL(sfURI);
+
+            if (uri.size() < 1 || uri.size() > 256)


what do you think about making 256 a constant in the header file?

ledhed2222 · 2023-06-21T20:12:11Z

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

+    switch (tt)
+    {
+        case ttURITOKEN_MINT: {
+            if (flags & tfURITokenMintMask)


what do you think about splitting out the bodies of each switch statement into their own functions, for readability?

answered below in followup question.

ledhed2222 · 2023-06-21T20:14:31Z

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

+            if (flags & tfURITokenNonMintMask)
+                return temINVALID_FLAG;
+
+            auto amt = ctx.tx.getFieldAmount(sfAmount);


i believe this could be const

ledhed2222 · 2023-06-21T20:17:02Z

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

+
+    switch (tt)
+    {
+        case ttURITOKEN_MINT: {


what's the rationale behind using a switch here (and above)? avoiding a new call stack frame?

@RichardAH thoughts?

a switch is the correct structure to use here:

preflight checks are identical between two pairs of the five txn types, the switch allows fall through logic.

the code is very clear, using another structure would make it less clear.

ledhed2222 · 2023-06-21T20:26:56Z

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

+
+        case ttURITOKEN_CREATE_SELL_OFFER: {
+            if (account_ != *owner)
+                return tecNO_PERMISSION;


nit - i think this is already checked in preclaim, right?

ledhed2222 · 2023-06-21T20:30:05Z

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

+            return tesSUCCESS;
+        }
+
+        case ttURITOKEN_CREATE_SELL_OFFER: {


what do you think about the idea of adding these fields to the mint transaction, so that a user can mint and set a sale price (poss with destination) in one tx? since this is the same object seems like that could be useful and easy

@RichardAH thoughts?

this a pretty good idea imho

ledhed2222 · 2023-06-21T20:32:54Z

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

+    if (!sle)
+        return tefINTERNAL;
+
+    uint16_t tt = ctx_.tx.getFieldU16(sfTransactionType);


could be const i believe

ledhed2222 · 2023-06-21T20:33:03Z

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

+            return tecINSUFFICIENT_RESERVE;
+    }
+
+    uint32_t flags = ctx_.tx.getFlags();


could be const i believe

ledhed2222 · 2023-06-21T20:34:49Z

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

+
+    switch (tt)
+    {
+        case ttURITOKEN_MINT: {


what do you think about making each of these switch bodies their own private methods, for readability?

Do you mean creating a preflight/preclaim/doApply for each or just moving them into a different method?

ledhed2222 · 2023-06-21T20:37:27Z

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

+                    sleU->makeFieldAbsent(sfDestination);
+                view().update(sleU);
+                return tesSUCCESS;
+            }


it's a bit surprising to me that buying my own token has the same behavior as canceling my sell offer. what's the rationale?

What behavior would you expect or anything specific that made you question it?

From an accounting/reconciliation POV I can see your point as this might need to be recorded for those purposes.

It's a valid purchase it just results in 0 movement of objects or funds.

i think a comment to clarify would be helpful :)

this also allows me to buy it from myself even though i haven't listed it for sale. it's really confusing to me and i'm not sure that that means it is a valid purchase. i could see it in the case where i have listed it for sale and then i attempt to buy it from myself

ledhed2222 · 2023-06-21T22:39:57Z

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

+
+                dstAmt = purchaseAmount;
+                static Rate const parityRate(QUALITY_ONE);
+                auto xferRate = transferRate(view(), saleAmount->getIssuer());


can be const

dangell7 · 2023-06-22T13:34:16Z

@ledhed2222 Thank you very much for your time and review of the code. I have marked everything that I have updated with a thumbs up.

Richard and I are going to address some of the other comments and then I will push the changes for a second review.

ledhed2222 · 2023-06-29T17:21:21Z

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

+                        0,                              // quality in
+                        0,                              // quality out
+                        j);                             // journal
+                    !isTesSuccess(ter))


i like that you commented all these arguments 👍 :)

ledhed2222 · 2023-06-29T17:25:31Z

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

+            // if a trustline was created then the ownercount stays the same on
+            // the seller +1 TL -1 URIToken
+            if (!lineCreated && !isXRP(purchaseAmount))
+                adjustOwnerCount(view(), sleOwner, -1, j);


shouldn't we also decrement the owner count if the purchase isXRP too?

ledhed2222 · 2023-06-29T17:33:13Z

src/ripple/protocol/TxFormats.h

@@ -139,6 +139,13 @@ enum TxType : std::uint16_t
    /** This transaction accepts an existing offer to buy or sell an existing  NFT. */
    ttNFTOKEN_ACCEPT_OFFER = 29,

+    /** This transaction mints/burns/buys/sells a URI TOKEN */


nit, These transactions mint/burn/buy/sell a URI TOKEN

ledhed2222

see comments :)

intelliot · 2023-09-08T05:52:13Z

@dangell7 - at your convenience, please respond to the comments above

intelliot · 2023-10-03T17:16:45Z

lacks reviews - community is welcome to review.

dangell7 force-pushed the uritoken-squash branch from fdf58df to 321d08c Compare April 24, 2023 21:08

amendment xls35

dbb3b51

dangell7 force-pushed the uritoken-squash branch from 321d08c to dbb3b51 Compare April 24, 2023 21:09

dangell7 added 4 commits April 24, 2023 18:30

Merge branch 'develop' into uritoken-squash

26dd698

fixup ledger entry count

7fd6430

clang-format

6a2e66b

clang-format

da5aaac

intelliot requested review from kennyzlei, nbougalis, ledhed2222 and shawnxie999 April 24, 2023 23:21

intelliot changed the title ~~Amendment XLS-35~~ Amendment XLS-35: URIToken — Lightweight first-class NFTs May 1, 2023

kennyzlei removed request for kennyzlei and shawnxie999 May 24, 2023 22:51

intelliot mentioned this pull request Jun 1, 2023

XLS-35d: URITokens — Lightweight first-class NFTs XRPLF/XRPL-Standards#110

Open

shawnxie999 reviewed Jun 17, 2023

View reviewed changes

ledhed2222 reviewed Jun 21, 2023

View reviewed changes

dangell7 added 2 commits June 23, 2023 15:30

update copyright date

f8baf09

fix review issues

3337eba

intelliot added API Change Feature Request Used to indicate requests to add new features Amendment Will Need Documentation labels Jun 23, 2023

ledhed2222 reviewed Jun 29, 2023

View reviewed changes

ledhed2222 suggested changes Jun 29, 2023

View reviewed changes

intelliot assigned dangell7 Jun 30, 2023

intelliot added this to the 2024 release milestone Oct 3, 2023

intelliot removed this from the 2.1.0 (Mar 2024) milestone Feb 2, 2024


		auto const uri = ctx.tx.getFieldVL(sfURI);

		if (uri.size() < 1 \|\| uri.size() > 256)

Amendment XLS-35: URIToken — Lightweight first-class NFTs #4456

Are you sure you want to change the base?

Amendment XLS-35: URIToken — Lightweight first-class NFTs #4456

Conversation

dangell7 commented Mar 9, 2023 • edited by intelliot Loading

High Level Overview of Change

intelliot commented Apr 24, 2023

intelliot commented Apr 24, 2023

dangell7 commented Apr 24, 2023

RichardAH commented May 23, 2023

kennyzlei commented May 24, 2023

ledhed2222 commented Jun 13, 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

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

Choose a reason for hiding this comment

dangell7 commented Jun 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledhed2222 Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

ledhed2222 left a comment

Choose a reason for hiding this comment

intelliot commented Sep 8, 2023

intelliot commented Oct 3, 2023

dangell7 commented Mar 9, 2023 •

edited by intelliot

Loading

ledhed2222 Jun 29, 2023 •

edited

Loading