{"action":"created","issue":{"url":"https://api.github.com/repos/0xcert/ethereum-erc721/issues/104","repository_url":"https://api.github.com/repos/0xcert/ethereum-erc721","labels_url":"https://api.github.com/repos/0xcert/ethereum-erc721/issues/104/labels{/name}","comments_url":"https://api.github.com/repos/0xcert/ethereum-erc721/issues/104/comments","events_url":"https://api.github.com/repos/0xcert/ethereum-erc721/issues/104/events","html_url":"https://github.com/0xcert/ethereum-erc721/issues/104","id":335179631,"node_id":"MDU6SXNzdWUzMzUxNzk2MzE=","number":104,"title":"[optimization] Gas use when in `removeNFToken`","user":{"login":"mg6maciej","id":1316369,"node_id":"MDQ6VXNlcjEzMTYzNjk=","avatar_url":"https://avatars1.githubusercontent.com/u/1316369?v=4","gravatar_id":"","url":"https://api.github.com/users/mg6maciej","html_url":"https://github.com/mg6maciej","followers_url":"https://api.github.com/users/mg6maciej/followers","following_url":"https://api.github.com/users/mg6maciej/following{/other_user}","gists_url":"https://api.github.com/users/mg6maciej/gists{/gist_id}","starred_url":"https://api.github.com/users/mg6maciej/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mg6maciej/subscriptions","organizations_url":"https://api.github.com/users/mg6maciej/orgs","repos_url":"https://api.github.com/users/mg6maciej/repos","events_url":"https://api.github.com/users/mg6maciej/events{/privacy}","received_events_url":"https://api.github.com/users/mg6maciej/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-06-24T13:31:09Z","updated_at":"2018-06-24T17:01:45Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"These two lines don't need to be executed when `tokenToRemoveIndex == lastTokenIndex`.\r\n```\r\n    ownerToIds[_from][tokenToRemoveIndex] = lastToken;\r\n    ownerToIds[_from][lastTokenIndex] = 0;\r\n```\r\nNext line covers clearing it:\r\n```\r\n    ownerToIds[_from].length--;\r\n```"},"comment":{"url":"https://api.github.com/repos/0xcert/ethereum-erc721/issues/comments/399771150","html_url":"https://github.com/0xcert/ethereum-erc721/issues/104#issuecomment-399771150","issue_url":"https://api.github.com/repos/0xcert/ethereum-erc721/issues/104","id":399771150,"node_id":"MDEyOklzc3VlQ29tbWVudDM5OTc3MTE1MA==","user":{"login":"mg6maciej","id":1316369,"node_id":"MDQ6VXNlcjEzMTYzNjk=","avatar_url":"https://avatars1.githubusercontent.com/u/1316369?v=4","gravatar_id":"","url":"https://api.github.com/users/mg6maciej","html_url":"https://github.com/mg6maciej","followers_url":"https://api.github.com/users/mg6maciej/followers","following_url":"https://api.github.com/users/mg6maciej/following{/other_user}","gists_url":"https://api.github.com/users/mg6maciej/gists{/gist_id}","starred_url":"https://api.github.com/users/mg6maciej/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mg6maciej/subscriptions","organizations_url":"https://api.github.com/users/mg6maciej/orgs","repos_url":"https://api.github.com/users/mg6maciej/repos","events_url":"https://api.github.com/users/mg6maciej/events{/privacy}","received_events_url":"https://api.github.com/users/mg6maciej/received_events","type":"User","site_admin":false},"created_at":"2018-06-24T17:01:44Z","updated_at":"2018-06-24T17:01:44Z","author_association":"CONTRIBUTOR","body":"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:\r\n\r\n```\r\n@@ -108,11 +108,12 @@ contract NFTokenEnumerable is\r\n     uint256 lastTokenIndex = ownerToIds[_from].length.sub(1);\r\n     uint256 lastToken = ownerToIds[_from][lastTokenIndex];\r\n\r\n-    ownerToIds[_from][tokenToRemoveIndex] = lastToken;\r\n+    if (tokenToRemoveIndex != lastToken) {\r\n+      ownerToIds[_from][tokenToRemoveIndex] = lastToken;\r\n+      idToOwnerIndex[lastToken] = tokenToRemoveIndex;\r\n+    }\r\n\r\n     ownerToIds[_from].length--;\r\n-    idToOwnerIndex[_tokenId] = 0;\r\n-    idToOwnerIndex[lastToken] = tokenToRemoveIndex;\r\n   }\r\n\r\n   /**\r\n```\r\n\r\nAs you may noticed, two updates are done conditionally, but also `idToOwnerIndex[_tokenId] = 0;` is not there anymore. This is because in `transfer`s 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 again is set in `addNFToken`."}}
{"actor":{"display_login":"mg6maciej"}}