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 internals refactor (API cleanup + gas savings) #1541

Closed
nventuro opened this issue Dec 5, 2018 · 1 comment · Fixed by #1549
Closed

ERC721 internals refactor (API cleanup + gas savings) #1541

nventuro opened this issue Dec 5, 2018 · 1 comment · Fixed by #1549
Assignees
Labels
contracts Smart contract code.
Milestone

Comments

@nventuro
Copy link
Contributor

nventuro commented Dec 5, 2018

We're currently exposing _addTokenTo and _removeTokenFrom due to language limitations, despite them not being useful due to incomplete behavior (e.g. no events are emitted). We do have a comment describing this and discouraging their usage, which IMO is enough to not consider them part of the API. We should get rid of these functions soon.

Once #1539 is merged, they won't be necessary anymore, since they can be inlined in _mint and _burn (which are the actual API functions).

Additionally, I advocate for removing some of the cleanup done in _removeTokenFrom, since it doesn't really help to prevent bugs.

This may be a last minute inclusion for 2.1, if there's enough time.

@nventuro
Copy link
Contributor Author

nventuro commented Dec 6, 2018

In #839 @frangio and I discussed not being sure of why setting a field to 0 in _removeTokenFrom wasn't making the whole transaction cheaper.

I finally understood this: we were measuring the gas cost of transfer, which calls _remove followed by _add. That last call sets _ownedTokensIndex again, which prevents the refund from being issued (which wouldn't make any sense - otherwise all SSTORE calls would be cheaper if preceded by another SSTORE clearing that slot).

So if this assignment is removed, transfer will be cheaper, but burn more expensive. Given this, I'd still advocate for getting rid of _removeTokenFrom, and clearing the storage slot in _burn.

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
Development

Successfully merging a pull request may close this issue.

2 participants