Open rlogiacco opened 2 years ago
@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. I would agree with you if only some invocations were missing, like those going through a proxy, but they all disappear, including cost of contract deployment...
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 npx hardhat test
, setting the REPORT_GAS env variable to true (because your reporter config says gas reporting is only enabled when that variable is set).
I saw:
Is something missing from this report?
[EDIT: updated screenshot to show case when coinmarketcap env variable is set]
have you switched to the upgradeable
branch? I believe you are still on the main
branch because I don't see the invocation of the proxy upgrade operation.
The entire command I'm using is REPORT_GAS=y npx hardhat test --network hardhat --deploy-fixture
@rlogiacco Sorry, you're right, I was on the wrong branch. On upgradeable without the --deploy-fixture
flag I see a report which suggests the problem comes from the way you're using hardhat-deploy. Could you verify this is the case on your end?
The --deploy-fixture
flag tells hardhat-deploy to execute the deployments before the tests launch and the mocha reporter has no record of those contracts. It begins tracking deployments and matching contract invocations to contract addresses when the tests start.
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 deploy/2.DroppableCollectionFactory.ts
and comment line 11 (and anything related to that, obviously). Then the report will be there again even if you use the --deploy-fixture
flag.
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 with-gas-report
) with the same content but a few variations which do not require the use of the ethers library.
Thanks a million for taking the time to help me on this.
Here you go, and at this PR you can see the difference https://github.com/agileware-org/nft-collection/pull/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 get
method executes the deployments if they're missing during the test. That would result in them being detected by the reporter.
get
is part of hardhat-deploy
.
Does it work with ethers and without the --deploy-fixutre
flag?
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 --deploy-fixture
flag!!!
That's pretty surprising!
So I guess the get
method somewhat triggers something parallel to the what the --deploy-fiXture
does!
Do you suggest I close this and open an issue on the hardhat-deploy
project?
Do you suggest I close this and open an issue on the hardhat-deploy project?
@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
·--------------------------------------------------|---------------------------|----------------|-----------------------------·
| Solc version: 0.8.9 · Optimizer enabled: true · Runs: 999999 · Block limit: 50000000 gas │
···················································|···························|················|······························
| Methods │
·····························|·····················|·············|·············|················|·············|················
| Contract · Method · Min · Max · Avg · # calls · usd (avg) │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · delegateBySig · 76512 · 156596 · 118510 · 3 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · mint · 93503 · 127468 · 119195 · 152 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · permit · 40878 · 57966 · 49422 · 2 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · permitForAll · 58066 · 75174 · 66620 · 2 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · safeTransferFrom · 141843 · 147854 · 144011 · 16 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · safeTransferFrom · 142445 · 148745 · 144566 · 12 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · setApprovalForAll · 24279 · 46203 · 42825 · 26 · - │
·····························|·····················|·············|·············|················|·············|················
| Bleeps · setMinter · 47171 · 47183 · 47181 · 120 · - │
·····························|·····················|·············|·············|················|·············|················
| ERC20 · approve · 28982 · 51429 · 49699 · 26 · - │
·····························|·····················|·············|·············|················|·············|················
| ERC20 · transferFrom · 107631 · 147110 · 138121 · 53 · - │
·····························|·····················|·············|·············|················|·············|················
| MeloBleeps · mint · - · - · 120110 · 2 · - │
·····························|·····················|·············|·············|················|·············|················
| MeloBleeps · reserve · - · - · 139861 · 2 · - │
·····························|·····················|·············|·············|················|·············|················
| MeloBleeps · setMinter · - · - · 30061 · 2 · - │
·····························|·····················|·············|·············|················|·············|················
| OpenSeaProxyRegistryMock · setProxy · - · - · 43895 · 1 · - │
·----------------------------|---------------------|-------------|-------------|----------------|-------------|---------------·
81 passing (2m)
The author seems aware of the limitation because there's actually a separate npm run void:deploy
command that executes hardhat deploy --report-gas
. For me it would be very convenient if the deployment cost was included in the main report though.
One more thing that may or may not be related. I noticed that I do not get the report if I run tests with mocha
. I only get them with hardhat test
directly. You can see that if you replace hardhat test
in the above repro with npm run test
. Is that something I should report as a bug or some known limitation?
@cameel Ah I've never tried that actually. 😅
If you run mocha --reporter eth-gas-reporter
does anything happen?
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 localhost:8545
. The options here are mostly mocha reporterOptions
consumed by eth-gas-reporter.
Just tried adding this to the command from Bleeps (HARDHAT_DEPLOY_FIXTURE=true HARDHAT_COMPILE=true mocha --bail --recursive test
) and looks like it invoked eth-gas-reporter
but it had problems connecting:
ERROR: eth-gas-reporter was unable to resolve a client url from the provider available in your test context. Try setting the url as a mocha reporter option (ex: url='http://localhost:8545')
In any case I think this answers my question - mocha is just using its own reporter and has to be configured to use eth-gas-reporter
- which is what Hardhat does by default. This unfortunately does not solve the problem I had with it - I was hoping I could somehow get an unmodified npm run test
command to report gas - but it's also not like it's a huge problem. npm run test
would just be a bit less likely to break in case a project changes the command to run tests in a different way. I just thought it might have been a bug in the reporter but looks like it's not :)
I'm using hardhat-deploy and my deploy files don't use ethers, I use
import { DeployFunction } from 'hardhat-deploy/types';
const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
const { deployments, getNamedAccounts } = hre;
const { deploy } = deployments;
const { deployer } = await getNamedAccounts();
await deploy('Contract', {
from: deployer,
log: true,
});
};
export default func;
func.tags = ['Contract'];
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 ethers
in the deployment script along with the --deploy-fixture
flag causes the gas report to be broken.
Please find an example in the repository https://github.com/nkuba/hardhat-gas-reporter-playground.
After running the yarn test
you will get a following report:
·-----------------------|----------------------------|-------------|-----------------------------·
| Solc version: 0.8.9 · Optimizer enabled: false · Runs: 200 · Block limit: 30000000 gas │
························|····························|·············|······························
| Methods │
·············|··········|··············|·············|·············|·············|················
| Contract · Method · Min · Max · Avg · # calls · eur (avg) │
·------------|----------|--------------|-------------|-------------|-------------|---------------·
1 passing (173ms)
It's enough to comment out the line referenced below to get a correct report.
ethers.utils.isAddress("0xFaKeAddrEss")
·-------------------------------|----------------------------|-------------|-----------------------------·
| Solc version: 0.8.9 · Optimizer enabled: false · Runs: 200 · Block limit: 30000000 gas │
································|····························|·············|······························
| Methods │
·················|··············|··············|·············|·············|···············|··············
| Contract · Method · Min · Max · Avg · # calls · eur (avg) │
·················|··············|··············|·············|·············|···············|··············
| TestContract · initialize · - · - · 68609 · 1 · - │
·················|··············|··············|·············|·············|···············|··············
| TestContract · setValue · - · - · 30190 · 1 · - │
·················|··············|··············|·············|·············|···············|··············
| Deployments · · % of limit · │
································|··············|·············|·············|···············|··············
| TestContract · - · - · 341398 · 1.1 % · - │
·-------------------------------|--------------|-------------|-------------|---------------|-------------·
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: