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

fix setMaxSupply to check total minted #27

Merged
merged 8 commits into from
Feb 22, 2024
Merged

Conversation

0xb337r007
Copy link
Collaborator

@0xb337r007 0xb337r007 commented Jan 22, 2024

closes #36

Description

Describe the changes made in your pull request here.

including specs from #28

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran forge snapshot?
  • Ran pnpm lint?
  • Ran forge test?
  • Ran pnpm verify?

function setMaxSupply(
uint256 newMaxSupply
) external virtual onlyCommunityOwnerOrTokenMaster {
if (newMaxSupply < mintedCount()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me, but the formatting seems off (which is probably why this isn't passing the linter on CI).

Can you please update this accordingly?

@0x-r4bbit
Copy link
Member

Also, it'd be wonderful if this had a dedicated certora rule that ensures this works as expected.

I've started something here #28
If you want, you can rebase on top of this and add your rule there.

@gravityblast gravityblast changed the title [WIP] fix setMaxSupply to check total minted fix setMaxSupply to check total minted Feb 19, 2024
@0x-r4bbit
Copy link
Member

This is unfortunately still not passing the prover.
Problem is that the invariant of maxSupply >= totalSupply() doesn't hold when setMaxSupply() is called with any value lower than totalSupply

@0x-r4bbit
Copy link
Member

Another invariant is failing similar to what I've pointed out here: #28 (comment)

I wonder if this is related to the ERC721Enumerable numer/id tracking

@0x-r4bbit 0x-r4bbit mentioned this pull request Feb 21, 2024
@0x-r4bbit
Copy link
Member

Approved changes.
Bonus points if commits are cleaned up.

Great job!

@0xb337r007 0xb337r007 merged commit 66ba281 into main Feb 22, 2024
7 of 8 checks passed
@0xb337r007 0xb337r007 deleted the set-max-supply-fix branch February 22, 2024 10:44
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.

Import and finish Certora specs from training week
3 participants