chore: deploy counterfactuals to all chains#1386
Conversation
tbwebb22
requested review from
bmzig,
mrice32,
nicholaspai and
pxrl
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97024bcf46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) internal { | ||
| // Append `|| true` so that non-fatal failures (e.g. etherscan verification | ||
| // timing out) don't cause ffi to revert and halt subsequent deployments. | ||
| string memory cmd = string.concat( | ||
| "forge script ", | ||
| "FOUNDRY_PROFILE=counterfactual forge script ", |
There was a problem hiding this comment.
_runForgeScript now forces FOUNDRY_PROFILE=counterfactual for every sub-script invocation, which breaks deployments on ZK_STACK chains (for example Lens/232) because scripts inheriting DeploymentUtils assert that ZK_STACK runs use the zksync profile. In that environment each child deploy script reverts before deployment, and this wrapper still reports success paths because failures are masked later in the shell command. This makes DeployAllCounterfactual unreliable for the exact multi-chain rollout this change targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hmm. Can we actually just pass the String profile as arg to this fn instead of hardcoding a profile here?
There was a problem hiding this comment.
One related thing I'm thinking is: can we make these "call foundry script" and "call cast" universal enough to put in their own script/utils/Forge.s.sol file?
So that other scripts could reuse these
| abi.encodePacked(type(AdminWithdrawManager).creationCode, abi.encode(deployer, deployer, signer)) | ||
| ); |
There was a problem hiding this comment.
The AdminWithdrawManager CREATE2 prediction now depends on deployer (abi.encode(deployer, deployer, signer)), but this script derives deployer from mnemonic index 0 while the called DeployAdminWithdrawManager script derives from DEPLOYER_INDEX. If operators set DEPLOYER_INDEX to a non-zero value, the actual deployed init code/address diverges from predictedAdmin, so subsequent existence checks and role-transfer calls target the wrong address and can fail unexpectedly.
Useful? React with 👍 / 👎.
grasphoper
left a comment
grasphoper
left a comment
There was a problem hiding this comment.
Looks good. A few nits on the checker script and otherwise.
Deployments look good, subject to changes you posted in Slack.
I did end up creating a new foundry-based checker script to double-check all configs (couldn't convince myself to trust the .sh one heh), PTAL here https://github.com/across-protocol/contracts/pull/1397/changes
IMO the foundry one is better for readability
| script_name_for() { | ||
| case "$1" in | ||
| CounterfactualDeposit) echo "DeployCounterfactualDeposit" ;; | ||
| CounterfactualDepositFactory) echo "DeployCounterfactualDepositFactory" ;; | ||
| WithdrawImplementation) echo "DeployWithdrawImplementation" ;; | ||
| CounterfactualDepositSpokePool) echo "DeployCounterfactualDepositSpokePool" ;; | ||
| CounterfactualDepositCCTP) echo "DeployCounterfactualDepositCCTP" ;; | ||
| CounterfactualDepositOFT) echo "DeployCounterfactualDepositOFT" ;; | ||
| AdminWithdrawManager) echo "DeployAdminWithdrawManager" ;; | ||
| *) echo "" ;; |
There was a problem hiding this comment.
Hm, this looks fragile. If we ever change the name of the deploy script. We should put AGENTS.md / CLAUDE.md here mentioning the dependency or something
|
|
||
| rpc_url_for_chain() { | ||
| local chain_id="$1" | ||
| python3 -c "import os; print(os.environ.get('NODE_URL_${chain_id}', ''))" |
There was a problem hiding this comment.
This is odd. Can't a bash script read env vars normally?
|
|
||
| block_explorer() { | ||
| local chain_id="$1" | ||
| # Override for chains where constants.json has the wrong explorer. |
There was a problem hiding this comment.
Hm interesting. Should we fix constants.json instead?
| return | ||
| fi | ||
|
|
||
| jq -r '.transactions[0].contractAddress // ""' "$broadcast_file" |
There was a problem hiding this comment.
Is this going to be true for every deployment script in the future too? *first TX being the deployment one
| return | ||
| fi | ||
|
|
||
| jq -r '.transactions[0].arguments // [] | .[]' "$broadcast_file" |
There was a problem hiding this comment.
Same as above. We might want to find a CREATE/CREATE2 tx instead of using index 0 always
| python3 -c " | ||
| import re, sys | ||
|
|
||
| chain_id = '${chain_id}' | ||
| key = '${key}' | ||
| in_chain = False | ||
| in_address = False | ||
|
|
||
| with open('${CONFIG_FILE}') as f: | ||
| for line in f: | ||
| line = line.strip() | ||
| if line == f'[{chain_id}]': | ||
| in_chain = True | ||
| in_address = False | ||
| continue | ||
| if line == f'[{chain_id}.address]': | ||
| in_chain = True | ||
| in_address = True | ||
| continue | ||
| if line.startswith('[') and line != f'[{chain_id}]' and line != f'[{chain_id}.address]': | ||
| if in_chain: | ||
| in_chain = False | ||
| in_address = False | ||
| if in_address: | ||
| m = re.match(rf'^{key}\s*=\s*\"(.+?)\"', line) | ||
| if m: | ||
| print(m.group(1)) | ||
| sys.exit(0) | ||
| print('') | ||
| " 2>/dev/null |
There was a problem hiding this comment.
Yeah these python excerpts are unfortunate.
.sh + .py in a contracts repo is a nasty combo 😄
I wonder if we should create some standard way of creating scripts like this one. cast is callable from any language we could want. I remember you had a foundry script for a similar purpose before. That script could also read constants easier
Wdyt?
There was a problem hiding this comment.
Actually if we did use a foundry script instead of .sh, we could conveniently read all of the latest addresses from config.toml, instead of fishing for them in the broadcast/ folder
| ) internal { | ||
| // Append `|| true` so that non-fatal failures (e.g. etherscan verification | ||
| // timing out) don't cause ffi to revert and halt subsequent deployments. | ||
| string memory cmd = string.concat( | ||
| "forge script ", | ||
| "FOUNDRY_PROFILE=counterfactual forge script ", |
There was a problem hiding this comment.
Hmm. Can we actually just pass the String profile as arg to this fn instead of hardcoding a profile here?
| ) internal { | ||
| // Append `|| true` so that non-fatal failures (e.g. etherscan verification | ||
| // timing out) don't cause ffi to revert and halt subsequent deployments. | ||
| string memory cmd = string.concat( | ||
| "forge script ", | ||
| "FOUNDRY_PROFILE=counterfactual forge script ", |
There was a problem hiding this comment.
One related thing I'm thinking is: can we make these "call foundry script" and "call cast" universal enough to put in their own script/utils/Forge.s.sol file?
So that other scripts could reuse these
| @@ -11,4 +11,4 @@ | |||
| "lib/sp1-contracts": { | |||
| "rev": "512b5e029abc27f6e46a3c7eba220dac83ecc306" | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
can fix via formatting prob
| [profile.counterfactual-zksync.zksync] | ||
| compile = true | ||
| fallback_oz = true | ||
| mode = '3' | ||
| zksolc = "1.5.15" |
There was a problem hiding this comment.
A Q: does this mean that we're compiling to the EraVM variant of the bytecode or is it unrelated?
yep I like that |
2 participants
latest deployment report: https://github.com/across-protocol/contracts/blob/taylor/counterfactual-oft-deployments/script/counterfactual/deployment-report.md