-
Notifications
You must be signed in to change notification settings - Fork 73
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
ECDSA: Workaround hardhat-gas-reporter bug #2970
Conversation
This version contains improvements from threshold-network/solidity-contracts#100 which help workaround problems with gas reporter.
There is a bug in hardhat-gas-reporter that we need to workaround in order to obtain gas report for tests: cgewecke/hardhat-gas-reporter#86 Long story short: we cannot use `hre.ethers` in the deployment script. Here we introduce an alternative deployment path just to get the report generated correctly. The alternative path usage is determined by `GAS_REPORTER_BUG_WORKAROUND` env property set to `true`. To run the tests with the alternative path to get the gas report execute `yarn test:gas-reporter-workaround`. It will execute the same test (upgrade tests were skipped as they are strictly related to the original deployment path). `yarn test` is still a preferred way to run the tests, as it does the deployment exactly the way we want it to test for mainnet. Use the alternative path only if you wish to get the gas report. Once a bug in the `hardhat-gas-reporter` is fixed, we should revert all the changes from this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one non-blocking comment. Let's address it in a separate PR @nkuba
We introduced an alternative deployment path as a workaround. | ||
To execute tests with the alternative deployment path run `yarn test:gas-reporter-workaround`. | ||
Please keep in mind that `yarn test` is preferred way for testing. Use alternative | ||
path only if you want to get the gas report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we clarify what are the differences between the alternative path and the default path?
My understanding is that the default one uses OpenZeppelin plugin for upgrades to deploy proxies and the alternative path is still deploying proxies but not using the OpenZeppelin plugin.
In #2970 we introduced a workaround for a bug in hardhat-gas-reporter plugin cgewecke/hardhat-gas-reporter#86. It turned out that there is a much simpler workaround, which is based on just removing `--deploy-fixture` flag. With the flag removed it is required that we load a global snapshot of the deployment (`deployments.fixture()`) instead of loading a specific tag (`deployments.fixture(<tag>)`. Which we do anyway after one of a recent code updates. We removed all the traces of the previous workaround.
…orkaround Workaround hardhat-gas-reporter bug with removing --deploy-fixture In this PR we replace a previous workaround (#2970) for a bug in the hardhat-gas-reporter plugin (cgewecke/hardhat-gas-reporter#86). It turned out that there is a much simpler workaround, which is based on just removing --deploy-fixture flag. With the flag removed it is required that we load a global snapshot of the deployment (deployments.fixture()) instead of loading a specific tag (deployments.fixture(<tag>). Which we do anyway after one of a recent code updates.
Depends on #2969 (draft until it's merged)
Refs #2955
There is a bug in
hardhat-gas-reporter
that we need to work around inorder to obtain a gas report for tests: cgewecke/hardhat-gas-reporter#86
Long story short: we cannot use
hre.ethers
in the deployment scripts.Here we introduce an alternative deployment path just to get the report generated correctly. The alternative path usage is determined by the
GAS_REPORTER_BUG_WORKAROUND
env property set totrue
.To run the tests with the alternative path to get the gas report execute
yarn test:gas-reporter-workaround
command. It will execute the same tests (upgrade tests were skipped as they are strictly related to the original deployment path).yarn test
is still a preferred way to run the tests, as it does the deployment exactly the way we want it to test for mainnet. Use the alternative path only if you wish to get the gas report.Once a bug in the
hardhat-gas-reporter
is fixed, we should revert all the changes related to the workaround.