-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(script): support custom create2 deployer #9278
Conversation
Makes sense, waiting for a follow up project to close it then 😀 |
Haha, so how about this one, is there anything I need to adjust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see the PR is only for script, but it won't be applied in other scopes, as for labels in traces, contract verification, (default) computeCreate2Address cheatcode and other places, which I think could lead to inconsistencies in deployment process? I'd say a consistent experience would be preferred here, that is when setting the address as env var then to be reflected in all places.
@grandizzy thanks for the advice, I use forge to deploy contracts in a private evm chain, so only tested the And If the changes are large, I'd prefer to split them into small PRs to include them all, else just in this one PR, WDYT? |
ah, I see, then maybe should be done as a new |
I've tried this approach, but failed, we need to register the create2 handler here foundry/crates/evm/core/src/utils.rs Lines 177 to 210 in 5e254e0
seems it's hard to pass the cli argument into create2 schema, else maybe we need to adjust it in revm's codebase. |
we can add a helper to |
Thanks for the advice, I'll have a try, cli argument is much more implicit than environment. |
918f485
to
109a713
Compare
abbb7b1
to
b5928e4
Compare
@grandizzy @klkvr sorry for the long delay, I think it's time to review. |
b5928e4
to
4f27792
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, looks good! pending other reviews before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Thanks @jsvisa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this is what we want, left some nits
crates/forge/tests/cli/script.rs
Outdated
tester | ||
.add_deployer(0) | ||
.load_private_keys(&[0]) | ||
.await | ||
.add_create2_deployer(create2) | ||
.add_sig("BroadcastTestNoLinking", "deployCreate2()") | ||
.simulate(ScriptOutcome::OkSimulation) | ||
.broadcast(ScriptOutcome::OkBroadcast) | ||
.assert_nonce_increment(&[(0, 2)]) | ||
.await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we assert here that it was deployed to the right address? either need a snapbox fixture or a cast.get_code
check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also add a test to check the create2's code is the same as the default one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still working on the deployed address test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klkvr sorry, can't think of a good way to test it with deployed contrat, so I test it by compare the contract address directly in the script in
foundry/testdata/default/cheats/Broadcast.t.sol
Lines 222 to 243 in 6454573
function deployCreate2(address deployer) public { | |
vm.startBroadcast(); | |
bytes32 salt = bytes32(uint256(1338)); | |
NoLink test_c2 = new NoLink{salt: salt}(); | |
assert(test_c2.view_me() == 1337); | |
address expectedAddress = address( | |
uint160( | |
uint256( | |
keccak256( | |
abi.encodePacked( | |
bytes1(0xff), | |
deployer, | |
salt, | |
keccak256(abi.encodePacked(type(NoLink).creationCode, abi.encode())) | |
) | |
) | |
) | |
) | |
); | |
require(address(test_c2) == expectedAddress, "Create2 address mismatch"); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use anvil's handle api
https://github.com/jsvisa/foundry/blob/64545738dd183c64bd8d7a4ec94f7ba03c8d8d05/crates/forge/tests/cli/script.rs#L885
and then just do something like `assert!(!api.get_code(...).is_empty());
but it's fine anyway :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick question, if we use anvil to get the contract code, but how could we get the contract address? I was thought of calculate the address by
- compiling the solidity to get the init code
- get the initcodehash + salt + deployer address
but stuck in the first step 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could use the anvil api to get the receipt and contract address from there, smth like we're getting the hash of last tx here
foundry/crates/cast/tests/cli/main.rs
Line 1745 in 7f41280
let tx_hash = api |
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]> unit test Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
7e8e64d
to
4337dd5
Compare
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Motivation
Currently when deploy contracts using CREATE2 method, it is default to
foundry/crates/evm/core/src/constants.rs
Line 44 in fcaa2a7
But in some circumstances, we can't access or deploy the 0x4e59b44847b379578588920ca78fbf26c0b4956c contract, may be caused of not able to send the pre EIP155 raw transaction. So try to introduce a env to change the default create2 deployer.
Solution
--create2-deployer
to theforge script
argumentcreate2_deployer
to thefoundry.toml
config file