wighawag / hardhat-deploy

hardhat deployment plugin
MIT License
1.2k stars 296 forks source link

Fixtures Don't Seem to Work #328

Open hickscorp opened 2 years ago

hickscorp commented 2 years ago

We are migrating a hardhat project to use hardhat-deploy, as we wanted to also leverage the Diamond pattern baked into hardhat-deploy.

Consider the following test code:

import * as chai from 'chai';
import { expect } from 'chai';
import { deployments, ethers } from 'hardhat';
import { FakeContract, smock } from '@defi-wonderland/smock';
import { Spc, Fast, Exchange } from '../typechain';
import { SignerWithAddress } from 'hardhat-deploy-ethers/signers';
chai.use(smock.matchers);

describe('FastAccessFacet', () => {
  let
    deployer: SignerWithAddress,
    spcMember: SignerWithAddress,
    governor: SignerWithAddress;
  let spc: FakeContract<Spc>,
    exchange: FakeContract<Exchange>,
    fast: Fast,
    governedFast: Fast,
    spcMemberFast: Fast;
  let deployFast: (options?: unknown) => Promise<Fast>;

  before(async () => {
    // Keep track of a few signers.
    [deployer, spcMember, governor] = await ethers.getSigners();
    // Mock an SPC and a FAST.
    spc = await smock.fake('Spc');
    exchange = await smock.fake('Exchange');

    deployFast = deployments.createFixture(async (hre) => {
      const deploy = await deployments.diamond.deploy('Fast', {
        from: deployer.address,
        owner: spcMember.address,
        facets: ['FastFacet', 'FastAccessFacet', 'FastTokenFacet', 'FastHistoryFacet'],
        execute: {
          contract: 'FastInitFacet', methodName: 'initialize', args: [{
            spc: spc.address,
            exchange: exchange.address,
            governor: governor.address,
            name: 'Better, Stronger, FASTer.',
            symbol: 'FST',
            decimals: 18,
            hasFixedSupply: true,
            isSemiPublic: true
          }]
        }
      });

      return await ethers.getContractAt('Fast', deploy.address) as Fast;
    });
  });

  beforeEach(async () => {
    spc.isMember.returns(false);
    spc.isMember.whenCalledWith(spcMember.address).returns(true);

    fast = await deployFast();
    governedFast = fast.connect(governor);
    spcMemberFast = fast.connect(spcMember);
  });

  describe('IHasGovernors implementation', async () => {
    describe('addGovernor', async () => {
      it('requires SPC membership (anonymous)', async () => {
        const subject = fast.addGovernor(ethers.constants.AddressZero);
        // Check that the registry
        await expect(subject).to.be
          .revertedWith('Missing SPC membership');
      });
    });
  });
});

This code behaves differently whether we clean the deployments/hardhat folder before running it. The first time, it runs (as long as deployments/hardhat/ is empty). But for subsequent runs we get:

     Error: call revert exception [ See: https://links.ethers.org/v5-errors-CALL_EXCEPTION ] (method="facets()", data="0x", errorArgs=null, errorName=null, errorSignature=null, reason=null, code=CALL_EXCEPTION, version=abi/5.6.3)
      at Logger.makeError (node_modules/@ethersproject/logger/src.ts/index.ts:261:28)
      at Logger.throwError (node_modules/@ethersproject/logger/src.ts/index.ts:273:20)
      at Interface.decodeFunctionResult (node_modules/@ethersproject/abi/src.ts/interface.ts:427:23)
      at Contract.<anonymous> (node_modules/@ethersproject/contracts/src.ts/index.ts:400:44)
      at step (node_modules/@ethersproject/contracts/lib/index.js:48:23)
      at Object.next (node_modules/@ethersproject/contracts/lib/index.js:29:53)
      at fulfilled (node_modules/@ethersproject/contracts/lib/index.js:20:58)
hickscorp commented 2 years ago

Another problem, related to the same thing - if I use fixtures in different test files, but they all call deployments.diamond.deploy('Fast', ...), the error above is thrown. If I use a unique name per test file for the diamond, it works...

wighawag commented 2 years ago

hmm, deployments/hardhat should never exit. can you share your config ?

hickscorp commented 2 years ago

@wighawag Here goes:

From what you said, I should remove this part, right?

    hardhat: {
      live: false,
      saveDeployments: true,
      tags: ['test', 'local']
    },

If I remove this from my config, the tests fail, but I have no way to make them pass by deleting deployments/hardhat.

import 'dotenv/config';
import * as dotenv from 'dotenv';
import { HardhatUserConfig } from 'hardhat/config';
import '@typechain/hardhat';
import 'hardhat-deploy';
import 'hardhat-deploy-ethers';
import 'hardhat-diamond-abi';
import 'solidity-coverage';
import 'hardhat-gas-reporter';

// Loads `.env` file into `process.env`.
dotenv.config();

// Import all of our tasks here!
import './tasks/accounts';
...

const config: HardhatUserConfig = {
  solidity: {
    version: '0.8.4',
    settings: {
      outputSelection: {
        '*': {
          '*': ['storageLayout']
        }
      }
    }
  },
  diamondAbi: {
    name: 'Fast',
    include: [
      'IERC173',
      'IERC165',
      'IDiamondCut',
      'IDiamondLoupe',
      ...
    ]
  },
  networks: {
    hardhat: {
      live: false,
      saveDeployments: true,
      tags: ['test', 'local']
    },
    localhost: {
      live: false,
      saveDeployments: true,
      tags: ['local'],
    }
  },
  namedAccounts: {
    deployer: {
      hardhat: 0,
      localhost: 0
    },
    spcMember: {
      hardhat: 1,
      localhost: 1
    },
    fastGovernor: {
      hardhat: 2,
      localhost: 2
    }
  },
  typechain: {
    outDir: 'typechain',
    target: 'ethers-v5',
  },
  gasReporter: {
    enabled: process.env.REPORT_GAS !== undefined,
    gasPrice: 21
  }
};
export default config;
wighawag commented 2 years ago

Ok, the issue arise because you save the deployment via saveDeployments: true,

It should never be needed for hardhat as it is an in memory network

hickscorp commented 2 years ago

No, if I remove the hardhat network, then only the first test file to run works. All the others fail.

The only way I found to have all test files to pass is to have:

EDIT: Scratch that, none of these solution work.

hickscorp commented 2 years ago

@wighawag if you want to reproduce:

Try running yarn hardhat test - the first test file will pass, the second will fail. But if you run yarn hardhat test file1.test.ts && yarn hardhat test file2.test.ts, both will pass. For us it's a problem, because running test files one by one doesn't allow us to get overall coverage.

wighawag commented 2 years ago

If you can create such repo, it would be helpful But yes remove saveDeployments: true,

hickscorp commented 2 years ago

@wighawag I can't create a repo now - I might be able to opensource our repository - would that work? I removed saveDeployments: true - in fact I removed the whole hardhat network configuration. No luck so far.

hickscorp commented 2 years ago

@wighawag You can try https://github.com/tableturn/tt-white-contracts/commit/0364008a7e40c77db426f0c86ebcec0609d9c447

EDIT: Sorry - this is the commit that shows the bug: https://github.com/tableturn/tt-white-contracts/commit/30d7cd6de066aa0b2b8ac03686f097ec2dd4cab9

hickscorp commented 2 years ago

@wighawag any idea?

wighawag commented 2 years ago

Did not have time yet to look at that, what are the step to reproduce ?

hickscorp commented 2 years ago

Simply run the test suite. If you run yarn test, the first test using the Fast diamond will succeed. Any subsequent test file using the Fast diamond will fail. However, running test files individually works - eg yarn test filename.test.ts.

hickscorp commented 2 years ago

@wighawag Any chance that we could have a look at this?

wighawag commented 2 years ago

Hi @hickscorp I reproduced your issue locally today but did not have time to see what is causing this issue yet.

Might be worth debugging to see what is happening. hardhat-deploy fixture reset the blockchain state, but maybe some js variable are kept in memory and so the values changes when you execute one file test versus many.