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

[IDEA] Don't rely on _beforeConsecutiveTokenTransfer for balance tracting #3744

Closed

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Sep 30, 2022

This PR

  • uses a new ERC721Concutive._batchBalances to keep track of the tokens minted sequentially.
  • don't write to ERC721._balances when minting batches.
  • rely on the unchecked sum of both values to get the actual result.

Note: this uses the fact that ERC721._balances will overflow when transferring out a token that was minted in batch.

The upside is that we don't need to manipulate the ERC721._balances in the _beforeConsecutiveTokenTransfer. Unfortunately the hooks still have to be defined in ERC721 so that ERC721Consecutive and ERC721Pausable can work together.

The downside is that we now have two storage slots involved in balance tracking. The cost of transferring should be similar in the long run, but a bit higher at first since the two slot needs to be initialized. Also, any account that got token minted and batch and then transfer them will have both slots set to non zero value (when the sum is actually zero). In addition, balanceOf now need 2 sload, increasing the cost by 2100 gas (plus the mapping lookup).

Is that worth it?

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Oct 6, 2022

The downside is that we now have two storage slots involved in balance tracking.

This is an important downside to consider.

The main downside that I see is the risk that a piece of code might accidentally read the overflowed balance instead of the adjusted balance that fixes the overflow.

@frangio
Copy link
Contributor

frangio commented Oct 25, 2022

We're not going with this idea for now. Might revisit for 5.0 if hooks are removed.

@frangio frangio closed this Oct 25, 2022
@frangio frangio mentioned this pull request Jan 6, 2023
3 tasks
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