[optimization] Gas use in removeNFToken

[mg6maciej]

These two lines don't need to be executed when tokenToRemoveIndex == lastTokenIndex.

    ownerToIds[_from][tokenToRemoveIndex] = lastToken;
    ownerToIds[_from][lastTokenIndex] = 0;

Next line covers clearing it:

    ownerToIds[_from].length--;

[mg6maciej]

Actually

ownerToIds[_from][lastTokenIndex] = 0;

is never necessary.

[mudgen]

I already reported about this here: #91

[MoMannn]

It was reported and fixed. I believe @mg6maciej is not using the last version. Please check #91.
We will check about tokenToRemoveIndex == lastTokenIndex state.

[MoMannn]

Lets continue the discussion about tokenToRemoveIndex == lastTokenIndex in #105.

[mg6maciej]

Oh, yeah. I even commented on removal of ownerToIds[_from][lastTokenIndex] = 0;, but the issue still stands. #105 is about a bug, this is about optimization. To make it clearer optimization can be applied every time a token which has the largest index for given user (assuming users have only a few tokens this may happen frequently) is transferred or burned.

Better yet, you may apply something like this: OpenZeppelin/openzeppelin-contracts#1031

[mg6maciej]

If you are not willing to go in the direction of 0x PR on OZ repo (removing removeNFToken and addNFToken altogether), but still brave enough, I believe (all tests pass) this change in Enumerable's removeNFToken will work:

@@ -108,11 +108,12 @@ contract NFTokenEnumerable is
     uint256 lastTokenIndex = ownerToIds[_from].length.sub(1);
     uint256 lastToken = ownerToIds[_from][lastTokenIndex];

-    ownerToIds[_from][tokenToRemoveIndex] = lastToken;
+    if (tokenToRemoveIndex != lastToken) {
+      ownerToIds[_from][tokenToRemoveIndex] = lastToken;
+      idToOwnerIndex[lastToken] = tokenToRemoveIndex;
+    }

     ownerToIds[_from].length--;
-    idToOwnerIndex[_tokenId] = 0;
-    idToOwnerIndex[lastToken] = tokenToRemoveIndex;
   }

   /**

As you may noticed, two updates are done conditionally, but also idToOwnerIndex[_tokenId] = 0; is not there anymore. This is because in transfers this update is overridden by one in addNFToken and for _burn leaving this value in unspecified state won't ever cause any problems as token with this id doesn't exists and will not be read. If later token with this id is minted again, the value is set in addNFToken.

[xpepermint]

@MoMannn let's test that.

[MoMannn]

The decision of what to do here is connected to discussion in #114. So if decision there is to lose inheritance and go to full optimization that would mean losing function and be an answer for this as well.

[xpepermint]

Closing in favor of #114.