Consider alternative design for token transfers and hooks

[frangio]

The way to customize token transfers today is by overriding the hooks _beforeTokenTransfer and _afterTokenTransfer. This was designed this way because there are multiple related operations (mint, transfer, burn) that would generally be customized in a uniform way (e.g. "pause transfers" should also pause mints), and they are different functions all of which invoke the same hooks.

An alternative that we can consider is instead to have a single entry point for all of these operations, a _transfer function that mints if the "from" argument is 0, etc., as suggested in #3109 for ERC20 (the same applies to other tokens). Hooks can then be removed because customizing transfers can be done by overriding this single function instead of overriding the hooks.

[songhobby]

In our ERC1155 contract, we have a single TokenTransfer function which is called by safeTransferFrom, safeBatchTransferFrom, mintBatch and burnBatch. For batch operations, the TokenTransfer is called inside of the loop for tokenIds. For minting and burning, we set the from and to to address(0)

We have token delegation and snapshots with majority of its logic in TokenTransfer

[frangio]

Other hooks that we have:

• Governor: _beforeExecute, _afterExecute
• TimelockController: _beforeCall, _afterCall
• Proxy: _beforeFallback

[frangio]

This is implemented in next-v5.0 (#3838, #3921, #3876). There was an attempt to implement it for ERC721 (#3893) but we ran into issues with the design summarized by @Amxx in this comment:

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

[RenanSouza2]

Hey everyone, I am writing my own implementation of the _update hook for ERC721 and I have one Idea that I would like to share

I made one compromise, to remove the batchSize from the update, all updates are for one token.

(I've updated the code in this PR but is not final yet: #4171)

How this interacts with the rest: there is a new function increaseBalance that is called in _mintBatch in ERC721Consecutive,

My biggest doubt so far, I'm struggling to refactor ERC721ConsecutiveMock,
Is ERC721 a good way to represent votes? isn't ERC20 or ERC1155 a better way to do this?

If this case is removes the _update function for one token only solves very well the other cases

[Amxx]

This was implemented in #3838, #3876 and #4377