From 23afc74b59ca536661638b09111460a8722b6458 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 17 Jan 2018 18:26:49 -0300 Subject: [PATCH] Address feedback comments for ERC721 --- contracts/mocks/ERC721TokenMock.sol | 8 +++---- contracts/token/ERC721Token.sol | 37 ++++++++++++++++------------- test/token/ERC721Token.test.js | 28 +++++++++++----------- 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/contracts/mocks/ERC721TokenMock.sol b/contracts/mocks/ERC721TokenMock.sol index 394d408f4..42b12cc29 100644 --- a/contracts/mocks/ERC721TokenMock.sol +++ b/contracts/mocks/ERC721TokenMock.sol @@ -9,11 +9,11 @@ import "../token/ERC721Token.sol"; contract ERC721TokenMock is ERC721Token { function ERC721TokenMock() ERC721Token() public { } - function publicMint(address _to, uint256 _tokenId) public { - super.mint(_to, _tokenId); + function mint(address _to, uint256 _tokenId) public { + super._mint(_to, _tokenId); } - function publicBurn(uint256 _tokenId) public { - super.burn(_tokenId); + function burn(uint256 _tokenId) public { + super._burn(_tokenId); } } diff --git a/contracts/token/ERC721Token.sol b/contracts/token/ERC721Token.sol index 8e30fba6e..0b86886ee 100644 --- a/contracts/token/ERC721Token.sol +++ b/contracts/token/ERC721Token.sol @@ -108,7 +108,7 @@ contract ERC721Token is ERC721 { * @param _tokenId uint256 ID of the token being claimed by the msg.sender */ function takeOwnership(uint256 _tokenId) public { - require(isApprovedFor(_tokenId)); + require(isApprovedFor(msg.sender, _tokenId)); clearApprovalAndTransfer(ownerOf(_tokenId), msg.sender, _tokenId); } @@ -117,7 +117,7 @@ contract ERC721Token is ERC721 { * @param _to The address that will own the minted token * @param _tokenId uint256 ID of the token to be minted by the msg.sender */ - function mint(address _to, uint256 _tokenId) internal { + function _mint(address _to, uint256 _tokenId) internal { require(_to != address(0)); addToken(_to, _tokenId); Transfer(0x0, _to, _tokenId); @@ -127,7 +127,7 @@ contract ERC721Token is ERC721 { * @dev Burns a specific token * @param _tokenId uint256 ID of the token being burned by the msg.sender */ - function burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal { + function _burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal { if (approvedFor(_tokenId) != 0) { clearApproval(msg.sender, _tokenId); } @@ -135,6 +135,17 @@ contract ERC721Token is ERC721 { Transfer(msg.sender, 0x0, _tokenId); } + /** + * @dev Tells whether the msg.sender is approved for the given token ID or not + * This function is not private so it can be extended in further implementations like the operatable ERC721 + * @param _owner address of the owner to query the approval of + * @param _tokenId uint256 ID of the token to query the approval of + * @return bool whether the msg.sender is approved for the given token ID or not + */ + function isApprovedFor(address _owner, uint256 _tokenId) internal view returns (bool) { + return approvedFor(_tokenId) == _owner; + } + /** * @dev Internal function to clear current approval and transfer the ownership of a given token ID * @param _from address which you want to send tokens from @@ -156,7 +167,7 @@ contract ERC721Token is ERC721 { * @dev Internal function to clear current approval of a given token ID * @param _tokenId uint256 ID of the token to be transferred */ - function clearApproval(address _owner, uint256 _tokenId) internal { + function clearApproval(address _owner, uint256 _tokenId) private { require(ownerOf(_tokenId) == _owner); tokenApprovals[_tokenId] = 0; Approval(_owner, 0, _tokenId); @@ -167,7 +178,7 @@ contract ERC721Token is ERC721 { * @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 addToken(address _to, uint256 _tokenId) internal { + function addToken(address _to, uint256 _tokenId) private { require(tokenOwner[_tokenId] == address(0)); tokenOwner[_tokenId] = _to; uint256 length = balanceOf(_to); @@ -181,8 +192,7 @@ contract ERC721Token is ERC721 { * @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 removeToken(address _from, uint256 _tokenId) internal { - require(balanceOf(_from) > 0); + function removeToken(address _from, uint256 _tokenId) private { require(ownerOf(_tokenId) == _from); uint256 tokenIndex = ownedTokensIndex[_tokenId]; @@ -192,18 +202,13 @@ contract ERC721Token is ERC721 { tokenOwner[_tokenId] = 0; ownedTokens[_from][tokenIndex] = lastToken; ownedTokens[_from][lastTokenIndex] = 0; + // 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 + ownedTokens[_from].length--; ownedTokensIndex[_tokenId] = 0; ownedTokensIndex[lastToken] = tokenIndex; totalTokens = totalTokens.sub(1); } - - /** - * @dev Tells whether the msg.sender is approved for the given token ID or not - * @param _tokenId uint256 ID of the token to query the approval of - * @return bool whether the msg.sender is approved for the given token ID or not - */ - function isApprovedFor(uint256 _tokenId) internal view returns (bool) { - return approvedFor(_tokenId) == msg.sender; - } } diff --git a/test/token/ERC721Token.test.js b/test/token/ERC721Token.test.js index d1ec5c81c..9d03f944b 100644 --- a/test/token/ERC721Token.test.js +++ b/test/token/ERC721Token.test.js @@ -17,8 +17,8 @@ contract('ERC721Token', accounts => { beforeEach(async function () { token = await ERC721Token.new({ from: _creator }); - await token.publicMint(_creator, _firstTokenId, { from: _creator }); - await token.publicMint(_creator, _secondTokenId, { from: _creator }); + await token.mint(_creator, _firstTokenId, { from: _creator }); + await token.mint(_creator, _secondTokenId, { from: _creator }); }); describe('totalSupply', function () { @@ -73,7 +73,7 @@ contract('ERC721Token', accounts => { it('mints the given token ID to the given address', async function () { const previousBalance = await token.balanceOf(to); - await token.publicMint(to, tokenId); + await token.mint(to, tokenId); const owner = await token.ownerOf(tokenId); owner.should.be.equal(to); @@ -83,7 +83,7 @@ contract('ERC721Token', accounts => { }); it('adds that token to the token list of the owner', async function () { - await token.publicMint(to, tokenId); + await token.mint(to, tokenId); const tokens = await token.tokensOf(to); tokens.length.should.be.equal(1); @@ -91,7 +91,7 @@ contract('ERC721Token', accounts => { }); it('emits a transfer event', async function () { - const { logs } = await token.publicMint(to, tokenId); + const { logs } = await token.mint(to, tokenId); logs.length.should.be.equal(1); logs[0].event.should.be.eq('Transfer'); @@ -105,7 +105,7 @@ contract('ERC721Token', accounts => { const to = ZERO_ADDRESS; it('reverts', async function () { - await assertRevert(token.publicMint(to, tokenId)); + await assertRevert(token.mint(to, tokenId)); }); }); }); @@ -114,7 +114,7 @@ contract('ERC721Token', accounts => { const tokenId = _firstTokenId; it('reverts', async function () { - await assertRevert(token.publicMint(accounts[1], tokenId)); + await assertRevert(token.mint(accounts[1], tokenId)); }); }); }); @@ -129,7 +129,7 @@ contract('ERC721Token', accounts => { it('burns the given token ID and adjusts the balance of the owner', async function () { const previousBalance = await token.balanceOf(sender); - await token.publicBurn(tokenId, { from: sender }); + await token.burn(tokenId, { from: sender }); await assertRevert(token.ownerOf(tokenId)); const balance = await token.balanceOf(sender); @@ -137,7 +137,7 @@ contract('ERC721Token', accounts => { }); it('removes that token from the token list of the owner', async function () { - await token.publicBurn(tokenId, { from: sender }); + await token.burn(tokenId, { from: sender }); const tokens = await token.tokensOf(sender); tokens.length.should.be.equal(1); @@ -145,7 +145,7 @@ contract('ERC721Token', accounts => { }); it('emits a burn event', async function () { - const { logs } = await token.publicBurn(tokenId, { from: sender }); + const { logs } = await token.burn(tokenId, { from: sender }); logs.length.should.be.equal(1); logs[0].event.should.be.eq('Transfer'); @@ -160,13 +160,13 @@ contract('ERC721Token', accounts => { }); it('clears the approval', async function () { - await token.publicBurn(tokenId, { from: sender }); + await token.burn(tokenId, { from: sender }); const approvedAccount = await token.approvedFor(tokenId); approvedAccount.should.be.equal(ZERO_ADDRESS); }); it('emits an approval event', async function () { - const { logs } = await token.publicBurn(tokenId, { from: sender }); + const { logs } = await token.burn(tokenId, { from: sender }); logs.length.should.be.equal(2); @@ -182,7 +182,7 @@ contract('ERC721Token', accounts => { const sender = accounts[1]; it('reverts', async function () { - await assertRevert(token.publicBurn(tokenId, { from: sender })); + await assertRevert(token.burn(tokenId, { from: sender })); }); }); }); @@ -191,7 +191,7 @@ contract('ERC721Token', accounts => { const tokenID = _unknownTokenId; it('reverts', async function () { - await assertRevert(token.publicBurn(tokenID, { from: _creator })); + await assertRevert(token.burn(tokenID, { from: _creator })); }); }); });