expressively test for errors (#8)

* add chai matchers

* use chain matchers for custom errors

* use changeEtherBalances instead of and

* fix basepaymaster before hook

* fix balance changes tests

* swap order of balance queries

* trigger another tx before balance checks

* change delay in bug fix test

contracts/hardhat.config.ts

@@ -1,5 +1,6 @@
 import { HardhatUserConfig } from "hardhat/config";
 
+import "@matterlabs/hardhat-zksync-chai-matchers";
 import "@matterlabs/hardhat-zksync-node";
 import "@matterlabs/hardhat-zksync-solc";
 import "@matterlabs/hardhat-zksync-deploy";

contracts/package.json

@@ -11,17 +11,20 @@
     "lint": "solhint \"contracts/**/*.sol\""
   },
   "devDependencies": {
+    "@matterlabs/hardhat-zksync-chai-matchers": "^1.2.1",
     "@matterlabs/hardhat-zksync-deploy": "^1.1.2",
     "@matterlabs/hardhat-zksync-node": "^1.0.1",
     "@matterlabs/hardhat-zksync-solc": "^1.0.6",
     "@matterlabs/hardhat-zksync-verify": "^1.2.2",
     "@matterlabs/zksync-contracts": "^0.6.1",
+    "@nomicfoundation/hardhat-chai-matchers": "^2.0.3",
+    "@nomiclabs/hardhat-ethers": "^2.2.3",
     "@openzeppelin/contracts": "^5.0.1",
     "@types/chai": "^4.3.4",
     "@types/mocha": "^10.0.1",
     "chai": "^4.3.7",
     "dotenv": "^16.0.3",
-    "ethers": "^6.7.1",
+    "ethers": "^6.10.0",
     "hardhat": "^2.19.4",
     "mocha": "^10.2.0",
     "solhint": "^3.6.2",

contracts/test/NODL.test.ts

@@ -21,24 +21,28 @@ describe("NODL", function () {
   });
 
   it("Should be mintable", async () => {
+    const balanceBefore = await tokenContract.balanceOf(userWallet.address);
     const mintAmount = ethers.parseEther("1000");
     const initialSupply = await tokenContract.totalSupply();
+
     await tokenContract.mint(userWallet.address, mintAmount);
 
-    const balance = await tokenContract.balanceOf(userWallet.address);
-    expect(balance.toString()).to.equal(mintAmount.toString());
+    const balanceAfter = await tokenContract.balanceOf(userWallet.address);
+    expect(balanceAfter).to.equal(balanceBefore + mintAmount);
 
     const finalSupply = await tokenContract.totalSupply();
     expect(finalSupply).to.equal(initialSupply + mintAmount);
   });
 
   it("Should be burnable", async () => {
+    const balanceBefore = await tokenContract.balanceOf(userWallet.address);
     const burnAmount = ethers.parseEther("1000");
     const initialSupply = await tokenContract.totalSupply();
+
     await tokenContract.connect(userWallet).burn(burnAmount);
 
-    const balance = await tokenContract.balanceOf(userWallet.address);
-    expect(balance.toString()).to.equal("0");
+    const balanceAfter = await tokenContract.balanceOf(userWallet.address);
+    expect(balanceAfter).to.equal(balanceBefore - burnAmount);
 
     const finalSupply = await tokenContract.totalSupply();
     expect(finalSupply).to.equal(initialSupply - burnAmount);
@@ -56,12 +60,11 @@ describe("NODL", function () {
 
     const maxMint = cap - currentSupply;
     await tokenContract.mint(userWallet.address, maxMint);
-
-    try {
-      await tokenContract.mint(userWallet.address, 1);
-      expect.fail("Should have reverted");
-    } catch (e) {
-      expect(e.message).to.contain("execution reverted");
-    }
+
+    await expect(
+      tokenContract.mint(userWallet.address, 1)
+    ).to.be
+      .revertedWithCustomError(tokenContract, "ERC20ExceededCap")
+      .withArgs(cap + 1n, cap);
   });
 });

contracts/test/contentsign/ContentSignNFT.test.ts

@@ -31,11 +31,13 @@ describe("ContentSignNFT", function () {
   it("Non minter cannot mint token", async function () {
     const tokenURI = "https://example.com";
 
-    try {
-        await tokenContract.connect(userWallet).safeMint(userWallet.address, tokenURI);
-        expect.fail("Should have reverted");
-    } catch (e) {
-        expect(e.message).to.contain("execution reverted");
-    }
+    const minterRole = await tokenContract.MINTER_ROLE();
+    await expect(
+      tokenContract
+        .connect(userWallet)
+        .safeMint(userWallet.address, tokenURI)
+    ).to.be
+      .revertedWithCustomError(tokenContract, "AccessControlUnauthorizedAccount")
+      .withArgs(userWallet.address, minterRole);
   });
 });

contracts/test/paymasters/BasePaymaster.test.ts

@@ -25,8 +25,10 @@ describe("BasePaymaster", function () {
         userWallet = result.userWallet;
         provider = result.provider;
 
-        flag = await deployContract("MockFlag", [], { wallet: adminWallet, silent: true, skipChecks: true });
-        nodl = await deployContract("NODL", [adminWallet.address, adminWallet.address], { wallet: adminWallet, silent: true, skipChecks: true });
+        // using the admin or sponsor wallet to deploy seem to have us run into
+        // a nonce management bug in zksync-ethers
+        flag = await deployContract("MockFlag", [], { wallet: withdrawerWallet, silent: true, skipChecks: true });
+        nodl = await deployContract("NODL", [adminWallet.address, adminWallet.address], { wallet: withdrawerWallet, silent: true, skipChecks: true });
     });
 
     async function executePaymasterTransaction(user: Wallet, type: "General" | "ApprovalBased", nonce: number, flagValue: string = "flag captured") {
@@ -45,23 +47,34 @@ describe("BasePaymaster", function () {
             });
         }
 
-        const tx = await flag.connect(user).setFlag(flagValue, {
+        await flag.connect(user).setFlag(flagValue, {
             nonce,
             customData: {
                 gasPerPubdata: utils.DEFAULT_GAS_PER_PUBDATA_LIMIT,
                 paymasterParams,
             },
         });
-        await tx.wait();
 
         expect(await flag.flag()).to.equal(flagValue);
     }
 
     it("Can withdraw excess ETH", async () => {
-        const tx = await paymaster.connect(withdrawerWallet).withdraw(withdrawerWallet.address, ethers.parseEther("0.5"));
+        const balancePaymasterBefore = await provider.getBalance(await paymaster.getAddress());
+        const balanceSponsorBefore = await provider.getBalance(sponsorWallet.address);
+
+        const withdrawValue = ethers.parseEther("0.5");
+
+        // we withdraw to the sponsor wallet so we can compute the balance changes
+        // without having to handle the tx fees
+        const tx = await paymaster.connect(withdrawerWallet).withdraw(sponsorWallet.address, withdrawValue);
         await tx.wait();
 
-        expect(await provider.getBalance(paymaster.getAddress())).to.equal(ethers.parseEther("0.5"));
+        // BUG: the RPC is not quite up to date with the withdrawal tx (ie. there is some lag)
+        // so we trigger another tx to force a refresh
+        await flag.connect(adminWallet).setFlag("bug fix delay");
+
+        expect(await provider.getBalance(sponsorWallet.address)).to.equal(balanceSponsorBefore + withdrawValue);
+        expect(await provider.getBalance(await paymaster.getAddress())).to.equal(balancePaymasterBefore - withdrawValue);
     });
 
     it("Works as a paymaster", async () => {
@@ -75,17 +88,20 @@ describe("BasePaymaster", function () {
 
     it("Fails if not enough ETH", async () => {
         // withdraw all the ETH
-        const toWithdraw = await provider.getBalance(paymaster.getAddress());
+        const toWithdraw = await provider.getBalance(await paymaster.getAddress());
         const tx = await paymaster.connect(withdrawerWallet).withdraw(withdrawerWallet.address, toWithdraw);
         await tx.wait();
 
         // paymaster cannot pay for txs anymore
-        try {
-            await executePaymasterTransaction(userWallet, "General", 2);
-            expect.fail("Should have reverted");
-        } catch (e) {
-            expect(e.message).to.include("Paymaster validation error");
-        }
+        await expect(
+            executePaymasterTransaction(userWallet, "General", 2)
+        ).to.be.reverted;
+
+        // sanity checks: make sure the paymaster balance was emptied
+        // not done earlier as it sounds like the RPC is not quite up
+        // to date with the withdrawal tx (ie. there is some lag)
+        const paymasterBalance = await provider.getBalance(await paymaster.getAddress());
+        expect(paymasterBalance).to.equal(ethers.toBigInt(0));
     });
 
     it("Sets correct roles", async () => {

contracts/test/paymasters/WhitelistedPaymaster.test.ts

@@ -34,8 +34,10 @@ describe("WhitelistPaymaster", function () {
 
         // whitelist user
         const whitelistedRole = await paymaster.WHITELISTED_USER_ROLE();
-        const tx = await paymaster.connect(adminWallet).grantRole(whitelistedRole, userWallet.address, { nonce: await adminWallet.getNonce() });
-        await tx.wait();
+        await paymaster.connect(adminWallet).grantRole(whitelistedRole, userWallet.address, {
+            // somehow if we do not do this, the underlying lib will not use an up to date nonce value
+            nonce: await adminWallet.getNonce()
+        });
     });
 
     it("Sets correct roles", async () => {
@@ -52,27 +54,27 @@ describe("WhitelistPaymaster", function () {
         const newFlag = await deployContract("MockFlag", [], { wallet: withdrawerWallet, silent: true, skipChecks: true });
         const newFlagAddress = await newFlag.getAddress();
 
-        try {
-            await paymaster.connect(userWallet).addWhitelistedContracts([newFlagAddress]);
-        } catch (e) {
-            expect(e.message).to.contain("execution reverted");
-        }
+        const whitelistAdminRole = await paymaster.WHITELIST_ADMIN_ROLE();
+
+        await expect(
+            paymaster.connect(userWallet).addWhitelistedContracts([newFlagAddress])
+        ).to.be
+            .revertedWithCustomError(paymaster, "AccessControlUnauthorizedAccount")
+            .withArgs(userWallet.address, whitelistAdminRole);
 
-        try {
-            await paymaster.connect(userWallet).removeWhitelistedContracts([newFlagAddress]);
-        } catch (e) {
-            expect(e.message).to.contain("execution reverted");
-        }
+        await expect(
+            paymaster.connect(userWallet).removeWhitelistedContracts([newFlagAddress])
+        ).to.be
+            .revertedWithCustomError(paymaster, "AccessControlUnauthorizedAccount")
+            .withArgs(userWallet.address, whitelistAdminRole);
 
         const nonce = await whitelistAdminWallet.getNonce();
 
-        const tx1 = await paymaster.connect(whitelistAdminWallet).addWhitelistedContracts([newFlagAddress], { nonce: nonce });
-        await tx1.wait();
+        await paymaster.connect(whitelistAdminWallet).addWhitelistedContracts([newFlagAddress], { nonce: nonce });
         expect(await paymaster.isWhitelistedContract(newFlagAddress)).to.be.true;
         expect(await paymaster.isWhitelistedContract(await flag.getAddress())).to.be.true;
 
-        const tx2 = await paymaster.connect(whitelistAdminWallet).removeWhitelistedContracts([newFlagAddress], { nonce: nonce + 1 });
-        await tx2.wait();
+        await paymaster.connect(whitelistAdminWallet).removeWhitelistedContracts([newFlagAddress], { nonce: nonce + 1 });
         expect(await paymaster.isWhitelistedContract(newFlagAddress)).to.be.false;
         expect(await paymaster.isWhitelistedContract(await flag.getAddress())).to.be.true;
     });
@@ -85,32 +87,31 @@ describe("WhitelistPaymaster", function () {
             innerInput: new Uint8Array(),
         });
 
-        try {
-            await flag.connect(userWallet).setFlag("flag captured", {
+        // bootloader / zksync logic strips our error context away so
+        // `.revertedWithCustomError(paymaster, "PaymasterFlowNotSupported");`
+        // would not work
+        await expect(
+            flag.connect(userWallet).setFlag("flag captured", {
                 customData: {
                     gasPerPubdata: utils.DEFAULT_GAS_PER_PUBDATA_LIMIT,
                     paymasterParams,
                 },
-            });
-            expect.fail("Should have reverted");
-        } catch (e) {
-            expect(e.message).to.contain("execution reverted");
-        }
+            })
+        ).to.be.revertedWithoutReason();
     });
 
     it("Supports calls to whitelisted contracts", async function () {
         const paymasterParams = utils.getPaymasterParams(await paymaster.getAddress(), {
             type: "General",
             innerInput: new Uint8Array(),
         });
-
-        const tx = await flag.connect(userWallet).setFlag("flag captured", {
+        
+        await flag.connect(userWallet).setFlag("flag captured", {
             customData: {
                 gasPerPubdata: utils.DEFAULT_GAS_PER_PUBDATA_LIMIT,
                 paymasterParams,
             },
         });
-        await tx.wait();
 
         expect(await flag.flag()).to.equal("flag captured");
     });
@@ -123,17 +124,14 @@ describe("WhitelistPaymaster", function () {
             innerInput: new Uint8Array(),
         });
 
-        try {
-            await newFlag.connect(userWallet).setFlag("flag captured", {
+        await expect(
+            newFlag.connect(userWallet).setFlag("flag captured", {
                 customData: {
                     gasPerPubdata: utils.DEFAULT_GAS_PER_PUBDATA_LIMIT,
                     paymasterParams,
                 },
-            });
-            expect.fail("Should have reverted");
-        } catch (e) {
-            expect(e.message).to.contain("execution reverted");
-        }
+            })
+        ).to.be.revertedWithoutReason();
     });
 
     it("Does not support calls from non-whitelisted users", async function () {
@@ -142,16 +140,13 @@ describe("WhitelistPaymaster", function () {
             innerInput: new Uint8Array(),
         });
 
-        try {
-            await flag.connect(sponsorWallet).setFlag("flag captured", {
+        await expect(
+            flag.connect(sponsorWallet).setFlag("flag captured", {
                 customData: {
                     gasPerPubdata: utils.DEFAULT_GAS_PER_PUBDATA_LIMIT,
                     paymasterParams,
                 },
-            });
-            expect.fail("Should have reverted");
-        } catch (e) {
-            expect(e.message).to.contain("execution reverted");
-        }
+            })
+        ).to.be.revertedWithoutReason();
     });
 });

contracts/test/paymasters/helpers.ts

@@ -11,12 +11,8 @@ export const setupEnv = async (paymasterContract: string, additionalArgs: any[]
 
     const paymaster = await deployContract(paymasterContract, [adminWallet.address, ...additionalArgs], { wallet: adminWallet, silent: true, skipChecks: true });
 
-    let tx = await paymaster.connect(adminWallet).grantRole(await paymaster.WITHDRAWER_ROLE(), withdrawerWallet.address);
-    await tx.wait();
-
-    // send some ETH to it
-    tx = await sponsorWallet.sendTransaction({ to: await paymaster.getAddress(), value: ethers.parseEther("1") });
-    await tx.wait();
+    await paymaster.connect(adminWallet).grantRole(await paymaster.WITHDRAWER_ROLE(), withdrawerWallet.address);
+    await sponsorWallet.sendTransaction({ to: await paymaster.getAddress(), value: ethers.parseEther("1") });
 
     const emptyWallet = Wallet.createRandom();
     const userWallet = new Wallet(emptyWallet.privateKey, provider);