-
Notifications
You must be signed in to change notification settings - Fork 55
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
Using --deploy-fixture (hardhat-deploy option) results in empty gas report #86
Comments
@rlogiacco Do you have a repo or branch I could look at to reproduce this? I suspect the problem may be coming from upgradeability & proxying rather than ethers. |
Sure I do: https://github.com/agileware-org/nft-collection branch upgradeable. Let me know if you need any help in understanding the repo |
@rlogiacco I wasn't able to reproduce this problem (on the hardhat network). After installing your repo, I commented out rinkeby, mainnet and default network settings in hardhat.config.ts (because I wasn't using a private key and it throws an error when loading) Then I ran I saw: Is something missing from this report? [EDIT: updated screenshot to show case when coinmarketcap env variable is set] |
@rlogiacco Sorry, you're right, I was on the wrong branch. On upgradeable without the The This should be fixable fwiw. There's special logic at eth-gas-reporter to handle a similar issue with Truffle migrations. We just need to inspect hardhat-deploy's artifacts and match contracts to their pre-deployed addresses. In the interim, is it viable for you to run your tests without that flag enabled if you want a gas report? Both commands seem to work (although --deploy-fixture may speed things up a bit) |
Sure, but the report becomes empty only if I use the ethers library in any of those deployment fixtures, while it works perfectly if I don't use the ethers lib in there. To verify just open the file In other words, the use of the ethers library in any deploy fixture breaks the gas report. I'm about to create a new branch (I'll call it Thanks a million for taking the time to help me on this. |
Here you go, and at this PR you can see the difference agileware-org/nft-collection#1 |
@cgewecke have you had any chance to review the link I provided? I believe it makes more clear why I'm considering this an issue with the ethers library... |
Apologies @rlogiacco I'm unsure what's happening in the new branch but perhaps the
Does it work with ethers and without the If so the differences you're seeing are very likely due to how hardhat-deploy behaves. |
Actually it does work and it reports gas correctly if I remove the Do you suggest I close this and open an issue on the |
@rlogiacco No, I think it's something we should fix here (e.g at eth-gas-reporter). Thanks for discovering it. Am going to edit the title a bit to reflect the underlying issue. |
Just to add more data points for testing - I have recently seen that in an NFT project called Bleeps. Here's how to reproduce it: git clone https://github.com/wighawag/bleeps
cd bleeps/common-lib/
npm install
npm run build
cd ../contracts
sed -i 's|"bleeps-common": "workspace:\*",|"bleeps-common": "file:../common-lib/",|g' package.json
rm test/BleepsDAO.governor.test.ts # Quick'n'dirty workaround for https://github.com/wighawag/bleeps/issues/2
npm install npm-run-all
npm install
HARDHAT_DEPLOY_FIXTURE=true REPORT_GAS=true npx --no hardhat test
The author seems aware of the limitation because there's actually a separate |
One more thing that may or may not be related. I noticed that I do not get the report if I run tests with |
@cameel Ah I've never tried that actually. 😅 If you run I think it should find the purely mocha version of the reporter (which this plugin wraps) in the dependencies and connect to a client on |
Just tried adding this to the command from Bleeps (
In any case I think this answers my question - mocha is just using its own reporter and has to be configured to use |
I'm using hardhat-deploy and my deploy files don't use ethers, I use
I think it would be very cool if the reporter can work with hardhat-deploy because having the deployment files and being able to reuse it with --deploy-fixture, makes things really easy. Otherwise we need to redeploy from each test suite? |
We experienced similar problems with missing gas reports for functions in many different use cases. I was trying to reproduce the problem with a minimal configuration and I came to a conclusion that any usage of Please find an example in the repository https://github.com/nkuba/hardhat-gas-reporter-playground. After running the
It's enough to comment out the line referenced below to get a correct report.
|
In deployment script we workaround a bug in hardhat-gas-reporter cgewecke/hardhat-gas-reporter#86. For some reason using `hre.ethers` makes gas report broken, the gas usage for functions is not included in the report. This is a problem that shows up in other projects using the deployment scripts of this project, e.g. @keep-network/ecdsa.
Workaround ethers import in deployment script for a bug in hardhat-ethers-plugin This PR is a workaround for an issue with hardhat-gas-reporter plugin (cgewecke/hardhat-gas-reporter#86). When hre.ethers is used in the deployment script it causes a gas report to be missing executed function's data. For tests executed in this repository, reports are fine, but when we use `@threshold-network/solidity-contracts` as a dependency in other projects (e.g. `@keep-network/ecdsa`) it breaks the reports there.
There is a but 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 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.
ECDSA: Workaround hardhat-gas-reporter bug There is a bug in hardhat-gas-reporter that we need to work around in order 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 to true. 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.
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.
I suspect it will only work if your contract is upgradable |
It looks like by adding a call to
ethers
my gas report goes from being populated to completely empty.Here is the deploy script disabling the gas reporting: if I comment out the lines I've marked gar report comes back.
My dev deps are:
The text was updated successfully, but these errors were encountered: