ERC721 extension for minting token batches

[Amxx]

Fixes #????

PR Checklist

• [x] Tests
• [ ] Documentation
• [ ] Changelog entry

[Amxx]

Waiting for #3589

[xinbenlv]

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:

• The blockscout is one of indexer expecting Transfer event, specifically the assumption in this particular line https://github.com/blockscout/blockscout/blob/708a6b00362689aab50db8f0cd996c2de66013ed/apps/indexer/lib/indexer/transform/token_transfers.ex#L23

    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:

• Author of ERC721 @fulldecent also raised the same compatible concern at the original EIP-2309 pull request
• Follow this discussion in https://ethereum-magicians.org/t/eip-6047-extend-erc-721-to-support-balance-counting-via-transfer-event/11894/11?u=xinbenlv

[Amxx]

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]

EIP-2309's author admittedd incompatibility: https://github.com/ethereum/EIPs/issues/2309#issuecomment-642189895

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

[frangio]

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.