Migrate Ownable tests (#4657)

Co-authored-by: ernestognw <ernestognw@gmail.com>

.github/workflows/checks.yml

@@ -64,6 +64,8 @@ jobs:
           cp -rnT contracts lib/openzeppelin-contracts/contracts
       - name: Transpile to upgradeable
         run: bash scripts/upgradeable/transpile.sh
+      - name: Compile contracts # TODO: Remove after migrating tests to ethers
+        run: npm run compile
       - name: Run tests
         run: npm run test
       - name: Check linearisation of the inheritance graph
@@ -92,7 +94,10 @@ jobs:
       - uses: actions/checkout@v4
       - name: Set up environment
         uses: ./.github/actions/setup
-      - run: npm run coverage
+      - name: Compile contracts # TODO: Remove after migrating tests to ethers
+        run: npm run compile
+      - name: Run coverage
+        run: npm run coverage
       - uses: codecov/codecov-action@v3
         with:
           token: ${{ secrets.CODECOV_TOKEN }}

hardhat.config.js

@@ -55,7 +55,9 @@ const argv = require('yargs/yargs')()
     },
   }).argv;
 
-require('@nomiclabs/hardhat-truffle5');
+require('@nomiclabs/hardhat-truffle5'); // deprecated
+require('@nomicfoundation/hardhat-toolbox');
+require('@nomicfoundation/hardhat-ethers');
 require('hardhat-ignore-warnings');
 require('hardhat-exposed');
 require('solidity-docgen');
@@ -69,7 +71,7 @@ for (const f of fs.readdirSync(path.join(__dirname, 'hardhat'))) {
   require(path.join(__dirname, 'hardhat', f));
 }
 
-const withOptimizations = argv.gas || argv.compileMode === 'production';
+const withOptimizations = argv.gas || argv.coverage || argv.compileMode === 'production';
 
 /**
  * @type import('hardhat/config').HardhatUserConfig
@@ -99,7 +101,6 @@ module.exports = {
   },
   networks: {
     hardhat: {
-      blockGasLimit: 10000000,
       allowUnlimitedContractSize: !withOptimizations,
     },
   },

hardhat/env-artifacts.js

@@ -1,18 +1,40 @@
 const { HardhatError } = require('hardhat/internal/core/errors');
 
-// Modifies `artifacts.require(X)` so that instead of X it loads the XUpgradeable contract.
+function isExpectedError(e, suffix) {
+  // HH700: Artifact not found - from https://hardhat.org/hardhat-runner/docs/errors#HH700
+  return HardhatError.isHardhatError(e) && e.number === 700 && suffix !== '';
+}
+
+// Modifies the artifact require functions so that instead of X it loads the XUpgradeable contract.
 // This allows us to run the same test suite on both the original and the transpiled and renamed Upgradeable contracts.
+extendEnvironment(hre => {
+  const suffixes = ['UpgradeableWithInit', 'Upgradeable', ''];
 
-extendEnvironment(env => {
-  const artifactsRequire = env.artifacts.require;
+  // Truffe (deprecated)
+  const originalRequire = hre.artifacts.require;
+  hre.artifacts.require = function (name) {
+    for (const suffix of suffixes) {
+      try {
+        return originalRequire.call(this, name + suffix);
+      } catch (e) {
+        if (isExpectedError(e, suffix)) {
+          continue;
+        } else {
+          throw e;
+        }
+      }
+    }
+    throw new Error('Unreachable');
+  };
 
-  env.artifacts.require = name => {
-    for (const suffix of ['UpgradeableWithInit', 'Upgradeable', '']) {
+  // Ethers
+  const originalReadArtifact = hre.artifacts.readArtifact;
+  hre.artifacts.readArtifact = async function (name) {
+    for (const suffix of suffixes) {
       try {
-        return artifactsRequire(name + suffix);
+        return await originalReadArtifact.call(this, name + suffix);
       } catch (e) {
-        // HH700: Artifact not found - from https://hardhat.org/hardhat-runner/docs/errors#HH700
-        if (HardhatError.isHardhatError(e) && e.number === 700 && suffix !== '') {
+        if (isExpectedError(e, suffix)) {
           continue;
         } else {
           throw e;

hardhat/env-contract.js

@@ -1,24 +1,40 @@
-extendEnvironment(env => {
-  const { contract } = env;
+// Remove the default account from the accounts list used in tests, in order
+// to protect tests against accidentally passing due to the contract
+// deployer being used subsequently as function caller
+//
+// This operation affects:
+// - the accounts (and signersAsPromise) parameters of `contract` blocks
+// - the return of hre.ethers.getSigners()
+extendEnvironment(hre => {
+  // TODO: replace with a mocha root hook.
+  // (see https://github.com/sc-forks/solidity-coverage/issues/819#issuecomment-1762963679)
+  if (!process.env.COVERAGE) {
+    // override hre.ethers.getSigner()
+    // note that we don't just discard the first signer, we also cache the value to improve speed.
+    const originalGetSigners = hre.ethers.getSigners;
+    const filteredSignersAsPromise = originalGetSigners().then(signers => signers.slice(1));
+    hre.ethers.getSigners = () => filteredSignersAsPromise;
+  }
 
-  env.contract = function (name, body) {
-    const { takeSnapshot } = require('@nomicfoundation/hardhat-network-helpers');
-
-    contract(name, accounts => {
-      // reset the state of the chain in between contract test suites
+  // override hre.contract
+  const originalContract = hre.contract;
+  hre.contract = function (name, body) {
+    originalContract.call(this, name, accounts => {
       let snapshot;
 
       before(async function () {
+        // reset the state of the chain in between contract test suites
+        // TODO: this should be removed when migration to ethers is over
+        const { takeSnapshot } = require('@nomicfoundation/hardhat-network-helpers');
         snapshot = await takeSnapshot();
       });
 
       after(async function () {
+        // reset the state of the chain in between contract test suites
+        // TODO: this should be removed when migration to ethers is over
         await snapshot.restore();
       });
 
-      // remove the default account from the accounts list used in tests, in order
-      // to protect tests against accidentally passing due to the contract
-      // deployer being used subsequently as function caller
       body(accounts.slice(1));
     });
   };

package.json

@@ -53,8 +53,11 @@
     "@changesets/cli": "^2.26.0",
     "@changesets/pre": "^1.0.14",
     "@changesets/read": "^0.5.9",
+    "@nomicfoundation/hardhat-chai-matchers": "^2.0.2",
+    "@nomicfoundation/hardhat-ethers": "^3.0.4",
     "@nomicfoundation/hardhat-foundry": "^1.1.1",
     "@nomicfoundation/hardhat-network-helpers": "^1.0.3",
+    "@nomicfoundation/hardhat-toolbox": "^3.0.0",
     "@nomiclabs/hardhat-truffle5": "^2.0.5",
     "@nomiclabs/hardhat-web3": "^2.0.0",
     "@openzeppelin/docs-utils": "^0.1.5",
@@ -68,9 +71,10 @@
     "eth-sig-util": "^3.0.0",
     "ethereumjs-util": "^7.0.7",
     "ethereumjs-wallet": "^1.0.1",
+    "ethers": "^6.7.1",
     "glob": "^10.3.5",
     "graphlib": "^2.1.8",
-    "hardhat": "^2.9.1",
+    "hardhat": "^2.17.4",
     "hardhat-exposed": "^0.3.13",
     "hardhat-gas-reporter": "^1.0.9",
     "hardhat-ignore-warnings": "^0.2.0",

test/access/Ownable.test.js

@@ -1,72 +1,73 @@
-const { constants, expectEvent } = require('@openzeppelin/test-helpers');
-const { expectRevertCustomError } = require('../helpers/customError');
-
-const { ZERO_ADDRESS } = constants;
-
+const { ethers } = require('hardhat');
 const { expect } = require('chai');
+const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
 
-const Ownable = artifacts.require('$Ownable');
-
-contract('Ownable', function (accounts) {
-  const [owner, other] = accounts;
+async function fixture() {
+  const [owner, other] = await ethers.getSigners();
+  const ownable = await ethers.deployContract('$Ownable', [owner]);
+  return { owner, other, ownable };
+}
 
+describe('Ownable', function () {
   beforeEach(async function () {
-    this.ownable = await Ownable.new(owner);
+    Object.assign(this, await loadFixture(fixture));
   });
 
   it('rejects zero address for initialOwner', async function () {
-    await expectRevertCustomError(Ownable.new(constants.ZERO_ADDRESS), 'OwnableInvalidOwner', [constants.ZERO_ADDRESS]);
+    await expect(ethers.deployContract('$Ownable', [ethers.ZeroAddress]))
+      .to.be.revertedWithCustomError({ interface: this.ownable.interface }, 'OwnableInvalidOwner')
+      .withArgs(ethers.ZeroAddress);
   });
 
   it('has an owner', async function () {
-    expect(await this.ownable.owner()).to.equal(owner);
+    expect(await this.ownable.owner()).to.equal(this.owner.address);
   });
 
   describe('transfer ownership', function () {
     it('changes owner after transfer', async function () {
-      const receipt = await this.ownable.transferOwnership(other, { from: owner });
-      expectEvent(receipt, 'OwnershipTransferred');
+      await expect(this.ownable.connect(this.owner).transferOwnership(this.other))
+        .to.emit(this.ownable, 'OwnershipTransferred')
+        .withArgs(this.owner.address, this.other.address);
 
-      expect(await this.ownable.owner()).to.equal(other);
+      expect(await this.ownable.owner()).to.equal(this.other.address);
     });
 
     it('prevents non-owners from transferring', async function () {
-      await expectRevertCustomError(
-        this.ownable.transferOwnership(other, { from: other }),
-        'OwnableUnauthorizedAccount',
-        [other],
-      );
+      await expect(this.ownable.connect(this.other).transferOwnership(this.other))
+        .to.be.revertedWithCustomError(this.ownable, 'OwnableUnauthorizedAccount')
+        .withArgs(this.other.address);
     });
 
     it('guards ownership against stuck state', async function () {
-      await expectRevertCustomError(
-        this.ownable.transferOwnership(ZERO_ADDRESS, { from: owner }),
-        'OwnableInvalidOwner',
-        [ZERO_ADDRESS],
-      );
+      await expect(this.ownable.connect(this.owner).transferOwnership(ethers.ZeroAddress))
+        .to.be.revertedWithCustomError(this.ownable, 'OwnableInvalidOwner')
+        .withArgs(ethers.ZeroAddress);
     });
   });
 
   describe('renounce ownership', function () {
     it('loses ownership after renouncement', async function () {
-      const receipt = await this.ownable.renounceOwnership({ from: owner });
-      expectEvent(receipt, 'OwnershipTransferred');
+      await expect(this.ownable.connect(this.owner).renounceOwnership())
+        .to.emit(this.ownable, 'OwnershipTransferred')
+        .withArgs(this.owner.address, ethers.ZeroAddress);
 
-      expect(await this.ownable.owner()).to.equal(ZERO_ADDRESS);
+      expect(await this.ownable.owner()).to.equal(ethers.ZeroAddress);
     });
 
     it('prevents non-owners from renouncement', async function () {
-      await expectRevertCustomError(this.ownable.renounceOwnership({ from: other }), 'OwnableUnauthorizedAccount', [
-        other,
-      ]);
+      await expect(this.ownable.connect(this.other).renounceOwnership())
+        .to.be.revertedWithCustomError(this.ownable, 'OwnableUnauthorizedAccount')
+        .withArgs(this.other.address);
     });
 
     it('allows to recover access using the internal _transferOwnership', async function () {
-      await this.ownable.renounceOwnership({ from: owner });
-      const receipt = await this.ownable.$_transferOwnership(other);
-      expectEvent(receipt, 'OwnershipTransferred');
+      await this.ownable.connect(this.owner).renounceOwnership();
+
+      await expect(this.ownable.$_transferOwnership(this.other))
+        .to.emit(this.ownable, 'OwnershipTransferred')
+        .withArgs(ethers.ZeroAddress, this.other.address);
 
-      expect(await this.ownable.owner()).to.equal(other);
+      expect(await this.ownable.owner()).to.equal(this.other.address);
     });
   });
 });

test/access/Ownable2Step.test.js

@@ -1,70 +1,85 @@
-const { constants, expectEvent } = require('@openzeppelin/test-helpers');
-const { ZERO_ADDRESS } = constants;
+const { ethers } = require('hardhat');
 const { expect } = require('chai');
-const { expectRevertCustomError } = require('../helpers/customError');
+const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
 
-const Ownable2Step = artifacts.require('$Ownable2Step');
-
-contract('Ownable2Step', function (accounts) {
-  const [owner, accountA, accountB] = accounts;
+async function fixture() {
+  const [owner, accountA, accountB] = await ethers.getSigners();
+  const ownable2Step = await ethers.deployContract('$Ownable2Step', [owner]);
+  return {
+    ownable2Step,
+    owner,
+    accountA,
+    accountB,
+  };
+}
 
+describe('Ownable2Step', function () {
   beforeEach(async function () {
-    this.ownable2Step = await Ownable2Step.new(owner);
+    Object.assign(this, await loadFixture(fixture));
   });
 
   describe('transfer ownership', function () {
     it('starting a transfer does not change owner', async function () {
-      const receipt = await this.ownable2Step.transferOwnership(accountA, { from: owner });
-      expectEvent(receipt, 'OwnershipTransferStarted', { previousOwner: owner, newOwner: accountA });
-      expect(await this.ownable2Step.owner()).to.equal(owner);
-      expect(await this.ownable2Step.pendingOwner()).to.equal(accountA);
+      await expect(this.ownable2Step.connect(this.owner).transferOwnership(this.accountA))
+        .to.emit(this.ownable2Step, 'OwnershipTransferStarted')
+        .withArgs(this.owner.address, this.accountA.address);
+
+      expect(await this.ownable2Step.owner()).to.equal(this.owner.address);
+      expect(await this.ownable2Step.pendingOwner()).to.equal(this.accountA.address);
     });
 
     it('changes owner after transfer', async function () {
-      await this.ownable2Step.transferOwnership(accountA, { from: owner });
-      const receipt = await this.ownable2Step.acceptOwnership({ from: accountA });
-      expectEvent(receipt, 'OwnershipTransferred', { previousOwner: owner, newOwner: accountA });
-      expect(await this.ownable2Step.owner()).to.equal(accountA);
-      expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA);
+      await this.ownable2Step.connect(this.owner).transferOwnership(this.accountA);
+
+      await expect(this.ownable2Step.connect(this.accountA).acceptOwnership())
+        .to.emit(this.ownable2Step, 'OwnershipTransferred')
+        .withArgs(this.owner.address, this.accountA.address);
+
+      expect(await this.ownable2Step.owner()).to.equal(this.accountA.address);
+      expect(await this.ownable2Step.pendingOwner()).to.equal(ethers.ZeroAddress);
     });
 
     it('guards transfer against invalid user', async function () {
-      await this.ownable2Step.transferOwnership(accountA, { from: owner });
-      await expectRevertCustomError(
-        this.ownable2Step.acceptOwnership({ from: accountB }),
-        'OwnableUnauthorizedAccount',
-        [accountB],
-      );
+      await this.ownable2Step.connect(this.owner).transferOwnership(this.accountA);
+
+      await expect(this.ownable2Step.connect(this.accountB).acceptOwnership())
+        .to.be.revertedWithCustomError(this.ownable2Step, 'OwnableUnauthorizedAccount')
+        .withArgs(this.accountB.address);
     });
   });
 
   describe('renouncing ownership', async function () {
     it('changes owner after renouncing ownership', async function () {
-      await this.ownable2Step.renounceOwnership({ from: owner });
+      await expect(this.ownable2Step.connect(this.owner).renounceOwnership())
+        .to.emit(this.ownable2Step, 'OwnershipTransferred')
+        .withArgs(this.owner.address, ethers.ZeroAddress);
+
       // If renounceOwnership is removed from parent an alternative is needed ...
       // without it is difficult to cleanly renounce with the two step process
       // see: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620#discussion_r957930388
-      expect(await this.ownable2Step.owner()).to.equal(ZERO_ADDRESS);
+      expect(await this.ownable2Step.owner()).to.equal(ethers.ZeroAddress);
     });
 
     it('pending owner resets after renouncing ownership', async function () {
-      await this.ownable2Step.transferOwnership(accountA, { from: owner });
-      expect(await this.ownable2Step.pendingOwner()).to.equal(accountA);
-      await this.ownable2Step.renounceOwnership({ from: owner });
-      expect(await this.ownable2Step.pendingOwner()).to.equal(ZERO_ADDRESS);
-      await expectRevertCustomError(
-        this.ownable2Step.acceptOwnership({ from: accountA }),
-        'OwnableUnauthorizedAccount',
-        [accountA],
-      );
+      await this.ownable2Step.connect(this.owner).transferOwnership(this.accountA);
+      expect(await this.ownable2Step.pendingOwner()).to.equal(this.accountA.address);
+
+      await this.ownable2Step.connect(this.owner).renounceOwnership();
+      expect(await this.ownable2Step.pendingOwner()).to.equal(ethers.ZeroAddress);
+
+      await expect(this.ownable2Step.connect(this.accountA).acceptOwnership())
+        .to.be.revertedWithCustomError(this.ownable2Step, 'OwnableUnauthorizedAccount')
+        .withArgs(this.accountA.address);
     });
 
     it('allows to recover access using the internal _transferOwnership', async function () {
-      await this.ownable2Step.renounceOwnership({ from: owner });
-      const receipt = await this.ownable2Step.$_transferOwnership(accountA);
-      expectEvent(receipt, 'OwnershipTransferred');
+      await this.ownable2Step.connect(this.owner).renounceOwnership();
+
+      await expect(this.ownable2Step.$_transferOwnership(this.accountA))
+        .to.emit(this.ownable2Step, 'OwnershipTransferred')
+        .withArgs(ethers.ZeroAddress, this.accountA.address);
 
-      expect(await this.ownable2Step.owner()).to.equal(accountA);
+      expect(await this.ownable2Step.owner()).to.equal(this.accountA.address);
     });
   });
 });

test/helpers/chainid.js

@@ -7,4 +7,6 @@ async function getChainId() {
 
 module.exports = {
   getChainId,
+  // TODO: when tests are ready to support bigint chainId
+  // getChainId: ethers.provider.getNetwork().then(network => network.chainId),
 };

test/helpers/create.js

@@ -1,22 +1,6 @@
-const RLP = require('rlp');
-
-function computeCreateAddress(deployer, nonce) {
-  return web3.utils.toChecksumAddress(web3.utils.sha3(RLP.encode([deployer.address ?? deployer, nonce])).slice(-40));
-}
-
-function computeCreate2Address(saltHex, bytecode, deployer) {
-  return web3.utils.toChecksumAddress(
-    web3.utils
-      .sha3(
-        `0x${['ff', deployer.address ?? deployer, saltHex, web3.utils.soliditySha3(bytecode)]
-          .map(x => x.replace(/0x/, ''))
-          .join('')}`,
-      )
-      .slice(-40),
-  );
-}
+const { ethers } = require('hardhat');
 
 module.exports = {
-  computeCreateAddress,
-  computeCreate2Address,
+  computeCreateAddress: (from, nonce) => ethers.getCreateAddress({ from, nonce }),
+  computeCreate2Address: (salt, bytecode, from) => ethers.getCreate2Address(from, salt, ethers.keccak256(bytecode)),
 };

test/helpers/customError.js

@@ -1,3 +1,5 @@
+// DEPRECATED: replace with hardhat-toolbox chai matchers.
+
 const { expect } = require('chai');
 
 /** Revert handler that supports custom errors. */

test/sanity.test.js

@@ -0,0 +1,40 @@
+const { ethers } = require('hardhat');
+const { expect } = require('chai');
+const { loadFixture, mine } = require('@nomicfoundation/hardhat-network-helpers');
+
+async function fixture() {
+  const signers = await ethers.getSigners();
+  const addresses = await Promise.all(signers.map(s => s.getAddress()));
+  return { signers, addresses };
+}
+
+contract('Environment sanity', function (accounts) {
+  beforeEach(async function () {
+    Object.assign(this, await loadFixture(fixture));
+  });
+
+  describe('[skip-on-coverage] signers', function () {
+    it('match accounts', async function () {
+      expect(this.addresses).to.deep.equal(accounts);
+    });
+
+    it('signer #0 is skipped', async function () {
+      const signer = await ethers.provider.getSigner(0);
+      expect(this.addresses).to.not.include(await signer.getAddress());
+    });
+  });
+
+  describe('snapshot', function () {
+    let blockNumberBefore;
+
+    it('cache and mine', async function () {
+      blockNumberBefore = await ethers.provider.getBlockNumber();
+      await mine();
+      expect(await ethers.provider.getBlockNumber()).to.be.equal(blockNumberBefore + 1);
+    });
+
+    it('check snapshot', async function () {
+      expect(await ethers.provider.getBlockNumber()).to.be.equal(blockNumberBefore);
+    });
+  });
+});

test/token/common/ERC2981.behavior.js

@@ -108,7 +108,7 @@ function shouldBehaveLikeERC2981() {
       const token2Info = await this.token.royaltyInfo(this.tokenId2, this.salePrice);
 
       // must be different even at the same this.salePrice
-      expect(token1Info[1]).to.not.be.equal(token2Info.royaltyFraction);
+      expect(token1Info[1]).to.not.be.bignumber.equal(token2Info[1]);
     });
 
     it('reverts if invalid parameters', async function () {