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 customizable _startTokenId() and _totalMinted() (Continued from #66) #129

Closed
wants to merge 38 commits into from

Conversation

Vectorized
Copy link
Collaborator

@Vectorized Vectorized commented Feb 23, 2022

Continued from #66. Kept the history of commits to preserve contributions.

Summary:

  • Enables the _startTokenId() to be customized via function override.
    • Prevents the need for a 3rd constructor argument, which can confuse users, as earlier versions of ERC721A used it for the maxBatchSize.
    • Compilers >= 0.8.2 will automatically inline the calls for zero-cost abstraction.
      Not a problem as we only support >= 0.8.4.
  • Includes an internal _totalMinted() function. See feat: _totalMinted() function. #124
    • Internal because it is not part of the standards, and we want to give users the choice to let it remain internal / wrap it in a public function.
    • Edited README to tell people to use _totalMinted().

Some quality of life suggestions (not yet added), please drop your opinions below
(include it in this PR / move to future PR / not needed / etc.):

  • Prettify the code to a max line length of 99. I know most of the long lines are from OZ.
    But that doesn't mean we can't prettify things a bit.
  • Add a _mint(to, quantity) function.
    This for the README example.
  • Add a cautionary comment to _safeMint that it is not re-entrancy safe, and encourage users to use _mint instead?
    This is for the README example.

Many projects are using ERC721A. Many of which are not listed.
Our contributions do have a huge impact, probably more than we can imagine.

@cygaar @ahbanavi @Pczek

Pczek and others added 30 commits February 2, 2022 16:56
@Vectorized Vectorized changed the title Add customizable _startTokenId() and _totalMinted() Add customizable _startTokenId() and _totalMinted() (Continued from #66) Feb 23, 2022
@ahbanavi
Copy link
Contributor

Thanks for opening this PR.

My opinion about suggestions:

Prettify the code to a max line length of 99. I know most of the long lines are from OZ.
But that doesn't mean we can't prettify things a bit.

Agree to use prettier for both solidity and js.
I have to manually disable my prettier before my contributions because it changed a lot of lines in the whole project, and it would be better if everyone were using this.
Also, It might help if write a Contributing guidelines and tell people to use prettier for their contribution.
But it's better we do this on a future PR.

Add a _mint(to, quantity) function.
This for the README example.

I opened a PR before about adding _unsafeMint #86, check it out.

Add a cautionary comment to _safeMint that it is not re-entrancy safe, and encourage users to use _mint instead?
This is for the README example.

Using _mint function without checking on _checkOnERC721Received may also cause problems, I mean developers should know the differences between safe and normal mint.
But it doesn't hurt to give some information on this matter in README and explain the different approaches (like using normal mint if their already revert requests from contracts).
This could be on another PR too, maybe the same PR for prettifying.

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.

4 participants