Coverage overwrites provider logic in extendEnvironment hook

[Amxx]

Here is a minimum reproductible example, based on OpenZeppelin's Ownable:

const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');

async function fixture() {
  const [owner, other] = await ethers.getSigners();
  const ownable = await ethers.deployContract('$Ownable', [owner]);
  return { owner, other, ownable };
}

describe('Ownable', function () {
  beforeEach(async function () {
    Object.assign(this, await loadFixture(fixture));
  });

  it('run #1', async function () {
    expect(await this.ownable.owner()).to.equal(this.owner.address, "unexpected ownership before");
    await this.ownable.connect(this.owner).transferOwnership(this.other);
    expect(await this.ownable.owner()).to.equal(this.other.address, "unexpected ownership after");
  });

  it('run #2', async function () {
    expect(await this.ownable.owner()).to.equal(this.owner.address, "unexpected ownership before");
    await this.ownable.connect(this.owner).transferOwnership(this.other);
    expect(await this.ownable.owner()).to.equal(this.other.address, "unexpected ownership after");
  });
});

What it does:

• define a fixture that get 2 accounts and create an ownable contract held by owner
• before each test, load that fixture.
• in run number one
  • check that owner, it the actual owner of the contract
  • transfer the ownership to other
  • check that other, it the new owner of the contract
• in run number two
  • do the exact same as run number one.

When running this test "normally", everything is fine !
When running this test in "coverage" mode, the second run fails with error

1) Contract: Ownable
       run #2:

      unexpected ownership before
      + expected - actual

      -0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
      +0x70997970C51812dc3A010C7d01b50e0d17dc79C8
      
      at Context.<anonymous> (test/access/minimal.test.js:23:43)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

It looks like the snapshot is not restored properly.

[Amxx]

Here is a reproductible example that does not require any artefact:

const { ethers } = require('hardhat');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');

async function fixture() {
  const [owner, other] = await ethers.getSigners();
  return { owner, other };
}

describe('Ownable', function () {
  beforeEach(async function () {
    Object.assign(this, await loadFixture(fixture));
    console.log("beforeEach:", await ethers.provider.getBlockNumber())
  });

  afterEach(async function () {
    console.log("afterEach:", await ethers.provider.getBlockNumber())
  });

  it('run #1', async function () {
    await this.owner.sendTransaction({ to: this.other });
  });

  it('run #2', async function () {
    await this.owner.sendTransaction({ to: this.other });
  });
});

it shows that when running the second it, the block number was not reset.

[cgewecke]

@Amxx Thanks for these reproductions. Will take a look.

[cgewecke]

Tried to reproduce the loadFixture failure here but it works as expected.

I think the problem in the Ownable PR you're working on is caused by redefining hre.ethers.getSigners in an extendEnvironment hook. Because the solidity-coverage task creates a new tracing-enabled provider with special settings, the extendEnvironment function no longer refers to the provider which runs the tests.

The execution stack at HH looks like:

• run extendProvider hook
  • modify existing provider behavior
• run coverage task
  • create new provider, discard old provider
  • run test task

One solution could be to move your signers logic to a mocha root hook where it would run as part of the test task after solidity-coverage has done its own provider stuff.

Was able to get the Ownable tests working in a fork of your PR here:

https://github.com/cgewecke/openzeppelin-contracts/actions/runs/6518313002/job/17703604132

A couple commits were necessary, including commenting out the coverage specific blockLimit in hardhat.config.ts. I think it's best if the coverage task sets that value but lmk if it's not working for some reason.

[cgewecke]

Closing because Zeppelin 4657 merged. Lmk if you think something else needs to be addressed.

[Amxx]

Lmk if you think something else needs to be addressed.

We found a dirty work around because we needed to move on, but its not a fix. We had to disable some of our env test from running when coverage is enabled ... and we know that traditional tests are no running in the same environment as coverage tests (this part if probably not affecting the security to much, but its not nice)

[Amxx]

And the issue can be summarized as:

Environment extensions (such as overriden hre.ethers.getSigners) will break the snpashotting that is part of hardhat-network-helpers's loadFixture.

I understand that its not as bad as loadFixture is not compatible with coverage ... but I also don't think its a non-issue because "devs just have to avoid such environment extensions".

[cgewecke]

We had to disable some of our env test from running when coverage is enabled ... and we know that traditional tests are no running in the same environment as coverage tests

Is it not possible to move the hardhat signers override to a mocha root hook(as suggested above)?

Unfortunately the coverage task has to recreate the provider and afaik there's no way to re-trigger the extendEnvironment hooks from within a hardhat task. Fwiw this problem has not arisen very frequently - it sometimes happens with other plugins but I suspect it's relatively uncommon for people to try to cache provider values the way you have.

(Am going to retitle the issue so it's more easily discoverable)

[cgewecke]

Re-opening because it may be possible to fix this by moving the plugin's provider recreation logic to a hardhat extendConfig hook where it won't conflict with downstream user provider mods.

[cgewecke]

Another update: in v0.8.12, the plugin defaults to using hardhat's extendConfig hook to configure the provider if the environment variable SOLIDITY_COVERAGE is set to true, e.g:

SOLIDITY_COVERAGE=true npx hardhat coverage

This prevents the provider from being overwritten in the task & preserves existing user modifications.

Unfortunately there's no other way to tell the plugin it's being invoked early in the command launch sequence and the changes solidity-coverage makes to the provider should only happen when it's being called. (Have documented this in the README trouble-shooting section)

(Leaving issue open until there's some structural change at HH that removes this requirement)