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

kennyzlei · 2023-02-13T07:28:02Z

High Level Overview of Change

Proposal to merge feature branch feature/nft-fixes to develop branch

Context of Change

Introduce a new amendment, fixNonFungibleTokensV1_2 which is a combination of bug fixes that have been individually merged into feature/nft-fixes through the pull request process:

Allow NFT to be burned when number of offers is greater than 500 (part of NonFungibleTokensV1_2 amendment) #4346
[Amendment] Fix 3 issues around NFToken offer acceptance #4380
Prevent brokered sale of NFToken to owner (fix #4374) #4403
[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

Each individual pull request contains details on the bugs they intend to fix

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

kennyzlei · 2023-02-13T13:31:38Z

@ximinez @intelliot I noticed the feature/nft-fixes branch is currently about 8 commits behind develop and causing a merge conflict. The resolution itself is a few lines of code to catch up on other features added from what I have tested. I assume feature/nft-fixes branch is protected and there are limitations to pushing commits to the branch directly to fix it. Would appreciate some recommendations on getting it to a good state. I was thinking these were some options:

Using an existing PR or new PR to pull in the ~8 commits into feature/nft-fixes. Might be a bit strange to have duplicate diff in that PR but it'll get the branch up-to-date
Have a maintainer or someone with elevated perms push a commit directly to the feature/nft-fixes branch to fix the conflict

intelliot · 2023-02-13T16:13:28Z

I don't think feature/nft-fixes is protected. ~~If we bring it up-to-date with a merge commit, there will not be any duplication in the diff.~~ Actually, I'm not sure what you mean about duplicate diff. This is a common situation with git and as long as you aren't cherry-picking commits off the base branch (which would create new git hashes for those commits) then there is no problem, whether we rebase or use merge commits.

nbougalis · 2023-02-13T17:06:12Z

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

+    // After this amendment, if any payment would cause a non-IOU-issuer to
+    // have a negative balance, or an IOU-issuer to have a positive balance in
+    // their own currency, we know that something went wrong. This was
+    // originally found in the context of IOU transfer fees. Since there are
+    // several payouts in this tx, just confirm that the end state is OK.


Note: negative balances are not a problem. Trustlines are bi-directional, and the issuer might have configured the trustline so that you can go into debt. The problem is a balance that exceeds the limit.

(@ledhed2222 wrote this comment here)

@ledhed2222 if it makes sense to you, could you clarify this comment? You can do so in a new PR, with feature/nft-fixes as the base branch.

(or, you can push a commit on feature/nft-fixes, if permissions allow. Either way is fine)

scottschurr · 2023-02-13T18:38:04Z

Just as a note, some of the the commit messages on this pull request do not follow guidelines we've been following for a long time. Here's the source of the guidelines we've followed in the past: https://cbea.ms/git-commit/

Specifically, we've requested that line lengths on commit messages be limited to 72 characters or so. We haven't always been great at enforcing that, but it has been a guideline.

I understand that expectations change over time. So if unlimited line lengths is the new norm, then so be it. But I thought ya'll should be aware that it is a change.

kennyzlei · 2023-02-13T19:07:46Z

Just as a note, some of the the commit messages on this pull request do not follow guidelines we've been following for a long time. Here's the source of the guidelines we've followed in the past: https://cbea.ms/git-commit/

Specifically, we've requested that line lengths on commit messages be limited to 72 characters or so. We haven't always been great at enforcing that, but it has been a guideline.

I understand that expectations change over time. So if unlimited line lengths is the new norm, then so be it. But I thought ya'll should be aware that it is a change.

Yeah I think this is something we should enforce before PR merge in the future. Since this existing feature branch going into develop, I think preserving the message to be easily referenced to its original PR might have some value, but we should enforce this char limit going forward

intelliot · 2023-02-13T19:11:10Z

It's a good guideline, but writing good (short) commit messages is extremely difficult, and I wouldn't hold up the release just for that. To take a specific example, how would you rewrite [Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) (#4399)?

It is the worst offender and I would be in favor of shortening it with a simple rebase before merging it into develop. However, in order to do that, we need a specific subject line to use.

ximinez · 2023-02-13T20:45:48Z

It's a good guideline, but writing good (short) commit messages is extremely difficult, and I wouldn't hold up the release just for that. To take a specific example, how would you rewrite [Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) (#4399)?

Only account specified as destination can settle through brokerage:

* Fixes 4373

First line is 68 characters long. (And I'd drop [Amendment] from all the commit messages.)

scottschurr · 2023-02-13T20:47:28Z

@intelliot, there are two guidelines I'm noticing:

The subject line should be limited to 50 characters and
The lines in the body should be limited to 72 characters.

You're noting a place where we broke the first of these guidelines. 50 characters is really hard. I tend to target 60 characters. For the case you pointer out I might suggest that we don't need all of those detail in the subject line. We can put additional details in the body:

Place new restrictions on brokering NFT offers:

When brokering NFT offers, if there's a destination field in the offer
we now require that the broker must be the destination.

These guidelines are hard. Even native English speaking programmers tend to not be great writers. Placing these constraints on non-native-English speakers is even harder. But the guidelines do help a great deal with the legibility of the log.

intelliot · 2023-02-13T20:49:51Z

It's a good guideline, but writing good (short) commit messages is extremely difficult, and I wouldn't hold up the release just for that. To take a specific example, how would you rewrite [Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) (#4399)?
Only account specified as destination can settle through brokerage:

* Fixes 4373
First line is 68 characters long. (And I'd drop [Amendment] from all the commit messages.)

To make it easier to navigate between the commit history and the relevant PRs, I'd like to include the PR # in the subject. Also, I believe putting the # in front of the issue number will make a link in the GitHub UI:

Only account specified as destination can settle through brokerage: (#4399)

* Fixes #4373

(Unfortunately, that is now 75 characters.)

I like @scottschurr 's suggestion, with the addition of the relevant issue/PR:

Place new restrictions on brokering NFT offers: (#4399)

When brokering NFT offers, if there's a destination field in the offer
we now require that the broker must be the destination.

* Fixes #4373

The issue/PR #s are helpful as they allow readers to easily find discussions and rationale related to the change.

kennyzlei · 2023-02-13T20:52:25Z

I'll do that cleanup to remove [Amendment] and rename that particular commit to Only account specified as destination can settle through brokerage: (#4399)

intelliot · 2023-02-13T20:54:07Z

@kennyzlei Given you're rebasing anyway, and given that we would like to merge this PR as multiple commits (not a single squashed commit), feel free to rebase on top of the current HEAD of develop.

kennyzlei · 2023-02-13T21:04:29Z

@intelliot @scottschurr @ximinez just rebased and fixed up commit messages, ready for review

thejohnfreeman

I have not reviewed the code itself, but I confirmed that this is a feature branch whose individual commits were previously approved by trusted reviewers.

intelliot · 2023-02-13T22:26:08Z

(Scott S is reviewing)

scottschurr

Looks like what I'd expect.

* Allow offers to be removable * Delete sell offers first Signed-off-by: Shawn Xie <[email protected]>

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.

@nixer89

Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering extracts money from the NFT owner and provides no benefit in return. With this amendment, the code detects when a broker is returning an NFToken to its initial owner and prohibits the transaction. This forbids a broker from selling an NFToken to the account that already owns the token. This fixes a bug in the original implementation of XLS-20. Thanks to @nixer89 for suggesting this fix.

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

kennyzlei mentioned this pull request Feb 13, 2023

Rename fixUnburnableNFToken to fixNonFungibleTokensV1_2 #4416

Closed

7 tasks

kennyzlei assigned intelliot and ximinez Feb 13, 2023

nbougalis reviewed Feb 13, 2023

View reviewed changes

intelliot changed the title ~~NonFungibleTokensV1_2 amendment to address various bug fixes~~ NonFungibleTokensV1_2: Multiple bug fixes for XLS-20 NFTs Feb 13, 2023

kennyzlei changed the title ~~NonFungibleTokensV1_2: Multiple bug fixes for XLS-20 NFTs~~ fixNonFungibleTokensV1_2: Multiple bug fixes for XLS-20 NFTs Feb 13, 2023

kennyzlei unassigned intelliot and ximinez Feb 13, 2023

kennyzlei force-pushed the feature/nft-fixes branch from 7fdd93e to 05602af Compare February 13, 2023 21:03

kennyzlei assigned intelliot, ximinez and scottschurr Feb 13, 2023

intelliot added this to the NFT bugfixes milestone Feb 13, 2023

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

kennyzlei assigned ledhed2222 and shawnxie999 and unassigned intelliot Feb 13, 2023

ledhed2222 approved these changes Feb 13, 2023

View reviewed changes

shawnxie999 approved these changes Feb 13, 2023

View reviewed changes

thejohnfreeman approved these changes Feb 13, 2023

View reviewed changes

intelliot unassigned ximinez, ledhed2222 and shawnxie999 Feb 13, 2023

intelliot requested a review from scottschurr February 13, 2023 22:58

scottschurr approved these changes Feb 13, 2023

View reviewed changes

kennyzlei force-pushed the feature/nft-fixes branch 2 times, most recently from 48ec19d to 2ec7471 Compare February 13, 2023 23:39

ledhed2222 and others added 6 commits February 13, 2023 15:52

Add fixUnburnableNFToken feature (#4391)

f7a8d2d

Allow NFT to be burned when number of offers is greater than 500 (#4346)

a828e24

* Allow offers to be removable * Delete sell offers first Signed-off-by: Shawn Xie <[email protected]>

Rename to fixNonFungibleTokensV1_2 and some cosmetic changes (#4419)

ac78b7a

kennyzlei force-pushed the feature/nft-fixes branch from 2ec7471 to ac78b7a Compare February 13, 2023 23:53

intelliot approved these changes Feb 14, 2023

View reviewed changes

intelliot mentioned this pull request Feb 14, 2023

Draft of release notes for 1.10.0 #4422

Closed

1 task

kennyzlei merged commit ac78b7a into develop Feb 14, 2023

kennyzlei unassigned scottschurr Feb 14, 2023

kennyzlei deleted the feature/nft-fixes branch February 14, 2023 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kennyzlei commented Feb 13, 2023 •

edited

Loading

kennyzlei commented Feb 13, 2023

intelliot commented Feb 13, 2023 •

edited

Loading

nbougalis Feb 13, 2023

intelliot Feb 13, 2023 •

edited

Loading

scottschurr commented Feb 13, 2023

kennyzlei commented Feb 13, 2023 •

edited

Loading

intelliot commented Feb 13, 2023 •

edited

Loading

ximinez commented Feb 13, 2023

scottschurr commented Feb 13, 2023

intelliot commented Feb 13, 2023 •

edited

Loading

kennyzlei commented Feb 13, 2023

intelliot commented Feb 13, 2023

kennyzlei commented Feb 13, 2023

thejohnfreeman left a comment

intelliot commented Feb 13, 2023

scottschurr left a comment

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

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

Conversation

kennyzlei commented Feb 13, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

kennyzlei commented Feb 13, 2023

intelliot commented Feb 13, 2023 • edited Loading

nbougalis Feb 13, 2023

Choose a reason for hiding this comment

intelliot Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

scottschurr commented Feb 13, 2023

kennyzlei commented Feb 13, 2023 • edited Loading

intelliot commented Feb 13, 2023 • edited Loading

ximinez commented Feb 13, 2023

scottschurr commented Feb 13, 2023

intelliot commented Feb 13, 2023 • edited Loading

kennyzlei commented Feb 13, 2023

intelliot commented Feb 13, 2023

kennyzlei commented Feb 13, 2023

thejohnfreeman left a comment

Choose a reason for hiding this comment

intelliot commented Feb 13, 2023

scottschurr left a comment

Choose a reason for hiding this comment

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

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

kennyzlei commented Feb 13, 2023 •

edited

Loading

intelliot commented Feb 13, 2023 •

edited

Loading

intelliot Feb 13, 2023 •

edited

Loading

kennyzlei commented Feb 13, 2023 •

edited

Loading

intelliot commented Feb 13, 2023 •

edited

Loading

intelliot commented Feb 13, 2023 •

edited

Loading