Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fixNFTokenReserve: Throw error when NFT buyer does not meet reserve requirement #4767

Merged
merged 28 commits into from
Feb 2, 2024
Merged

fixNFTokenReserve: Throw error when NFT buyer does not meet reserve requirement #4767

merged 28 commits into from
Feb 2, 2024

Conversation

shawnxie999
Copy link
Collaborator

@shawnxie999 shawnxie999 commented Oct 13, 2023 •
edited
Loading

High Level Overview of Change

Add checks in the NFTokenAcceptOffer transactor to check if OwnerCount changed, if it did, then check for the reserve requirement.

Context of Change

Fixes a problem where someone could accept an NFT sell offer without needing reserve.

This problem, however, does not occur when buy offer is involved or in brokered mode.

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

@shawnxie999 shawnxie999 linked an issue Oct 17, 2023 that may be closed by this pull request
NFTokenAcceptOffer doesn't check the reserves of new NFT holder (Version: [1.11.0, 1.12.0-b3]) #4679
Closed
@shawnxie999 shawnxie999 marked this pull request as ready for review October 17, 2023 14:54
@shawnxie999 shawnxie999 changed the title fixNFTokenReserve fixNFTokenReserve: Throw error when NFT buyer does not meet reserve requirement Oct 17, 2023
@intelliot intelliot requested review from nbougalis, scottschurr, dangell7 and ckeshava and removed request for dangell7 October 17, 2023 18:27
@intelliot intelliot added this to the 2024 release milestone Oct 18, 2023
scottschurr
scottschurr reviewed Oct 25, 2023
View reviewed changes
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't let the comments I left discourage you. This is a great pull request. The fix looks good. I think you did an especially good job with the tests.

I did leave some comments for you to consider, but they are mostly about different ways to structure the same code. Feel free to push back on any of them.

Nice job!

src/test/app/NFToken_test.cpp Outdated Show resolved Hide resolved
src/test/app/NFToken_test.cpp Show resolved Hide resolved
@shawnxie999
Copy link
Collaborator Author

@scottschurr thanks for the great suggestions! this PR is now ready for re-review

@shawnxie999 shawnxie999 changed the base branch from develop to api-changelog-delivermax October 31, 2023 14:28
@shawnxie999 shawnxie999 changed the base branch from api-changelog-delivermax to develop October 31, 2023 14:28
scottschurr
scottschurr approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

ckeshava
ckeshava approved these changes Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@ckeshava ckeshava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your work @shawnxie999 , this looks nice

@shawnxie999 shawnxie999 added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 6, 2023
@intelliot intelliot self-assigned this Nov 30, 2023
@intelliot
Copy link
Collaborator

Intend to merge this after 2.0.1 is released

@tequdev
Copy link
Contributor

tequdev commented Nov 30, 2023

@shawnxie999 Thank you for fixing the issue.

@intelliot intelliot added the Perf impact not expected Change is not expected to improve nor harm performance. label Jan 9, 2024
@intelliot intelliot removed their assignment Jan 20, 2024
@intelliot
Copy link
Collaborator

Some unit tests were renamed. @shawnxie999 to resolve conflicts (no hurry).

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2024 •
edited
Loading

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (6f00d32) 61.50% compared to head (cdbbb0d) 61.50%.

Files Patch % Lines
src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp 76.92% 3 Missing and 3 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4767      +/-   ##
===========================================
- Coverage    61.50%   61.50%   -0.01%     
===========================================
  Files          797      797              
  Lines        70122    70130       +8     
  Branches     36238    36244       +6     
===========================================
+ Hits         43128    43130       +2     
- Misses       19755    19758       +3     
- Partials      7239     7242       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@intelliot
Copy link
Collaborator

@shawnxie999 could you have a look at the "6 lines in your changes are missing coverage" reported above?

@shawnxie999
Copy link
Collaborator Author

shawnxie999 commented Feb 2, 2024 •
edited
Loading

@intelliot these are the lines that return tecInternal errors, which are guarded by the preclaim checks already. I dont think we need additional tests needed for these lines 👍

@intelliot intelliot merged commit 828bb64 into XRPLF:develop Feb 2, 2024
17 checks passed
@intelliot
Copy link
Collaborator

sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
@shawnxie999 @sophiax851
fixNFTokenReserve: ensure NFT tx fails when reserve is not met (XRP…
110afec 
…LF#4767)

Without this amendment, an NFTokenAcceptOffer transaction can succeed
even when the NFToken recipient does not have sufficient reserves for
the new NFTokenPage. This allowed accounts to accept NFT sell offers
without having a sufficient reserve. (However, there was no issue in
brokered mode or when a buy offer is involved.)

Instead, the transaction should fail with `tecINSUFFICIENT_RESERVE` as
appropriate. The `fixNFTokenReserve` amendment adds checks in the
NFTokenAcceptOffer transactor to check if the OwnerCount changed. If it
did, then it checks the new reserve requirement.

Fix XRPLF#4679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottschurr scottschurr scottschurr approved these changes

@ckeshava ckeshava ckeshava approved these changes

@nbougalis nbougalis Awaiting requested review from nbougalis

Assignees

@sgramkumar sgramkumar

Labels
Amendment Bug Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Perf impact not expected Change is not expected to improve nor harm performance.
Projects
Core Ledger
Status: 🚢 Released in 2.2.0
Milestone
2.1.0 (Fix)
Development

Successfully merging this pull request may close these issues.

NFTokenAcceptOffer doesn't check the reserves of new NFT holder (Version: [1.11.0, 1.12.0-b3])
7 participants
@shawnxie999 @intelliot @tequdev @codecov-commenter @scottschurr @ckeshava @sgramkumar