{"action":"created","issue":{"url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/issues/934","repository_url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity","labels_url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/issues/934/labels{/name}","comments_url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/issues/934/comments","events_url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/issues/934/events","html_url":"https://github.com/OpenZeppelin/openzeppelin-solidity/issues/934","id":320671485,"number":934,"title":"Questions about burn() function in ","user":{"login":"roschler","id":4660487,"avatar_url":"https://avatars2.githubusercontent.com/u/4660487?v=4","gravatar_id":"","url":"https://api.github.com/users/roschler","html_url":"https://github.com/roschler","followers_url":"https://api.github.com/users/roschler/followers","following_url":"https://api.github.com/users/roschler/following{/other_user}","gists_url":"https://api.github.com/users/roschler/gists{/gist_id}","starred_url":"https://api.github.com/users/roschler/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roschler/subscriptions","organizations_url":"https://api.github.com/users/roschler/orgs","repos_url":"https://api.github.com/users/roschler/repos","events_url":"https://api.github.com/users/roschler/events{/privacy}","received_events_url":"https://api.github.com/users/roschler/received_events","type":"User","site_admin":false},"labels":[{"id":798348872,"url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/labels/question","name":"question","color":"006b75","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-05-07T04:37:50Z","updated_at":"2018-05-10T14:26:19Z","closed_at":null,"author_association":"NONE","body":"I and too new to both Solidity coding and the Open Zeppelin library be forking anything in this great project and attempting a pull request.  But after reading this medium article that talks about your ERC721 implementation:\r\n\r\nhttps://medium.com/blockchannel/walking-through-the-erc721-full-implementation-72ad72735f3c\r\n\r\nI do have some questions about the _burn()_ function in this full implementation of the ERC721 standard.  If I should have simply posted this in the Open Zeppelin slack instead then my apologies in advance.  If so, please go ahead and close this issue and I will do exactly that.\r\n\r\nGiven the code for the burn() function below, taken from:\r\n\r\nhttps://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC721/ERC721Token.sol\r\n\r\n    function _burn(address _owner, uint256 _tokenId) internal {\r\n        super._burn(_owner, _tokenId);\r\n\r\n        // Clear metadata (if any)\r\n        if (bytes(tokenURIs[_tokenId]).length != 0) {\r\n        delete tokenURIs[_tokenId];\r\n        }\r\n\r\n        // Reorg all tokens array\r\n        uint256 tokenIndex = allTokensIndex[_tokenId];\r\n        uint256 lastTokenIndex = allTokens.length.sub(1);\r\n        uint256 lastToken = allTokens[lastTokenIndex];\r\n\r\n        allTokens[tokenIndex] = lastToken;\r\n        allTokens[lastTokenIndex] = 0;\r\n\r\n        allTokens.length--;\r\n        allTokensIndex[_tokenId] = 0;\r\n        allTokensIndex[lastToken] = tokenIndex;\r\n    }\r\n\r\nI have these two questions:\r\n\r\n1)  In the calculation of _lastTokenIndex_, why isn't there a check to see if _allTokens.length_ is 0, so that the subsequent subtraction operation _.sub(1)_ does not cause an arithmetic error resulting in a potentially invalid value for _lastTokenIndex_?\r\n\r\n2) Why is the decrement operator being used on _allTokens.length_ instead of the _sub(1)_ method and again, why is there no check to see if it is already zero first?\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/issues/comments/388069691","html_url":"https://github.com/OpenZeppelin/openzeppelin-solidity/issues/934#issuecomment-388069691","issue_url":"https://api.github.com/repos/OpenZeppelin/openzeppelin-solidity/issues/934","id":388069691,"user":{"login":"roschler","id":4660487,"avatar_url":"https://avatars2.githubusercontent.com/u/4660487?v=4","gravatar_id":"","url":"https://api.github.com/users/roschler","html_url":"https://github.com/roschler","followers_url":"https://api.github.com/users/roschler/followers","following_url":"https://api.github.com/users/roschler/following{/other_user}","gists_url":"https://api.github.com/users/roschler/gists{/gist_id}","starred_url":"https://api.github.com/users/roschler/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roschler/subscriptions","organizations_url":"https://api.github.com/users/roschler/orgs","repos_url":"https://api.github.com/users/roschler/repos","events_url":"https://api.github.com/users/roschler/events{/privacy}","received_events_url":"https://api.github.com/users/roschler/received_events","type":"User","site_admin":false},"created_at":"2018-05-10T14:26:19Z","updated_at":"2018-05-10T14:26:19Z","author_association":"NONE","body":"I see what you're saying about allTokens.length being protected at the operation level by SafeMath.  But that's not what I meant with my question about that.  I'm more concerned about what conditions would lead to attempting to do a \"sub(1)\" from _allTokens.length_ if it is _already_ zero length.  That seems to indicate a failure in the logic flow that needs handling rather than the simple protection of the _lastTokenIndex_ at the arithmetic leve.\r\n\r\nIf not, then there are two distinct pathways that can lead to _lastTokenIndex_:\r\n\r\n1) _allTokens_ has one element in it at the time of execution (length = 1)\r\n2) _allTokens_ is empty at the time of execution (length = 0)\r\n\r\nIn both cases, this will lead to _lastTokenIndex_ resolving to a value of 0.   I just don't see how this could be \"logically sound\".  It would seem to worthy of a protective _assert()_ at least that checks that the _allTokens_ is not of zero length, or there could be unintended consequences."}}
{"actor":{"display_login":"roschler"}}