Add interface for CrosschainDeployAdapter

[stonecharioteer]

Addresses foundry-rs/foundry#2

[stonecharioteer]

@mpetrunic @lastperson
Just lemme know if I'm on the right track? I'm assuming people will subclass my new contract type CrosschainDeployScript and then call the function?

[mpetrunic]

Looking better now, maybe create some test script that extends it and deploys some sample contract on multiple networks to ensure it works? :)

[mpetrunic]

There should be more test cases, especially regarding constructor args and initDatas. For example when initData is submitted for one chain bot not other

[stonecharioteer]

There should be more test cases, especially regarding constructor args and initDatas. For example when initData is submitted for one chain bot not other

For this, is it enough to check if the adapter is called with different arguments or should I check if the upstream contracts are actually instantiated with different arguments?

[lastperson]

Maybe also useful to have this function with network name instead of chainId?

[stonecharioteer]

YEah, that's a good idea. Changed it.

[mpetrunic]

There should be more test cases, especially regarding constructor args and initDatas. For example when initData is submitted for one chain bot not other

For this, is it enough to check if the adapter is called with different arguments or should I check if the upstream contracts are actually instantiated with different arguments?

it enough that adapter is called with different args

[stonecharioteer]

@mpetrunic @lastperson
I've made the requested changes.

1. The integration test uses vm.expectCall now, and checks on multiple networks.
2. For some reason, the second test with the multiple targets is giving me compilation errors that say the stack is too deep. I checked what that could mean and what I understood is that it could be that there are too many local variables? The EVM only supports 16? I had to enable the --via-ir flag to enable the newer intermediate representation (IR) for code generation get this to compile now, so I've added that to the justfile and added a FIXME to the function.
3. I've updated the SimpleContract so that it has a few more methods and takes a value for the constructor, so now instead of the bytes memory constructorArg = '' I can use abi.encoded(uint256(1)) instead, serving as an example to developers as to how they can do it.
4. OR I could show them how they can use cast abi-encode to do this and pipe the output to the script call to make things easier for them.

I checked if there was a way users could maybe pass in the argument list in a series of strings to a script, but there's a problem with that approach (this is for when we have to build a script to wrap around this so users don't have to write abi.encode all the time), and there's no way that I can find that foundry could do something like abi.encodePacked("add(uint256)", "(10)"); where the second argument is any string representation of the arguments to the function. That would have been a better UX I think.

If this PR looks fine so far, let me know and I'll rebase all the commits so you can have a final look.

@lastperson I'm not sure what I should do with the msg.sender bit that you highlighted above. Could we talk about that? That's the only thing pending as far as I can tell.

[mpetrunic]

shouldn't it make more sense for SampleContract to "extend" CrosschainDeployScript instead?

[stonecharioteer]

Anything that's inheriting Script cannot be deployed. It runs locally, which is why the cheatcodes work there. So users would have to do something like use forge script and provide the contract name and the ABI code for the init variables (using cast abi-encode perhaps) to get those values and then pass them on, or use it this way, which seems much cleaner to be honest.

[mpetrunic]

But foundry recommends deploying using Script: https://book.getfoundry.sh/tutorials/solidity-scripting#writing-the-script
we don't want CrosschainDeployScript to end up being deployed (just ran locally) so when user is scripting deployment he would create DeployMyContract.sol which will extend ˙ScriptandCrosschainDeployScript` and script deployment

[stonecharioteer]

I'm confused I think. Isn't that what the test case shows? I'll write up the script in the next PR that shows how users will be using this library, where they will be making a Script that calls their contract and uses the deploy script to call the upstream contract. I'd have written that here, but I thought this PR was growing big as it is and that could have been separate.

[mpetrunic]

I'm just disagreen with the sample here that instantiates CrosschainDeployScript instead of extending it

[mpetrunic]

Have you tried deploying on testnet?

[stonecharioteer]

I thought about this but would it make sense to setup the constructorargs and initdatas before the contract itself? Because these are tied to the contract so it doesn't make sense that the contract name is something users have to supply in the deploy function, does it? Is there a scenario where a user would have to provide the same arguments to the same networks to another contract? But at that point, the deployment target queue is reset in the deploy function, isn't it?

(Sorry, was on the flight and saw these commits, wanted to understand more)

[mpetrunic]

Honestly, contructor thing was wierd. You could have script that deploy more than one contract multichain, this ensures that you can do that

[stonecharioteer]

But it's still weird, isn't it? You add deployment targets against a particular contract, but it would make sense to "fix" the contract before queuing up the arguments to its later functions. Now if someone messes up the contract name, it's a waste.

How about we give a step saying setContractString instead? And check that it has been set when someone adds deployment targets, and then set that string to "" after deploy so that our script is ready to use again?

[mpetrunic]

But you can now do:

contract SampleScript is CrosschainDeployScript {

 run() {
   addDeploymentTarget("sepolia", constructorArgs, initData);
   addDeploymentTarget("holesky", constructorArgs, initData);
   deploy("SampleContract:Contract1")
   addDeploymentTarget("sepolia", constructorArgs2, initData2);
   addDeploymentTarget("holesky", constructorArgs2, initData2);
   deploy("SampleContract:SomeOtherContract")
 }

}

why would this be weird?

[stonecharioteer]

I'm thinking of it like a classical function in computer programming paradigm I guess. We're setting up the arguments to a function and the "machine" on which to run it before we declare the "module" from which to call the function. 😅 But you're more used to solidity, so if this is normal then that makes more sense.

…

On Thu, 8 Feb 2024, 18:56 Marin Petrunić, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In README.md <#5 (comment)> : > addDeploymentTarget("sepolia", constructorArgs, initData); - deploy{value: msg.value}(50000, false); + addDeploymentTarget("holesky", constructorArgs, initData); + deploy{value: msg.value}("SimpleContract.sol:SimpleContract", 50000, false); But you can now do: contract SampleScript is CrosschainDeployScript { run() { addDeploymentTarget("sepolia", constructorArgs, initData); addDeploymentTarget("holesky", constructorArgs, initData); deploy("SampleContract:Contract1") addDeploymentTarget("sepolia", constructorArgs2, initData2); addDeploymentTarget("holesky", constructorArgs2, initData2); deploy("SampleContract:SomeOtherContract") } } why would this be weird? — Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACXSLC4BNNXZO3UVJEY3PBTYSTG7DAVCNFSM6AAAAABB4L44JGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZQGIZDKMJSGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>