Remove ERC721 tokenTransfer hooks #3893

JulissaDantes · 2022-12-21T18:34:49Z

Fixes #3535
Replaces #3636

The current way to customize token transfers(mint, burn, transfer) is by overriding the hooks _beforeTokenTransfer and _afterTokenTransfer. This refactor removes both hooks from the ERC721 contract, and implements the logic inside the new _update internal function, so future customizations only rely on overriding a single entry point.

PR Checklist

Tests
Documentation
Changelog entry

contracts/token/ERC721/ERC721.sol

Amxx · 2023-01-06T10:38:43Z

Overall I feel uncomfortable with this change.

Devs that call _update(from, to, tokenId, batchSize) will expect the batch to be transferred, including ownership transfers and events.

The truth is, only batchsize == 1 does the transfer, and any other value is just a hack to update the balances. I think this is very error prone and is not aligned with our guidelines!

frangio · 2023-01-06T14:42:15Z

@Amxx What alternative do you recommend? We can revisit #3744, but I would like to consider other options.

Amxx · 2023-01-06T15:31:41Z

My first idea would be to remove the batchSize from the _update function, and manage the balances change of batch minting in a different way.

frangio · 2023-01-07T22:27:22Z

But how? And we should keep in mind that the goal is to minimize overrides for the end user.

frangio · 2023-01-09T16:38:47Z

We might have to consider adding _mintConsecutive by default in ERC721. It stretches the limits of what's comfortably possible with inheritance.

Amxx · 2023-01-10T12:47:52Z

We might have to consider adding _mintConsecutive by default in ERC721. It stretches the limits of what's comfortably possible with inheritance.

That is one option

There was another option that used overflows, but it used 2 slots per account ... and feels very "hacky"

My concern is that putting _mintConsecutive by default in ERC721 will add gas cost to usecases that don't do any concecutive minting.

Amxx

Maybe not for this PR, but we will have to worry about these extensions.

Amxx · 2023-01-10T15:14:20Z

contracts/token/ERC721/extensions/ERC721URIStorage.sol

-            delete _tokenURIs[tokenId];
+    function _update(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual override {
+        super._update(from, to, firstTokenId, batchSize);
+        if (to == address(0)) {


if to == address(0) and batchSize > 1, we should probably do something. Revert?

Amxx · 2023-01-10T15:14:29Z

contracts/token/ERC721/extensions/ERC721Royalty.sol

-        _resetTokenRoyalty(tokenId);
+    function _update(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual override {
+        super._update(from, to, firstTokenId, batchSize);
+        if (to == address(0)) {


if to == address(0) and batchSize > 1, we should probably do something. Revert?

Amxx · 2023-01-12T08:58:48Z

contracts/token/ERC721/extensions/ERC721Consecutive.sol

+            // After construction, {_mintConsecutive} is no longer available and {_mint} becomes available.
+            require(Address.isContract(address(this)), "ERC721Consecutive: can't mint during construction");
+        }
+        super._update(from, to, firstTokenId, batchSize);


Also realized that here we are not following the rule that overrides should do their own stuff before calling super (and not after). We need to add comments documenting why we are doing so.

Amxx · 2023-01-12T08:59:42Z

contracts/token/ERC721/extensions/ERC721Enumerable.sol

-    ) internal virtual override {
-        super._beforeTokenTransfer(from, to, firstTokenId, batchSize);
-
+    function _update(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual override {
        if (batchSize > 1) {


We have to consider batchSize == 0

Amxx · 2023-01-13T15:58:39Z

Here is a summary of the identified issues that would need to be fixed:

_update does not check the from. This means that a dev could call the _update function with an invalid from, and cause:
- an invalid event to be emitted
- corruption of the balances mapping.
_update does different operation depending on the batchSize:
- if the batchSize is 1, it will update the ownership, reset the approval, update the balances, and emit an event.
- if the batchSize is not 0, it will just update the balance, but not do the transfer.
  This is error prone, because the dev have to understand that this function will do two very different thinks depending on this "small" parameter. It is possible that devs call this with a batchsize > 1 hoping that it will transfer a batch of token. This argues against having a single _update for both single and batched operations.
Cleanly support batchSize = 0 in extensions.
- Some of them are not designed to handle batches, and just don't know not to process burns of size > 1. The fact that they "hook" into _update(address,address,uint256,uint256) means they could see signals they don't know how to handle. Hopefully they never have to, but this also argues against having a single _update for both single and batched operations.
- Affected extensions:
  - ERC721Storage
  - ERC721Royalty
  - ERC721Enumerable
Document situations where we don't follow our own guidelines.
- In ERC721Consecutive, we do some operations before, and other after the super._uodate(...) call. This is in conflict with our guidelines

frangio · 2023-01-24T20:42:36Z

Closing this PR in favor of the issue, where I shared the above summary: #3535

JulissaDantes added 8 commits December 16, 2022 14:51

Update ERC721 base contract

509d2d1

update extensions

5da6e47

Update ERC721Consecutive

90c6161

Update overrides and extensions

fee2eb1

Merge branch 'next-v5.0' into refactor/erc721-mint-burn-transfer

3f6b875

Update changelog

d1c9e5d

Add PR number

3448534

Add space

2990a1d

frangio mentioned this pull request Dec 28, 2022

[next-v5.0] Refactor ERC721 _transfer function to support minting and burning #3636

Closed

3 tasks

JulissaDantes added 2 commits January 5, 2023 10:06

Merge branch 'next-v5.0' into refactor/erc721-mint-burn-transfer

acbc9b7

run lint

e1d2f33

frangio requested a review from Amxx January 5, 2023 22:20

run lint

749e6c8

Amxx reviewed Jan 6, 2023

View reviewed changes

contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved

Amxx reviewed Jan 10, 2023

View reviewed changes

Merge branch 'next-v5.0' into refactor/erc721-mint-burn-transfer

c5f47b4

Amxx reviewed Jan 12, 2023

View reviewed changes

JulissaDantes added 3 commits January 12, 2023 15:17

update ERC721 contracts

c4ea20c

improve format

b3bf848

update reason

769c6ec

JulissaDantes force-pushed the refactor/erc721-mint-burn-transfer branch from 7383061 to 769c6ec Compare January 12, 2023 19:29

JulissaDantes requested review from frangio and Amxx January 12, 2023 19:29

frangio marked this pull request as draft January 13, 2023 16:34

frangio mentioned this pull request Jan 24, 2023

Consider alternative design for token transfers and hooks #3535

Closed

frangio closed this Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ERC721 tokenTransfer hooks #3893

Remove ERC721 tokenTransfer hooks #3893

JulissaDantes commented Dec 21, 2022

Amxx commented Jan 6, 2023

frangio commented Jan 6, 2023

Amxx commented Jan 6, 2023

frangio commented Jan 7, 2023

frangio commented Jan 9, 2023 •

edited

Loading

Amxx commented Jan 10, 2023

Amxx left a comment

Amxx Jan 10, 2023

Amxx Jan 10, 2023

Amxx Jan 12, 2023

Amxx Jan 12, 2023

Amxx commented Jan 13, 2023 •

edited

Loading

frangio commented Jan 24, 2023

Remove ERC721 tokenTransfer hooks #3893

Remove ERC721 tokenTransfer hooks #3893

Conversation

JulissaDantes commented Dec 21, 2022

PR Checklist

Amxx commented Jan 6, 2023

frangio commented Jan 6, 2023

Amxx commented Jan 6, 2023

frangio commented Jan 7, 2023

frangio commented Jan 9, 2023 • edited Loading

Amxx commented Jan 10, 2023

Amxx left a comment

Choose a reason for hiding this comment

Amxx Jan 10, 2023

Choose a reason for hiding this comment

Amxx Jan 10, 2023

Choose a reason for hiding this comment

Amxx Jan 12, 2023

Choose a reason for hiding this comment

Amxx Jan 12, 2023

Choose a reason for hiding this comment

Amxx commented Jan 13, 2023 • edited Loading

frangio commented Jan 24, 2023

frangio commented Jan 9, 2023 •

edited

Loading

Amxx commented Jan 13, 2023 •

edited

Loading