5300 Gas Cost for Unnecessary Delete in ERC721Token.sol

[mudgen]

In the removeTokenFrom function ownedTokens[_from][lastTokenIndex] is set to 0 twice instead of once. The main reason this is not good beside redundant is because it costs gas. From my tests this unneeded redundancy costs 5300 gas.

Here is the code I am talking about:

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

The first line obviously sets ownedTokens[_from][lastTokenIndex] to 0. Reducing the length of the array also sets ownedTokens[_from][lastTokenIndex] to 0.

So it makes sense to get rid of this line: ownedTokens[_from][lastTokenIndex] = 0;.

[come-maiz]

Thanks for the report @mudgen. Can you please make a pull request making sure that the removeFrom is fully tested to ensure that the behaviour is preserved?

[mudgen]

@ElOpio I don't have sufficient incentive/motivation to make a pull request and test this further. I tested it for myself and that is enough for me. I mentioned it to OpenZepplin to help out.

[come-maiz]

Alright, thanks @mudgen. We'll take a look when our backlog is smaller, or when somebody else jumps to it.

[YZhenY]

I would love to work on this @ElOpio. Starting on it