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

Incosistent Gas Report Per Test. Caching? #77

Open
simondlr opened this issue Sep 11, 2021 · 4 comments
Open

Incosistent Gas Report Per Test. Caching? #77

simondlr opened this issue Sep 11, 2021 · 4 comments

Comments

@simondlr
Copy link

Hey. Trying to figure out why gas-report is showing inconsistent gas reporting per unit test. The averages box seems correct, but the gas reported per test seems off/wrong?

I'm using a hardhat node. 2.6.1
And secondly, using gas-reporter 1.0.4.

Example of quirk. This test uses 17m gas because it's deploying two contracts + doing some individual transactions.
✓ S: test claim by transferring OG id to another account after claimed. (17735020 gas)

If you copy the test, the second test shows a much smaller gas usage number.

✓ S: test claim by transferring OG id to another account after claimed. (17735008 gas)
✓ S: test claim by transferring OG id to another account after claimed. (24608 gas)

Here's the test. It mints an NFT and then allows an original holder to claim an NFT of a new project.

  it("S: test claim by transferring OG id to another account after claimed.", async () => {
    const s2 = await S.deploy("Souls", "SOULS3", accounts[2], '100', '3541431094', '0xaF69610ea9ddc95883f97a6a3171d52165b69B03'); // wide campaign window for tests. dates tested separately
    await s2.deployed();
    const tx = await s2.connect(signers[1]).mintSoul({value: ethers.utils.parseEther(dxPrice), gasLimit});
    const receipt = await tx.wait();
    const tokenId = receipt.events[0].args.tokenId.toString();

    const s3 = await S.deploy("Souls", "SOULS2", accounts[2], '100', '3541431094', s2.address); // wide campaign window for tests. dates tested separately
    await s3.deployed();

    await s3.connect(signers[1]).claimSoul(tokenId, {gasLimit});
    await s2.connect(signers[1]).transferFrom(accounts[1], accounts[2], tokenId, {gasLimit});

    await expect(s3.connect(signers[2]).claimSoul(tokenId, {gasLimit})).to.be.revertedWith("AC_ID ALREADY CLAIMED");
  });

The only indication I can gather here is that because I'm reverting to a clean snapshot in between each test, that the contracts that end up being deployed have the same addresses.

 this.beforeEach(async function() {
    await provider.send('evm_revert', [snapshot]);
    snapshot = await provider.send('evm_snapshot', []);
  });

So, maybe this revert process is tripping up gas reporting? Is it the hardhat node doing optimizations automatically (no idea what it could be), or is the gas reporter? It's not urgent since I'm more interested in the average numbers of function calls vs gas per test, which seems to be reporting appropriately.

Any thoughts?

@simondlr simondlr changed the title Caching? Incosistent Gas Report Per Test. Caching? Sep 11, 2021
@cgewecke
Copy link
Owner

@simondlr I think something is cacheing but if you're deploying within the it block, snapshot/revert shouldn't interfere with the gas spent.

Are you using hardhat_reset to do that? Or evm_snapshot, etc?

Are you using any other plugins like hardhat-deploy? If so I think hh-deploy is smart about using deployed contracts when possible ... could be happening there.

@simondlr
Copy link
Author

simondlr commented Sep 13, 2021

Hey @cgewecke!

using evm_snapshot & custom deploy.

snapshot/revert shouldn't interfere with the gas spent.

I believe that the revert is causing all kinds of problems. I did some more tests.

beforeAll is only used to create a snapshot. In the following report, the snapshot is not saved. And then, if you are not doing revert in beforeEach, then all tests produce the same gas report.

✓ S: test claim by transferring OG id to another account after claimed. (18016259 gas)
✓ S: test claim by transferring OG id to another account after claimed. (18016259 gas)
✓ S: test claim by transferring OG id to another account after claimed. (18016259 gas)

But. If you add in a snapshot in beforeAll.

  this.beforeAll(async function() {
    provider = new ethers.providers.Web3Provider(web3.currentProvider);
    signers = await ethers.getSigners();
    accounts = await Promise.all(signers.map(async function(signer) {return await signer.getAddress(); }));
    S = await ethers.getContractFactory("Souls");
    snapshot = await provider.send('evm_snapshot', []);
  });

and then add the revert in beforeEach back in again, it throws the wonky gas reports.

✓ S: test claim by transferring OG id to another account after claimed. (17886795 gas)
✓ S: test claim by transferring OG id to another account after claimed. (129464 gas)
✓ S: test claim by transferring OG id to another account after claimed. (129464 gas)

Not sure where is the best next step to do more debugging. It doesn't make sense why the first test would be different here either.

@cgewecke
Copy link
Owner

Ah ok, thanks. Will take a look and see if I can reproduce.

@simondlr
Copy link
Author

There might be a simpler test here that replicates the issues, but it's not a big problem atm for me, so not prioritisting it. Let me know however if you need more samples or configurations. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants