Use the _update mechanism in ERC721 (#4377)

Co-authored-by: Francisco Giordano <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>

.changeset/bright-tomatoes-sing.md

@@ -2,4 +2,4 @@
 'openzeppelin-solidity': major
 ---
 
-`ERC20`, `ERC1155`: Deleted `_beforeTokenTransfer` and `_afterTokenTransfer` hooks, added a new internal `_update` function for customizations, and refactored all extensions using those hooks to use `_update` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838), [#3876](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876))
+`ERC20`, `ERC721`, `ERC1155`: Deleted `_beforeTokenTransfer` and `_afterTokenTransfer` hooks, added a new internal `_update` function for customizations, and refactored all extensions using those hooks to use `_update` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838), [#3876](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876), [#4377](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4377))

.changeset/eighty-lemons-shake.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`ERC721`: `_approve` no longer allows approving the owner of the tokenId. `_setApprovalForAll` no longer allows setting address(0) as an operator.

CHANGELOG.md

@@ -36,7 +36,7 @@ These removals were implemented in the following PRs: [#3637](https://github.com
 
 These breaking changes will require modifications to ERC20, ERC721, and ERC1155 contracts, since the `_afterTokenTransfer` and `_beforeTokenTransfer` functions were removed. Any customization made through those hooks should now be done overriding the new `_update` function instead.
 
-Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies.
+Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_transfer`, `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies.
 
 For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have to be changed in the following way.
 
@@ -53,6 +53,18 @@ For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have t
   }
 ```
 
+### More about ERC721
+
+In the case of `ERC721`, the `_update` function does not include a `from` parameter, as the sender is implicitly the previous owner of the `tokenId`. The address of
+this previous owner is returned by the `_update` function, so it can be used for a posteriori checks. In addition to `to` and `tokenId`, a third parameter (`auth`) is
+present in this function. This parameter enabled an optional check that the caller/spender is approved to do the transfer. This check cannot be performed after the transfer (because the transfer resets the approval), and doing it before `_update` would require a duplicate call to `_ownerOf`.
+
+In this logic of removing hidden SLOADs, the `_isApprovedOrOwner` function was removed in favor of a new `_isAuthorized` function. Overrides that used to target the
+`_isApprovedOrOwner` should now be performed on the `_isAuthorized` function. Calls to `_isApprovedOrOwner` that preceded a call to `_transfer`, `_burn` or `_approve`
+should be removed in favor of using the `auth` argument in `_update` and `_approve`. This is showcased in `ERC721Burnable.burn` and in `ERC721Wrapper.withdrawTo`.
+
+The `_exists` function was removed. Calls to this function can be replaced by `_ownerOf(tokenId) != address(0)`.
+
 #### ERC165Storage
 
 Users that were registering EIP-165 interfaces with `_registerInterface` from `ERC165Storage` should instead do so so by overriding the `supportsInterface` function as seen below:

contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol

@@ -28,25 +28,15 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable
         return super._ownerOf(tokenId);
     }
 
-    function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) {
-        super._mint(to, tokenId);
-    }
-
-    function _beforeTokenTransfer(
-        address from,
+    function _update(
         address to,
-        uint256 firstTokenId,
-        uint256 batchSize
-    ) internal virtual override(ERC721, ERC721Enumerable) {
-        super._beforeTokenTransfer(from, to, firstTokenId, batchSize);
+        uint256 tokenId,
+        address auth
+    ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) {
+        return super._update(to, tokenId, auth);
     }
 
-    function _afterTokenTransfer(
-        address from,
-        address to,
-        uint256 firstTokenId,
-        uint256 batchSize
-    ) internal virtual override(ERC721, ERC721Consecutive) {
-        super._afterTokenTransfer(from, to, firstTokenId, batchSize);
+    function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Enumerable) {
+        super._increaseBalance(account, amount);
     }
 }

contracts/mocks/token/ERC721ConsecutiveMock.sol

@@ -41,26 +41,16 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes
         return super._ownerOf(tokenId);
     }
 
-    function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) {
-        super._mint(to, tokenId);
-    }
-
-    function _beforeTokenTransfer(
-        address from,
+    function _update(
         address to,
-        uint256 firstTokenId,
-        uint256 batchSize
-    ) internal virtual override(ERC721, ERC721Pausable) {
-        super._beforeTokenTransfer(from, to, firstTokenId, batchSize);
+        uint256 tokenId,
+        address auth
+    ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) {
+        return super._update(to, tokenId, auth);
     }
 
-    function _afterTokenTransfer(
-        address from,
-        address to,
-        uint256 firstTokenId,
-        uint256 batchSize
-    ) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) {
-        super._afterTokenTransfer(from, to, firstTokenId, batchSize);
+    function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Votes) {
+        super._increaseBalance(account, amount);
     }
 }