-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: AccountNFT with invalid marker #1589
fix: AccountNFT with invalid marker #1589
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1589 +/- ##
===========================================
+ Coverage 69.54% 69.87% +0.33%
===========================================
Files 254 260 +6
Lines 9892 9921 +29
Branches 5463 5470 +7
===========================================
+ Hits 6879 6932 +53
+ Misses 1595 1589 -6
+ Partials 1418 1400 -18 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, just a few nits 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff 👍
I think the marker is different in Clio, comparing with rippled. So rippled's approach might not work on Clio.
Also please check the document https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/account-methods/account_nfts#account_nfts, I think Clio's limit is also different.
@cindyyan317 rippled can directly fetch a page based on the NFTokenID marker. But clio can't do this, it has to traverse from the last page. So I assume that's why the marker is the ID of the page itself. I think we can still try to return a NFTokenID as a marker, but it would require clio to traverse from the last token page everytime a marker is provided, which would make the API less efficient |
@shawnxie999 Thanks for the suggestion. According to the doc , the marker should be page id. We will flag this mismatch to rippled and discuss the correct fix. For this PR, I think we don't mimic rippled. The marker is considered valid if it is a valid page object of account . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Fixes [#1497](#1497) Mimics the behavior of the [fix on Rippled side](XRPLF/rippled#5045)
Fixes [#1497](#1497) Mimics the behavior of the [fix on Rippled side](XRPLF/rippled#5045)
Fixes #1497
Mimics the behavior of the fix on Rippled side