Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Ownable tests #4657

Merged
merged 31 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
cf3c377
migrate Ownable tests to ethers
Amxx Oct 5, 2023
673ff79
fix lint
Amxx Oct 5, 2023
c9b4e8d
Update hardhat.config.js
Amxx Oct 5, 2023
c46a45e
add Ownable2Step
Amxx Oct 5, 2023
328a344
remove deploy helper
Amxx Oct 5, 2023
7c12232
add deprecation notices
Amxx Oct 5, 2023
517ad87
up
Amxx Oct 5, 2023
9002e5f
up
Amxx Oct 5, 2023
74558f8
remove .address when doing ethers call that support addressable
Amxx Oct 6, 2023
9ea2db1
Fix flaky test in ERC2981.behavior
ernestognw Oct 10, 2023
fd7100a
Update test/access/Ownable.test.js
Amxx Oct 10, 2023
1a9a046
Update test/access/Ownable.test.js
Amxx Oct 10, 2023
b569ca8
Fix lint
ernestognw Oct 10, 2023
b24fec4
Fix upgradeable tests
ernestognw Oct 10, 2023
5e96021
redesign signers/fixture
Amxx Oct 10, 2023
f382329
Fix vanilla tests by overriding require and readArtifact with sync an…
ernestognw Oct 10, 2023
95be46d
Revert "redesign signers/fixture"
Amxx Oct 10, 2023
55b0406
Optimize ethers.getSigners call by passing a promise to use within fi…
ernestognw Oct 10, 2023
e45e482
Slice ethers accounts
ernestognw Oct 11, 2023
b391c2f
rename
Amxx Oct 11, 2023
74bab81
override hre.ethers.getSigners
Amxx Oct 11, 2023
98e5ecd
unify coding style/naming between env-contracts.js and env-artifacts.js
Amxx Oct 11, 2023
4db1800
Attempt to fix tests by avoid overriding `this` in Truffle require
ernestognw Oct 12, 2023
0a8a69d
Revert "Attempt to fix tests by avoid overriding `this` in Truffle re…
Amxx Oct 12, 2023
ec3bfc5
Merge branch 'master' into test/migration/ownable
Amxx Oct 12, 2023
5d5a150
Force compile on upgradeable and coverage workflows
Amxx Oct 12, 2023
300fa1e
use cached getSigners() in fixtures
Amxx Oct 13, 2023
ae4381e
use describe instead of contract for ethers test that don't need the …
Amxx Oct 13, 2023
b02770e
add enviornment sanity check
Amxx Oct 16, 2023
4e6a1ca
skip signer slice when running coverage
Amxx Oct 16, 2023
69b91a0
up
Amxx Oct 16, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Comment on lines +97 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- name: Run coverage
run: npm run coverage
- uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }}
Expand Down
7 changes: 4 additions & 3 deletions hardhat.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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
Expand Down Expand Up @@ -99,7 +101,6 @@ module.exports = {
},
networks: {
hardhat: {
blockGasLimit: 10000000,
allowUnlimitedContractSize: !withOptimizations,
},
},
Expand Down
38 changes: 30 additions & 8 deletions hardhat/env-artifacts.js
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
36 changes: 26 additions & 10 deletions hardhat/env-contract.js
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// 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));
});
};
Expand Down
Loading
Loading