Suggestion. Optional fail if local bytecode changed compared to a deployment.

0xCalibur commented 1 year ago

I'm not sure if what I'm going to suggest totally make sense. But (my fault), I literally spent 2 lovely days on trying to understand why my test was failing even if I was changing a contract code. Well, thing is I was using the deployment name of my contract which was already deployed. So, forge-deploy doing it's duty, takes the address from the deployment file and return the contract from it...

Would be super awesome if there was some flag I could set on the deployer where if the "local" contract bytecode is different from the deployment file it could just abort. This way, I won't stupidly forget to change the deployment name so it deploys a new one (locally and then once it's tested and deployed live).

wighawag commented 1 year ago

Yes, I think I need to add a function that let you compare

Right now, you can call deployer.ignoreDeployment("<deployname nane>") to let future deploy call to overwrite the deployment, but you ll have to do the comparison logic yourself

0xCalibur commented 1 year ago

I think it's the same behavior as hardhat-deploy, right?

I'm not sure of the solution here. Maybe it's just a matter of having your own template, with you own ENV variable that force to revert if bytecode is different from deployed one?

wighawag commented 1 year ago

in hardhat-deploy you can specify an option to deploy even if already deployed

The issue is that with solidity you cannot have optional arg for struct and you need to create a struct type for each case, so it is not very handy to add option

I ll have a think how I can handle this better

0xCalibur commented 1 year ago

what about something like:

deployer.forceDeploy(true);
deployer.deploy_1..
deployer.deploy_2...
deployer.forceDeploy(false);
deployer.deploy_3...

0xCalibur commented 1 year ago

I'm leaving another note here from a recent observation.

let's say I'm in a test:

fork block height 1337
I'm loading a script and deploy
deployment files not yet exist because I'm testing them out before deployment
now I deploy them and the deployment files now exists
if I run my test again, they fail because the deployment file are loading contracts addresses from the deployments
they didn't exist back in that block 1337

Now one could say I have to update my test fork block to a more recent one where the addresses exists. But, that is a bummer because if I hardcoded some values there, depending on that block, I have to basically fix all my tests.

Another solution be to have something to ignore the deployment while testing. Easiest but not automatic.

Or maybe another cleaner solution would be to store the block height in the deployment file and load only if block.number >= block number for a given deployment? Would be transparent to the dev

wighawag commented 1 year ago

ha interesting, I guess, I could make it that it read the current block and load only deployments before or on that block

wighawag commented 1 year ago

By the way, this is off topic but I am considering rewriting the cli (sync and gen-deployer) part in typescript as this will speed up my development since I am working mostly on typescript for other project, might even help having a compatible hardhat-deploy/forge-deploy system.

not sure if you install forge-deploy via npm or if you are using cargo ?

if the later, would you mind if I remove that option ?

0xCalibur commented 1 year ago

i'm using forge lib and was building it with cargo. Whatever you chose to move to, it will be a pleasure to migrate and try and it out

wighawag / forge-deploy

Suggestion. Optional fail if local bytecode changed compared to a deployment. #11