Inconsistencies in Diamond Post Deployment Function Execution

[unibeck]

I am attempting to deploy my Diamond contract whose facets' require initialization. Before using Diamonds this was done via a constructor, however, hardhat-deploy does not support facets with constructors. Thus, I am stuck using an initializer function that is called post deployment that will act as a constructor. For example:

execute: {
      methodName: 'initTuffToken',
      args: [contractOwner],
}, 

While this works in an isolated facet deployment, I see inconsistencies with the default value for variables when deploying multiple facets. For example, if I first deploy Facet1 its bool and uint variables will default to false and 0 respectively. This is correct per solidity's spec. However, when I then deploy Facet2 after Facet1, the default values for Facet2 are the opposite true and 1. Digging into this further it appears that the default values alternate as you deploy each additional facet.

You can find a reproducible example, with contracts and deployment scripts, here. I am not sure if this is the behavior of the Diamond standard (thoughts @mudgen ?) or if hardhat-deploy is responsible (thoughts @wighawag ?). In my attempt to solve this I updated hardhat-deploy to use the latest diamond contracts (per #160), though I still encounter the same issue.

My team and I are willing to chat further via Discord, email, or even a video call/screenshare. This is extremely important to us as it is blocking our development. We would appreciate any insight, direction, or feedback on this issue. Thank you.

[wighawag]

HI @unibeck thanks for the report, need to spend more time to look into it, but one thing you can do for facet is deploy them first the way you want (with whatever constructor argument you want, which have to be immutable to be considered as DIamond is a proxy), and then use their name in the deployment of the diamond, this should work.

[unibeck]

@wighawag thank you for the prompt response. Are you suggesting something like this?

module.exports = async () => {
  const { deployments, getNamedAccounts } = hre;
  const { deployer, contractOwner } = await getNamedAccounts();

  console.log(`Deployer address [${deployer}]`)
  console.log(`Contract owner address [${contractOwner}]`)

  const tuffToken = await deployments.deploy('TuffToken', {
    from: deployer,
    args: [contractOwner],
    log: true,
  });

  const aaveLPManager = await deployments.deploy('AaveLPManager', {
    from: deployer,
    log: true,
  });

  let tuffTokenDiamondDeployment = await deployments.diamond.deploy('TuffTokenDiamond', {
    from: deployer,
    owner: contractOwner,
    facets: ['TuffToken', 'AaveLPManager']
  });
};

I get a Error: Facet with constructor not yet supported error when trying to run a deployment like that

[wighawag]

Ok, I see, I ll have to look deeper

[mudgen]

The diamondCut function from EIP-2535 Diamonds is used to upgrade a diamond and execute an optional function using delegatecall to initialize state variables for the upgrade and/or execute other logic. @wighawag Does hardhat-deploy let users execute the optional function to initialize/setup upgrades?

Info on diamondCut here: https://eips.ethereum.org/EIPS/eip-2535#the-diamondcut-function

[wighawag]

HI @mudgen yes hardhat-deploy let you execute a function at the time of creation and upgrades

[unibeck]

@wighawag I went ahead and added support for facets by artifact, instead of by name. https://github.com/BuffChain/hardhat-deploy/tree/feature/support_facet_deployments_by_artifact. I did this because 1) it was a todo and 2) added flexibility to deploying contract that need special care. I was hoping that deploying the two facet contracts manually, then "initializing" them, then including them into the diamond contract would remove the bug we see above. However, it sadly does not.

Using the aforementioned fork, take this code for example:

module.exports = async () => {
  const { deployments, getNamedAccounts } = hre;
  const { deployer, contractOwner } = await getNamedAccounts();

  console.log(`Deployer address [${deployer}]`)
  console.log(`Contract owner address [${contractOwner}]`)

  const tuffToken = await deployments.deploy({
    name: 'TuffToken',
    from: deployer,
    log: true
  });
  let tuffTokenContract = await hre.ethers.getContractAt(tuffToken.abi, tuffToken.address, contractOwner);
  console.log(`TuffToken address [${await tuffTokenContract.address}]`);
  await tuffTokenContract.initTuffToken(contractOwner);

  const aaveLPManager = await deployments.deploy({
    name: 'AaveLPManager',
    from: deployer,
    log: true
  });
  let aaveLPManagerContract = await hre.ethers.getContractAt(aaveLPManager.abi, aaveLPManager.address, contractOwner);
  console.log(`AaveLPManager address [${await aaveLPManagerContract.address}]`);
  await aaveLPManagerContract.initAaveLPManager();

  let tuffTokenDiamond = await deployments.diamond.deploy({
    name: 'TuffTokenDiamond',
    from: deployer,
    owner: contractOwner,
    facets: [aaveLPManager, tuffToken],
    log: true
  });
  let tuffTokenDiamondContract = await hre.ethers.getContractAt(tuffTokenDiamond.abi, tuffTokenDiamond.address, contractOwner);
  console.log(`TuffTokenDiamond address [${await tuffTokenDiamondContract.address}]`);
  // console.log(tuffTokenDiamondContract);

  await tuffTokenDiamondContract.initTuffToken(contractOwner);
  await tuffTokenDiamondContract.initAaveLPManager();

  console.log(`Is TuffToken init [${await tuffTokenDiamondContract.isTuffTokenInitialized()}]`);
  console.log(`Is AaveLPManager init [${await tuffTokenDiamondContract.isAaveInit()}]`);
};

Where the init functions look like this:

    //Basically a constructor, but the hardhat-deploy plugin does not support diamond contracts with facets that has
    // constructors. We imitate a constructor with a one-time only function. This is called immediately after deployment
    function initAaveLPManager() public {
        console.log("Aave starting initialization [%s]", isAaveInit());
        require(isAaveInit() == false, 'TUFF: AaveLPManager: ALREADY_INITIALIZED');

        # do some things here

        _isAaveInit = true;
        console.log("Aave completed initialization [%s]", isAaveInit());
    }

And produces the following output:

Deployer address [0x...]
Contract owner address [0x...]
TuffToken address [0xCD8a1C3ba11CF5ECfa6267617243239504a98d90]
TuffToken starting initialization [false]
TuffToken completed initialization [true]
AaveLPManager address [0x82e01223d51Eb87e16A03E24687EDF0F294da6f1]
Aave starting initialization [false]
Aave completed initialization [true]
TuffTokenDiamond address [0xa3dD61b906451482eAd7Cb1Ca283112274ddFcd8]
TuffToken starting initialization [false]
TuffToken completed initialization [true]
Aave starting initialization [true]
...
Error: VM Exception while processing transaction: reverted with reason string 'TUFF: AaveLPManager: ALREADY_INITIALIZED'

So I can initialize them separately and that works. Then when I add them to the Diamond contract I observe two issues: 1) Upon initializing the TuffToken contract when it is part the diamond contract, it's state is reset and has to be reinitialized 2) Upon initializing the Aave contract when it is part the diamond contract, it is already initialized

Hopefully this helps with your investigation. Have you found anything of interest?

[unibeck]

@wighawag the overwriting of variables I was seeing was due to an implementation detail of diamond contracts. Diamond facets share the same storage layout and the variables I were using in the two separate contracts were in the same position in that storage layout, thus they were overwriting each other. To avoid this you can use on of several storage layout implementations, which of I ended up using the diamond storage pattern.

However, not all is lost. During my journey of debugging my problem, I created two PRs which still have value. So while this issue can be closed, I would still like to see these pushed forwarded and merged in: 1) https://github.com/wighawag/hardhat-deploy/pull/222 2) https://github.com/wighawag/hardhat-deploy/pull/232