Add `nftoken_id`, `nftoken_ids` and `offer_id` meta fields into NFT `Tx` responses #4447

shawnxie999 · 2023-03-03T14:14:34Z

High Level Overview of Change

Added new metadata fields into tx and account_tx responses

`nftoken_id` field

Added a new field nftoken_id into the responses of NFTokenMint andNFTokenAcceptOffer . This new field shows the NFTokenID for the NFToken that changed on the ledger as a result of the transaction.

`nftoken_ids` field

Added a new array field nftoken_ids into the response ofNFTokenCancelOffer . This new field shows all the NFTokenIDs for the NFTokens that changed on the ledger as a result of the transaction.

`offer_id` field

Added a new offer_id field into the response of a NFTokenCreateOffer to show the OfferID of the new NFTokenOffer.

(Credit to @ledhed2222 for providing code from clio to extract NFTokenIDs from mint transactions.)

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 Updates
Release

shawnxie999 · 2023-03-04T17:25:24Z

Made some changes for improvements. This is ready for review again.

dangell7 · 2023-03-21T15:48:54Z

I'm curious why the goal for this was to iterate through the pages instead of using a bitwise operation to find the nftokenID and offerID?

If a user has 500 pages you're going to iterate though all 500 when you could do this without iterating through a single page.

Just curious

shawnxie999 · 2023-03-21T16:07:08Z

I'm curious why the goal for this was to iterate through the pages instead of using a bitwise operation to find the nftokenID and offerID?

@dangell7 could you elaborate how NFT ID could be fetched by bitwise for NFT transaction? AFAIK, NFT id isn't stored anywhere in the transaction data. Feel free to correct me if I misunderstood anything.

If a user has 500 pages you're going to iterate though all 500 when you could do this without iterating through a single page.

The change simply iterates the AffectedNodes of the metadata, which is bounded(I believe) and is way less than 500. Then, NFTokenPage(which has less or equal to 32 entries) is iterated to find the NFT id. So it would be bounded and not iterate through infinite pages as you mentioned

dangell7 · 2023-03-21T16:28:27Z

@shawnxie999 Here is the operation to create an NFTokenID. I'm pretty sure you have all these variables available where you are attempting to iterate through the pages.

The offer can be done the same way.

shawnxie999 · 2023-03-21T16:38:06Z

@shawnxie999 Here is the operation to create an NFTokenID. I'm pretty sure you have all these variables available where you are attempting to iterate through the pages.

The offer can be done the same way.

I have looked at it before. I believe we don’t have the NFT sequence param in the transaction data. I will double check tho

dangell7 · 2023-03-21T16:39:06Z

@shawnxie999 I thought so too for the longest time. Its actually on the AccountRoot ledger entry. :)

shawnxie999 · 2023-03-21T16:46:06Z

@shawnxie999 I thought so too for the longest time. Its actually on the AccountRoot ledger entry. :)

The MintedNFTokens only stores the latest nft sequence. What the PR does is it returns the NFT id for any Tx responses of type NFTokenMint, so we still wouldn’t know the NFT sequence for the NFT requested

dangell7 · 2023-03-21T16:54:44Z

@shawnxie999 I thought so too for the longest time. Its actually on the AccountRoot ledger entry. :)

The MintedNFTokens only stores the latest nft sequence. What the PR does is it returns the NFT id for any Tx responses of type NFTokenMint, so we still wouldn’t know the NFT sequence for the NFT requested

@shawnxie999 The AccountRoot is returned on the tx meta and it does include both the current MintedNFTokens and the previous MintedNFTokens.

shawnxie999 · 2023-03-21T18:02:02Z

@shawnxie999 The AccountRoot is returned on the tx meta and it does include both the current MintedNFTokens and the previous MintedNFTokens.

@dangell7 But then that would be similar to what this PR doing currently. Instead of iterating AffectedNodes to find changed NFTokenPage, we would be finding the the AccountRoot. And It would probably be more robust to search for the NFTokenPage to get the NFTokenID right away.

scottschurr

In general a very nice job. I left a number of comments for your consideration. Please feel free to ask questions, or disagree, on any of the points I left.

src/ripple/rpc/NFTokenID.h

src/ripple/rpc/NFTokenOfferID.h

src/ripple/rpc/TxMetaSerializer.h

src/ripple/rpc/impl/NFTokenID.cpp

src/ripple/rpc/impl/NFTokenOfferID.cpp

src/ripple/rpc/impl/TxMetaSerializer.cpp

src/test/app/NFToken_test.cpp

shawnxie999 · 2023-03-24T20:27:48Z

This is ready for re-review. @scottschurr I appreciate the detailed feedback and explanations, thanks!

scottschurr

👍 LGTM. Thanks for putting up with my picky review.

ledhed2222 · 2023-03-28T00:11:15Z

src/ripple/rpc/NFTokenOfferID.h

+//==============================================================================
+
+#ifndef RIPPLE_RPC_NFTOKENOFFERID_H_INCLUDED
+#define RIPPLE_RPC_NFTOKENOFFERID_H_INCLUDED


cc @scottschurr - is this the right directory for these files? it seems like this directory should contain implementations of RPC calls, not implementations of helpers, which is what these are to me.

Regarding

is this the right directory for these files?

Good question, and something I was not thinking about during my review.

The best I can suggest is that we look at the placement of similar functionality. If we use the insertDeliveredAmount() function as an example, then we see that the header file is in src/ripple/rpc and the implementation is in src/ripple/rpc/impl. So, if we follow that example, then these files are in the right places.

On the other hand, if we use the RPCHelpers collection of utilities as our example, then we see that both the header and the implementation are in src/ripple/rpc/impl. That would suggest the new header files should be moved down one level to src/ripple/rpc/impl .

I don't personally have a strong opinion, since I don't play in this section of the code very often. I think it would be fine to leave these files where they are. But I'd also be fine if both the headers and implementation files were in impl. There may also be other viable ways of organizing the files.

ledhed2222 · 2023-03-28T00:15:17Z

src/ripple/rpc/impl/NFTokenID.cpp

+ // We expect NFTs to be added one at a time. So finalIDs should be one
+ // longer than prevIDs. If that's not the case something is messed up.
+ if (finalIDs.size() != prevIDs.size() + 1)
+ return std::nullopt;


cc @scottschurr - i haven't seen the use of std::nullopt in our codebases, instead seeing just {}. IMO we should stick with what is used elsewhere.

@ledhed2222, that's a fair question. As a sanity check I just tried

grep -r -o std\:\:nullopt src/ripple | wc -l

That gave me over 250 hits of uses of std::nullopt in the code base.

Personally, I usually prefer things like std::nullopt over {} because when I read the source code (as text) I know what they are. {} can be anything with a default constructor. But I also know not everyone agrees with me.

i think ill go with nullopt. rippled does use it fairly common

ledhed2222 · 2023-03-28T00:15:45Z

src/ripple/rpc/impl/NFTokenID.cpp

+ // Find the first NFT ID that doesn't match. We're looking for an
+ // added NFT, so the one we want will be the mismatch in finalIDs.
+ auto const diff = std::mismatch(
+ finalIDs.begin(), finalIDs.end(), prevIDs.begin(), prevIDs.end());


why the change from std::set_difference to std::mismatch?

@ledhed2222, here's a link to the motivation: #4447 (comment)

I get stuff wrong, so the choice is open for discussion.

I think mismatch would work since there is at most one NFT changed

shawnxie999 · 2023-05-09T18:29:38Z

resolved conflicts

src/ripple/rpc/NFTSyntheticSerializer.h

src/ripple/rpc/NFTokenID.h

ledhed2222 · 2023-05-17T14:08:46Z

src/ripple/rpc/NFTSyntheticSerializer.h

+/**
+ Adds common synthetic fields to transaction-related JSON responses
+
+ @{


is this @{ here on purpose?

It looks like the intention is to provide Doxygen markdown. Perhaps this documents the intent: https://www.doxygen.nl/manual/commands.html#cmdaddtogroup

this format is from the deliveredAmount file, so i just followed the same format

are we using doxygen markdown in this repo? if so are the generated docs hosted somewhere?

There have been people who were excited about Doxygen years ago. Certainly any Doxygen generated from the current source code is going to be very spotty. My past experience with Doxygen is it takes a lot of dedication to make truly useful Doxygen documentation. I don't think that level of effort has ever gone in. It's certainly not happening right now.

Speaking only for myself I'd be fine if all of the Doxygen markdown were removed. But I don't object to it being present either.

I think leaving it wouldn't really matter.

ledhed2222 · 2023-05-17T14:08:50Z

src/ripple/rpc/NFTokenID.h

+ and NFTokenCancelOffer transactions.
+
+ Helper functions are not static because they can be used by Clio.
+ @{


is this @{ here on purpose?

ledhed2222

few comments

shawnxie999 · 2023-05-18T16:26:59Z

ready to m erge

smitha-d · 2023-06-22T08:16:32Z

Awaiting this one. Any updates on tentatively when this can be merged to master?

shawnxie999 · 2023-06-22T18:06:45Z

@smitha-d this change has already been released in v1.11. i think @intelliot would have a better idea on this.

intelliot · 2023-06-22T18:15:52Z

Correct - this was merged to master earlier this week, and was released in version 1.11.0. See the docs for how to install or update.

smitha-d · 2023-06-22T19:00:26Z

When will version 1.11.0 start reflecting in mainnet (https://livenet.xrpl.org/)

intelliot · 2023-06-22T22:05:50Z

1.11.0 is a client version, not a network version (there are no network versions, but the protocol is defined by the amendments that are active).

If you run your own node, then the updated API is available to you as soon as you update to 1.11.0.

If you use public servers, it depends on when the operator updates their nodes. You can call server_info to learn more about the rippled server you're connecting to.

It is worth noting that not all operators expose the rippled API, as many are beginning to deploy Clio API servers to handle client requests. Furthermore, some operators (like xrplcluster.com) use reverse proxies which can block certain requests, or modify certain responses - so the public API may differ from what is implemented in rippled itself.

…ion parsers (#1018) ## High Level Overview of Change utilize added ID fields based on [this rippled PR](XRPLF/rippled#4447), rather than manually parsing for NFToken transactions. ### Context of Change #708

shawnxie999 marked this pull request as ready for review March 3, 2023 14:23

shawnxie999 changed the title ~~Add nftokens_id and offer_id meta fields into NFT Tx responses~~ Add nftokens_id and offer_id meta fields into NFT Tx responses Mar 3, 2023

intelliot requested review from scottschurr and ledhed2222 March 3, 2023 16:36

intelliot added API Change Feature Request Used to indicate requests to add new features Testable Will Need Documentation labels Mar 4, 2023

shawnxie999 marked this pull request as draft March 4, 2023 17:07

shawnxie999 changed the title ~~Add nftokens_id and offer_id meta fields into NFT Tx responses~~ Add nftoken_id, nftoken_ids and offer_id meta fields into NFT Tx responses Mar 4, 2023

shawnxie999 marked this pull request as ready for review March 4, 2023 17:25

shawnxie999 force-pushed the add-nft-id-meta branch from e8e0d8b to 74d149f Compare March 6, 2023 14:08

shawnxie999 force-pushed the add-nft-id-meta branch from 74d149f to ae58582 Compare March 21, 2023 15:23

scottschurr reviewed Mar 22, 2023

View reviewed changes

shawnxie999 requested review from scottschurr and removed request for ledhed2222 March 24, 2023 20:27

scottschurr approved these changes Mar 24, 2023

View reviewed changes

ledhed2222 reviewed Mar 28, 2023

View reviewed changes

shawnxie999 added 7 commits May 9, 2023 14:07

nft_id and serilization module

b270a53

[fold] addressed some syntax changes

c04cfab

[fold]renaming and remove general serializer

6f50003

[fold]clang

1cfe23b

[fold] address greg comments

839b129

[fold] clang

9b9e203

[fold] add comments for nonstatic helper func

ce66dfd

shawnxie999 force-pushed the add-nft-id-meta branch from 7c60913 to ce66dfd Compare May 9, 2023 18:08

ledhed2222 reviewed May 17, 2023

View reviewed changes

src/ripple/rpc/NFTSyntheticSerializer.h Show resolved Hide resolved

ledhed2222 reviewed May 17, 2023

View reviewed changes

src/ripple/rpc/NFTokenID.h Show resolved Hide resolved

ledhed2222 reviewed May 17, 2023

View reviewed changes

ledhed2222 suggested changes May 17, 2023

View reviewed changes

ledhed2222 approved these changes May 18, 2023

View reviewed changes

intelliot merged commit 3620ac2 into XRPLF:develop May 18, 2023

intelliot added this to the 1.11.0 milestone Jun 5, 2023

nkramer44 mentioned this pull request Jun 29, 2023

Add nftoken_id, nftoken_ids, and offer_id fields to TransactionMetadata XRPLF/xrpl4j#435

Closed

intelliot added the Added to API Changelog label Jul 25, 2023

achowdhry-ripple mentioned this pull request Jul 10, 2024

Leverage nftoken_id, nftoken_ids and offer_id meta fields in transaction parsers ripple/explorer#1018

Merged

10 tasks

Add nftoken_id, nftoken_ids and offer_id meta fields into NFT Tx responses #4447

Add nftoken_id, nftoken_ids and offer_id meta fields into NFT Tx responses #4447

Conversation

shawnxie999 commented Mar 3, 2023 • edited Loading

High Level Overview of Change

nftoken_id field

nftoken_ids field

offer_id field

shawnxie999 commented Mar 4, 2023

dangell7 commented Mar 21, 2023

shawnxie999 commented Mar 21, 2023 • edited Loading

dangell7 commented Mar 21, 2023

shawnxie999 commented Mar 21, 2023

dangell7 commented Mar 21, 2023

shawnxie999 commented Mar 21, 2023 • edited Loading

dangell7 commented Mar 21, 2023 • edited Loading

shawnxie999 commented Mar 21, 2023 • edited Loading

scottschurr left a comment

Choose a reason for hiding this comment

shawnxie999 commented Mar 24, 2023

scottschurr left a 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

shawnxie999 commented May 9, 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

ledhed2222 left a comment

Choose a reason for hiding this comment

shawnxie999 commented May 18, 2023

smitha-d commented Jun 22, 2023

shawnxie999 commented Jun 22, 2023 • edited Loading

intelliot commented Jun 22, 2023

smitha-d commented Jun 22, 2023

intelliot commented Jun 22, 2023 • edited Loading

Add `nftoken_id`, `nftoken_ids` and `offer_id` meta fields into NFT `Tx` responses #4447

Add `nftoken_id`, `nftoken_ids` and `offer_id` meta fields into NFT `Tx` responses #4447

shawnxie999 commented Mar 3, 2023 •

edited

Loading

`nftoken_id` field

`nftoken_ids` field

`offer_id` field

shawnxie999 commented Mar 21, 2023 •

edited

Loading

shawnxie999 commented Mar 21, 2023 •

edited

Loading

dangell7 commented Mar 21, 2023 •

edited

Loading

shawnxie999 commented Mar 21, 2023 •

edited

Loading

shawnxie999 commented Jun 22, 2023 •

edited

Loading

intelliot commented Jun 22, 2023 •

edited

Loading