[Amendment] Fix 3 issues around NFToken offer acceptance #4380

ledhed2222 · 2023-01-05T17:32:56Z

(This PR will share an amendment with #4346 [merged to feature branch on 2023-02-02]. We want them to go out in the same release.)

High Level Overview of Change

Fixes 3 separate issues:

Issue 1

In the following scenario, an account cannot perform NFTokenAcceptOffer even though it should be allowed to:

BROKER has < S
ALICE offers to sell token for S
BOB offers to buy token for > S
BROKER tries to bridge the two offers

This currently results in tecINSUFFICIENT_FUNDS, but should not because BROKER is not spending any funds in this transaction, beyond the transaction fee.

Issue 2

Situation - when trading an NFT using IOUs, and when the issuer of the IOU has any non-zero value set for TransferFee on their account via AccountSet (not a TransferFee on the NFT), and when the sale amount is equal to the total balance of that IOU that the buyer has, the resulting balance for the issuer of the IOU will become positive, which essentially means that the buyer of the NFT was supposed to have caused a certain amount of IOU to be burned. That amount was unable to be burned because the buyer couldn't cover it. This results in the buyer owing this amount back to the issuer. In a real world scenario, this is appropriate and can be settled off-chain.

Issue 3

Currency issuers could not make offers for NFTs using their own currency, receiving tecINSUFFICIENT_FUNDS if they tried to do so.

With this fix, they are now able to buy/sell NFTs using their own currency.

Context of Change

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

nbougalis · 2023-01-13T21:49:14Z

I haven't reviewed this closely and don't think I will have time to, but wanted to point out that you're doing unnecessary work because you're using accountHolds instead of accountFunds. The latter properly handles the case when you're checking if an account has sufficient funds in a given asset available and the account is the issuer of the asset.

ledhed2222 · 2023-01-13T22:40:07Z

@nbougalis how come GitHub doesn't have a 🐐 emoji response yet

scottschurr

I'm impressed! Good code, great comments, very thorough testing. Thanks for fixing these! 👍

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

intelliot · 2023-01-20T06:41:32Z

I think this is ready for @ximinez to review

src/test/app/NFToken_test.cpp

intelliot · 2023-02-08T21:16:42Z

@ledhed2222 as mentioned, this is ready for your perusal

shawnxie999 · 2023-02-08T22:43:24Z

@ledhed2222 A question about issued currency. Does the account that issued a particular currency have an unlimited supply of it? When I try to understand function accountFunds, it seems the currency issuer can never go into debt with its own currency

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

shawnxie999

non-important comments, looks good

intelliot · 2023-02-09T06:51:56Z

@ledhed2222 if you agree that this PR is ready to merge, go ahead and put the Passed label on it (or just comment saying that it's ready)

(from my perspective, it is ready. we have confirmed with Scott and Ed that no additional reviews are required)

Fixes 3 issues: In the following scenario, an account cannot perform NFTokenAcceptOffer even though it should be allowed to: - BROKER has < S - ALICE offers to sell token for S - BOB offers to buy token for > S - BROKER tries to bridge the two offers This currently results in `tecINSUFFICIENT_FUNDS`, but should not because BROKER is not spending any funds in this transaction, beyond the transaction fee. When trading an NFT using IOUs, and when the issuer of the IOU has any non-zero value set for TransferFee on their account via AccountSet (not a TransferFee on the NFT), and when the sale amount is equal to the total balance of that IOU that the buyer has, the resulting balance for the issuer of the IOU will become positive. This means that the buyer of the NFT was supposed to have caused a certain amount of IOU to be burned. That amount was unable to be burned because the buyer couldn't cover it. This results in the buyer owing this amount back to the issuer. In a real world scenario, this is appropriate and can be settled off-chain. Currency issuers could not make offers for NFTs using their own currency, receiving `tecINSUFFICIENT_FUNDS` if they tried to do so. With this fix, they are now able to buy/sell NFTs using their own currency.

Fixes 3 issues: In the following scenario, an account cannot perform NFTokenAcceptOffer even though it should be allowed to: - BROKER has < S - ALICE offers to sell token for S - BOB offers to buy token for > S - BROKER tries to bridge the two offers This currently results in `tecINSUFFICIENT_FUNDS`, but should not because BROKER is not spending any funds in this transaction, beyond the transaction fee. When trading an NFT using IOUs, and when the issuer of the IOU has any non-zero value set for TransferFee on their account via AccountSet (not a TransferFee on the NFT), and when the sale amount is equal to the total balance of that IOU that the buyer has, the resulting balance for the issuer of the IOU will become positive. This means that the buyer of the NFT was supposed to have caused a certain amount of IOU to be burned. That amount was unable to be burned because the buyer couldn't cover it. This results in the buyer owing this amount back to the issuer. In a real world scenario, this is appropriate and can be settled off-chain. Currency issuers could not make offers for NFTs using their own currency, receiving `tecINSUFFICIENT_FUNDS` if they tried to do so. With this fix, they are now able to buy/sell NFTs using their own currency. Signed-off-by: Kenny Lei <kennyzlei@gmail.com>

Fixes 3 issues: In the following scenario, an account cannot perform NFTokenAcceptOffer even though it should be allowed to: - BROKER has < S - ALICE offers to sell token for S - BOB offers to buy token for > S - BROKER tries to bridge the two offers This currently results in `tecINSUFFICIENT_FUNDS`, but should not because BROKER is not spending any funds in this transaction, beyond the transaction fee. When trading an NFT using IOUs, and when the issuer of the IOU has any non-zero value set for TransferFee on their account via AccountSet (not a TransferFee on the NFT), and when the sale amount is equal to the total balance of that IOU that the buyer has, the resulting balance for the issuer of the IOU will become positive. This means that the buyer of the NFT was supposed to have caused a certain amount of IOU to be burned. That amount was unable to be burned because the buyer couldn't cover it. This results in the buyer owing this amount back to the issuer. In a real world scenario, this is appropriate and can be settled off-chain. Currency issuers could not make offers for NFTs using their own currency, receiving `tecINSUFFICIENT_FUNDS` if they tried to do so. With this fix, they are now able to buy/sell NFTs using their own currency.

* upstream/develop: Rename to fixNonFungibleTokensV1_2 and some cosmetic changes (XRPLF#4419) Only account specified as destination can settle through brokerage: (XRPLF#4399) Prevent brokered sale of NFToken to owner: (XRPLF#4403) Fix 3 issues around NFToken offer acceptance (XRPLF#4380) Allow NFT to be burned when number of offers is greater than 500 (XRPLF#4346) Add fixUnburnableNFToken feature (XRPLF#4391) Change default vote on fixUniversalNumber from yes to no (XRPLF#4414)

* upstream/develop: Rename to fixNonFungibleTokensV1_2 and some cosmetic changes (XRPLF#4419) Only account specified as destination can settle through brokerage: (XRPLF#4399) Prevent brokered sale of NFToken to owner: (XRPLF#4403) Fix 3 issues around NFToken offer acceptance (XRPLF#4380) Allow NFT to be burned when number of offers is greater than 500 (XRPLF#4346) Add fixUnburnableNFToken feature (XRPLF#4391)

…ctionality * upstream/develop: Rename to fixNonFungibleTokensV1_2 and some cosmetic changes (XRPLF#4419) Only account specified as destination can settle through brokerage: (XRPLF#4399) Prevent brokered sale of NFToken to owner: (XRPLF#4403) Fix 3 issues around NFToken offer acceptance (XRPLF#4380) Allow NFT to be burned when number of offers is greater than 500 (XRPLF#4346) Add fixUnburnableNFToken feature (XRPLF#4391) Change default vote on fixUniversalNumber from yes to no (XRPLF#4414)

…tpage * upstream/develop: Rename to fixNonFungibleTokensV1_2 and some cosmetic changes (XRPLF#4419) Only account specified as destination can settle through brokerage: (XRPLF#4399) Prevent brokered sale of NFToken to owner: (XRPLF#4403) Fix 3 issues around NFToken offer acceptance (XRPLF#4380) Allow NFT to be burned when number of offers is greater than 500 (XRPLF#4346) Add fixUnburnableNFToken feature (XRPLF#4391) Change default vote on fixUniversalNumber from yes to no (XRPLF#4414)

Fixes 3 issues: In the following scenario, an account cannot perform NFTokenAcceptOffer even though it should be allowed to: - BROKER has < S - ALICE offers to sell token for S - BOB offers to buy token for > S - BROKER tries to bridge the two offers This currently results in `tecINSUFFICIENT_FUNDS`, but should not because BROKER is not spending any funds in this transaction, beyond the transaction fee. When trading an NFT using IOUs, and when the issuer of the IOU has any non-zero value set for TransferFee on their account via AccountSet (not a TransferFee on the NFT), and when the sale amount is equal to the total balance of that IOU that the buyer has, the resulting balance for the issuer of the IOU will become positive. This means that the buyer of the NFT was supposed to have caused a certain amount of IOU to be burned. That amount was unable to be burned because the buyer couldn't cover it. This results in the buyer owing this amount back to the issuer. In a real world scenario, this is appropriate and can be settled off-chain. Currency issuers could not make offers for NFTs using their own currency, receiving `tecINSUFFICIENT_FUNDS` if they tried to do so. With this fix, they are now able to buy/sell NFTs using their own currency.

ledhed2222 marked this pull request as draft January 5, 2023 20:53

ledhed2222 force-pushed the gw/fix-nft-positive-balance branch 3 times, most recently from da12eda to be4d436 Compare January 6, 2023 19:50

ledhed2222 requested a review from scottschurr January 6, 2023 21:42

ledhed2222 marked this pull request as ready for review January 7, 2023 06:17

ledhed2222 requested a review from ximinez January 7, 2023 06:17

scottschurr approved these changes Jan 14, 2023

View reviewed changes

src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp Outdated Show resolved Hide resolved

ledhed2222 changed the base branch from develop to feature/nft-fixes January 18, 2023 21:39

ledhed2222 force-pushed the gw/fix-nft-positive-balance branch from be4d436 to 75d617f Compare January 18, 2023 21:54

ledhed2222 added 3 commits January 18, 2023 17:00

Fixes various issues around NFToken offer acceptance

36c8745

add test for first issue addressed

77b57c8

adds more tests and cleans up NFTokenCreateOffer

47cd7c3

ledhed2222 force-pushed the gw/fix-nft-positive-balance branch from 75d617f to 47cd7c3 Compare January 18, 2023 22:00

intelliot assigned ximinez Jan 20, 2023

intelliot added Bug Amendment High Priority Testable labels Jan 20, 2023

scottschurr mentioned this pull request Jan 31, 2023

Prevent brokered sale of NFToken to owner (fix #4374) #4403

Merged

2 tasks

shawnxie999 reviewed Jan 31, 2023

View reviewed changes

src/test/app/NFToken_test.cpp Show resolved Hide resolved

intelliot added this to the 1.10 milestone Feb 1, 2023

ledhed2222 added 3 commits February 1, 2023 16:59

simplify by responding to comments

c8d1a43

further simplifications

51372b8

add a description of

2897095

intelliot requested a review from drlongle February 3, 2023 00:10

intelliot removed the Needs Review label Feb 7, 2023

kennyzlei assigned shawnxie999 and unassigned ledhed2222 Feb 8, 2023

kennyzlei assigned ledhed2222 and unassigned shawnxie999 Feb 8, 2023

shawnxie999 reviewed Feb 8, 2023

View reviewed changes

src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp Show resolved Hide resolved

shawnxie999 approved these changes Feb 8, 2023

View reviewed changes

intelliot unassigned ximinez Feb 9, 2023

intelliot mentioned this pull request Feb 9, 2023

Add fixUnburnableNFToken feature #4391

Merged

7 tasks

ledhed2222 added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 9, 2023

shawnxie999 approved these changes Feb 9, 2023

View reviewed changes

intelliot merged commit ce42b74 into XRPLF:feature/nft-fixes Feb 9, 2023

This was referenced Feb 13, 2023

Rename fixUnburnableNFToken to fixNonFungibleTokensV1_2 #4416

Closed

fixNonFungibleTokensV1_2: Multiple bug fixes for XLS-20 NFTs #4417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Amendment] Fix 3 issues around NFToken offer acceptance #4380

[Amendment] Fix 3 issues around NFToken offer acceptance #4380

ledhed2222 commented Jan 5, 2023 •

edited by intelliot

Loading

nbougalis commented Jan 13, 2023

ledhed2222 commented Jan 13, 2023

scottschurr left a comment

intelliot commented Jan 20, 2023

intelliot commented Feb 8, 2023

shawnxie999 commented Feb 8, 2023 •

edited

Loading

shawnxie999 left a comment

intelliot commented Feb 9, 2023

[Amendment] Fix 3 issues around NFToken offer acceptance #4380

[Amendment] Fix 3 issues around NFToken offer acceptance #4380

Conversation

ledhed2222 commented Jan 5, 2023 • edited by intelliot Loading

High Level Overview of Change

Issue 1

Issue 2

Issue 3

Context of Change

Type of Change

nbougalis commented Jan 13, 2023

ledhed2222 commented Jan 13, 2023

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented Jan 20, 2023

intelliot commented Feb 8, 2023

shawnxie999 commented Feb 8, 2023 • edited Loading

shawnxie999 left a comment

Choose a reason for hiding this comment

intelliot commented Feb 9, 2023

ledhed2222 commented Jan 5, 2023 •

edited by intelliot

Loading

shawnxie999 commented Feb 8, 2023 •

edited

Loading