Migrate Ownable tests #4657

Amxx · 2023-10-05T11:40:36Z

This is the first PR of a long series that will migrate from truffle + web3js to hardhat-toolbox + ethers v6.

The goal of this PR is to

setup the necessary packages
migrate a simple test file
- decide what format/syntax the new tests should use (how to deal with signers? use of fixture?)
identify the helpers needed
list the deprecated packages that should be removed once the transition is over

Obviously, this being a small test file, not all issues will appear from the start. This is intended. Here we resolve the most obvious things. As we progress in the migration, we will identify and resolve more subtle challenges. Homefully, we don't have to reconsider on past decision when we get there.

Guidelines

replace all numbers and BigNumber with bigint.

Deprecations:

@nomiclabs/hardhat-truffle5
@nomiclabs/hardhat-web3
@openzeppelin/test-helpers
web3
./helpers/customError

Possible deprecations (to evaluate in upcomming PR:

merkletreejs → @openzeppelin/merkle-tree
keccak256 → ethers?
eth-sig-util → ethers?
ethereumjs-util → ethers?
ethereumjs-wallet → ethers?

Question

what happens to ./hardhat/env-contracts.js
- body(accounts.slice(1)) has no effect over on ethers.getSigners()
- do we need snapshots if we use fixtures ?

PR Checklist

Tests
Changeset entry (run npx changeset add)

changeset-bot · 2023-10-05T11:40:39Z

⚠️ No Changeset found

Latest commit: 69b91a0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

hardhat.config.js

socket-security · 2023-10-05T11:46:02Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@nomicfoundation/hardhat-toolbox	3.0.0	environment	`+58`	62 MB	fvictorio
ethers	6.7.1	network, filesystem	`+6`	17.1 MB	ricmoo
@nomicfoundation/hardhat-chai-matchers	2.0.2	None	`+11`	17.7 MB	fvictorio
@nomicfoundation/hardhat-ethers	3.0.4	None	`+8`	17.4 MB	fvictorio

frangio · 2023-10-05T16:10:09Z

test/access/Ownable.test.js

+const { deploy, getFactory } = require('../helpers/deploy');
+
+async function fixture() {
+  const accounts = await ethers.getSigners(); // this is slow :/


We could cache it in a global helper. But it should run only once per contract test suite anyway.

This is easily cacheable in env.contract.js

Do you know how this will behave with ethers?
Would it work if in the tests we do this.accounts.shift() ?

I was thinking in something like:

extendEnvironment(env => { const { contract } = env; 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 let snapshot; let signers; before(async function () { signers = signers ?? await ethers.getSigners(); snapshot = await takeSnapshot(); }); after(async function () { 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 // The signers array is also cloned using the spread operator to avoid changes // to the original array. body([...signers].slice(1)); }); }; });

And we would get the signers as an argument of contract("...", accounts => { ... });
?

That makes sens, but it will also break the existing truffle tests, which we don't want.

what do you think of keeping a registry of already migrated contract suites?

Too maintenance heavy (and error prone) IMO.

We couldn't confirm such theory, but if that's the case, we should keep loadFixtures per contract suite and also this snapshot.

If loadFixture does a "per contract suite" snapshot, I don't see why we would have to also do a "cross contract suite" snapshot. That is making everything slower, and I really don't see what it brings in exchange. The only usecase I see for that, is if we wanted our test to all start with blockNumber = 0 ... and I don't think that is an assumption we should make. IMO our tests (each test.js file) should work regardless of the initial state of the chain.

we could do

body(accounts.slice(1), signers.slice(1));

And then do

// truffle, no change needed contract("...", accounts => { ... });

and

// ethers contract("...", (_, signers) => { ... });

Keeping the high-level snapshot makes sense if not all the contract suites implement a fixture. Let's try removing the high-level snapshot once we finish with the migration as well.

we could do

body(accounts.slice(1), signers.slice(1));

I like this, let's implement it that way 😄

Side note, I also added a comment in #4658 to keep track of things we should do to consolidate the migration once done

frangio · 2023-10-05T16:31:34Z

test/access/Ownable.test.js


-      expect(await this.ownable.owner()).to.equal(other);
+      expect(await this.ownable.owner()).to.equal(this.other.address);


Using .address everywhere will be a pain. There is an issue about it:

Use ethersjs v6 AddressLike type in hardhat-chai-matchers NomicFoundation/hardhat#4097

Are there any workarounds? Is it worth it to contribute to Hardhat to improve this?

I once wrote an helper that is

const getAddress = (account : string | { address: string }) : string => account.address ?? account;

but calling it everywhere is even worst.

Maybe it could be integrated by default ...

ethers.resolveAddress already does it even better ...

Note: I'd love to simplify the code by removing .address when its better supported, but I don't think we should wait for that. We should migrate with .address and then remove them when possible.

Agree with migrating using .address without waiting for Hardhat support.

For anyone curious, this PR NomicFoundation/hardhat#4449 resolves the issue in hardhat-chai-matchers

ernestognw

Looking good. It's very weird that the CI is failing for an ERC721 test. Any idea?

test/access/Ownable.test.js

ernestognw · 2023-10-10T03:59:09Z

test/access/Ownable.test.js


-      expect(await this.ownable.owner()).to.equal(other);
+      expect(await this.ownable.owner()).to.equal(this.other.address);


Agree with migrating using .address without waiting for Hardhat support.

Accidental approval

test/access/Ownable.test.js

Co-authored-by: Ernesto García <[email protected]>

ernestognw · 2023-10-10T18:12:34Z

Upgradeable tests are failing because we need to override env-artifacts to also override ethers' equivalent.

Taking a look

Amxx · 2023-10-10T20:47:12Z

In env-contract.js:

In before, we could do signer = await Promise.all(accounts.map(ethers.getSigner));
- If we do that, signer is not when doing body ... so its not possible to pass it ...
I don't like it, but we could technically do this.signer = await Promise.all(accounts.map(ethers.getSigner)); and that would be passed to the test's this
- but this this would not be available in loadFixture, were its super usefull.

I'm starting to think that env-contract.js should probably go

ernestognw · 2023-10-11T03:35:25Z

Tests are failing because custom errors can't be decoded. Not sure what may be causing it but perhaps it's related to #4349

ernestognw · 2023-10-12T05:50:04Z

After reviewing, I have the suspicion the decoding errors were caused by overriding the require with .call(this. This seemed to be interfering with Hardhat heuristics for decoding errors. We no longer need to bind this for the case of legacy tests so I reverted and tests seem to be passing.

LGTM if they pass.

.

ernestognw · 2023-10-12T06:01:29Z

Ok it didn't work. An ugly workaround for now would be to run npm run compile in the CI first. For some reason, it solves the issue locally after npm run clean. We should trying removing it after the migration

…quire" This reverts commit 4db1800.

Amxx · 2023-10-12T11:50:46Z

Checklist:

tests
upgradeable tests
coverage

Note: yarn coverage --testfiles test/access/Ownable.test.js locally doesnt give any error :/

Edit: this might ba the npm run compile that was added to the test workflow that is missing.

Amxx · 2023-10-12T14:35:02Z

Adding npm run compile to the test-upgradeable workflow fixed it.
Coverage still not working :/

…snapshot

ernestognw · 2023-10-17T04:17:55Z

.github/workflows/checks.yml

+      - name: Compile contracts # TODO: Remove after migrating tests to ethers
+        run: npm run compile


I don't think this is needed because the solcover.js config specifies that compileCommand: 'npm run compile'. Are you sure this actually did something?

Just curious, we can leave if it'll be removed at the end of the migration

I'm not sure it actually does somthing. I just added that to all workflows

Lets see if we can remove them after the migration.

ernestognw · 2023-10-17T04:26:53Z

hardhat/env-contract.js

+// - 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)


Honestly I don't see how to use these hooks because they require using a --require flag that I don't see exposed through Hardhat's test CLI (based on mocha).

pcaversaccio · 2023-10-18T15:16:30Z

I hope it's ok to ask two questions here:

Have you considered internally moving most tests (private functions and variables as exceptions) to Foundry? Also, IMO, the OZ test suite is very exhaustive, but many unit tests could be replaced with fuzz tests, e.g. you don't test for a specific value but fuzz msg.value directly, or the token minting amount etc. Nowadays, I only write unit tests where it's really required (e.g. you want to test something with address(0)) and for the rest I simply let the params be fuzzed.
Is there any specific reason why you still stick to JS after the migration instead of using TS?

Co-authored-by: ernestognw <[email protected]>

migrate Ownable tests to ethers

cf3c377

fix lint

673ff79

Amxx commented Oct 5, 2023

View reviewed changes

hardhat.config.js Outdated Show resolved Hide resolved

Update hardhat.config.js

c9b4e8d

Amxx added the tests Test suite and helpers. label Oct 5, 2023

Amxx modified the milestone: 5.1 Oct 5, 2023

Amxx added this to the tests migration milestone Oct 5, 2023

Amxx mentioned this pull request Oct 5, 2023

Test migration tracking #4658

Closed

Amxx added 5 commits October 5, 2023 15:47

add Ownable2Step

c46a45e

remove deploy helper

328a344

add deprecation notices

7c12232

up

517ad87

up

9002e5f

frangio reviewed Oct 5, 2023

View reviewed changes

remove .address when doing ethers call that support addressable

74558f8

ernestognw previously approved these changes Oct 10, 2023

View reviewed changes

Fix flaky test in ERC2981.behavior

9ea2db1

ernestognw added the ignore-changeset label Oct 10, 2023

Amxx commented Oct 10, 2023

View reviewed changes

test/access/Ownable.test.js Outdated Show resolved Hide resolved

Amxx and others added 3 commits October 10, 2023 18:09

Update test/access/Ownable.test.js

fd7100a

Update test/access/Ownable.test.js

1a9a046

Co-authored-by: Ernesto García <[email protected]>

Fix lint

b569ca8

Fix upgradeable tests

b24fec4

redesign signers/fixture

5e96021

Slice ethers accounts

e45e482

Amxx added 3 commits October 11, 2023 15:26

rename

b391c2f

override hre.ethers.getSigners

74bab81

unify coding style/naming between env-contracts.js and env-artifacts.js

98e5ecd

Amxx requested a review from ernestognw October 11, 2023 16:04

Attempt to fix tests by avoid overriding this in Truffle require

4db1800

ernestognw previously approved these changes Oct 12, 2023

View reviewed changes

ernestognw mentioned this pull request Oct 12, 2023

Add compile step in checks.yml #4675

Merged

3 tasks

Amxx added 2 commits October 12, 2023 11:30

Revert "Attempt to fix tests by avoid overriding this in Truffle re…

0a8a69d

…quire" This reverts commit 4db1800.

Merge branch 'master' into test/migration/ownable

ec3bfc5

Force compile on upgradeable and coverage workflows

5d5a150

Amxx added 2 commits October 13, 2023 15:49

use cached getSigners() in fixtures

300fa1e

use describe instead of contract for ethers test that don't need the …

ae4381e

…snapshot

cgewecke mentioned this pull request Oct 14, 2023

Coverage overwrites provider logic in extendEnvironment hook sc-forks/solidity-coverage#819

Open

Amxx added 3 commits October 16, 2023 12:32

add enviornment sanity check

b02770e

skip signer slice when running coverage

4e6a1ca

up

69b91a0

ernestognw approved these changes Oct 17, 2023

View reviewed changes

Amxx merged commit 149e1b7 into OpenZeppelin:master Oct 17, 2023
12 checks passed

Amxx deleted the test/migration/ownable branch October 17, 2023 08:06

ernestognw mentioned this pull request Oct 23, 2023

Migrate AccessControl tests #4694

Merged

3 tasks

ernestognw added a commit to ernestognw/openzeppelin-contracts that referenced this pull request Oct 26, 2023

Migrate Ownable tests (OpenZeppelin#4657)

da02c39

Co-authored-by: ernestognw <[email protected]>

ernestognw mentioned this pull request Oct 29, 2023

Migrate AccessManager tests to ethers #4710

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Ownable tests #4657

Migrate Ownable tests #4657

Amxx commented Oct 5, 2023 •

edited

Loading

changeset-bot bot commented Oct 5, 2023 •

edited

Loading

socket-security bot commented Oct 5, 2023 •

edited

Loading

frangio Oct 5, 2023

ernestognw Oct 10, 2023

Amxx Oct 10, 2023

ernestognw Oct 10, 2023

Amxx Oct 10, 2023 •

edited

Loading

Amxx Oct 10, 2023

Amxx Oct 10, 2023

Amxx Oct 10, 2023 •

edited

Loading

ernestognw Oct 10, 2023

ernestognw Oct 10, 2023 •

edited

Loading

frangio Oct 5, 2023

Amxx Oct 5, 2023 •

edited

Loading

Amxx Oct 5, 2023

Amxx Oct 6, 2023

ernestognw Oct 10, 2023

Amxx Oct 10, 2023

ernestognw left a comment

ernestognw Oct 10, 2023

ernestognw commented Oct 10, 2023

Amxx commented Oct 10, 2023

ernestognw commented Oct 11, 2023

ernestognw commented Oct 12, 2023

ernestognw commented Oct 12, 2023

Amxx commented Oct 12, 2023 •

edited

Loading

Amxx commented Oct 12, 2023

ernestognw Oct 17, 2023

Amxx Oct 17, 2023

ernestognw Oct 17, 2023

pcaversaccio commented Oct 18, 2023


		expect(await this.ownable.owner()).to.equal(other);
		expect(await this.ownable.owner()).to.equal(this.other.address);

		- name: Compile contracts # TODO: Remove after migrating tests to ethers
		run: npm run compile

Migrate Ownable tests #4657

Migrate Ownable tests #4657

Conversation

Amxx commented Oct 5, 2023 • edited Loading

Guidelines

Deprecations:

Possible deprecations (to evaluate in upcomming PR:

Question

PR Checklist

changeset-bot bot commented Oct 5, 2023 • edited Loading

⚠️ No Changeset found

socket-security bot commented Oct 5, 2023 • edited Loading

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

Amxx Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amxx Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestognw Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amxx Oct 5, 2023 • edited Loading

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

ernestognw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestognw commented Oct 10, 2023

Amxx commented Oct 10, 2023

ernestognw commented Oct 11, 2023

ernestognw commented Oct 12, 2023

ernestognw commented Oct 12, 2023

Amxx commented Oct 12, 2023 • edited Loading

Amxx commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcaversaccio commented Oct 18, 2023

Amxx commented Oct 5, 2023 •

edited

Loading

changeset-bot bot commented Oct 5, 2023 •

edited

Loading

socket-security bot commented Oct 5, 2023 •

edited

Loading

Amxx Oct 10, 2023 •

edited

Loading

Amxx Oct 10, 2023 •

edited

Loading

ernestognw Oct 10, 2023 •

edited

Loading

Amxx Oct 5, 2023 •

edited

Loading

Amxx commented Oct 12, 2023 •

edited

Loading