Remove ERC721 tokenTransfer hooks

[JulissaDantes]

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

• [x] Tests
• [x] Documentation
• [x] Changelog entry

[Amxx]

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]

@Amxx What alternative do you recommend? We can revisit https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3744, but I would like to consider other options.

[Amxx]

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]

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

[frangio]

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

[Amxx]

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]

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]

Closing this PR in favor of the issue, where I shared the above summary: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3535