Idea: Messages to Help with Deployments

[defido]

Here is an idea, feel free to add your input or suggest if you don't like it. I am sure you might have a better idea how to make the code base cleaner. I have written it in JS (tested in JS), although it should work in TS. Some of the colours constants could be put at the top, it could also be refactored to be better.

I have tested on hardhat, BSC, Polygon etc. It should work on all EVM chains. I have used in built terminal colours, instead of an external package for colours. I find the all white of so much information was getting me confused, thus the colours.

FILE LOCATION:

Index.ts

ISSUE:

When deploying to multiple chains, it's easy to get lost on which chain was being deployed to. It's also confusing what the contract address is, as it's never returned, only the hash. There is also times where a contract is deployed and the contract address would be different, possibly due to different constructor inputs, or different byte codes.

IDEA:

Deployment success with number of which deployment it is on
Chain name
Contract Address
TX Hash
Output location (I think this could be removed or the message shortened)

Errors still work as normal in subtasks.

NEXT:

Chain ID
Link to TX URL explorer of the specific chain in the terminal to shorten testing times.

GIST EXAMPLE:

https://gist.github.com/defido/0d251cd6eb106e93a045d28dd0308caa

[pcaversaccio]

Thx for sharing your idea @defido - I like it! The calculation of the contract address is however not so simple. In your case, you wait for the event logs. But this only works if there are events in the contract creation transaction (i.e. there are constructor arguments). If there are none you can't rely on the logs. However, this can be resolved by directly calling the CREATE2 helper contract function computeAddress. The chainId & Block explorer link should not be too much of a challenge either. Will look into this soon.

[defido]

This above would change to calling the CREATE2 helper.

However, this can be resolved by directly calling the CREATE2 helper contract function computeAddress.

Great, I didn't know that, I'll need to look into how that works. I was also thinking about adding a warning if contracts don't match, as that can happen often if people don't write specifically to keep their byte code the same. A simple warning, without a break error, just to show which chain didn't deploy correctly.

I will do more testing tomorrow or this week.

I'll keep these in the 'discussion' section for now, as they are simple additions. I'll keep testing to see what works.

[pcaversaccio]

well, this edge case regarding different contract addresses can not happen if you use xdeployer as a dependency (ofc if people change the source code anything can happen). The plugin uses always the same init bytecode (& same salt value) specified by the contract name in the hardhat.config.ts (or hardhat.config.js) file to deploy across all chains. This is design choice was made on purpose by me in order to exactly not allow such changes and to ensure the exact same contract deployed across all chains (the same reasoning applies to the salt value).

[defido]

This would be a user error message. I have found sometimes I made a mistake, or there was a random change in the code base, such as a different constructor input that I didn't pick up during development, of which a simple error message would be great. Or if the chain randomly did return a different address (although impossible or unlikely), would allow people to see in the terminal.

I think with these ideas, because if people deploy to 3-5 chains, it just makes the UX much better.

Most of these ideas are focused on a smoother UX experience after deploying 100+ tests now xD.

I have certainly deployed and thought, 'this will work', and it returned different contract addresses. Currently users have to copy, open the browser, paste into the explorer of the chain, check logs.

Great addition for testing I think.

[defido]

But this only works if there are events in the contract creation transaction (i.e. there are constructor arguments). If there are none you can't rely on the logs.

constructor() {
        // console.log("Deploying a Greeter with greeting:", _greeting);
        // greeting = _greeting;
        // setup_wallet = _setup_wallet;
        _transferOwnership(setup_wallet);
    }

I am not sure if I miss-understood this, but I did a test without constructor arguments & it seems to work. I will test more tomorrow, but maybe it does work?

[pcaversaccio]

The ownership transfer also emits an event! That's why it worked in your case.

[defido]

Although this is not a 'fix', your method above most likely is. Here are my tests.

It seems the chains return the contract address as the first address.

returnData["logs"][i].address

returnData["logs"][0].address Changing to 0 returns the address.

returnData["logs"][0].address Hardhat

constructor() {
        // console.log("Deploying a Greeter with greeting:", _greeting);
        // greeting = _greeting;
        // setup_wallet = _setup_wallet;
        // _transferOwnership(setup_wallet);
    }

[pcaversaccio]

I'm talking about a constructor that is invoked in the base contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L29. That's why there is an event ;-).

To test without events remove all OZ (any other) libs that have constructor args involved in the base contracts.

[defido]

You are 100% correct.

Remove Ownable, no logs, no events & no contract address. Tomorrow I'll test your recommendation.

{
	"to": "0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2",
	"from": "REDACTED",
	"contractAddress": null,
	"transactionIndex": 1,
	"gasUsed": { "type": "BigNumber", "hex": "0x03eade" },
	"logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
	"blockHash": "0x9582f44579dbfd9ace948e3b6c68eb7c3bac90dff143c24128a08c72f6f65fd8",
	"transactionHash": "0x7c1c00a0e59361feae15580191c8589c62a3241e916ff75dbd2995442e76891e",
	"logs": [],
	"blockNumber": 17897511,
	"confirmations": 1,
	"cumulativeGasUsed": { "type": "BigNumber", "hex": "0x043ce6" },
	"status": 1,
	"type": 0,
	"byzantium": true,
	"events": []
}

[defido]

const consoleColours = {
    success: '\x1b[32m',
    break: `\x1b[0m`,
    error: '\x1b[31m'
}
const returnData = await createReceipt[i].wait();
// console.log(returnData)
const contractAddressOnChain = await create2Deployer[i].computeAddress(hre.ethers.utils.id(hre.config.xdeploy.salt), hre.ethers.utils.keccak256(initcode.data));
console.log(hre.ethers.utils.keccak256(initcode.data))
if(returnData && contractAddressOnChain) {
    console.log(
    `\n${consoleColours.success}---------------- XDEPLOY SUCCESS ${i + 1} --------------${consoleColours.break}\n\n` +
    `Deployed to: ${consoleColours.success}${hre.config.xdeploy.networks[i]}\n${consoleColours.break}\n` +
    `Contract address: ${consoleColours.success}${contractAddressOnChain}\n${consoleColours.break}\n` +
    `Transaction hash: ${consoleColours.success}${createReceipt[i].hash}${consoleColours.break}\n\n` +
    `Transaction details written to ${consoleColours.success}${saveDir}${consoleColours.break}\n`);
};

It's not pretty as it's late. But this seems to work:

Warning is in the works. I need to figure out how to run 2 or more hardhat networks at xdeployment so that testing locally is much faster. Currently to test if the contract address is the same, I must test on testnets.

[pcaversaccio]

@defido I've created a PR here that implements the discussed changes. I will review my own code in the coming days again and if ok will merge it. The error handling of different contract addresses is not something I will implement since this can't happen by design (the same contract is deployed with the same arguments across all chains). The hardhat & localhost networks will have obviously different addresses due to the different addresses of the helper contract. But that I remarked that already in the README:

[defido]

I have the following ideas: (Move the contract checks to BEFORE the deployment)
Due to the nature of the EVM/Solidity, contracts are immutable and the addresses, irreversible, to take the onus on mistakes away from xDeployer, asking the user to confirm, or visually confirming the address before deployment solves many issues, and improves testing.

const contractAddressChecks: Array<any> = []; <--- optional checker
const getContractByteCodeHash = (input: string): string => {return hre.ethers.utils.keccak256(input)}; <--- useful addition

Add a helper function to the top of the codebase so that retrieving the bytecodeHash is simple.

 // Make a call to the CREATE2 contract to check what the contract address will be
          const contractAddressOnChain = await create2Deployer[i].computeAddress(
            hre.ethers.utils.id(hre.config.xdeploy.salt), 
            getContractByteCodeHash(initcode.data)
          )
          contractAddressChecks.push(contractAddressOnChain) 

The above makes a round trip call to the chain, calls computeAddress and waits for the on chain contract address before deploying anything. I moved the deployment parts into a function that is called on successful checks.

For user prompt I was using, on continue, it runs a function that executes the deployment and returns the data similar to what you've done in your PR on success.

I have moved to writing it in Typescript using npm link, as it was easier.

Term <---- Optional, make the user confirm they're OK with the contract address being different.

Simulated.

In addition to the above there is also this idea:

Add a ENV variable to check a pre-existing chain address before deployment.

CONTRACT_ADDRESS_CHECK=0x0c6D2f4D0896c34607341C9422fc950b1EAa2c8D

Then in the index.ts file const contractAddressChecks: Array<any> = [process.env.CONTRACT_ADDRESS_CHECK]; the contract address to confirm against becomes 0 in the array, and then run the const getContractByteCodeHash = (input: string): string => {return hre.ethers.utils.keccak256(input)}; checks as normal with the address the user wants to make sure are correct on all chains.

    // Contract addresses are pushed here on deployment, to check if contracts match or not
    const contractAddressChecks: Array<any> = [];
    const previousContractCheck = process.env.CONTRACT_ADDRESS_CHECK;
    let previousContractCheckTrue: boolean = false
    if(previousContractCheck != '' || "") {
      contractAddressChecks.push(previousContractCheck);
      previousContractCheckTrue = true
    }

//Announce checks line 124

  if(previousContractCheckTrue === true) {
      term.yellow(`\nChecking against previous address: ${previousContractCheck}\n`)
    } else {
      term.green(`\nNo previous contract to check, calling chains now...\n`)
    }

Check a contract address you already deployed before:
CONTRACT_ADDRESS_CHECK=0x0h6D2f4D0896c35607341C9422fc950b1EAa2c9U

CONTRACT_ADDRESS_CHECK=

A simple PRE deployment check. Currently not working 100%, but shows the idea in general.

[pcaversaccio]

We need to be careful here: I really don't want to overengineer this plugin since many appreciate the general low complexity. Concerning your points:

• Using my plugin with the current Create2Deployer will either successfully compute the address for all selected EVM chains or it will fail for all (except for the case that one specific RPC provider is doing something wrong but this would already throw before). That's the nature of just using EVM chains with the same underlying opcodes. So no need to handle this case;
• Also, there is no possibility to have different contract addresses (using my Create2Deployer contract) for the EVM chains (not for hardhat & localhost) in one run since the plugin uses always the same function arguments for all EVM chains and CREATE2 behaves similarly for all chains. So no need to handle this case;
• Checking on-chain whether there is a contract would mean having deployed a to be called contract on all chains that does the following:

function isContract(address account) internal view returns (bool) {
    return account.code.length > 0;
}

However, implementing such a feature would also mean we would allow interacting via the CLI saying I just want to deploy to the non-existing chains. There are a lot of subtleties involved that the user could solve via writing their own hardhat tasks. I will not implement this for the time being. But open for PRs that implement this feature for all EVM chains that xdeployer targets.

[defido]

OK, I will follow your PR & then try implement something that works that you can review.

I can also look into a task that could be pushed to both our starters.

For your current PR, it returns an error, say on the first chain, and then continues, it should probably just exit if the first chain fails.

I'm a little worried about bad outcomes for projects that deploy to production and then one chain is randomly different to the other.

It is a common use case to deploy one contract to BSC for example, change code and deploy to POLYGON. But not catch the fact you just changed the bytecode.

Prechecks would solve this issue and put the onus back on the user to check what the onchain deployer says, and then continue in anycase.

[pcaversaccio]

What would you like to see in the starter kit after this upgrade?

So let's assume your use case: I deploy on BSC successfully. My deployment results are saved. I change my code and run again deployment now on Polygon. How do you want to prevent this? You would need to check in the deployment folder for already deployed contracts but what if you deleted them or this is intended behavior? Keeping track of the deployment addresses within the xdeploy run makes sense (and we already do that). Keeping track of the deployments across different xdeploy runs is too much of complexity right now IMHO and violates the goal of this plugin to focus on one specific goal.

For your current PR, it returns an error, say on the first chain, and then continues, it should probably just exit if the first chain fails.

Well, if it fails for one chain it fails also for the other chains. So it's good to inform the user about the consistency that all other chains also have failed as expected. You know what I mean?

[defido]

I don't think it's an addition for the starter kits, the issue is someone will not write a task in their starter kit and the deployment checks will not be there. Then you have many starter kits built around xDeployer, some with checks and others without them.

I see it like this:

xDeployer deploys single contract addresses (The gun)
xDeployer provides warnings and assistance to stop you from deploying a different contract address to the one you wanted. (The safety)

Outside of these two things, it only does the above. You want more tasks to check gas and the likes? You can write tasks for that.

That's where this comes into play:
CONTRACT_ADDRESS_CHECK=

  const previousContractCheck = process.env.CONTRACT_ADDRESS_CHECK;
    let previousContractCheckTrue: boolean = false
    if(previousContractCheck != '' || "") {
      contractAddressChecks.push(previousContractCheck);
      previousContractCheckTrue = true
    }

const contractAddressOnChain = await create2Deployer[i].computeAddress(
            hre.ethers.utils.id(hre.config.xdeploy.salt), 
            getContractByteCodeHash(initcode.data)
)
if(contractAddressChecks[i] !== contractAddressChecks[0]) {
            console.log(
              `${contractAddressOnChain} CONTRACT MISSMATCH`
         )
  }