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
Contributor

@nventuro nventuro 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 kind:improvement contracts Smart contract code. labels Dec 11, 2018
@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 December 11, 2018 19:52
@nventuro
Copy link
Contributor Author

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
Copy link
Contributor Author

nventuro 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
Copy link
Contributor

frangio commented Dec 11, 2018

Merged #1550 and restarted the build.

@nventuro
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. 😊

contracts/token/ERC721/ERC721Enumerable.sol Outdated Show resolved Hide resolved
@nventuro nventuro merged commit 12533bc into OpenZeppelin:master Dec 12, 2018
@nventuro nventuro deleted the erc721-bugfix-optim branch December 12, 2018 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code.
Projects
None yet
2 participants