diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index a2ee8199a..9219ebfca 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -27,25 +27,11 @@ 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); - } - - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { + return super._update(to, tokenId, constraints); } } diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 4aae388e0..ed68ecce5 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -39,26 +39,12 @@ 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); - } - - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { + return super._update(to, tokenId, constraints); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 21ed95813..207d2af5e 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -151,12 +151,14 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev See {IERC721-transferFrom}. */ - function transferFrom(address from, address to, uint256 tokenId) public virtual { - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); + function transferFrom(address from, address to, uint256 tokenId) public virtual override { + if (to == address(0)) { + revert ERC721InvalidReceiver(address(0)); + } + address owner = _update(to, tokenId, _constraintApprovedOrOwner); + if (owner != from) { + revert ERC721IncorrectOwner(from, tokenId, owner); } - - _transfer(from, to, tokenId); } /** @@ -257,6 +259,46 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } + /** + * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner + * (or `to`) is the zero address. + * + * The `constraints` argument is used to specify constraints, and eventually revert. For example this can be used + * to ensure that the current owner is what was expected. + + * All customizations to transfers, mints, and burns should be done by overriding this function. + * + * Emits a {Transfer} event. + */ + function _update( + address to, + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual returns (address) { + address from = _ownerOf(tokenId); + + constraints(from, to, tokenId); + + if (from != address(0)) { + delete _tokenApprovals[tokenId]; + unchecked { + _balances[from] -= 1; + } + } + + if (to != address(0)) { + unchecked { + _balances[to] += 1; + } + } + + _owners[tokenId] = to; + + emit Transfer(from, to, tokenId); + + return from; + } + /** * @dev Mints `tokenId` and transfers it to `to`. * @@ -269,34 +311,11 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _mint(address to, uint256 tokenId) internal virtual { + function _mint(address to, uint256 tokenId) internal { if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - if (_exists(tokenId)) { - revert ERC721InvalidSender(address(0)); - } - - _beforeTokenTransfer(address(0), to, tokenId, 1); - - // Check that tokenId was not minted by `_beforeTokenTransfer` hook - if (_exists(tokenId)) { - revert ERC721InvalidSender(address(0)); - } - - unchecked { - // Will not overflow unless all 2**256 token ids are minted to the same owner. - // Given that tokens are minted one by one, it is impossible in practice that - // this ever happens. Might change if we allow batch minting. - // The ERC fails to describe this case. - _balances[to] += 1; - } - - _owners[tokenId] = to; - - emit Transfer(address(0), to, tokenId); - - _afterTokenTransfer(address(0), to, tokenId, 1); + _update(to, tokenId, _constraintNotMinted); } /** @@ -310,26 +329,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _burn(uint256 tokenId) internal virtual { - address owner = ownerOf(tokenId); - - _beforeTokenTransfer(owner, address(0), tokenId, 1); - - // Update ownership in case tokenId was transferred by `_beforeTokenTransfer` hook - owner = ownerOf(tokenId); - - // Clear approvals - delete _tokenApprovals[tokenId]; - - // Decrease balance with checked arithmetic, because an `ownerOf` override may - // invalidate the assumption that `_balances[from] >= 1`. - _balances[owner] -= 1; - - delete _owners[tokenId]; - - emit Transfer(owner, address(0), tokenId); - - _afterTokenTransfer(owner, address(0), tokenId, 1); + function _burn(uint256 tokenId) internal { + _update(address(0), tokenId, _constraintMinted); } /** @@ -343,41 +344,14 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _transfer(address from, address to, uint256 tokenId) internal virtual { - address owner = ownerOf(tokenId); - if (owner != from) { - revert ERC721IncorrectOwner(from, tokenId, owner); - } + function _transfer(address from, address to, uint256 tokenId) internal { if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - - _beforeTokenTransfer(from, to, tokenId, 1); - - // Check that tokenId was not transferred by `_beforeTokenTransfer` hook - owner = ownerOf(tokenId); + address owner = _update(to, tokenId, _constraintMinted); if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } - - // Clear approvals from the previous owner - delete _tokenApprovals[tokenId]; - - // Decrease balance with checked arithmetic, because an `ownerOf` override may - // invalidate the assumption that `_balances[from] >= 1`. - _balances[from] -= 1; - - unchecked { - // `_balances[to]` could overflow in the conditions described in `_mint`. That would require - // all 2**256 token ids to be minted, which in practice is impossible. - _balances[to] += 1; - } - - _owners[tokenId] = to; - - emit Transfer(from, to, tokenId); - - _afterTokenTransfer(from, to, tokenId, 1); } /** @@ -412,6 +386,34 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } + /** + * @dev Contraint: revert if token is already minted + */ + function _constraintNotMinted(address from, address, uint256) internal pure { + if (from != address(0)) { + revert ERC721InvalidSender(address(0)); + } + } + + /** + * @dev Contraint: revert if token is not yet minted + */ + function _constraintMinted(address from, address, uint256 tokenId) internal pure { + if (from == address(0)) { + revert ERC721NonexistentToken(tokenId); + } + } + + /** + * @dev Contraint: check that the caller is the current owner, or approved by the current owner + */ + function _constraintApprovedOrOwner(address owner, address, uint256 tokenId) internal view { + address spender = _msgSender(); + if (spender != owner && !isApprovedForAll(owner, spender) && getApproved(tokenId) != spender) { + revert ERC721InsufficientApproval(_msgSender(), tokenId); + } + } + /** * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address. * The call is not executed if the target address is not a contract. @@ -446,38 +448,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } - /** - * @dev Hook that is called before any token transfer. This includes minting and burning. If {ERC721Consecutive} is - * used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1. - * - * Calling conditions: - * - * - When `from` and `to` are both non-zero, ``from``'s tokens will be transferred to `to`. - * - When `from` is zero, the tokens will be minted for `to`. - * - When `to` is zero, ``from``'s tokens will be burned. - * - `from` and `to` are never both zero. - * - `batchSize` is non-zero. - * - * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. - */ - function _beforeTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {} - - /** - * @dev Hook that is called after any token transfer. This includes minting and burning. If {ERC721Consecutive} is - * used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1. - * - * Calling conditions: - * - * - When `from` and `to` are both non-zero, ``from``'s tokens were transferred to `to`. - * - When `from` is zero, the tokens were minted for `to`. - * - When `to` is zero, ``from``'s tokens were burned. - * - `from` and `to` are never both zero. - * - `batchSize` is non-zero. - * - * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. - */ - function _afterTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {} - /** * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. * diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index f1308cdab..621b3676a 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -120,9 +120,6 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { revert ERC721ExceededMaxBatchMint(batchSize, maxBatchSize); } - // hook before - _beforeTokenTransfer(address(0), to, next, batchSize); - // push an ownership checkpoint & emit event uint96 last = next + batchSize - 1; _sequentialOwnership.push(last, uint160(to)); @@ -132,49 +129,39 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { __unsafe_increaseBalance(to, batchSize); emit ConsecutiveTransfer(next, last, address(0), to); - - // hook after - _afterTokenTransfer(address(0), to, next, batchSize); } return next; } /** - * @dev See {ERC721-_mint}. Override version that restricts normal minting to after construction. + * @dev See {ERC721-_update}. Override version that restricts normal minting to after construction. * - * WARNING: Using {ERC721Consecutive} prevents using {_mint} during construction in favor of {_mintConsecutive}. - * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. + * Warning: Using {ERC721Consecutive} prevents minting during construction in favor of {_mintConsecutive}. + * After construction, {_mintConsecutive} is no longer available and minting through {_update} becomes available. */ - function _mint(address to, uint256 tokenId) internal virtual override { - if (address(this).code.length == 0) { + function _update( + address to, + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); + + // only mint after construction + if (from == address(0) && address(this).code.length == 0) { revert ERC721ForbiddenMint(); } - super._mint(to, tokenId); - } - /** - * @dev See {ERC721-_afterTokenTransfer}. Burning of tokens that have been sequentially minted must be explicit. - */ - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { + // record burn if ( to == address(0) && // if we burn - firstTokenId >= _firstConsecutiveId() && - firstTokenId < _nextConsecutiveId() && - !_sequentialBurn.get(firstTokenId) - ) // and the token was never marked as burnt - { - if (batchSize != 1) { - revert ERC721ForbiddenBatchBurn(); - } - _sequentialBurn.set(firstTokenId); + tokenId < _nextConsecutiveId() && // and the tokenId was minted in a batch + !_sequentialBurn.get(tokenId) // and the token was never marked as burnt + ) { + _sequentialBurn.set(tokenId); } - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + + return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 18e2ba5d6..36a8e5524 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -73,22 +73,10 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } /** - * @dev See {ERC721-_beforeTokenTransfer}. + * @dev See {ERC721-_update}. */ - function _beforeTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - - if (batchSize > 1) { - // Will only trigger during construction. Batch transferring (minting) is not available afterwards. - revert ERC721EnumerableForbiddenBatchMint(); - } - - uint256 tokenId = firstTokenId; + function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); @@ -97,9 +85,11 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } if (to == address(0)) { _removeTokenFromAllTokensEnumeration(tokenId); - } else if (to != from) { + } else if (from != to) { _addTokenToOwnerEnumeration(to, tokenId); } + + return from; } /** @@ -108,7 +98,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { * @param tokenId uint256 ID of the token to be added to the tokens list of the given address */ function _addTokenToOwnerEnumeration(address to, uint256 tokenId) private { - uint256 length = balanceOf(to); + uint256 length = balanceOf(to) - 1; _ownedTokens[to][length] = tokenId; _ownedTokensIndex[tokenId] = length; } @@ -134,7 +124,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { // To prevent a gap in from's tokens array, we store the last token in the index of the token to delete, and // then delete the last slot (swap and pop). - uint256 lastTokenIndex = balanceOf(from) - 1; + uint256 lastTokenIndex = balanceOf(from); uint256 tokenIndex = _ownedTokensIndex[tokenId]; // When the token to delete is the last token, the swap operation is unnecessary diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index a9472c5dc..47990f407 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -21,20 +21,18 @@ import "../../../security/Pausable.sol"; */ abstract contract ERC721Pausable is ERC721, Pausable { /** - * @dev See {ERC721-_beforeTokenTransfer}. + * @dev See {ERC721-_update}. * * Requirements: * * - the contract must not be paused. */ - function _beforeTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { _requireNotPaused(); + return super._update(to, tokenId, constraints); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 7d6ef6c04..8b16cc9be 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,10 +29,15 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { } /** - * @dev See {ERC721-_burn}. This override additionally clears the royalty information for the token. + * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); - _resetTokenRoyalty(tokenId); + function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); + + if (to == address(0)) { + _resetTokenRoyalty(tokenId); + } + + return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index ae625fc28..53f118b07 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -62,15 +62,17 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { } /** - * @dev See {ERC721-_burn}. This override additionally checks to see if a + * @dev See {ERC721-_update}. This override additionally checks to see if a * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); + function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); - if (bytes(_tokenURIs[tokenId]).length != 0) { + if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } + + return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 89b2e073c..eea2fd515 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -18,18 +18,18 @@ import "../../../governance/utils/Votes.sol"; */ abstract contract ERC721Votes is ERC721, Votes { /** - * @dev See {ERC721-_afterTokenTransfer}. Adjusts votes when tokens are transferred. + * @dev See {ERC721-_update}. Adjusts votes when tokens are transferred. * * Emits a {IVotes-DelegateVotesChanged} event. */ - function _afterTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - _transferVotingUnits(from, to, batchSize); - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); + _transferVotingUnits(from, to, 1); + return from; } /**