Follow _approve overrides in ERC721._update (#4552)

GUIDELINES.md

@@ -95,8 +95,18 @@ In addition to the official Solidity Style Guide we have a number of other conve
   }
   ```
 
-* Events should be emitted immediately after the state change that they
-  represent, and should be named in the past tense.
+* Functions should be declared virtual, with few exceptions listed below. The
+  contract logic should be written considering that these functions may be
+  overridden by developers, e.g. getting a value using an internal getter rather
+  than reading directly from a state variable.
+
+  If function A is an "alias" of function B, i.e. it invokes function B without
+  significant additional logic, then function A should not be virtual so that
+  any user overrides are implemented on B, preventing inconsistencies.
+
+* Events should generally be emitted immediately after the state change that they
+  represent, and should be named in the past tense. Some exceptions may be made for gas
+  efficiency if the result doesn't affect observable ordering of events.
 
   ```solidity
   function _burn(address who, uint256 value) internal {

contracts/token/ERC20/ERC20.sol

@@ -313,13 +313,15 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors {
      *
      * - `owner` cannot be the zero address.
      * - `spender` cannot be the zero address.
+     *
+     * Overrides to this logic should be done to the variant with an additional `bool emitEvent` argument.
      */
-    function _approve(address owner, address spender, uint256 value) internal virtual {
+    function _approve(address owner, address spender, uint256 value) internal {
         _approve(owner, spender, value, true);
     }
 
     /**
-     * @dev Alternative version of {_approve} with an optional flag that can enable or disable the Approval event.
+     * @dev Variant of {_approve} with an optional flag to enable or disable the {Approval} event.
      *
      * By default (when calling {_approve}) the flag is set to true. On the other hand, approval changes made by
      * `_spendAllowance` during the `transferFrom` operation set the flag to false. This saves gas by not emitting any

contracts/token/ERC721/ERC721.sol

@@ -251,7 +251,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
 
         // Execute the update
         if (from != address(0)) {
-            delete _tokenApprovals[tokenId];
+            // Clear approval. No need to re-authorize or emit the Approval event
+            _approve(address(0), tokenId, address(0), false);
+
             unchecked {
                 _balances[from] -= 1;
             }
@@ -395,19 +397,33 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
      * either the owner of the token, or approved to operate on all tokens held by this owner.
      *
      * Emits an {Approval} event.
+     *
+     * Overrides to this logic should be done to the variant with an additional `bool emitEvent` argument.
+     */
+    function _approve(address to, uint256 tokenId, address auth) internal {
+        _approve(to, tokenId, auth, true);
+    }
+
+    /**
+     * @dev Variant of `_approve` with an optional flag to enable or disable the {Approval} event. The event is not
+     * emitted in the context of transfers.
      */
-    function _approve(address to, uint256 tokenId, address auth) internal virtual returns (address) {
-        address owner = ownerOf(tokenId);
+    function _approve(address to, uint256 tokenId, address auth, bool emitEvent) internal virtual {
+        // Avoid reading the owner unless necessary
+        if (emitEvent || auth != address(0)) {
+            address owner = ownerOf(tokenId);
 
-        // We do not use _isAuthorized because single-token approvals should not be able to call approve
-        if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) {
-            revert ERC721InvalidApprover(auth);
+            // We do not use _isAuthorized because single-token approvals should not be able to call approve
+            if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) {
+                revert ERC721InvalidApprover(auth);
+            }
+
+            if (emitEvent) {
+                emit Approval(owner, to, tokenId);
+            }
         }
 
         _tokenApprovals[tokenId] = to;
-        emit Approval(owner, to, tokenId);
-
-        return owner;
     }
 
     /**

test/token/ERC721/ERC721.behavior.js

@@ -87,8 +87,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper
           expectEvent(receipt, 'Transfer', { from: owner, to: this.toWhom, tokenId: tokenId });
         });
 
-        it('clears the approval for the token ID', async function () {
+        it('clears the approval for the token ID with no event', async function () {
           expect(await this.token.getApproved(tokenId)).to.be.equal(ZERO_ADDRESS);
+          expectEvent.notEmitted(receipt, 'Approval');
         });
 
         it('adjusts owners balances', async function () {