search

OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.97k stars 11.8k forks source link

redundent restrictions in ERC721Consecutive #3740

Open TrejGun opened 2 years ago

TrejGun commented 2 years ago

Please remove 2 restrictions from ERC721Consecutive

1 minting with the _mint function https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.0-rc.1/contracts/token/ERC721/extensions/ERC721Consecutive.sol#L98

2 minting in the constructor https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.0-rc.1/contracts/token/ERC721/extensions/ERC721Consecutive.sol#L72

https://eips.ethereum.org/EIPS/eip-2309 says nothing about restrictions, only about event

These restrictions make it easy to mint 10_000 tokens on collection creation as OpenSea does. But it actually does not, it just creates 10_000 rows in DB and then mints on demand (lazy mint). However, this makes it impossible for me to sell items from the store one-by-one and in small batches (of say 10) like I can do with ERC1155

💻 Environment

📝 Details

🔢 Code to reproduce bug

Amxx commented 2 years ago

Hello @TrejGun

/// @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);

This means that batch minting, using ERC-2309, is only compatible with ERC-721 if its done during construction. Allowing batch minting after the construction would break ERC721, and this is in our opinion not acceptable.

It would be nice to support minting single tokens during construction ... but the implementation we have would be susceptible to bugs if we allowed that. The issue would happen if someone first does one or multiple _mint and then _mintConsecutive a batch that overlaps with the mints done before that.

The _mintConsecutive cannot realistically check that none of the token already exists (that would be super inefficient, we are not going to do that), it will succeed, emit the ERC-2309 event, but some of the tokens in the batch will still be owner by the owner specified during the "simple" mint, which is in violation of what the dev intended, and what the events describe.

If these restrictions don't match your usecase, then you should probably write a message on the forum to discuss your usecase. Maybe ERC721Consecutive.sol is not for you.

If you want these checks gone, also fill free to propose alternative implementations ... but if you do so, keep in mint that we would need something that is safe, cheap to run, and that abides by the ERCs.

frangio commented 2 years ago

@Amxx Could we allow _mint in the constructor for tokenIds that are outside the consecutive range? I suppose they would need to be greater than 2**96.

Could we allow _mint in the constructor as long as it is not followed by _consecutiveMint?

I think we actually discussed these options and decided against adding the complexity required to implement that. But it's something that we can consider again.

Personally I feel that the current tradeoffs are not bad. But it's something that users should also comment on.

TrejGun commented 2 years ago

@Amxx I agree with your point about overlapping. Could this be combined with Enumerable to prevent overlapping?

frangio commented 2 years ago

Could this be combined with Enumerable to prevent overlapping?

I don't really see how that's relevant.

Amxx commented 2 years ago
TrejGun commented 2 years ago

Could this be combined with Enumerable to prevent overlapping?

I don't really see how that's relevant.

because you don't pass tokenId and all numbers go in order

TrejGun commented 1 year ago

Please advise, what should I do if I need to mint 10k of tokens on contract deployment and another 10k later?

Amxx commented 1 year ago

Please advise, what should I do if I need to mint 10k of tokens on contract deployment and another 10k later?

Options: