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 extension for efficient batch minting #3311

Merged
merged 51 commits into from
Sep 5, 2022

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Apr 1, 2022

Fixes #2355

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx changed the title ERC721 extension for gas effective of token batches ERC721 extension for minting token batches Jun 8, 2022
@Amxx Amxx added this to the 4.8 milestone Jun 8, 2022
@Amxx Amxx marked this pull request as ready for review July 26, 2022 20:42
@Amxx
Copy link
Collaborator Author

Amxx commented Jul 29, 2022

Waiting for #3589

@Amxx Amxx force-pushed the feature/ERC721/sequential branch from 1e9dba2 to 03835b3 Compare August 10, 2022 14:49
@frangio frangio changed the title ERC721 extension for minting token batches ERC721 extension for efficient batch minting Sep 5, 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!

@Amxx Amxx merged commit 171fa40 into OpenZeppelin:master Sep 5, 2022
@Amxx Amxx deleted the feature/ERC721/sequential branch September 5, 2022 21:10
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>
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 4, 2022
Co-authored-by: Francisco <frangio.1@gmail.com>
@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 28, 2022

Hi @Amxx and @frangio sorry for being late to the discussion.

TL;DR: I discover this PR only when researching for EIP-6047. And I have concern about this PR going into next OZ release because backward incompatibility at its current version. At the very least it should be named differently and removed ERC721 for the file name ERC721Consecutive and warn users who try to use them anticipating it's compatible with ERC721.

The longer version:

ERC-2309 is incompatible with ERC-721, and its title is confusing. If adopting ERC-2309, including this PR if release to prod of OZ and being used by OZ users who don't necessarily understand the incompatibility, which is very likely given so many people use OZ contracts, will break all indexers who assumes Transfer is the only event name they are looking.

Example of breakage:

    erc20_and_erc721_token_transfers =
      logs
      |> Enum.filter(&(&1.first_topic == unquote(TokenTransfer.constant())))

will be broken without something like the following update:

    erc20_and_erc721_token_transfers =
      logs
      |> Enum.filter(&(
        &1.first_topic == unquote(TokenTransfer.constant()) ||
        &1.first_topic == TokenTransfer.erc2309_consecutive_transfer_signature()))

While this may seem trivial, imagine across the whole web we need get all the indexers to each make similar updates, including Blockscan, OpenSea ... This is why I say it's backward incompatible.

FYI:

@Amxx
Copy link
Collaborator Author

Amxx commented Nov 29, 2022

Hello @xinbenlv

If you check out the ERC712 specifications, it is clearly stated that

/// @dev This emits when ownership of any NFT changes by any mechanism. 
/// This event emits when NFTs are created (`from` == 0) and destroyed 
/// (`to` == 0). Exception: during contract creation, any number of NFTs
/// may be created and assigned without emitting Transfer. At the time of 
/// any transfer, the approved address for that NFT (if any) is reset to none. 
event Transfer(address indexed _from, address indexed _to, uint256 indexed _tokenId);

Link: https://eips.ethereum.org/EIPS/eip-721

As a consequence, this IS compatible with ERC712, and indexers that don't support this exception (which is part of the standard) are the ones that need fixing.

@xinbenlv
Copy link
Contributor

EIP-2309's author admittedd incompatibility: ethereum/EIPs#2309 (comment)

ERC-721's exception you referrenced is only at "contract creation". Is ERC721Consecutive.sol only used in contract creation?

@frangio
Copy link
Contributor

frangio commented Nov 29, 2022

Is ERC721Consecutive.sol only used in contract creation?

Yes. Batch minting reverts when used outside of the constructor. Furthermore, this has already been released. Please see the code or the documentation before commenting.

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.

Support EIP-2309: ERC-721 Consecutive Transfer Extension
3 participants