remove testFail from tests and prefer vm.expectRevert

.github/workflows/gas-diff.yml

@@ -0,0 +1,57 @@
+name: Report gas diff
+
+on: 
+  workflow_call:
+# on:
+#   push:
+#     branches:
+#       - main
+#   pull_request:
+#     # Optionally configure to run only for changes in specific files. For example:
+#     # paths:
+#     # - src/**
+#     # - test/**
+#     # - foundry.toml
+#     # - remappings.txt
+#     # - .github/workflows/foundry-gas-diff.yml
+
+jobs:
+  compare_gas_reports:
+    name: Compare
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with: 
+          submodules: recursive
+
+      - uses: ./.github/actions/install
+
+      - name: Build contracts
+        run: forge build --sizes
+
+      - name: Check gas snapshots
+        run: forge snapshot --diff
+
+      - name: Run tests
+        run: forge test --gas-report > gasreport.ansi
+        env:
+          # make fuzzing semi-deterministic to avoid noisy gas cost estimation
+          # due to non-deterministic fuzzing (but still use pseudo-random fuzzing seeds)
+          FOUNDRY_FUZZ_SEED: 0x${{ github.event.pull_request.base.sha || github.sha }}
+
+      - name: Compare gas reports
+        uses: Rubilmax/[email protected]
+        with:
+          summaryQuantile: 0.9 # only display the 10% most significant gas diffs in the summary (defaults to 20%)
+          sortCriteria: avg,max # sort diff rows by criteria
+          sortOrders: desc,asc # and directions
+          ignore: test-foundry/**/* # filter out gas reports from specific paths (test/ is included by default)
+        id: gas_diff
+
+      - name: Add gas diff to sticky comment
+        if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
+        uses: marocchino/sticky-pull-request-comment@v2
+        with:
+          # delete the comment in case changes no longer impact gas costs
+          delete: ${{ !steps.gas_diff.outputs.markdown }}
+          message: ${{ steps.gas_diff.outputs.markdown }}

.github/workflows/pull_request.yml

@@ -22,4 +22,10 @@ jobs:
   analyse:
     name: Slither analysis
     uses: ./.github/workflows/slither.yml
-    secrets: inherit
+    secrets: inherit
+
+  gas-diff:
+    name: Gas diff
+    uses: ./.github/workflows/gas-diff.yml
+    secrets: inherit
+

src/wallet/KintoWallet.sol

@@ -121,7 +121,7 @@ contract KintoWallet is Initializable, BaseAccount, TokenCallbackHandler, IKinto
     /* ============ Signer Management ============ */
 
     /**
-     * @dev Change the signer policy
+     * @dev Change signer policy
      * @param policy new policy
      */
     function setSignerPolicy(uint8 policy) public override onlySelf {
@@ -132,11 +132,11 @@ contract KintoWallet is Initializable, BaseAccount, TokenCallbackHandler, IKinto
     }
 
     /**
-     * @dev Changed the signers
+     * @dev Change signers and policy (if new)
      * @param newSigners new signers array
      */
     function resetSigners(address[] calldata newSigners, uint8 policy) external override onlySelf {
-        require(newSigners[0] == owners[0], "KW-rs: first signer must same unless recovery");
+        require(newSigners[0] == owners[0], "KW-rs: first signer must be the same unless recovery");
         _resetSigners(newSigners, policy);
     }
 
@@ -220,7 +220,7 @@ contract KintoWallet is Initializable, BaseAccount, TokenCallbackHandler, IKinto
     }
 
     /**
-     * @dev Allos the wallet to transact with a specific app
+     * @dev Allows the wallet to transact with a specific app
      * @param apps apps array
      * @param flags whether to allow or disallow the app
      */

test/DeployTests.t.sol

@@ -59,15 +59,6 @@ contract DeveloperDeployTest is Create2Helper, UserOp, AATestScaffolding {
 
     uint256 _chainID = 1;
 
-    address payable _owner = payable(vm.addr(1));
-    address _secondowner = address(2);
-    address payable _user = payable(vm.addr(3));
-    address _user2 = address(4);
-    address _upgrader = address(5);
-    address _kycProvider = address(6);
-    address _recoverer = address(7);
-    address payable _funder = payable(vm.addr(8));
-
     UUPSProxy _proxyc;
     Counter _counter;
     CounterInitializable _counterInit;
@@ -77,7 +68,7 @@ contract DeveloperDeployTest is Create2Helper, UserOp, AATestScaffolding {
         vm.startPrank(address(1));
         _owner.transfer(1e18);
         vm.stopPrank();
-        deployAAScaffolding(_owner, _kycProvider, _recoverer);
+        deployAAScaffolding(_owner, 1, _kycProvider, _recoverer);
         vm.startPrank(_owner);
 
         address created =

test/ETHPriceIsRight.t.sol

@@ -63,10 +63,10 @@ contract ETHPriceIsRightTest is Test {
         assertEq(_priceIsRight.avgGuess(), 3000 ether);
     }
 
-    function testFailCannotEnterGuessAfterTime() public {
-        vm.startPrank(_user);
+    function test_RevertWhen_CannotEnterGuessAfterTime() public {
         vm.warp(_priceIsRight.END_ENTER_TIMESTAMP() + 1);
+
+        vm.expectRevert("You cannot enter guesses anymore");
         _priceIsRight.enterGuess(2000 ether);
-        vm.stopPrank();
     }
 }

test/EngenCredits.t.sol

@@ -23,20 +23,14 @@ contract EngenCreditsTest is Create2Helper, UserOp, AATestScaffolding {
 
     uint256 _chainID = 1;
 
-    address payable _owner = payable(vm.addr(1));
-    address _user = vm.addr(3);
-    address _user2 = vm.addr(4);
-    address _kycProvider = address(6);
-    address _recoverer = address(7);
-
     function setUp() public {
         vm.chainId(1);
         vm.startPrank(address(1));
         _owner.transfer(1e18);
         vm.stopPrank();
-        deployAAScaffolding(_owner, _kycProvider, _recoverer);
-        _setPaymasterForContract(address(_engenCredits));
-        _setPaymasterForContract(address(_kintoWalletv1));
+        deployAAScaffolding(_owner, 1, _kycProvider, _recoverer);
+        _fundPaymasterForContract(address(_engenCredits));
+        _fundPaymasterForContract(address(_kintoWalletv1));
     }
 
     function testUp() public {
@@ -52,20 +46,18 @@ contract EngenCreditsTest is Create2Helper, UserOp, AATestScaffolding {
         vm.startPrank(_owner);
         EngenCreditsV2 _implementationV2 = new EngenCreditsV2();
         _engenCredits.upgradeTo(address(_implementationV2));
-        // re-wrap the _proxy
+
+        // ensure that the implementation has been upgraded
         _engenCreditsV2 = EngenCreditsV2(address(_engenCredits));
         assertEq(_engenCreditsV2.newFunction(), 1);
         vm.stopPrank();
     }
 
-    function testFailOthersCannotUpgrade() public {
-        vm.startPrank(_recoverer);
+    function test_RevertWhen_OthersCannotUpgrade() public {
         EngenCreditsV2 _implementationV2 = new EngenCreditsV2();
+
+        vm.expectRevert("Ownable: caller is not the owner");
         _engenCredits.upgradeTo(address(_implementationV2));
-        // re-wrap the _proxy
-        _engenCreditsV2 = EngenCreditsV2(address(_engenCredits));
-        assertEq(_engenCreditsV2.newFunction(), 1);
-        vm.stopPrank();
     }
 
     /* ============ Token Tests ============ */
@@ -139,7 +131,7 @@ contract EngenCreditsTest is Create2Helper, UserOp, AATestScaffolding {
             address(_paymaster)
         );
         UserOperation[] memory userOps = new UserOperation[](2);
-        userOps[0] = createApprovalUserOp(
+        userOps[0] = createWhitelistAppOp(
             _chainID,
             privateKeys,
             address(_kintoWalletv1),
@@ -178,7 +170,7 @@ contract EngenCreditsTest is Create2Helper, UserOp, AATestScaffolding {
             address(_paymaster)
         );
         UserOperation[] memory userOps = new UserOperation[](2);
-        userOps[0] = createApprovalUserOp(
+        userOps[0] = createWhitelistAppOp(
             _chainID,
             privateKeys,
             address(_kintoWalletv1),
@@ -212,7 +204,7 @@ contract EngenCreditsTest is Create2Helper, UserOp, AATestScaffolding {
             address(_paymaster)
         );
         UserOperation[] memory userOps = new UserOperation[](2);
-        userOps[0] = createApprovalUserOp(
+        userOps[0] = createWhitelistAppOp(
             _chainID,
             privateKeys,
             address(_kintoWalletv1),

test/Faucet.t.sol

@@ -54,16 +54,19 @@ contract FaucetTest is Test {
         vm.stopPrank();
     }
 
-    function testFailOwnerCannotStartWithoutAmount() public {
-        vm.startPrank(_owner);
-        _faucet.startFaucet{value: 0.1 ether}();
-        vm.stopPrank();
+    function testStart_RevertWhen_OwnerCannotStartWithoutAmount(uint256 amt) public {
+        vm.assume(amt < _faucet.FAUCET_AMOUNT());
+        vm.prank(_owner);
+        vm.expectRevert("Not enough ETH to start faucet");
+        _faucet.startFaucet{value: amt}();
     }
 
-    function testFailStartFaucetByOthers() public {
-        vm.startPrank(_user);
+    function testStart_RevertWhen_CallerIsNotOwner(address someone) public {
+        vm.assume(someone != _faucet.owner());
+        vm.deal(someone, 1 ether);
+        vm.prank(someone);
+        vm.expectRevert("Ownable: caller is not the owner");
         _faucet.startFaucet{value: 1 ether}();
-        vm.stopPrank();
     }
 
     function testClaim() public {
@@ -78,12 +81,14 @@ contract FaucetTest is Test {
         vm.stopPrank();
     }
 
-    function testFailIfClaimedTwice() public {
-        vm.startPrank(_owner);
+    function testClaim_RevertWhen_ClaimedTwice() public {
+        vm.prank(_owner);
         _faucet.startFaucet{value: 1 ether}();
-        vm.stopPrank();
+
         vm.startPrank(_user);
         _faucet.claimKintoETH();
+
+        vm.expectRevert("You have already claimed your KintoETH");
         _faucet.claimKintoETH();
         vm.stopPrank();
     }

test/KYCViewer.t.sol

@@ -39,14 +39,6 @@ contract KYCViewerTest is Create2Helper, UserOp, AATestScaffolding {
 
     uint256 _chainID = 1;
 
-    address payable _owner = payable(vm.addr(1));
-    address _secondowner = address(2);
-    address payable _user = payable(vm.addr(3));
-    address _user2 = address(4);
-    address _upgrader = address(5);
-    address _kycProvider = address(6);
-    address _recoverer = address(7);
-    address payable _funder = payable(vm.addr(8));
     UUPSProxy _proxyViewer;
     KYCViewer _implkycViewer;
     KYCViewerV2 _implkycViewerV2;
@@ -58,7 +50,7 @@ contract KYCViewerTest is Create2Helper, UserOp, AATestScaffolding {
         vm.startPrank(address(1));
         _owner.transfer(1e18);
         vm.stopPrank();
-        deployAAScaffolding(_owner, _kycProvider, _recoverer);
+        deployAAScaffolding(_owner, 1, _kycProvider, _recoverer);
         vm.startPrank(_owner);
         _implkycViewer = new KYCViewer{salt: 0}(address(_walletFactory));
         // deploy _proxy contract and point it to _implementation
@@ -90,12 +82,10 @@ contract KYCViewerTest is Create2Helper, UserOp, AATestScaffolding {
         vm.stopPrank();
     }
 
-    function testFailOthersCannotUpgradeFactory() public {
+    function test_RevertWhen_OthersCannotUpgradeFactory() public {
         KYCViewerV2 _implementationV2 = new KYCViewerV2(address(_walletFactory));
+        vm.expectRevert("only owner");
         _kycViewer.upgradeTo(address(_implementationV2));
-        // re-wrap the _proxy
-        _kycViewer2 = KYCViewerV2(address(_kycViewer));
-        assertEq(_kycViewer2.newFunction(), 1);
     }
 
     /* ============ Viewer Tests ============ */

test/KintoEntryPoint.t.sol

@@ -25,20 +25,12 @@ contract KintoEntryPointTest is AATestScaffolding, UserOp {
 
     uint256 _chainID = 1;
 
-    address payable _owner = payable(vm.addr(1));
-    address _secondowner = address(2);
-    address _user = vm.addr(3);
-    address _user2 = vm.addr(5);
-    address _upgrader = address(5);
-    address _kycProvider = address(6);
-    address _recoverer = address(7);
-
     function setUp() public {
         vm.chainId(_chainID);
         vm.startPrank(address(1));
         _owner.transfer(1e18);
         vm.stopPrank();
-        deployAAScaffolding(_owner, _kycProvider, _recoverer);
+        deployAAScaffolding(_owner, 1, _kycProvider, _recoverer);
     }
 
     function testUp() public {