Address feedback comments for ERC721

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);
   }
 }

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);
     }
@@ -136,6 +136,17 @@ contract ERC721Token is ERC721 {
   }
 
   /**
+   * @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
   * @param _to address which you want to transfer the token to
@@ -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;
-  }
 }

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 }));
       });
     });
   });