From 0fc96060402ded3780b3a1865e770992857540a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 4 Dec 2018 17:26:37 -0300 Subject: [PATCH 1/3] Added _transferToken. --- contracts/token/ERC721/ERC721.sol | 21 +++++++++++++-- contracts/token/ERC721/ERC721Enumerable.sol | 29 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d64c97b32a9..7d3d5263b70 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -133,8 +133,7 @@ contract ERC721 is ERC165, IERC721 { require(to != address(0)); _clearApproval(from, tokenId); - _removeTokenFrom(from, tokenId); - _addTokenTo(to, tokenId); + _transferToken(from, to, tokenId); emit Transfer(from, to, tokenId); } @@ -249,6 +248,24 @@ contract ERC721 is ERC165, IERC721 { _tokenOwner[tokenId] = address(0); } + /** + * @dev Internal function to transfer a token ID from the list of a given address to another one. + * Note that this function is left internal to make ERC721Enumerable possible, but is not + * intended to be called by custom derived contracts: in particular, it emits no Transfer event, + * and doesn't clear approvals. + * @param from address representing the previous owner of the given token ID + * @param to address representing the new owner of the given token ID + * @param tokenId uint256 ID of the token to be transferred to the tokens list of the given address + */ + function _transferToken(address from, address to, uint256 tokenId) internal { + require(ownerOf(tokenId) == from); + + _ownedTokensCount[from] = _ownedTokensCount[from].sub(1); + _ownedTokensCount[to] = _ownedTokensCount[to].add(1); + + _tokenOwner[tokenId] = to; + } + /** * @dev Internal function to invoke `onERC721Received` on a target address * The call is not executed if the target address is not a contract diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol index ff8c1b9eecc..367fb5f0b8e 100644 --- a/contracts/token/ERC721/ERC721Enumerable.sol +++ b/contracts/token/ERC721/ERC721Enumerable.sol @@ -110,6 +110,35 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { _ownedTokensIndex[lastToken] = tokenIndex; } + /** + * @dev Internal function to transfer a token ID from the list of a given address to another one. + * This function is internal due to language limitations, see the note in ERC721.sol. + * It is not intended to be called by custom derived contracts: in particular, it emits no Transfer event, + * and doesn't clear approvals. + * @param from address representing the previous owner of the given token ID + * @param to address representing the new owner of the given token ID + * @param tokenId uint256 ID of the token to be transferred from the tokens list of the given address + */ + function _transferToken(address from, address to, uint256 tokenId) internal { + super._transferToken(from, to, tokenId); + + // To prevent a gap in from's array, we store from's last token in the index of the token to delete, and + // then delete the last slot (swap and pop). + uint256 lastTokenIndex = _ownedTokens[from].length.sub(1); + uint256 lastTokenId = _ownedTokens[from][lastTokenIndex]; + + uint256 preTokenIndex = _ownedTokensIndex[tokenId]; + _ownedTokens[from][preTokenIndex] = lastTokenId; + _ownedTokensIndex[lastTokenId] = preTokenIndex; + + // This also deletes the contents at the last position of the array + _ownedTokens[from].length--; + + // Now we add the token to the recipient + _ownedTokens[to].push(tokenId); + _ownedTokensIndex[tokenId] = _ownedTokens[to].length.sub(1); + } + /** * @dev Internal function to mint a new token * Reverts if the given token ID already exists From 02efdfd24315d022cab68a04aaa3f926cb6feb19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 5 Dec 2018 16:48:49 -0300 Subject: [PATCH 2/3] _transferFrom is now usable by derived contracts, abstracted away enumerable behavior. --- contracts/token/ERC721/ERC721.sol | 27 +++--- contracts/token/ERC721/ERC721Enumerable.sol | 102 +++++++++++--------- 2 files changed, 72 insertions(+), 57 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 7d3d5263b70..7194fd88ffe 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -130,12 +130,8 @@ contract ERC721 is ERC165, IERC721 { */ function transferFrom(address from, address to, uint256 tokenId) public { require(_isApprovedOrOwner(msg.sender, tokenId)); - require(to != address(0)); - - _clearApproval(from, tokenId); - _transferToken(from, to, tokenId); - emit Transfer(from, to, tokenId); + _transferFrom(from, to, tokenId); } /** @@ -249,21 +245,24 @@ contract ERC721 is ERC165, IERC721 { } /** - * @dev Internal function to transfer a token ID from the list of a given address to another one. - * Note that this function is left internal to make ERC721Enumerable possible, but is not - * intended to be called by custom derived contracts: in particular, it emits no Transfer event, - * and doesn't clear approvals. - * @param from address representing the previous owner of the given token ID - * @param to address representing the new owner of the given token ID - * @param tokenId uint256 ID of the token to be transferred to the tokens list of the given address - */ - function _transferToken(address from, address to, uint256 tokenId) internal { + * @dev Internal function to transfer ownership of a given token ID to another address. + * As opposed to transferFrom, this imposes no restrictions on msg.sender. + * @param from current owner of the token + * @param to address to receive the ownership of the given token ID + * @param tokenId uint256 ID of the token to be transferred + */ + function _transferFrom(address from, address to, uint256 tokenId) internal { require(ownerOf(tokenId) == from); + require(to != address(0)); + + _clearApproval(from, tokenId); _ownedTokensCount[from] = _ownedTokensCount[from].sub(1); _ownedTokensCount[to] = _ownedTokensCount[to].add(1); _tokenOwner[tokenId] = to; + + emit Transfer(from, to, tokenId); } /** diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol index 367fb5f0b8e..65916164601 100644 --- a/contracts/token/ERC721/ERC721Enumerable.sol +++ b/contracts/token/ERC721/ERC721Enumerable.sol @@ -76,9 +76,8 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { */ function _addTokenTo(address to, uint256 tokenId) internal { super._addTokenTo(to, tokenId); - uint256 length = _ownedTokens[to].length; - _ownedTokens[to].push(tokenId); - _ownedTokensIndex[tokenId] = length; + + _addTokenToOwnerEnumeration(to, tokenId); } /** @@ -92,51 +91,26 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { function _removeTokenFrom(address from, uint256 tokenId) internal { super._removeTokenFrom(from, tokenId); - // To prevent a gap in the array, we store the last token in the index of the token to delete, and - // then delete the last slot. - uint256 tokenIndex = _ownedTokensIndex[tokenId]; - uint256 lastTokenIndex = _ownedTokens[from].length.sub(1); - uint256 lastToken = _ownedTokens[from][lastTokenIndex]; - - _ownedTokens[from][tokenIndex] = lastToken; - // This also deletes the contents at the last position of the array - _ownedTokens[from].length--; - - // Note that this will handle single-element arrays. In that case, both tokenIndex and lastTokenIndex are going to - // be zero. Then we can make sure that we will remove tokenId from the ownedTokens list since we are first swapping - // the lastToken to the first position, and then dropping the element placed in the last position of the list + _removeTokenFromOwnerEnumeration(from, tokenId); + // Since the token is being destroyed, we also clear its index + // TODO(nventuro): 0 is still a valid index, so arguably this isnt really helpful, remove? _ownedTokensIndex[tokenId] = 0; - _ownedTokensIndex[lastToken] = tokenIndex; } /** - * @dev Internal function to transfer a token ID from the list of a given address to another one. - * This function is internal due to language limitations, see the note in ERC721.sol. - * It is not intended to be called by custom derived contracts: in particular, it emits no Transfer event, - * and doesn't clear approvals. - * @param from address representing the previous owner of the given token ID - * @param to address representing the new owner of the given token ID - * @param tokenId uint256 ID of the token to be transferred from the tokens list of the given address - */ - function _transferToken(address from, address to, uint256 tokenId) internal { - super._transferToken(from, to, tokenId); - - // To prevent a gap in from's array, we store from's last token in the index of the token to delete, and - // then delete the last slot (swap and pop). - uint256 lastTokenIndex = _ownedTokens[from].length.sub(1); - uint256 lastTokenId = _ownedTokens[from][lastTokenIndex]; - - uint256 preTokenIndex = _ownedTokensIndex[tokenId]; - _ownedTokens[from][preTokenIndex] = lastTokenId; - _ownedTokensIndex[lastTokenId] = preTokenIndex; - - // This also deletes the contents at the last position of the array - _ownedTokens[from].length--; - - // Now we add the token to the recipient - _ownedTokens[to].push(tokenId); - _ownedTokensIndex[tokenId] = _ownedTokens[to].length.sub(1); + * @dev Internal function to transfer ownership of a given token ID to another address. + * As opposed to transferFrom, this imposes no restrictions on msg.sender. + * @param from current owner of the token + * @param to address to receive the ownership of the given token ID + * @param tokenId uint256 ID of the token to be transferred + */ + function _transferFrom(address from, address to, uint256 tokenId) internal { + super._transferFrom(from, to, tokenId); + + _removeTokenFromOwnerEnumeration(from, tokenId); + + _addTokenToOwnerEnumeration(to, tokenId); } /** @@ -173,4 +147,46 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { _allTokensIndex[tokenId] = 0; _allTokensIndex[lastToken] = tokenIndex; } + + /** + * @dev Private function to add a token to this extension's ownership-tracking data structures. + * @param to address representing the new owner of the given token ID + * @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 newOwnedTokensLength = _ownedTokens[to].push(tokenId); + // No need to use SafeMath since the length after a push cannot be zero + _ownedTokensIndex[tokenId] = newOwnedTokensLength - 1; + } + + /** + * @dev Private function to remove a token from this extension's ownership-tracking data structures. Note that + * while the token is not assigned a new owner, the _ownedTokensIndex mapping is _not_ updated: this allows for + * gas optimizations e.g. when performing a transfer operation (avoiding double writes). + * This has O(1) time complexity, but alters the order of the _ownedTokens array. + * @param from address representing the previous owner of the given token ID + * @param tokenId uint256 ID of the token to be removed from the tokens list of the given address + */ + function _removeTokenFromOwnerEnumeration(address from, uint256 tokenId) private { + // 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 = _ownedTokens[from].length.sub(1); + uint256 lastTokenId = _ownedTokens[from][lastTokenIndex]; + + uint256 tokenIndex = _ownedTokensIndex[tokenId]; + + _ownedTokens[from][tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token + _ownedTokensIndex[lastTokenId] = tokenIndex; // Update the moved token's index + + // Note that this will handle single-element arrays. In that case, both tokenIndex and lastTokenIndex are going + // to be zero. The swap operation will therefore have no effect, but the token _will_ be deleted during the + // 'pop' operation. + + // This also deletes the contents at the last position of the array + _ownedTokens[from].length--; + + // Note that _ownedTokensIndex[tokenId] hasn't been cleared: it still points to the old slot (now occcupied by + // lasTokenId). + } } From 827c3915c7fa7a7935f7cc6c51ae558591212b8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 10 Dec 2018 11:36:10 -0300 Subject: [PATCH 3/3] Removed unnecesary check from _clearApprovals --- contracts/token/ERC721/ERC721.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 7194fd88ffe..e0557ddbf9b 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -212,7 +212,7 @@ contract ERC721 is ERC165, IERC721 { * @param tokenId uint256 ID of the token being burned by the msg.sender */ function _burn(address owner, uint256 tokenId) internal { - _clearApproval(owner, tokenId); + _clearApproval(tokenId); _removeTokenFrom(owner, tokenId); emit Transfer(owner, address(0), tokenId); } @@ -255,7 +255,7 @@ contract ERC721 is ERC165, IERC721 { require(ownerOf(tokenId) == from); require(to != address(0)); - _clearApproval(from, tokenId); + _clearApproval(tokenId); _ownedTokensCount[from] = _ownedTokensCount[from].sub(1); _ownedTokensCount[to] = _ownedTokensCount[to].add(1); @@ -285,12 +285,9 @@ contract ERC721 is ERC165, IERC721 { /** * @dev Private function to clear current approval of a given token ID - * Reverts if the given address is not indeed the owner of the token - * @param owner owner of the token * @param tokenId uint256 ID of the token to be transferred */ - function _clearApproval(address owner, uint256 tokenId) private { - require(ownerOf(tokenId) == owner); + function _clearApproval(uint256 tokenId) private { if (_tokenApprovals[tokenId] != address(0)) { _tokenApprovals[tokenId] = address(0); }