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

feat: _totalMinted() function. #124

Closed
Vectorized opened this issue Feb 19, 2022 · 3 comments
Closed

feat: _totalMinted() function. #124

Vectorized opened this issue Feb 19, 2022 · 3 comments

Comments

@Vectorized
Copy link
Collaborator

Vectorized commented Feb 19, 2022

With the burn feature, collections will need to check against _currentIndex if they want to limit the total number of mints.
Checking against totalSupply() can result in new mints being possible if someone burns their token(s).

With upcoming PR #66, users will have to check against _currentIndex - _startTokenId() instead.

This can cause some confusion.

I suggest adding an internal _totalMinted() function,
and update the README to encourage checking against _totalMinted() instead.

Another option is to put this into #66.

Let me know your comments.

If you want to make a PR for this issue, please feel free to do so. Drop a comment below too.

Otherwise, I will probably wait a few / several days after #66 is merged before starting a PR for this (if this feature is not in a PR or merged by then).

@cygaar @ahbanavi

@ahbanavi
Copy link
Contributor

Yes, _totalMinted() method would be convenient for this.
IMO, we better wait for #66 to be merged first.
Putting it in #66 also do the job I think, and it would be faster, but it's out of context of that PR.

@cygaar
Copy link
Collaborator

cygaar commented Feb 20, 2022

I think I'd prefer it in #66 just so no one accidentally copies this code and runs into an issue checking against _currentIndex. Thoughts on that approach? I'd like to make that change "atomic"

@ahbanavi
Copy link
Contributor

ahbanavi commented Feb 20, 2022

I agree to add in #66. True, being “atomic” is important on this matter.
I will work on this on a fork of #66 and make a PR to @Pczek branch.

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

No branches or pull requests

3 participants