Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERC721 bugfix + gas optimizations #1549

Merged
merged 7 commits into from Dec 12, 2018

Conversation

@nventuro
Copy link
Member

commented Dec 11, 2018

This removes the _addTokenTo and _removeTokenFrom internal functions (which had a notice indicating they were not meant to be used, and only existed due to a language limitation, and are therefore not considered part of the API). They have been inlined into _mint and _burn, making it easier to understand how each function is called in ERC721Enumerable.

Additionally, #839 had already been accidentally fixed in #1539, but the current implementation makes it more obvious that the issues is gone.

Fixes #839
Closes #912
Closes #1541

@nventuro nventuro added this to the v2.1 milestone Dec 11, 2018
@nventuro nventuro self-assigned this Dec 11, 2018
@nventuro nventuro requested a review from frangio Dec 11, 2018
@nventuro

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

I just noted _burn has the same swap and pop behavior, and therefore contains its own copy of #839: I'll follow up on this and fix that.

@nventuro

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

Coverage dropped due to a check that is now in _burn and used to be tested via _removeTokenFrom in the tests I got rid of, but that require won't make sense after #1550 goes in - we could first merge that PR and then this one on top.

@frangio

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Merged #1550 and restarted the build.

@nventuro

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

Hmm, I guess we really can't get rid of that require. We'll need to add a public method on a mock to explicitly test it.

@nventuro

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

I know the _mint test is sort of out of scope, but the _burn tests require _mint, and having that doesn't really make sense unless _mint is also tested.

@nventuro

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

Should be ready now, @frangio PTAL. The functions for adding/removing tokens from both enumerations should be replaced by a function that receives a storage mapping (they are pretty much the same), but sadly that isn't supported in 0.4.x.

Copy link
Member

left a comment

Nice work. 😊

contracts/token/ERC721/ERC721Enumerable.sol Outdated Show resolved Hide resolved
@nventuro nventuro merged commit 12533bc into OpenZeppelin:master Dec 12, 2018
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
coverage/coveralls First build on erc721-bugfix-optim at 100.0%
Details
@nventuro nventuro deleted the nventuro:erc721-bugfix-optim branch Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.