Browse Source

removed and tested unecessary gas cost (#1017)

* removed and tested unecessary gas cost

* linted

* Revert package-lock.json update.

This reverts commit 054598ce7f125b0e1aa4c463f047ac77771cf635.

* Added clarifying comments.
Yong Zhen Yu 7 years ago
parent
commit
6bd8842ab5

+ 4 - 0
contracts/mocks/ERC721TokenMock.sol

@@ -24,4 +24,8 @@ contract ERC721TokenMock is ERC721Token {
   function setTokenURI(uint256 _tokenId, string _uri) public {
   function setTokenURI(uint256 _tokenId, string _uri) public {
     super._setTokenURI(_tokenId, _uri);
     super._setTokenURI(_tokenId, _uri);
   }
   }
+  
+  function _removeTokenFrom(address _from, uint256 _tokenId) public {
+    super.removeTokenFrom(_from, _tokenId);
+  }
 }
 }

+ 1 - 0
contracts/mocks/RBACCappedTokenMock.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.4.24;
 import "../token/ERC20/RBACMintableToken.sol";
 import "../token/ERC20/RBACMintableToken.sol";
 import "../token/ERC20/CappedToken.sol";
 import "../token/ERC20/CappedToken.sol";
 
 
+
 contract RBACCappedTokenMock is CappedToken, RBACMintableToken {
 contract RBACCappedTokenMock is CappedToken, RBACMintableToken {
   constructor(
   constructor(
     uint256 _cap
     uint256 _cap

+ 1 - 0
contracts/mocks/WhitelistMock.sol

@@ -2,6 +2,7 @@ pragma solidity ^0.4.24;
 
 
 import "../access/Whitelist.sol";
 import "../access/Whitelist.sol";
 
 
+
 contract WhitelistMock is Whitelist {
 contract WhitelistMock is Whitelist {
 
 
   function onlyWhitelistedCanDoThis()
   function onlyWhitelistedCanDoThis()

+ 4 - 2
contracts/token/ERC721/ERC721Token.sol

@@ -140,17 +140,19 @@ contract ERC721Token is SupportsInterfaceWithLookup, ERC721BasicToken, ERC721 {
   function removeTokenFrom(address _from, uint256 _tokenId) internal {
   function removeTokenFrom(address _from, uint256 _tokenId) internal {
     super.removeTokenFrom(_from, _tokenId);
     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 tokenIndex = ownedTokensIndex[_tokenId];
     uint256 lastTokenIndex = ownedTokens[_from].length.sub(1);
     uint256 lastTokenIndex = ownedTokens[_from].length.sub(1);
     uint256 lastToken = ownedTokens[_from][lastTokenIndex];
     uint256 lastToken = ownedTokens[_from][lastTokenIndex];
 
 
     ownedTokens[_from][tokenIndex] = lastToken;
     ownedTokens[_from][tokenIndex] = lastToken;
-    ownedTokens[_from][lastTokenIndex] = 0;
+    ownedTokens[_from].length--; // This also deletes the contents at the last position of the array
+
     // Note that this will handle single-element arrays. In that case, both tokenIndex and lastTokenIndex are going to
     // 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
     // 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
     // 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[_tokenId] = 0;
     ownedTokensIndex[lastToken] = tokenIndex;
     ownedTokensIndex[lastToken] = tokenIndex;
   }
   }

+ 25 - 0
test/token/ERC721/ERC721Token.test.js

@@ -77,6 +77,31 @@ contract('ERC721Token', function (accounts) {
       });
       });
     });
     });
 
 
+    describe('removeTokenFrom', function () {
+      beforeEach(async function () {
+        await this.token._removeTokenFrom(creator, firstTokenId, { from: creator });
+      });
+
+      it('has been removed', async function () {
+        await assertRevert(this.token.tokenOfOwnerByIndex(creator, 1));
+      });
+
+      it('adjusts token list', async function () {
+        const token = await this.token.tokenOfOwnerByIndex(creator, 0);
+        token.toNumber().should.be.equal(secondTokenId);
+      });
+
+      it('adjusts owner count', async function () {
+        const count = await this.token.balanceOf(creator);
+        count.toNumber().should.be.equal(1);
+      });
+
+      it('does not adjust supply', async function () {
+        const total = await this.token.totalSupply();
+        total.toNumber().should.be.equal(2);
+      });
+    });
+
     describe('metadata', function () {
     describe('metadata', function () {
       const sampleUri = 'mock://mytoken';
       const sampleUri = 'mock://mytoken';