From 4abe9f7a66cec5d808212a869b37c8e4cb4cb20a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 11 Dec 2018 15:49:28 -0300 Subject: [PATCH 1/6] Now only swapping when needed. --- contracts/token/ERC721/ERC721Enumerable.sol | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol index 441624c0271..0f74885e114 100644 --- a/contracts/token/ERC721/ERC721Enumerable.sol +++ b/contracts/token/ERC721/ERC721Enumerable.sol @@ -147,10 +147,10 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { _allTokensIndex[tokenId] = 0; _allTokensIndex[lastToken] = tokenIndex; } - + /** * @dev Gets the list of token IDs of the requested owner - * @param owner address owning the tokens + * @param owner address owning the tokens * @return uint256[] List of token IDs owned by the requested address */ function _tokensOfOwner(address owner) internal view returns (uint256[] storage) { @@ -181,16 +181,15 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { // 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 + // When the token to delete is the last token, the swap operation is unnecessary + if (tokenIndex != lastTokenIndex) { + uint256 lastTokenId = _ownedTokens[from][lastTokenIndex]; - // 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. + _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 + } // This also deletes the contents at the last position of the array _ownedTokens[from].length--; From e0505b7b20e6abacb6c332dfb7745d2dc1a53052 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 11 Dec 2018 16:23:24 -0300 Subject: [PATCH 2/6] Removed _addTokenTo and _removeTokenFrom --- contracts/mocks/ERC721FullMock.sol | 8 ++--- contracts/token/ERC721/ERC721.sol | 38 ++++++-------------- contracts/token/ERC721/ERC721Enumerable.sol | 39 ++++----------------- test/token/ERC721/ERC721Full.test.js | 2 +- 4 files changed, 22 insertions(+), 65 deletions(-) diff --git a/contracts/mocks/ERC721FullMock.sol b/contracts/mocks/ERC721FullMock.sol index 66999755759..d95bc32cd9e 100644 --- a/contracts/mocks/ERC721FullMock.sol +++ b/contracts/mocks/ERC721FullMock.sol @@ -8,7 +8,7 @@ import "../token/ERC721/ERC721Burnable.sol"; /** * @title ERC721FullMock * This mock just provides public functions for setting metadata URI, getting all tokens of an owner, - * checking token existence, removal of a token from an address + * checking token existence, removal of a token from an address */ contract ERC721FullMock is ERC721Full, ERC721Mintable, ERC721MetadataMintable, ERC721Burnable { constructor (string name, string symbol) public ERC721Mintable() ERC721Full(name, symbol) {} @@ -25,7 +25,7 @@ contract ERC721FullMock is ERC721Full, ERC721Mintable, ERC721MetadataMintable, E _setTokenURI(tokenId, uri); } - function removeTokenFrom(address from, uint256 tokenId) public { - _removeTokenFrom(from, tokenId); - } + //function removeTokenFrom(address from, uint256 tokenId) public { + // _removeTokenFrom(from, tokenId); + //} } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index e0557ddbf9b..a0194da348c 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -202,7 +202,11 @@ contract ERC721 is ERC165, IERC721 { */ function _mint(address to, uint256 tokenId) internal { require(to != address(0)); - _addTokenTo(to, tokenId); + require(!_exists(tokenId)); + + _tokenOwner[tokenId] = to; + _ownedTokensCount[to] = _ownedTokensCount[to].add(1); + emit Transfer(address(0), to, tokenId); } @@ -212,36 +216,14 @@ 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(tokenId); - _removeTokenFrom(owner, tokenId); - emit Transfer(owner, address(0), tokenId); - } + require(ownerOf(tokenId) == owner); - /** - * @dev Internal function to add a token ID to the list of a given address - * 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. - * @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 _addTokenTo(address to, uint256 tokenId) internal { - require(_tokenOwner[tokenId] == address(0)); - _tokenOwner[tokenId] = to; - _ownedTokensCount[to] = _ownedTokensCount[to].add(1); - } + _clearApproval(tokenId); - /** - * @dev Internal function to remove a token ID from the list of a given address - * 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 tokenId uint256 ID of the token to be removed from the tokens list of the given address - */ - function _removeTokenFrom(address from, uint256 tokenId) internal { - require(ownerOf(tokenId) == from); - _ownedTokensCount[from] = _ownedTokensCount[from].sub(1); + _ownedTokensCount[owner] = _ownedTokensCount[owner].sub(1); _tokenOwner[tokenId] = address(0); + + emit Transfer(owner, address(0), tokenId); } /** diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol index 0f74885e114..b0357ff643d 100644 --- a/contracts/token/ERC721/ERC721Enumerable.sol +++ b/contracts/token/ERC721/ERC721Enumerable.sol @@ -67,37 +67,6 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { return _allTokens[index]; } - /** - * @dev Internal function to add a token ID to the list of a given address - * 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. - * @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 _addTokenTo(address to, uint256 tokenId) internal { - super._addTokenTo(to, tokenId); - - _addTokenToOwnerEnumeration(to, tokenId); - } - - /** - * @dev Internal function to remove a token ID from the list of a given address - * 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 tokenId uint256 ID of the token to be removed from the tokens list of the given address - */ - function _removeTokenFrom(address from, uint256 tokenId) internal { - super._removeTokenFrom(from, tokenId); - - _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; - } - /** * @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. @@ -122,6 +91,8 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { function _mint(address to, uint256 tokenId) internal { super._mint(to, tokenId); + _addTokenToOwnerEnumeration(to, tokenId); + _allTokensIndex[tokenId] = _allTokens.length; _allTokens.push(tokenId); } @@ -135,6 +106,10 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { function _burn(address owner, uint256 tokenId) internal { super._burn(owner, tokenId); + _removeTokenFromOwnerEnumeration(owner, tokenId); + // Since tokenId will be deleted, we can clear its slot in _ownedTokensIndex to trigger a gas refund + _ownedTokensIndex[tokenId] = 0; + // Reorg all tokens array uint256 tokenIndex = _allTokensIndex[tokenId]; uint256 lastTokenIndex = _allTokens.length.sub(1); @@ -195,6 +170,6 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { _ownedTokens[from].length--; // Note that _ownedTokensIndex[tokenId] hasn't been cleared: it still points to the old slot (now occcupied by - // lasTokenId). + // lasTokenId, or just over the end of the array if the token was the last one). } } diff --git a/test/token/ERC721/ERC721Full.test.js b/test/token/ERC721/ERC721Full.test.js index 3870a95f671..848fa61eb2d 100644 --- a/test/token/ERC721/ERC721Full.test.js +++ b/test/token/ERC721/ERC721Full.test.js @@ -70,7 +70,7 @@ contract('ERC721Full', function ([ }); }); - describe('removeTokenFrom', function () { + describe.skip('removeTokenFrom', function () { it('reverts if the correct owner is not passed', async function () { await shouldFail.reverting( this.token.removeTokenFrom(anyone, firstTokenId, { from: owner }) From 55bdedab651bb6a15cc300b62d676a078653182c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 11 Dec 2018 16:24:45 -0300 Subject: [PATCH 3/6] Removed removeTokenFrom test. --- contracts/mocks/ERC721FullMock.sol | 4 ---- test/token/ERC721/ERC721Full.test.js | 31 ---------------------------- 2 files changed, 35 deletions(-) diff --git a/contracts/mocks/ERC721FullMock.sol b/contracts/mocks/ERC721FullMock.sol index d95bc32cd9e..d917e8f6382 100644 --- a/contracts/mocks/ERC721FullMock.sol +++ b/contracts/mocks/ERC721FullMock.sol @@ -24,8 +24,4 @@ contract ERC721FullMock is ERC721Full, ERC721Mintable, ERC721MetadataMintable, E function setTokenURI(uint256 tokenId, string uri) public { _setTokenURI(tokenId, uri); } - - //function removeTokenFrom(address from, uint256 tokenId) public { - // _removeTokenFrom(from, tokenId); - //} } diff --git a/test/token/ERC721/ERC721Full.test.js b/test/token/ERC721/ERC721Full.test.js index 848fa61eb2d..a7642184faf 100644 --- a/test/token/ERC721/ERC721Full.test.js +++ b/test/token/ERC721/ERC721Full.test.js @@ -23,7 +23,6 @@ contract('ERC721Full', function ([ owner, newOwner, another, - anyone, ] = accounts; beforeEach(async function () { @@ -70,36 +69,6 @@ contract('ERC721Full', function ([ }); }); - describe.skip('removeTokenFrom', function () { - it('reverts if the correct owner is not passed', async function () { - await shouldFail.reverting( - this.token.removeTokenFrom(anyone, firstTokenId, { from: owner }) - ); - }); - - context('once removed', function () { - beforeEach(async function () { - await this.token.removeTokenFrom(owner, firstTokenId, { from: owner }); - }); - - it('has been removed', async function () { - await shouldFail.reverting(this.token.tokenOfOwnerByIndex(owner, 1)); - }); - - it('adjusts token list', async function () { - (await this.token.tokenOfOwnerByIndex(owner, 0)).toNumber().should.be.equal(secondTokenId); - }); - - it('adjusts owner count', async function () { - (await this.token.balanceOf(owner)).toNumber().should.be.equal(1); - }); - - it('does not adjust supply', async function () { - (await this.token.totalSupply()).toNumber().should.be.equal(2); - }); - }); - }); - describe('metadata', function () { const sampleUri = 'mock://mytoken'; From b4dae05be1bdf0f64e9e27943e2673e94d064ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 12 Dec 2018 13:59:27 -0300 Subject: [PATCH 4/6] Added tests for ERC721 _mint and _burn --- contracts/mocks/ERC721Mock.sol | 4 ++ test/token/ERC721/ERC721.test.js | 107 +++++++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/contracts/mocks/ERC721Mock.sol b/contracts/mocks/ERC721Mock.sol index 2a429fbae97..f953d944d41 100644 --- a/contracts/mocks/ERC721Mock.sol +++ b/contracts/mocks/ERC721Mock.sol @@ -11,6 +11,10 @@ contract ERC721Mock is ERC721 { _mint(to, tokenId); } + function burn(address owner, uint256 tokenId) public { + _burn(owner, tokenId); + } + function burn(uint256 tokenId) public { _burn(tokenId); } diff --git a/test/token/ERC721/ERC721.test.js b/test/token/ERC721/ERC721.test.js index 87e895f143b..8ae1c05eb15 100644 --- a/test/token/ERC721/ERC721.test.js +++ b/test/token/ERC721/ERC721.test.js @@ -1,13 +1,112 @@ -const { shouldBehaveLikeERC721 } = require('./ERC721.behavior'); +require('../../helpers/setup'); +const { ZERO_ADDRESS } = require('../../helpers/constants'); +const expectEvent = require('../../helpers/expectEvent'); +const send = require('../../helpers/send'); +const shouldFail = require('../../helpers/shouldFail'); +const { shouldBehaveLikeERC721 } = require('./ERC721.behavior'); const ERC721Mock = artifacts.require('ERC721Mock.sol'); -require('../../helpers/setup'); - -contract('ERC721', function ([_, creator, ...accounts]) { +contract('ERC721', function ([_, creator, tokenOwner, anyone, ...accounts]) { beforeEach(async function () { this.token = await ERC721Mock.new({ from: creator }); }); shouldBehaveLikeERC721(creator, creator, accounts); + + describe('internal functions', function () { + const tokenId = 5042; + + describe('_mint(address, uint256)', function () { + it('reverts with a null destination address', async function () { + await shouldFail.reverting(this.token.mint(ZERO_ADDRESS, tokenId)); + }); + + context('with minted token', async function () { + beforeEach(async function () { + ({ logs: this.logs } = await this.token.mint(tokenOwner, tokenId)); + }); + + it('emits a Transfer event', function () { + expectEvent.inLogs(this.logs, 'Transfer', { from: ZERO_ADDRESS, to: tokenOwner, tokenId }); + }); + + it('creates the token', async function () { + (await this.token.balanceOf(tokenOwner)).should.be.bignumber.equal(1); + (await this.token.ownerOf(tokenId)).should.equal(tokenOwner); + }); + + it('reverts when adding a token id that already exists', async function () { + await shouldFail.reverting(this.token.mint(tokenOwner, tokenId)); + }); + }); + }); + + describe('_burn(address, uint256)', function () { + it('reverts when burning a non-existent token id', async function () { + await shouldFail.reverting(send.transaction(this.token, 'burn', 'address,uint256', [tokenOwner, tokenId])); + }); + + context('with minted token', function () { + beforeEach(async function () { + await this.token.mint(tokenOwner, tokenId); + }); + + it('reverts when the account is not the owner', async function () { + await shouldFail.reverting(send.transaction(this.token, 'burn', 'address,uint256', [anyone, tokenId])); + }); + + context('with burnt token', function () { + beforeEach(async function () { + ({ logs: this.logs } = + await send.transaction(this.token, 'burn', 'address,uint256', [tokenOwner, tokenId])); + }); + + it('emits a Transfer event', function () { + expectEvent.inLogs(this.logs, 'Transfer', { from: tokenOwner, to: ZERO_ADDRESS, tokenId }); + }); + + it('deletes the token', async function () { + (await this.token.balanceOf(tokenOwner)).should.be.bignumber.equal(0); + await shouldFail.reverting(this.token.ownerOf(tokenId)); + }); + + it('reverts when burning a token id that has been deleted', async function () { + await shouldFail.reverting(send.transaction(this.token, 'burn', 'address,uint256', [tokenOwner, tokenId])); + }); + }); + }); + }); + + describe('_burn(uint256)', function () { + it('reverts when burning a non-existent token id', async function () { + await shouldFail.reverting(send.transaction(this.token, 'burn', 'uint256', [tokenId])); + }); + + context('with minted token', function () { + beforeEach(async function () { + await this.token.mint(tokenOwner, tokenId); + }); + + context('with burnt token', function () { + beforeEach(async function () { + ({ logs: this.logs } = await send.transaction(this.token, 'burn', 'uint256', [tokenId])); + }); + + it('emits a Transfer event', function () { + expectEvent.inLogs(this.logs, 'Transfer', { from: tokenOwner, to: ZERO_ADDRESS, tokenId }); + }); + + it('deletes the token', async function () { + (await this.token.balanceOf(tokenOwner)).should.be.bignumber.equal(0); + await shouldFail.reverting(this.token.ownerOf(tokenId)); + }); + + it('reverts when burning a token id that has been deleted', async function () { + await shouldFail.reverting(send.transaction(this.token, 'burn', 'uint256', [tokenId])); + }); + }); + }); + }); + }); }); From 5c7f09b449ce65eb978676d7c9d6e3b6382c0b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 12 Dec 2018 15:48:16 -0300 Subject: [PATCH 5/6] _burn now uses the same swap and pop mechanism as _removeFromOwner --- contracts/token/ERC721/ERC721Enumerable.sol | 54 +++++++++++++++------ 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol index 326a94bc4e0..dff786c65c5 100644 --- a/contracts/token/ERC721/ERC721Enumerable.sol +++ b/contracts/token/ERC721/ERC721Enumerable.sol @@ -93,8 +93,7 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { _addTokenToOwnerEnumeration(to, tokenId); - _allTokensIndex[tokenId] = _allTokens.length; - _allTokens.push(tokenId); + _addTokenToAllTokensEnumeration(tokenId); } /** @@ -111,17 +110,7 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { // Since tokenId will be deleted, we can clear its slot in _ownedTokensIndex to trigger a gas refund _ownedTokensIndex[tokenId] = 0; - // Reorg all tokens array - uint256 tokenIndex = _allTokensIndex[tokenId]; - uint256 lastTokenIndex = _allTokens.length.sub(1); - uint256 lastToken = _allTokens[lastTokenIndex]; - - _allTokens[tokenIndex] = lastToken; - _allTokens[lastTokenIndex] = 0; - - _allTokens.length--; - _allTokensIndex[tokenId] = 0; - _allTokensIndex[lastToken] = tokenIndex; + _removeTokenFromAllTokensEnumeration(tokenId); } /** @@ -139,9 +128,17 @@ contract ERC721Enumerable is ERC165, 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 newOwnedTokensLength = _ownedTokens[to].push(tokenId); - // No need to use SafeMath since the length after a push cannot be zero - _ownedTokensIndex[tokenId] = newOwnedTokensLength - 1; + _ownedTokensIndex[tokenId] = _ownedTokens[to].length; + _ownedTokens[to].push(tokenId); + } + + /** + * @dev Private function to add a token to this extension's token tracking data structures. + * @param tokenId uint256 ID of the token to be added to the tokens list + */ + function _addTokenToAllTokensEnumeration(uint256 tokenId) private { + _allTokensIndex[tokenId] = _allTokens.length; + _allTokens.push(tokenId); } /** @@ -173,4 +170,29 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { // Note that _ownedTokensIndex[tokenId] hasn't been cleared: it still points to the old slot (now occcupied by // lasTokenId, or just over the end of the array if the token was the last one). } + + /** + * @dev Private function to remove a token from this extension's token tracking data structures. + * This has O(1) time complexity, but alters the order of the _allTokens array. + * @param tokenId uint256 ID of the token to be removed from the tokens list + */ + function _removeTokenFromAllTokensEnumeration(uint256 tokenId) private { + // To prevent a gap in the 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 = _allTokens.length.sub(1); + uint256 tokenIndex = _allTokensIndex[tokenId]; + + // When the token to delete is the last token, the swap operation is unnecessary + if (tokenIndex != lastTokenIndex) { + uint256 lastTokenId = _allTokens[lastTokenIndex]; + + _allTokens[tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token + _allTokensIndex[lastTokenId] = tokenIndex; // Update the moved token's index + } + + // This also deletes the contents at the last position of the array + _allTokens.length--; + _allTokensIndex[tokenId] = 0; + } } From 6255db8d106bd5e3ba92905b6536d6cc263952ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 12 Dec 2018 18:16:02 -0300 Subject: [PATCH 6/6] Gas optimization on burn --- contracts/token/ERC721/ERC721Enumerable.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/token/ERC721/ERC721Enumerable.sol b/contracts/token/ERC721/ERC721Enumerable.sol index dff786c65c5..a543c75f7a0 100644 --- a/contracts/token/ERC721/ERC721Enumerable.sol +++ b/contracts/token/ERC721/ERC721Enumerable.sol @@ -183,13 +183,13 @@ contract ERC721Enumerable is ERC165, ERC721, IERC721Enumerable { uint256 lastTokenIndex = _allTokens.length.sub(1); uint256 tokenIndex = _allTokensIndex[tokenId]; - // When the token to delete is the last token, the swap operation is unnecessary - if (tokenIndex != lastTokenIndex) { - uint256 lastTokenId = _allTokens[lastTokenIndex]; + // When the token to delete is the last token, the swap operation is unnecessary. However, since this occurs so + // rarely (when the last minted token is burnt) that we still do the swap here to avoid the gas cost of adding + // an 'if' statement (like in _removeTokenFromOwnerEnumeration) + uint256 lastTokenId = _allTokens[lastTokenIndex]; - _allTokens[tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token - _allTokensIndex[lastTokenId] = tokenIndex; // Update the moved token's index - } + _allTokens[tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token + _allTokensIndex[lastTokenId] = tokenIndex; // Update the moved token's index // This also deletes the contents at the last position of the array _allTokens.length--;