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

Use unchecked for ERC721 balance updates #3524

Merged
merged 8 commits into from
Aug 23, 2022

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jul 1, 2022

Following the logic in described in #3512 (comment).

  • _balances are private
  • _balances are only increased/decreased when tokens are minted, burned, or transferred

A balance cannot decrease below 0 because that would require transferring/burning more tokens out of a wallet than were transferred/minted to this wallet in the first place.

The balance cannot overflow, because that would require minting all the tokenIds, and putting them all in the same wallet (there are 2**256 tokenIds, so that would overflow by one). While this is mathematically possible, it is impossible in practice (with token being transferred/minted one at a time)

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Jul 5, 2022

Given that we want to explore batch minting in ERC721 it seems we should not merge this, as the assumption that minting happens one at a time will be invalidated. Also please include benchmarks, what kind of improvement do you see with this PR?

Use of unchecked should be accompanied by comments explaining why it's safe.

@Amxx
Copy link
Collaborator Author

Amxx commented Jul 6, 2022

Benchmark of the changes (running the transactions in test/token/ERC721/ERC721.test.js)
Capture d’écran du 2022-07-06 10-03-21

@Amxx
Copy link
Collaborator Author

Amxx commented Jul 6, 2022

I understand that we want to experiment new minting mechanism. However, I believe that these minting mechanisms should remain ERC721 compliant, which means emitting a Transfer event for all the tokens ids. This means that, in practice, it's impossible to mint all the tokens, and the balance increment overflow is not an issue.

@Amxx
Copy link
Collaborator Author

Amxx commented Jul 6, 2022

For reference: https://twitter.com/optimizoor/status/1544626818996613120?t=8tV84gD3blTSsxk14EotiQ

TLDR: batch minting should be limited to "not to big batches" ... which means that the limit should never be reached, even minting 10**18 token each time

contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
Co-authored-by: Francisco <frangio.1@gmail.com>
@Amxx
Copy link
Collaborator Author

Amxx commented Aug 11, 2022

I believe once #3611 is merged, the requested changes will no longer be needed.

@Amxx Amxx added this to the 4.8 milestone Aug 11, 2022
@frangio frangio changed the title Use unchecked to improve gas usage of ERC721 Use unchecked for ERC721 balance updates Aug 23, 2022
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.

LGTM

@frangio frangio enabled auto-merge (squash) August 23, 2022 02:39
@frangio frangio merged commit f491e98 into OpenZeppelin:master Aug 23, 2022
@Amxx Amxx deleted the optimization/erc721 branch August 23, 2022 09:32
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
Co-authored-by: Francisco <frangio.1@gmail.com>
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
Co-authored-by: Francisco <frangio.1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants