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

Add ERC-2309 support for mints during contract creation #311

Merged
merged 14 commits into from
Jun 14, 2022

Conversation

Vectorized
Copy link
Collaborator

@Vectorized Vectorized commented Jun 4, 2022

This PR will be refactored into providing a _mintERC2309(to, quantity) function, that is intended for efficient minting during contract creation.

The NatSpec will clearly document that calling the function outside of contract creation, WILL make the contract non-compliant to the ERC721 standard (even though it is still supported by some marketplaces on a de facto basis).

Todo:

  • Refactor into _mintERC2309(to, quantity).

@Vectorized Vectorized linked an issue Jun 4, 2022 that may be closed by this pull request
@cygaar
Copy link
Collaborator

cygaar commented Jun 5, 2022

Like we talked about, we should let users decide what they want to do for the limit, but add some comments/docs regarding the potential issues with higher limits

@fulldecent
Copy link
Contributor

fulldecent commented Jun 6, 2022

I recommend that this PR be closed without further review.

ERC721a is advertised as "an improved implementation of the [ERC-721] standard". And the goal of the project is "to provide a fully compliant implementation of [ERC-721]".

This is the first sentence in both places.

This PR violates both of those guiding principles by removing ERC-721 compatibility. Or if you want to pedant, it violates the principle by allowing the customer to remove ERC-721 compatibility by quickly doing a thing.


If it is not clear to anybody, ERC-2309 is NOT compatible with ERC-721. The ERC-2309 specification is poorly named ("ERC-721 Consecutive Transfer Extension") whereas it is not an extension of ERC-721 but instead it is an alternative.


If there is interest in this PR, that's fine. But it shouldn't be called ERC-721a, it shouldn't be maintained as part of this repository, and it should stop associating itself with ERC-721.

@Vectorized Vectorized closed this Jun 6, 2022
@Vectorized
Copy link
Collaborator Author

Vectorized commented Jun 7, 2022

We can still provide a fully compliant implementation of ERC721 (by default),
but still give users the flexibility to break standard if they wish to.

This does not make the advertisement false -- the codebase is still perfectly capable of fully adhering to the standards by default.

Consider the case of a C compiler that provides full compatibility of the IEEE-754 standard, but allows users to turn it off with optimization flags (e.g. -ffast-math). Do we still consider the compiler as incapable of meeting its advertised objectives?

Also, nothing is stopping users from copy-pasting the code and adding ERC2309 themselves.

Additionally, all marketplaces that support ERC2309 classify the contracts as ERC721.
Are these marketplaces breaking standard?

Previous versions of ERC721A didn't stop users from breaking logic (and thus breaking the standard) if they directly write invalid values to the internal variables.
Would that make older versions of ERC721A not fully standards compliant?


Making a new repo with different class names for purist reasons is not exactly a practical solution.

Breaking standards for practical reasons has been a common practice in the history of software engineering,
and the benefits can outweigh the costs if done in a conscious, careful manner.

@Vectorized Vectorized reopened this Jun 7, 2022
@0xdeployer
Copy link

My biggest concern was marketplace support, but if looksrare and OS support w/ limits I'm all for this. 2309 has existed since 2019, but hasn't seen widespread adoption. I'd assume that's because there was no public, solid implementation. This is both public, vetted fully 721 compliant but can add 2309 and shine light on a standard which obviously adds tremendous efficiencies to initial mints. Imo everyone should use this for initial mints and adding it directly 721a will increase the likelihood that people do that. Creating a separate repo, or project just to add this additional event increases the likelihood the people DON'T use it. Regardless of the name, or if it's compliant or not compliant with the original 721 standard the amount of money that can be saved if every new projects uses this is astronomical. It's good for the NFT community and good for Ethereum. We should be doing everything we can to push this forward.

@Vectorized
Copy link
Collaborator Author

Vectorized commented Jun 7, 2022

I just thought of an idea to remain fully ERC721 compliant regardless to satisfy the purists/pendantics.

Just include a _mintERC2309(to, quantity) function.

If called in the constructor, it is perfectly valid ERC721. The ERC721 standard says we don’t need to emitt Transfer events for NFTs created upon contract creation. It also doesn’t state that we cannot emit other kinds of events in the constructor, and doesn’t restrict what we can call the internal minting function.

The official stance will be that the function is provided for efficient minting in the constructor.

But for performance issues, we will omit the check for this.code.length == 0 in the function.

So it can be called anywhere on a de facto basis, although doing that WILL break standard. ;)

@JohnChangUK
Copy link

This is a really neat optimization! But as @Vectorized stated keeping compliant with the ERC721 standard holds more importance in my opinion.

@fulldecent
Copy link
Contributor

Below are notes about allowing ER-2309 support broadly, even if behind an option.


If this PR is accepted then the tagline of this project should change from:

ERC721A is an improved implementation of the IERC721 standard that supports minting multiple tokens for close to the cost of one.

to

ERC721A is an improved implementation of the IERC721 standard that supports minting multiple tokens for close to the cost of one, unless you override this one function in which case this is no longer an implementation of ERC721 and it is instead an implementation of ERC2903 which claims to be related to ERC721 but isn't.

That is a completely fair disclaimer of what is happening here.


@Vectorized The LLVM project, the best compiler, introduces itself as:

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies. Despite its name, LLVM has little to do with traditional virtual machines. The name "LLVM" itself is not an acronym; it is the full name of the project.

Implementation of IEEE-754 is not a core tenet of that project. So the LLVM argument is distinguishable from the ERC721a situation.

But instead, if there were a C compiler named IEEE754improved, and the tagline and project scope for that project was based on IEEE-754 support, then yes it would be inappropriate for such a project to have a feature that disabled IEEE-754 support.


Breaking standards for practical reasons has been a common practice in the history of software engineering, and the benefits can outweigh the costs if done in a conscious, careful manner.

I agree that breaking standards is important. And I'm am the first person to tell people to NOT use ERC-721 or blockchain (instead, most companies should use traditional software with more transparency and assurance features).

But the discussion here is not on the principle of which software in the universe should exist. Instead, we are narrowly focused on which software should be part of the ERC721a project.

We have a guiding principle for this. And this is the project scope. The project scope is clearly defined, it is highly visible and it is inside the name of this project. And that scope does exclude ERC-2309. Therefore this PR should be closed without merging or further consideration.


This is both public, vetted fully 721 compliant but can add 2309 and shine light on a standard which obviously adds tremendous efficiencies to initial mints.

The goal of this project/repository is not to promote new competing standards against ERC-721.Open source does not mean your company can depend on the world’s best labor without spending money..

Quite the contrary, this repository is premised on continuing support of ERC-721, the standard for non-fungible tokens.


Creating a separate repo, or project just to add this additional event increases the likelihood the people DON'T use it. Regardless of the name, or if it's compliant or not compliant with the original 721 standard the amount of money that can be saved if every new projects uses this is astronomical. It's good for the NFT community and good for Ethereum. We should be doing everything we can to push this forward.

For a detailed discussion on why fragmentation of non-fungible token standards is terrible for the community, please refer to the discussions in EIP issues 721, 841 and proceedings from other meetings on finalization. I have cataloged those for you at at https://phor.net.

Quite plainly, if there had been multiple competing standards 4 years ago (ERC-721's finalization birthday is in two weeks) then very people would know about NFTs today. Fragmenting now is much worse than 4 years ago.

There are many other practical ways to reduce dollar cost of deploying contracts which this margin is too narrow to contain.

@fulldecent
Copy link
Contributor

Below are notes about the limited use case of implementing ERC-2309 only during contract creation.


Using ERC-2309 only at contract creation (and not generally at other times) is an allowed and anticipated application of ERC-721.

In case you're curious, the reason that is there is to allow NFT contract upgrades or redeployments. And the first ERC-721 NFT project does use that feature of the standard.

I support this limited use. But if we do add this feature, I hope we can please document it clearly in NatSpec. Lest other people read ERC-2309, get confused to think it is compatible with ERC-721, come here, see ERC-2309 support, and wonder why they only get a limited use of it.

@Vectorized
Copy link
Collaborator Author

Vectorized commented Jun 7, 2022

We will clearly document in the NatSpec that usage of the _mintERC2309 function is intended for efficient minting during contract creation, and calling it outside of contract creation WILL break the ERC721 standard.

@Vectorized Vectorized changed the title Add ERC-2309 support for mints Add ERC-2309 support for mints during contract creation Jun 7, 2022
@fulldecent
Copy link
Contributor

I reserve further comments until I can see the full proposed PR.

@Vectorized
Copy link
Collaborator Author

@fulldecent

@Vectorized
Copy link
Collaborator Author

Last call for comments. Merging in a day or few.

contracts/ERC721A.sol Outdated Show resolved Hide resolved
contracts/ERC721A.sol Outdated Show resolved Hide resolved
contracts/ERC721A.sol Outdated Show resolved Hide resolved
contracts/ERC721A.sol Outdated Show resolved Hide resolved
contracts/mocks/ERC721AWithERC2309Mock.sol Outdated Show resolved Hide resolved
test/GasUsage.test.js Outdated Show resolved Hide resolved
test/GasUsage.test.js Outdated Show resolved Hide resolved
@fulldecent
Copy link
Contributor

This is my complete review for the pull request. ^^^

@Vectorized
Copy link
Collaborator Author

Vectorized commented Jun 13, 2022

@fulldecent Thank you. The requested changes have been added.

@fulldecent
Copy link
Contributor

The test cases are still showing out-of-scope non-ERC-721 use cases.

If these test cases are desired, they should be explained that if you do that you are non-compliant with ERC-721.

@Vectorized
Copy link
Collaborator Author

@fulldecent The non-compliant code is included only for gas comparisons. I have added comments to clarify that.

@Vectorized Vectorized merged commit f6cc94d into chiru-labs:main Jun 14, 2022
@Vectorized Vectorized deleted the feature/erc2309 branch July 18, 2022 00:05
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.

ERC-2309
5 participants