[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

dangell7 · 2023-01-25T23:29:45Z

High Level Overview of Change

Currently with NFTs using broker mode if the sell offer contains a destination and that destination is the buyer account, anyone can broker the transaction.

It is also true that if a buy offer contains a destination and that destination is the seller account, anyone can broker the transaction.

Imo this is not ideal and is misleading to everyone.

The comments in the code are;

// If the buyer specified a destination, that destination must be
// the seller or the broker.

This is misleading because the broker is the account submitting the transaction. Its anyone. Again, if the buyer specified a destination and that destination is the seller, anyone can settle the tx. I don't think that makes sense.

I believe the goal should be; If you set a destination, that destination needs to be the person settling the transaction.

This PR introduces a fix amendment called fixNFTokenBrokerAccept.

Context of Change

The changes made now force the broker to be the destination if they want to settle. If the buyer is the destination, then the buyer must accept the sell offer, as you cannot broker your own offers.

If users want their offers open to the public then not setting a destination would be the recommended way. Inversely, if users want to limit who can settle the offers, then they would set a destination.

This PR fixes #4373.

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

Test Plan

I also added a test function to show one of the more recent issues. There are 2 things tested.

The broker cannot broker a destination offer to the buyer and the buyer must accept the sell offer. (0 transfer)
If the broker is the destination, the broker will take the difference. (broker mode)

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

nixer89 · 2023-02-01T12:08:52Z

@intelliot : this bug really hurts people out there. I request that this fix is included in the next planned release.

This PR solves the issue 4373

bharathchari · 2023-02-01T12:20:57Z

Pending any issues that come up during reviews, I think this should go into the next release. These are some example transactions that are a result of this issue:

https://bithomp.com/explorer/BA4F42227EC87C5447B94E3FFD376D58B562601C413B103B6EB1FEC3621F72D0
https://bithomp.com/explorer/580ED05BD1F866700C7FA2882065D0CDCBDC1B6D98A7D76F281C39AA7B19F5DA
https://bithomp.com/explorer/E1E70B5F407A13F894258699033D26171501F4AC4455CF1E75F215519EF0E41F

intelliot · 2023-02-03T00:32:19Z

@bharathchari Would it be OK for this to be rolled into the same NFT bugfix amendment that is currently in development? i.e. this branch: https://github.com/XRPLF/rippled/tree/feature/nft-fixes - which will likely include #4380 and #4403, if approved.

Or, should this be its own amendment?

nixer89 · 2023-02-03T17:53:43Z

@bharathchari Would it be OK for this to be rolled into the same NFT bugfix amendment that is currently in development? i.e. this branch: https://github.com/XRPLF/rippled/tree/feature/nft-fixes - which will likely include #4380 and #4403, if approved.

Or, should this be its own amendment?

I think this would be okay. Maybe we can then rename the amendment, away from unburnableNFT and into something more general, since it includes so many different fixes? any thoughts? :)

scottschurr

👍 Looks like a reasonable solution to the problem to me. Nice work with the unit tests.

I did leave comments on a few things that I think might be improvements, but they needn't hold up this pull request. Those changes can be made at your discretion. Thanks for the contribution!

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

src/test/app/NFToken_test.cpp

ledhed2222 · 2023-02-13T03:53:19Z

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

+                *dest != ctx.tx[sfAccount])
+            {
+                return tecNO_PERMISSION;
+            }


it's not a blocker, but you are checking the rule twice when you could check it just once with some conditional re-ordering

I refactored this if you want to check it out.

it's better looking to me! this is fine for merge, but IMO I would do this to remove a level of indentation:

if (ctx.view.rules().enabled(fixUnburnableNFToken)) { if (*dest != ctx.tx[sfAccount]) return tecNO_PERMISSION; } else if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) return tecNFTOKEN_BUY_SELL_MISMATCH;

src/test/app/NFToken_test.cpp

ledhed2222

approved - there's some cleanup that can occur here but it's largely cosmetic

dangell7 · 2023-02-13T07:03:07Z

@ledhed2222 made the suggested changes. Thank you for reviewing

ledhed2222 · 2023-02-13T16:23:23Z

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

+                // the destination must be the buyer or the broker.
+                if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount])
+                    return tecNFTOKEN_BUY_SELL_MISMATCH;
+            }


ledhed2222 · 2023-02-13T16:25:07Z

src/test/app/NFToken_test.cpp

@@ -5088,4 +5121,4 @@ class NFToken_test : public beast::unit_test::suite

 BEAST_DEFINE_TESTSUITE_PRIO(NFToken, tx, ripple, 2);

-}  // namespace ripple
+}  // namespace ripple


missing a newline but not a blocker

intelliot · 2023-02-13T18:16:07Z

Feel free to open a new PR for any styling nitpicks - or just leave them alone. Either way is fine :)

…4399) Without this amendment, for NFTs using broker mode, if the sell offer contains a destination and that destination is the buyer account, anyone can broker the transaction. Also, if a buy offer contains a destination and that destination is the seller account, anyone can broker the transaction. This is not ideal and is misleading. Instead, with this amendment: If you set a destination, that destination needs to be the account settling the transaction. So, the broker must be the destination if they want to settle. If the buyer is the destination, then the buyer must accept the sell offer, as you cannot broker your own offers. If users want their offers open to the public, then they should not set a destination. On the other hand, if users want to limit who can settle the offers, then they would set a destination. Unit tests: 1. The broker cannot broker a destination offer to the buyer and the buyer must accept the sell offer. (0 transfer) 2. If the broker is the destination, the broker will take the difference. (broker mode)

…4399) Without this amendment, for NFTs using broker mode, if the sell offer contains a destination and that destination is the buyer account, anyone can broker the transaction. Also, if a buy offer contains a destination and that destination is the seller account, anyone can broker the transaction. This is not ideal and is misleading. Instead, with this amendment: If you set a destination, that destination needs to be the account settling the transaction. So, the broker must be the destination if they want to settle. If the buyer is the destination, then the buyer must accept the sell offer, as you cannot broker your own offers. If users want their offers open to the public, then they should not set a destination. On the other hand, if users want to limit who can settle the offers, then they would set a destination. Unit tests: 1. The broker cannot broker a destination offer to the buyer and the buyer must accept the sell offer. (0 transfer) 2. If the broker is the destination, the broker will take the difference. (broker mode) Signed-off-by: Kenny Lei <kennyzlei@gmail.com>

…4399) Without this amendment, for NFTs using broker mode, if the sell offer contains a destination and that destination is the buyer account, anyone can broker the transaction. Also, if a buy offer contains a destination and that destination is the seller account, anyone can broker the transaction. This is not ideal and is misleading. Instead, with this amendment: If you set a destination, that destination needs to be the account settling the transaction. So, the broker must be the destination if they want to settle. If the buyer is the destination, then the buyer must accept the sell offer, as you cannot broker your own offers. If users want their offers open to the public, then they should not set a destination. On the other hand, if users want to limit who can settle the offers, then they would set a destination. Unit tests: 1. The broker cannot broker a destination offer to the buyer and the buyer must accept the sell offer. (0 transfer) 2. If the broker is the destination, the broker will take the difference. (broker mode)

* 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)

…RPLF#4399) Without this amendment, for NFTs using broker mode, if the sell offer contains a destination and that destination is the buyer account, anyone can broker the transaction. Also, if a buy offer contains a destination and that destination is the seller account, anyone can broker the transaction. This is not ideal and is misleading. Instead, with this amendment: If you set a destination, that destination needs to be the account settling the transaction. So, the broker must be the destination if they want to settle. If the buyer is the destination, then the buyer must accept the sell offer, as you cannot broker your own offers. If users want their offers open to the public, then they should not set a destination. On the other hand, if users want to limit who can settle the offers, then they would set a destination. Unit tests: 1. The broker cannot broker a destination offer to the buyer and the buyer must accept the sell offer. (0 transfer) 2. If the broker is the destination, the broker will take the difference. (broker mode)

dangell7 changed the title ~~fixNFTokenBrokerAccept~~ [FIX] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination Jan 25, 2023

dangell7 changed the title ~~[FIX] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination~~ Proposal: NFTokenAcceptOffer Change the way destination is used. Jan 26, 2023

dangell7 force-pushed the xls20brokerfix branch 2 times, most recently from 9e58900 to aa730fb Compare January 26, 2023 07:52

dangell7 changed the title ~~Proposal: NFTokenAcceptOffer Change the way destination is used.~~ FIX: fixNFTokenBrokerAccept If you set a destination on the offer, only that destination can settle through brokerage. Jan 26, 2023

nixer89 added the Amendment label Jan 31, 2023

nixer89 reviewed Jan 31, 2023

View reviewed changes

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

dangell7 force-pushed the xls20brokerfix branch from aa730fb to 9cd2375 Compare February 1, 2023 05:49

nixer89 requested review from scottschurr, RichardAH and ledhed2222 February 1, 2023 12:09

dangell7 force-pushed the xls20brokerfix branch 2 times, most recently from 281c8db to 17f70d0 Compare February 1, 2023 17:29

dangell7 requested review from nixer89 and removed request for ledhed2222, scottschurr and RichardAH February 1, 2023 17:31

dangell7 force-pushed the xls20brokerfix branch 2 times, most recently from 61888ec to 2664f83 Compare February 1, 2023 19:33

nixer89 requested review from scottschurr, RichardAH and ledhed2222 February 2, 2023 07:27

intelliot added this to the 1.10 milestone Feb 3, 2023

intelliot assigned scottschurr and shawnxie999 Feb 3, 2023

scottschurr approved these changes Feb 4, 2023

View reviewed changes

ledhed2222 reviewed Feb 13, 2023

View reviewed changes

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

ledhed2222 reviewed Feb 13, 2023

View reviewed changes

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

ledhed2222 reviewed Feb 13, 2023

View reviewed changes

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

ledhed2222 approved these changes Feb 13, 2023

View reviewed changes

dangell7 added 4 commits February 13, 2023 01:37

refactor

723f15c

remove feature test set

5cfdcd7

clean comments

f519f90

clang-format

7cadd0c

This was referenced Feb 13, 2023

Rename fixUnburnableNFToken to fixNonFungibleTokensV1_2 #4416

Closed

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

Merged

ledhed2222 reviewed Feb 13, 2023

View reviewed changes

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

intelliot merged commit 4ea7d56 into XRPLF:feature/nft-fixes Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

dangell7 commented Jan 25, 2023 •

edited by intelliot

Loading

nixer89 commented Feb 1, 2023

bharathchari commented Feb 1, 2023

intelliot commented Feb 3, 2023

nixer89 commented Feb 3, 2023

scottschurr left a comment

ledhed2222 Feb 13, 2023

dangell7 Feb 13, 2023 •

edited

Loading

ledhed2222 Feb 13, 2023

ledhed2222 left a comment

dangell7 commented Feb 13, 2023

ledhed2222 Feb 13, 2023

ledhed2222 Feb 13, 2023

intelliot commented Feb 13, 2023

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

Conversation

dangell7 commented Jan 25, 2023 • edited by intelliot Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

nixer89 commented Feb 1, 2023

bharathchari commented Feb 1, 2023

intelliot commented Feb 3, 2023

nixer89 commented Feb 3, 2023

scottschurr left a comment

Choose a reason for hiding this comment

ledhed2222 Feb 13, 2023

Choose a reason for hiding this comment

dangell7 Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

ledhed2222 Feb 13, 2023

Choose a reason for hiding this comment

ledhed2222 left a comment

Choose a reason for hiding this comment

dangell7 commented Feb 13, 2023

ledhed2222 Feb 13, 2023

Choose a reason for hiding this comment

ledhed2222 Feb 13, 2023

Choose a reason for hiding this comment

intelliot commented Feb 13, 2023

dangell7 commented Jan 25, 2023 •

edited by intelliot

Loading

dangell7 Feb 13, 2023 •

edited

Loading