feat: capped minter v2 pause #11

marcomariscal · 2024-12-04T16:34:35Z

No description provided.

…ter-v2-pause and update to use oz errors

alexkeating

Some initial thoughts

alexkeating · 2024-12-11T15:55:47Z

l2-contracts/test/ZkCappedMinterV2.t.sol

@@ -46,6 +57,7 @@ contract Mint is ZkCappedMinterV2Test {
    uint256 _amount
  ) public {
    _cap = bound(_cap, 0, MAX_MINT_SUPPLY);
+    vm.assume(_cap > 0);


Let's use bound for this assume as it is more efficient

fca3ffb

originally had because it's implied that the cap can indeed be 0.

given that this is a permissioned minter where admins are deploying it, and a zero cap would be caught quickly in testing/first use, we probably don't need to check this in the contract or the factory, but just want to call out. omitting a zero cap check here, but lemme know if you think otherwise.

originally had because it's implied that the cap can indeed be 0.

I thought this assume will remove the possibility of the cap being 0 because it must be greater than 0

removes the possibility of the cap being 0 in the test, but you can init the capped minter with a cap of 0.

how we set the bounds now with the bound starting at 1, it's implied that the cap can't be zero, but it can (was how i was thinking bout it).

alexkeating · 2024-12-11T15:57:06Z

l2-contracts/test/ZkCappedMinterV2.t.sol

    _cap = bound(_cap, 0, MAX_MINT_SUPPLY);
-    ZkCappedMinterV2 cappedMinter = createCappedMinter(_cappedMinterAdmin, _cap);
+    vm.assume(_admin != address(0));
+    vm.assume(_nonMinter != address(0) && _nonMinter != _admin);


Aren't both of these addresses non minters?

truth; remove all unnecessary assumptions including the above:

93e4af3

alexkeating · 2024-12-11T16:02:04Z

l2-contracts/test/ZkCappedMinterV2.t.sol

-        )
-      )
-    );
+    vm.expectRevert(_formatAccessControlError(_admin, MINTER_ROLE));


I think the receiver assume can be removed as there should be no restriction on address

alexkeating · 2024-12-11T16:03:37Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+    uint256 _amount
+  ) public {
+    _cap = bound(_cap, 0, MAX_MINT_SUPPLY);
+    vm.assume(_cap > 0);


This can be removed if the bound above starts with 1

#11 (comment)

alexkeating · 2024-12-11T16:04:02Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+    vm.assume(_cap > 0);
+    _amount = bound(_amount, 1, _cap);
+    vm.assume(_admin != address(0));
+    vm.assume(_minter != address(0) && _minter != _admin);


There is nothing stopping the minter from being these values

#11 (comment)

alexkeating · 2024-12-11T16:04:10Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+    _amount = bound(_amount, 1, _cap);
+    vm.assume(_admin != address(0));
+    vm.assume(_minter != address(0) && _minter != _admin);
+    vm.assume(_receiver != address(0) && _receiver != initMintReceiver);


#11 (comment)

alexkeating · 2024-12-11T16:04:57Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+
+    vm.prank(_minter);
+    cappedMinter.mint(_receiver, _amount);
+    assertEq(token.balanceOf(_receiver), _amount);


I think this can be removed as it is tested in another test

alexkeating · 2024-12-11T16:07:42Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+    _cap = bound(_cap, 0, MAX_MINT_SUPPLY);
+    vm.assume(_admin != address(0));
+
+    ZkCappedMinterV2 cappedMinter = createCappedMinter(_admin, _cap);


Creating the capped minter is a bit redundant. It may be good to move this setup to a helper or the setup and transfer admin role when necessary

alexkeating · 2024-12-11T16:08:34Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+}
+
+contract Unpause is ZkCappedMinterV2Test {
+  function testFuzz_CorrectlyAllowsNewMintsWhenUnpaused(


I would take a look at this test with some of the recommendations above

if addressing all of the comments above doesn't resolve this comment, lemme know (not sure what exactly you were talking bout here).

It seems resolved, was asking to cleanup test assumes etc..

alexkeating · 2024-12-11T16:09:20Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+  }
+}
+
+contract Pause is ZkCappedMinterV2Test {


It would be good to have a test case for pause, unpause and then pause again

alexkeating

Looking close a couple small changes left I think

alexkeating · 2024-12-12T12:31:26Z

l2-contracts/test/ZkCappedMinterV2.t.sol

@@ -8,29 +8,46 @@ import {ZkCappedMinterV2} from "src/ZkCappedMinterV2.sol";
 import {console2} from "forge-std/Test.sol";

 contract ZkCappedMinterV2Test is ZkTokenTest {
+  ZkCappedMinterV2 public cappedMinter;
+  uint256 constant DEFAULT_CAP = 1000e18;


I would increase this default cap to be more realistic

Maybe 100_000_000_000e18

alexkeating · 2024-12-12T12:37:11Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+    _amount1 = bound(_amount1, 1, DEFAULT_CAP / 2);
+    _amount2 = bound(_amount2, 1, DEFAULT_CAP / 2);
+    vm.assume(_amount1 + _amount2 < DEFAULT_CAP);
+    vm.assume(_receiver1 != address(0) && _receiver2 != address(0));


Is it necessary to assume this?

I am seeing this in a bunch of spots and I think it can be removed

receiver being address zero? erc20 throws an error on zero address receiver

alexkeating · 2024-12-12T12:38:56Z

l2-contracts/test/ZkCappedMinterV2.t.sol

+}
+
+contract Unpause is ZkCappedMinterV2Test {
+  function testFuzz_CorrectlyAllowsNewMintsWhenUnpaused(


It seems resolved, was asking to cleanup test assumes etc..

marcomariscal linked an issue Dec 4, 2024 that may be closed by this pull request

Pausable Functionality #5

Open

5 tasks

marcomariscal changed the base branch from master to feat/capped-minter-v2-minter-role December 4, 2024 16:35

marcomariscal force-pushed the feat/capped-minter-v2-pause branch 3 times, most recently from f96ae77 to 29362c9 Compare December 4, 2024 17:44

marcomariscal force-pushed the feat/capped-minter-v2-minter-role branch from 7483d1f to f417601 Compare December 4, 2024 17:46

marcomariscal force-pushed the feat/capped-minter-v2-pause branch from b3e1cb4 to ca259f4 Compare December 4, 2024 23:04

marcomariscal force-pushed the feat/capped-minter-v2-minter-role branch from bbaec92 to 31d626f Compare December 4, 2024 23:17

marcomariscal and others added 6 commits December 4, 2024 15:17

feat: basic pause functionality

fb88648

feat: pause/unpause tests

946dc79

fix: assume cap is not zero

d9b3dba

fix: check for pauser role

1cfbef7

fix: separate role errors

3379852

feat: _revertIfNotPauser

004c789

marcomariscal force-pushed the feat/capped-minter-v2-pause branch from ca259f4 to 004c789 Compare December 4, 2024 23:17

marcomariscal force-pushed the feat/capped-minter-v2-minter-role branch from af16880 to 007784b Compare December 10, 2024 19:18

marcomariscal added 2 commits December 10, 2024 15:32

Merge branch 'feat/capped-minter-v2-minter-role' into feat/capped-min…

8a5f057

…ter-v2-pause and update to use oz errors

feat: _formatAccessControlError

493c1a6

marcomariscal requested a review from alexkeating December 11, 2024 00:12

alexkeating reviewed Dec 11, 2024

View reviewed changes

marcomariscal added 7 commits December 11, 2024 10:08

fix: remove assume cap greater than 0 in favor of bound

fca3ffb

fix: remove unnecessary assumptions

93e4af3

fix: use grant minter role

225ba92

fix: remove redundant balance check

1942a64

feat: refactor to set up capped minter in setup func

77acf62

feat: testFuzz_CorrectlyPausesMintsWhenTogglingPause

ecb275d

fix: move func

43fa666

marcomariscal requested a review from alexkeating December 11, 2024 19:38

alexkeating reviewed Dec 12, 2024

View reviewed changes

fix: higher default cap

7362b6b

fix: assume receiver

0000a63

alexkeating approved these changes Dec 16, 2024

View reviewed changes

marcomariscal merged commit 8289ce2 into feat/capped-minter-v2-minter-role Dec 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: capped minter v2 pause #11

feat: capped minter v2 pause #11

marcomariscal commented Dec 4, 2024

alexkeating left a comment

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 12, 2024

marcomariscal Dec 16, 2024

marcomariscal Dec 16, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating Dec 12, 2024

alexkeating Dec 11, 2024

marcomariscal Dec 11, 2024

alexkeating left a comment

alexkeating Dec 12, 2024

alexkeating Dec 12, 2024

marcomariscal Dec 16, 2024

alexkeating Dec 12, 2024

alexkeating Dec 12, 2024

marcomariscal Dec 16, 2024

alexkeating Dec 12, 2024

feat: capped minter v2 pause #11

feat: capped minter v2 pause #11

Conversation

marcomariscal commented Dec 4, 2024

alexkeating left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkeating left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment