Add NFTokenPages to account_objects RPC #4352

RichardAH · 2022-11-25T10:02:06Z

High Level Overview of Change

NFTokenPages consume owner count and therefore should be returned by the account_objects RPC, lest users be unaware where their owner count has been allocated.

Credit to @scottschurr for unit test / bug fixes and polishing.

It is technically possible to iterate NFTokenPages using existing RPC calls, but it is hacky to do this, requires additional calls and increases complexity (without gain) for wallet / app developers. Therefore I strongly advise this PR is merged.

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

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1203482814597350

RichardAH · 2022-11-25T10:03:24Z

Longest suite times:
   43.0s ripple.tx.Offer
   35.1s ripple.app.ValidatorSite
   21.7s ripple.rpc.NodeToShardRPC
   21.2s ripple.tx.NFToken
   18.4s ripple.tx.NFTokenBurn
   14.0s ripple.app.Flow
   14.0s ripple.app.ShardArchiveHandler
   13.6s ripple.app.TheoreticalQuality
    8.8s ripple.tx.NFTokenDir
    8.2s ripple.app.LedgerReplayer
341.8s, 204 suites, 1528 cases, 532270 tests total, 0 failures

cjcobb23 · 2022-11-29T16:55:04Z

Why exactly is it hacky to do this client side? We have the account_nfts RPC for this: https://xrpl.org/account_nfts.html. Maybe I am misunderstanding something though.

RichardAH · 2022-11-30T13:24:50Z

Why exactly is it hacky to do this client side? We have the account_nfts RPC for this: https://xrpl.org/account_nfts.html. Maybe I am misunderstanding something though.

account_nfts does not tell you anything about how many NFT pages you have and thus nothing about how much ownercount has been reserved by those pages. Everything that contributes to your ownercount should be displayed in account_objects.

cjcobb23 · 2022-11-30T17:37:07Z

Why exactly is it hacky to do this client side? We have the account_nfts RPC for this: https://xrpl.org/account_nfts.html. Maybe I am misunderstanding something though.

account_nfts does not tell you anything about how many NFT pages you have and thus nothing about how much ownercount has been reserved by those pages. Everything that contributes to your ownercount should be displayed in account_objects.

Does account_objects tell you about pages? It looks like account_objects just shows the raw ledger objects themselves, and doesn't say anything about the pages those objects are in.

Edit: Nevermind, I think I get it. We charge users per owned object, as well as per owned NFTokenPage. NFTs don't exist outside of pages, whereas account objects are stored by themselves, and the directory nodes just link to them.

RichardAH · 2022-12-02T12:55:16Z

Edit: Nevermind, I think I get it. We charge users per owned object, as well as per owned NFTokenPage. NFTs don't exist outside of pages, whereas account objects are stored by themselves, and the directory nodes just link to them.

Correct. Normally users are not charged for directory pages, but in the NFT amendment, they are. So, although they wouldn't normally be displayed, in this case these pages need to be displayed in account_objects.

ximinez

The only real dealbreaker here is dereferencing the unseated optional.

src/ripple/rpc/impl/RPCHelpers.cpp

src/test/rpc/AccountObjects_test.cpp

src/ripple/rpc/impl/RPCHelpers.cpp

RichardAH · 2022-12-19T17:24:20Z

@ximinez I added a fix for the unguarded std::optional. As to the unit tests, those were written by @scottschurr so I will let him discuss. Unfortunately I don't have time at the moment.

scottschurr

Sorry to be slow responding to the earlier comments.

I've incorporated the changes suggested by @ximinez in the top commit of this branch: https://github.com/scottschurr/rippled/commits/RichardAH-nftpage_accobjs. @RichardAH, if you could cherry pick that commit onto this branch then I believe all of @ximinez's comments will be addressed. Thanks.

intelliot · 2023-02-08T20:03:25Z

@RichardAH this is ready for your perusal, at your convenience

RichardAH · 2023-03-22T18:46:16Z

Sorry for the late reply. Cherry-picked @scottschurr 's commit (thanks!)
Looks like this still merge-conflict-free so whenever you want to merge it is fine by me

intelliot · 2023-03-22T19:13:14Z

This requires at least 2 code reviews. @HowardHinnant @scottschurr could you review, or suggest other reviewer(s)?

scottschurr

👍 LGTM. It might be good to have @ximinez give it a quick looking over so he can make sure all the comments he made have been addressed. But as far as I'm concerned it's good to go.

shawnxie999 · 2023-03-24T21:32:17Z

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -171,25 +252,27 @@ getAccountObjects(
            found = true;
        }

+        // it's possible that the returned NFTPages exactly filled the
+        // response.  Check for that condition.
+        if (i == mlimit && mlimit < limit)


curious why i == mlimit was preferred here over than writing 0 == mlimit

It's been a while. As I recall this change was motivated by one of the new unit tests (that failed before this change). FWIW, i is incrementing inside the outer for loop, so I suspect that a comparison to 0 would not give the correct results.

ah sorry i misread the code.

shawnxie999

looks good

shawnxie999 · 2023-03-27T14:48:51Z

src/test/rpc/AccountObjects_test.cpp

@@ -510,7 +730,7 @@ class AccountObjects_test : public beast::unit_test::suite
            auto const& ticket = resp[jss::result][jss::account_objects][0u];


test failing on this line

@shawnxie999, sorry, more details please? I'm not seeing any unit test failures on my local Mac build. Are you seeing a unit test failure on your local machine? Are you reporting a CI failure (which is known to be flaky)?

oh sorry, i was just pointing out the CI failure, which seemed to have failed on both ubuntu and mac

Thanks for the clarification @shawnxie999. I am seeing two CI unit tests failures currently:

Ubuntu: #65 failed: AccountLinesRPC_test.cpp(730)

Mac: #65 failed: AccountLinesRPC_test.cpp(732)

AccountLinesRPC_test.cpp has a file name that looks a lot like this file, but it's not the same file. When I look in the source for that file the reported failing lines don't align with a BEAST_EXPECT. So I'm inclined to suspect those are just two more flaky failures. I wish that were not the case...

Thanks for the clarification. It's my first time seeing a flaky ubuntu test, if it is the case. All good.

shawnxie999 · 2023-03-27T14:58:28Z

src/test/rpc/AccountObjects_test.cpp

+            BEAST_EXPECT(nftPage[sfNFTokens.jsonName].size() == 1);
+            BEAST_EXPECT(
+                nftPage[sfNFTokens.jsonName][0u][sfNFToken.jsonName]
+                       [sfNFTokenID.jsonName] == to_string(nftID));


@scottschurr just curious isnt string comparison more expensive than comparing in uint256?

@shawnxie999, yes. string comparison is more expensive than numeric comparisons. However nftPage is a JSON object. All accounts are represented in a JSON object as std::strings. So something had to be converted, either a uint256 to a std::string or a std::string to a uint256, before the comparison could take place.

I probably chose the conversion to string so it would make debugging easier if something went wrong. But I wasn't thinking very hard about efficiency in the unit test.

intelliot · 2023-03-29T05:48:08Z

It might be good to have @ximinez give it a quick looking over so he can make sure all the comments he made have been addressed.

HowardHinnant

Looks good.

ximinez

Everything looks good from my earlier comments. One unit test is failing right now, due to interactions with my earlier PR #4361. Fortunately, I noticed the incompatibility at the time, and coded up a fix. Cherry pick this commit, and you should be good to go: ximinez@b633d47

ximinez

Looks good to go.

intelliot · 2023-07-25T17:02:06Z

Related Clio PR: XRPLF/clio#780

RichardAH and others added 4 commits November 17, 2022 15:13

add nft_pages to account_objects rpc (compiling, untested)

25ded37

bug fix

3e3ac94

fix marker bugs

10190bd

[FOLD] Add unit tests and fix bugs in a couple of corner cases

0fe0afb

ximinez requested changes Dec 8, 2022

View reviewed changes

[FOLD] guard std::optional

019b956

intelliot requested a review from scottschurr December 20, 2022 00:11

intelliot added API Change Bug Needs Review Testable Will Need Documentation labels Dec 20, 2022

scottschurr reviewed Jan 13, 2023

View reviewed changes

intelliot assigned RichardAH Jan 18, 2023

intelliot removed the Needs Review label Feb 8, 2023

[FOLD] Incorporate unit test review comments

fc446d2

intelliot requested review from scottschurr, ximinez, nbougalis and HowardHinnant March 22, 2023 19:11

intelliot assigned HowardHinnant and unassigned RichardAH Mar 22, 2023

intelliot assigned scottschurr Mar 22, 2023

intelliot requested review from ckniffen, kennyzlei and shawnxie999 March 22, 2023 23:53

scottschurr approved these changes Mar 23, 2023

View reviewed changes

shawnxie999 reviewed Mar 24, 2023

View reviewed changes

shawnxie999 approved these changes Mar 27, 2023

View reviewed changes

shawnxie999 reviewed Mar 27, 2023

View reviewed changes

intelliot assigned ximinez and unassigned scottschurr Mar 28, 2023

HowardHinnant approved these changes Apr 3, 2023

View reviewed changes

Merge branch 'develop' into nftpage_accobjs

01b8b64

ximinez requested changes Apr 4, 2023

View reviewed changes

Update to account for NFTPages being returned by account objects

5b4f0a7

ximinez approved these changes Apr 5, 2023

View reviewed changes

intelliot merged commit f191c91 into XRPLF:develop Apr 5, 2023

cindyyan317 mentioned this pull request Jun 15, 2023

Add NFTokenPage objects in account_objects response. XRPLF/clio#696

Closed

mDuo13 mentioned this pull request Jun 24, 2023

rippled 1.11.0 changes XRPLF/xrpl-dev-portal#1976

Merged

6 tasks

nkramer44 mentioned this pull request Jun 28, 2023

Add NFTokenPage Object XRPLF/xrpl4j#430

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NFTokenPages to account_objects RPC #4352

Add NFTokenPages to account_objects RPC #4352

RichardAH commented Nov 25, 2022 •

edited by intelliot

Loading

RichardAH commented Nov 25, 2022

cjcobb23 commented Nov 29, 2022

RichardAH commented Nov 30, 2022

cjcobb23 commented Nov 30, 2022 •

edited

Loading

RichardAH commented Dec 2, 2022

ximinez left a comment

RichardAH commented Dec 19, 2022

scottschurr left a comment

intelliot commented Feb 8, 2023

RichardAH commented Mar 22, 2023

intelliot commented Mar 22, 2023

scottschurr left a comment

shawnxie999 Mar 24, 2023

scottschurr Mar 27, 2023

shawnxie999 Mar 27, 2023

shawnxie999 left a comment

shawnxie999 Mar 27, 2023

scottschurr Mar 27, 2023

shawnxie999 Mar 27, 2023

scottschurr Mar 27, 2023

shawnxie999 Mar 27, 2023

shawnxie999 Mar 27, 2023

scottschurr Mar 27, 2023 •

edited

Loading

intelliot commented Mar 29, 2023

HowardHinnant left a comment

ximinez left a comment

ximinez left a comment

intelliot commented Jul 25, 2023

		@@ -510,7 +730,7 @@ class AccountObjects_test : public beast::unit_test::suite
		auto const& ticket = resp[jss::result][jss::account_objects][0u];

Add NFTokenPages to account_objects RPC #4352

Add NFTokenPages to account_objects RPC #4352

Conversation

RichardAH commented Nov 25, 2022 • edited by intelliot Loading

High Level Overview of Change

Type of Change

RichardAH commented Nov 25, 2022

cjcobb23 commented Nov 29, 2022

RichardAH commented Nov 30, 2022

cjcobb23 commented Nov 30, 2022 • edited Loading

RichardAH commented Dec 2, 2022

ximinez left a comment

Choose a reason for hiding this comment

RichardAH commented Dec 19, 2022

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented Feb 8, 2023

RichardAH commented Mar 22, 2023

intelliot commented Mar 22, 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

shawnxie999 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

scottschurr Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

intelliot commented Mar 29, 2023

HowardHinnant left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

intelliot commented Jul 25, 2023

RichardAH commented Nov 25, 2022 •

edited by intelliot

Loading

cjcobb23 commented Nov 30, 2022 •

edited

Loading

scottschurr Mar 27, 2023 •

edited

Loading