-
Notifications
You must be signed in to change notification settings - Fork 0
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: capped minter v2 close #12
base: feat/capped-minter-v2-pause
Are you sure you want to change the base?
feat: capped minter v2 close #12
Conversation
ca259f4
to
004c789
Compare
3fcf95b
to
bceb0f7
Compare
/// @dev Once closed, the contract cannot be reopened and all minting operations will be permanently blocked. | ||
/// @dev Only callable by accounts with the PAUSER_ROLE. | ||
function close() external { | ||
_checkRole(PAUSER_ROLE, msg.sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want the admin to be able to close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -45,6 +51,7 @@ contract ZkCappedMinterV2 is AccessControl, Pausable { | |||
|
|||
/// @notice Unpauses token minting | |||
function unpause() external { | |||
_revertIfClosed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It don't think we need to revert here, since unpausing will have no effect right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yee you're right:
function close() external { | ||
_checkRole(PAUSER_ROLE, msg.sender); | ||
closed = true; | ||
_pause(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pause here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yep unnecessary:
uint256 _amount | ||
) public { | ||
_cap = bound(_cap, 0, MAX_MINT_SUPPLY); | ||
vm.assume(_cap > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as last pr take a look at some of these assumes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned up in feat/capped-minter-v2-pause
and some here as well.
…ting default admin string error
Co-authored-by: Alexander Keating <[email protected]>
No description provided.