diff --git a/GUIDELINES.md b/GUIDELINES.md index 71f16640524..4b7f5e76e30 100644 --- a/GUIDELINES.md +++ b/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 { diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index e8212e92b0c..343881ff374 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/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 diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d56e3377577..e72c459f300 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/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; } /** diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 4dd6e234c45..10f84826547 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/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 () {