ChainSafe / foundry-multichain-deploy

9 stars 3 forks source link

Add interface for CrosschainDeployAdapter #5

Closed stonecharioteer closed 9 months ago

stonecharioteer commented 10 months ago

Addresses #2

stonecharioteer commented 10 months ago

@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 commented 9 months ago

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 commented 9 months ago

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?

mpetrunic commented 9 months ago

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 commented 9 months ago

@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.

stonecharioteer commented 9 months ago

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 https://github.com/ChainSafe/foundry-multichain-deploy/pull/5#discussion_r1482975094 :

     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 https://github.com/ChainSafe/foundry-multichain-deploy/pull/5#discussion_r1482975094, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXSLC4BNNXZO3UVJEY3PBTYSTG7DAVCNFSM6AAAAABB4L44JGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZQGIZDKMJSGA . You are receiving this because you were mentioned.Message ID: @.***>