wighawag / hardhat-deploy

hardhat deployment plugin
MIT License
1.17k stars 282 forks source link

factoryDepths for zksync is incorrectly built #537

Open novaknole opened 2 months ago

novaknole commented 2 months ago

Describe the bug As we know, zksync introduces factoryDepths field in the transaction to make sure bytecodes are known.

Assume the following contracts:

contract Test2 { uint public x = 35; }

contract Test1 {
   address public addr;

   function start() public {
       addr = address(new Test2());
   }
}

contract Main {
    address public addr;

    function start() {
          addr = address(new Test1());
    }
}

Inside hardhat-deploy script, let's assume we deploy the Main contract.

await deploy("Main", {
    contract: MainArtifact,
    from: wallet.address,
    args: [],
    log: true,
 });

What actually happens is before the actual deploy transaction is passed to the zksync operator, hardhat-deploy builds the factoryDepths. see. The problem is in this function, because for our contracts, factoryDepths array would only contain a single bytecode(Test1Bytecode), because it doesn't recursively goes through with artifacts. The deployment of Main works fine, but once you do the following steps after deployment:

  1. get Main's address and call start on it. It will deploy Test1 and store it on addr. Get this addr value.
  2. Then call start on that addr which would deploy Test2.
  3. Call x on the Test2.

This will fail with the code hash is not known. This is actually expected, because deployment transaction's factoryDepths didn't contain the bytecode of Test2.

If you look into the original package from zksync, you will realize that factoryDepths is correctly constructed. See. You can see that it recursively goes through artifacts to generate the correct factoryDepths. In this case, factoryDepths would contain bytecodes of Test1 and Test2 and we wouldn't get the same problems anymore.

versions


I wonder if this problem has been missed and why factoryDepths construction is not recursive in hardhat-deploy package.

@mikemcdonald made the PR with the extractFactoryDepths written in a wrong way in my opinion. What's your thoughts ? @wighawag

wighawag commented 2 months ago

@novaknole thanks for pointing this out,

I unfortunately do not know the intricacies as you seems to understand and I don't have the bandwidth to look at it. Do you think you could make a PR to fix it ?

novaknole commented 2 months ago

@wighawag hey Ronan, I just made a PR. Please take a look https://github.com/wighawag/hardhat-deploy/pull/541