Conversation
📝 WalkthroughWalkthroughAdds end-to-end multi-network deployment tooling: new per-network environment variables and Sepolia manifests, Foundry/Hardhat config updates, a DeployBase Solidity utility, many Forge deployment scripts (protocols, provers, pushers, buffers), shell orchestration scripts, and removal of legacy ad-hoc deploy scripts. Solidity toolchain bumped to 0.8.30. Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator as "deploy.sh"
participant Protocol as "DeployProtocol.s.sol"
participant Provers as "Prover scripts"
participant Buffers as "Buffer/Pusher scripts"
participant DeployState as "deployments/*.json"
participant Chain as "Sepolia Networks"
Orchestrator->>Protocol: invoke (CHAIN_TYPE, RPC, PRIVATE_KEY)
Protocol->>Chain: check & deploy Broadcaster/Receiver
Protocol->>DeployState: write contracts
Protocol-->>Orchestrator: status
Orchestrator->>Provers: invoke per-chain prover scripts (HOME/TARGET IDs)
Provers->>Chain: check existing prover/copy
Provers->>Chain: deploy ParentToChild/ChildToParent and optional Pointer
Provers->>DeployState: write provers/copies/pointers
Provers-->>Orchestrator: status
Orchestrator->>Buffers: run pusher/buffer scripts (messenger addrs)
Buffers->>Chain: deploy pushers/buffers
Buffers->>DeployState: write pusher/buffer addresses
Buffers-->>Orchestrator: status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nahimterrazas
approved these changes
Dec 17, 2025
frangio
reviewed
Jan 5, 2026
Collaborator
There was a problem hiding this comment.
It seems optimizations are not enabled.
scripts/deployment/DeployBase.s.sol
Outdated
Comment on lines
+11
to
+15
| if (chainId == vm.envUint("ETHEREUM_SEPOLIA_CHAIN_ID")) return "ethereum-sepolia"; | ||
| if (chainId == vm.envUint("ARBITRUM_SEPOLIA_CHAIN_ID")) return "arbitrum-sepolia"; | ||
| if (chainId == vm.envUint("LINEA_SEPOLIA_CHAIN_ID")) return "linea-sepolia"; | ||
| if (chainId == vm.envUint("SCROLL_SEPOLIA_CHAIN_ID")) return "scroll-sepolia"; | ||
| if (chainId == vm.envUint("OPTIMISM_SEPOLIA_CHAIN_ID")) return "optimism-sepolia"; |
Collaborator
There was a problem hiding this comment.
Seems strange to me to receive these via env vars. Is there a reason not to hardcode them here instead?
Comment on lines
+13
to
+16
|
|
||
| function run() public { | ||
|
|
||
|
|
Collaborator
There was a problem hiding this comment.
Whitespace/formatting is weird in this PR, can you run a Solidity formatter?
Comment on lines
+26
to
+27
| if(block.chainid == targetChainId){ | ||
| return; |
Collaborator
There was a problem hiding this comment.
Wouldn't it make more sense to revert to signal that the script was run with the wrong chain?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
scripts/deployment/DeployBase.s.sol (1)
142-147: Use a consistent JSON path for buffer deployment checks.Line 146 checks
.buffers.<chainKey>, but buffer state in this file is written/read under.contracts.buffer(see Line 104 and Line 153). This helper can return false negatives.Proposed fix
function _isBufferDeployed(string memory chainKey) internal returns (bool) { string memory path = _path(); _ensureRootScaffold(path); string memory json = vm.readFile(path); - return vm.keyExistsJson(json, string.concat(".buffers.", chainKey)); + return vm.keyExistsJson(json, string.concat(".contracts.", chainKey)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deployment/DeployBase.s.sol` around lines 142 - 147, The helper _isBufferDeployed currently checks JSON at ".buffers.<chainKey>" but the file reads/writes buffer state under ".contracts.buffer", causing false negatives; update the key used in _isBufferDeployed to match the stored path (use ".contracts.buffer." + chainKey or the same concatenation pattern used where buffers are written), so vm.keyExistsJson(json, string.concat(".contracts.buffer.", chainKey)) is checked (or centralize the path into a shared constant used by both the writer and _isBufferDeployed) ensuring consistency with the code that reads/writes buffer state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 8: Rename the environment variable key ONWER to OWNER in .env.example so
it matches the deployments that call vm.envAddress("OWNER"); update the single
line currently showing ONWER=... to OWNER=... ensuring the exact key OWNER is
used (this will align with DeployArbitrumParentToChild.s.sol,
DeployLineaParentToChild.s.sol, DeployOptimismChildToParent.s.sol,
DeployLineaChildToParent.s.sol, DeployOptimismParentToChild.s.sol,
DeployArbitrumChildToParent.s.sol, DeployScrollParentToChild.s.sol and
DeployScrollChildToParent.s.sol which all read vm.envAddress("OWNER")).
In `@scripts/deployment/block-hash-pusher/deploy-buffers.sh`:
- Around line 3-6: The deployment script exports environment variables
PUSHER_CHAIN and BUFFER_CHAIN but DeployBuffers.s.sol expects PARENT_CHAIN and
CHILD_CHAIN; update the script (deploy-buffers.sh) to export the expected names
(PARENT_CHAIN and CHILD_CHAIN) or export both sets (PUSHER_CHAIN/B UFFER_CHAIN
and PARENT_CHAIN/CHILD_CHAIN) so DeployBuffers.s.sol can read them; locate the
export block that sets CHAIN_TYPE/PUSHER_CHAIN/BUFFER_CHAIN/MESSENGER and change
or duplicate the variable names accordingly to match the DeployBuffers.s.sol
inputs.
In `@scripts/deployment/block-hash-pusher/DeployBuffers.s.sol`:
- Around line 26-30: The broadcast is started with vm.startBroadcast() but may
return early when _isContractDeployed("buffer") is true without calling
vm.stopBroadcast(); modify the early-return path in DeployBuffers.s.sol so that
vm.stopBroadcast() is invoked before returning (i.e., call vm.stopBroadcast()
immediately after the console.log and prior to the return when
_isContractDeployed("buffer") is true) to ensure broadcasts are always stopped.
In `@scripts/deployment/deploy-protocol.sh`:
- Around line 3-6: Add explicit checks at the start of deploy-protocol.sh to
validate required environment variables (CHAIN_TYPE, RPC_URL,
DEPLOYER_PRIVATE_KEY) and fail fast with an informative error and non-zero exit
if any are missing or empty; locate the top of the script near the
CHAIN_TYPE=$CHAIN_TYPE assignment and add tests (e.g., test -z or parameter
expansion) for each variable, printing which variable is missing and exiting, so
the subsequent call to forge script scripts/deployment/DeployProtocol.s.sol
never runs with empty inputs.
In `@scripts/deployment/deploy.sh`:
- Around line 3-5: Replace the bare "set -a; source .env; set +a" startup with a
fail-fast guard: add "set -euo pipefail" at the top, check the .env file exists
(e.g. test -f .env || { echo 'Missing .env'; exit 1; }) before sourcing, then
source .env (preserving the exported-vars behavior if needed) and only then
continue; also ensure any invoked sub-scripts from this deploy.sh are allowed to
fail-fast by relying on the top-level "set -e" (or explicitly checking each
invocation's exit status and exiting on non-zero) so partial multi-chain
deployments cannot proceed with unset RPC_URLs.
In `@scripts/deployment/DeployBase.s.sol`:
- Around line 197-203: The _deploy function computes predictedAddress with
Create2.computeAddress then calls LowLevelCall.callNoReturn on DEPLOYMENT_PROXY
but ignores the returned bool; modify _deploy to check the success boolean
returned from LowLevelCall.callNoReturn and handle failure before returning
predictedAddress — for example revert with a clear error (including
predictedAddress or a descriptive message) or return address(0) on failure so
callers/guards don’t treat an undeployed predictedAddress as deployed; update
references in _deploy, ensuring DEPLOYMENT_PROXY, predictedAddress and success
are used consistently.
In `@scripts/deployment/DeployProtocol.s.sol`:
- Around line 31-45: The code currently logs failures and continues when a
deployment returns address(0), which leaves partial state; change both
deployments to fail-fast by replacing the console.log/else branches with require
checks that revert on address(0). Specifically, in the broadcaster deployment
block (broadcasterAddress returned from _deploy) and in the receiver block
(creationCode/type(Receiver).creationCode and receiverAddress from _deploy),
remove the console.log path and add require(broadcasterAddress != address(0),
"Failed to deploy broadcaster on chain") and require(receiverAddress !=
address(0), "Failed to deploy receiver on chain") (include
_chainName(block.chainid) in the message if desired), then call _writeContract
only after the require passes.
In `@scripts/deployment/provers/deploy-child-provers.sh`:
- Around line 1-29: The script runs four sequential "forge script ...
--broadcast" calls with no fail-fast or env validation; add "set -euo pipefail"
at the top of scripts/deployment/provers/deploy-child-provers.sh and validate
all required env vars (ARBITRUM_SEPOLIA_CHAIN_ID, LINEA_SEPOLIA_CHAIN_ID,
SCROLL_SEPOLIA_CHAIN_ID, OPTIMISM_SEPOLIA_CHAIN_ID, ETHEREUM_SEPOLIA_CHAIN_ID,
ARBITRUM_SEPOLIA_RPC_URL, LINEA_SEPOLIA_RPC_URL, SCROLL_SEPOLIA_RPC_URL,
OPTIMISM_SEPOLIA_RPC_URL, DEPLOYER_PRIVATE_KEY) before any "forge script"
invocation, failing with a clear error if any are unset/empty (use shell
parameter expansion checks or explicit if-test + exit); keep the four "forge
script" commands (DeployArbitrumChildToParent.s.sol,
DeployLineaChildToParent.s.sol, DeployScrollChildToParent.s.sol,
DeployOptimismChildToParent.s.sol) as-is so a failing forge run aborts the rest.
In `@scripts/deployment/provers/deploy-parent-provers.sh`:
- Around line 1-2: Enable strict/fail-fast mode at the top of the
deploy-parent-provers.sh script by adding the shell safety flags (e.g., set -euo
pipefail and a safe IFS) immediately after the shebang so any failing forge
script command aborts the rest of the deployment; update
deploy-parent-provers.sh to set these options and ensure subsequent commands
rely on them (no further code changes needed beyond toggling strict mode at
script start).
In `@scripts/deployment/provers/DeployArbitrumChildToParent.s.sol`:
- Around line 44-63: The current logic treats a missing pointer (pointer ==
address(0)) as a "copy" and calls _writeCopy even when pointer deployment failed
on the home chain; instead detect deployment failure vs. legitimate copy and
fail fast on home chain: after attempting _deploy (and before vm.stopBroadcast
ends), check block.chainid == homeChainId and if pointer == address(0) revert or
vm.stopBroadcast and revert with a clear message that StateProverPointer
deployment via _deploy failed; only call _writeCopy when block.chainid !=
homeChainId and pointer == address(0), otherwise call _writeProver as before
(references: pointer, _deploy, StateProverPointer, homeChainId, targetChainId,
_writeCopy, _writeProver).
In `@scripts/deployment/provers/DeployLineaChildToParent.s.sol`:
- Around line 45-62: The code treats a zero pointer as a signal to call
_writeCopy, but on the canonical/home chain a failed pointer deployment should
abort; modify the logic so that when block.chainid == homeChainId (the canonical
chain) the script requires pointer != address(0) (e.g. revert or require with a
clear message like "pointer deployment failed on home chain"), and only when not
on the home chain allow falling back to _writeCopy; update the block that
currently checks pointer == address(0) before calling _writeCopy/_writeProver
accordingly, referencing pointer, StateProverPointer, _deploy, _writeCopy,
_writeProver, homeChainId and targetChainId.
In `@scripts/deployment/provers/DeployOptimismChildToParent.s.sol`:
- Around line 45-62: The current persistence logic can write a copy when pointer
deployment failed on the home chain because it only checks pointer ==
address(0); modify the branch so that when block.chainid == homeChainId you
never call _writeCopy on pointer failure—use a deployed flag or check the
original condition (e.g., track bool pointerDeployed = (pointer != address(0) &&
StateProverPointer(pointer).implementationAddress() != address(0))) or
explicitly guard the final persistence with if (block.chainid == homeChainId) {
if (pointer != address(0)) _writeProver(...); } else { if (pointer ==
address(0)) _writeCopy(...); else _writeProver(...); } to ensure _writeCopy is
only called for non-home chains and _writeProver only for a valid pointer;
reference StateProverPointer, _deploy, pointer, homeChainId, targetChainId,
_writeCopy, and _writeProver.
In `@scripts/deployment/provers/DeployOptimismParentToChild.s.sol`:
- Line 10: The contract name currently declared as DeployArbitrumParentToChild
conflicts with the script filename DeployOptimismParentToChild.s.sol and will
break forge script invocation; rename the contract declaration from
DeployArbitrumParentToChild to DeployOptimismParentToChild (keeping it
inheriting DeployBase) so the contract name matches the file/script target used
by tooling.
- Around line 47-65: The current post-deploy branch treats pointer == address(0)
as a copy and calls _writeCopy, which can corrupt manifests when running on
homeChainId; update the logic in DeployOptimismParentToChild.s.sol (the block
after vm.stopBroadcast that references pointer, _writeCopy, and _writeProver) so
that _writeCopy is only called when pointer == address(0) AND block.chainid !=
homeChainId, and treat pointer == address(0) on homeChainId as a deployment
failure (e.g., revert or emit an error) instead of writing copy metadata; keep
the existing behavior that writes the pointer via _writeProver when pointer !=
address(0).
In `@scripts/deployment/provers/DeployScrollParentToChild.s.sol`:
- Line 10: The contract declared as DeployArbitrumParentToChild is misnamed for
this Scroll-specific deployment; rename the contract to
DeployScrollParentToChild (update the contract identifier wherever used) so it
matches the file and imported Scroll-specific items (ParentToChildProver,
SCROLL_CHAIN, FINALIZED_STATE_ROOTS_SLOT) and any external callers referencing
the contract name; ensure the contract inheritance (DeployBase) and any
constructor or deployment metadata remain unchanged after renaming.
---
Nitpick comments:
In `@scripts/deployment/DeployBase.s.sol`:
- Around line 142-147: The helper _isBufferDeployed currently checks JSON at
".buffers.<chainKey>" but the file reads/writes buffer state under
".contracts.buffer", causing false negatives; update the key used in
_isBufferDeployed to match the stored path (use ".contracts.buffer." + chainKey
or the same concatenation pattern used where buffers are written), so
vm.keyExistsJson(json, string.concat(".contracts.buffer.", chainKey)) is checked
(or centralize the path into a shared constant used by both the writer and
_isBufferDeployed) ensuring consistency with the code that reads/writes buffer
state.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.env.exampledeployments/arbitrum-sepolia.jsondeployments/ethereum-sepolia.jsondeployments/linea-sepolia.jsondeployments/optimism-sepolia.jsondeployments/scroll-sepolia.jsondeployments/zksync-sepolia.jsonfoundry.tomlhardhat.config.tsscripts/Deploy.s.solscripts/DeployBroadcaster.s.solscripts/DeployZkSyncBroadcaster.s.solscripts/deployment/DeployBase.s.solscripts/deployment/DeployProtocol.s.solscripts/deployment/block-hash-pusher/DeployBuffers.s.solscripts/deployment/block-hash-pusher/DeployPushers.s.solscripts/deployment/block-hash-pusher/deploy-buffers.shscripts/deployment/block-hash-pusher/deploy-pushers.shscripts/deployment/deploy-protocol.shscripts/deployment/deploy.shscripts/deployment/provers/DeployArbitrumChildToParent.s.solscripts/deployment/provers/DeployArbitrumParentToChild.s.solscripts/deployment/provers/DeployLineaChildToParent.s.solscripts/deployment/provers/DeployLineaParentToChild.s.solscripts/deployment/provers/DeployOptimismChildToParent.s.solscripts/deployment/provers/DeployOptimismParentToChild.s.solscripts/deployment/provers/DeployScrollChildToParent.s.solscripts/deployment/provers/DeployScrollParentToChild.s.solscripts/deployment/provers/deploy-child-provers.shscripts/deployment/provers/deploy-parent-provers.shscripts/taiko/broadcast-l1.s.solscripts/taiko/broadcast-l2.s.solscripts/taiko/deploy-all.s.solscripts/taiko/provers-l2.s.solscripts/taiko/provers.s.solscripts/taiko/verify-message.s.solscripts/taiko/verify-on-chain.s.sol
💤 Files with no reviewable changes (3)
- scripts/DeployBroadcaster.s.sol
- scripts/DeployZkSyncBroadcaster.s.sol
- scripts/Deploy.s.sol
Comment on lines
+26
to
+30
| vm.startBroadcast(); | ||
| if (_isContractDeployed("buffer")) { | ||
| console.log("Buffer already deployed on chain ", _chainName(block.chainid)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Stop broadcast before the early return path.
At Line 27, broadcast starts, but Line 29 returns without vm.stopBroadcast(). This can leave script execution in an inconsistent state.
Proposed fix
vm.startBroadcast();
if (_isContractDeployed("buffer")) {
console.log("Buffer already deployed on chain ", _chainName(block.chainid));
+ vm.stopBroadcast();
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vm.startBroadcast(); | |
| if (_isContractDeployed("buffer")) { | |
| console.log("Buffer already deployed on chain ", _chainName(block.chainid)); | |
| return; | |
| } | |
| vm.startBroadcast(); | |
| if (_isContractDeployed("buffer")) { | |
| console.log("Buffer already deployed on chain ", _chainName(block.chainid)); | |
| vm.stopBroadcast(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/block-hash-pusher/DeployBuffers.s.sol` around lines 26 -
30, The broadcast is started with vm.startBroadcast() but may return early when
_isContractDeployed("buffer") is true without calling vm.stopBroadcast(); modify
the early-return path in DeployBuffers.s.sol so that vm.stopBroadcast() is
invoked before returning (i.e., call vm.stopBroadcast() immediately after the
console.log and prior to the return when _isContractDeployed("buffer") is true)
to ensure broadcasts are always stopped.
Comment on lines
+3
to
+6
| CHAIN_TYPE=$CHAIN_TYPE forge script scripts/deployment/DeployProtocol.s.sol \ | ||
| --rpc-url "$RPC_URL" \ | ||
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | ||
| --broadcast |
There was a problem hiding this comment.
Fail fast on missing deployment environment variables.
This script currently executes without asserting required inputs. Empty/missing values can cause incorrect chain behavior or opaque deployment failures.
Proposed hardening patch
#!/usr/bin/env bash
+set -euo pipefail
+
+: "${CHAIN_TYPE:?CHAIN_TYPE is required}"
+: "${RPC_URL:?RPC_URL is required}"
+: "${DEPLOYER_PRIVATE_KEY:?DEPLOYER_PRIVATE_KEY is required}"
-CHAIN_TYPE=$CHAIN_TYPE forge script scripts/deployment/DeployProtocol.s.sol \
+CHAIN_TYPE="$CHAIN_TYPE" forge script scripts/deployment/DeployProtocol.s.sol \
--rpc-url "$RPC_URL" \
--private-key "$DEPLOYER_PRIVATE_KEY" \
--broadcast📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CHAIN_TYPE=$CHAIN_TYPE forge script scripts/deployment/DeployProtocol.s.sol \ | |
| --rpc-url "$RPC_URL" \ | |
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | |
| --broadcast | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| : "${CHAIN_TYPE:?CHAIN_TYPE is required}" | |
| : "${RPC_URL:?RPC_URL is required}" | |
| : "${DEPLOYER_PRIVATE_KEY:?DEPLOYER_PRIVATE_KEY is required}" | |
| CHAIN_TYPE="$CHAIN_TYPE" forge script scripts/deployment/DeployProtocol.s.sol \ | |
| --rpc-url "$RPC_URL" \ | |
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | |
| --broadcast |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/deploy-protocol.sh` around lines 3 - 6, Add explicit
checks at the start of deploy-protocol.sh to validate required environment
variables (CHAIN_TYPE, RPC_URL, DEPLOYER_PRIVATE_KEY) and fail fast with an
informative error and non-zero exit if any are missing or empty; locate the top
of the script near the CHAIN_TYPE=$CHAIN_TYPE assignment and add tests (e.g.,
test -z or parameter expansion) for each variable, printing which variable is
missing and exiting, so the subsequent call to forge script
scripts/deployment/DeployProtocol.s.sol never runs with empty inputs.
Comment on lines
+3
to
+5
| set -a | ||
| source .env | ||
| set +a |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '1,20p' scripts/deployment/deploy.shRepository: openintentsframework/broadcaster
Length of output: 713
🏁 Script executed:
wc -l scripts/deployment/deploy.sh && sed -n '20,50p' scripts/deployment/deploy.shRepository: openintentsframework/broadcaster
Length of output: 1251
🏁 Script executed:
cat -n scripts/deployment/deploy.sh | head -70 | tail -20Repository: openintentsframework/broadcaster
Length of output: 1096
🏁 Script executed:
cat -n scripts/deployment/deploy.sh | tail -40Repository: openintentsframework/broadcaster
Length of output: 1980
🏁 Script executed:
# Check if there are any error handling patterns: set -e, trap, || exit, && checks
grep -n "set -e\|trap\||| exit\|&& \|set -u\|set -o pipefail" scripts/deployment/deploy.shRepository: openintentsframework/broadcaster
Length of output: 58
🏁 Script executed:
# Check bash behavior: does source .env fail if file is missing when -e is not set?
# Let's also look for any validation of env vars after sourcing
grep -n "if \[\|if test\|-z \|\${.*:-\|test -f" scripts/deployment/deploy.sh | head -20Repository: openintentsframework/broadcaster
Length of output: 58
🏁 Script executed:
# Let's check what happens with bash behavior when sourcing a missing file
# without -e flag - create a test to demonstrate the issue
bash -c '
source .env 2>&1 || true
echo "Exit code after source: $?"
echo "Script continued after missing source"
RPC_URL="${ETHEREUM_SEPOLIA_RPC_URL:-unset}"
echo "RPC_URL value: $RPC_URL"
'Repository: openintentsframework/broadcaster
Length of output: 206
Add fail-fast guards for .env sourcing to prevent partial multi-chain deployments.
Lines 3–5 lack error handling. If .env is missing or unreadable, the script continues silently with unset environment variables and passes empty RPC_URL values to all six chain deployments. Without set -e, source errors do not halt execution. Additionally, there are no error checks on sub-script failures, so invalid deployments proceed unchecked.
Add set -euo pipefail and verify .env existence before sourcing:
Proposed startup guard patch
+set -euo pipefail
+
+if [[ ! -f .env ]]; then
+ echo "Missing .env file" >&2
+ exit 1
+fi
+
set -a
source .env
set +a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/deploy.sh` around lines 3 - 5, Replace the bare "set -a;
source .env; set +a" startup with a fail-fast guard: add "set -euo pipefail" at
the top, check the .env file exists (e.g. test -f .env || { echo 'Missing .env';
exit 1; }) before sourcing, then source .env (preserving the exported-vars
behavior if needed) and only then continue; also ensure any invoked sub-scripts
from this deploy.sh are allowed to fail-fast by relying on the top-level "set
-e" (or explicitly checking each invocation's exit status and exiting on
non-zero) so partial multi-chain deployments cannot proceed with unset RPC_URLs.
Comment on lines
+45
to
+62
| if (block.chainid == homeChainId) { | ||
| bytes memory pointerCreationCode = | ||
| abi.encodePacked(type(StateProverPointer).creationCode, abi.encode(owner)); | ||
| pointer = _deploy(pointerCreationCode, bytes32(targetChainId)); | ||
| if (pointer != address(0)) { | ||
| if (StateProverPointer(pointer).implementationAddress() == address(0)) { | ||
| // This will only work if `msg.sender` is the owner of the pointer. | ||
| StateProverPointer(pointer).setImplementationAddress(address(prover)); | ||
| } | ||
| } | ||
| } | ||
| vm.stopBroadcast(); | ||
|
|
||
| if (pointer == address(0)) { | ||
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover); | ||
| } else { | ||
| _writeProver(_chainName(targetChainId), address(pointer), prover); | ||
| } |
There was a problem hiding this comment.
Persist by chain role, not by pointer zero sentinel.
At Line 58, a zero pointer currently routes to _writeCopy(...). On the canonical chain, pointer deployment failure should fail the script, not produce copy metadata.
Suggested fix
- if (pointer == address(0)) {
- _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover);
- } else {
- _writeProver(_chainName(targetChainId), address(pointer), prover);
- }
+ if (block.chainid == homeChainId) {
+ if (pointer == address(0)) {
+ revert DeploymentFailed();
+ }
+ _writeProver(_chainName(targetChainId), address(pointer), prover);
+ } else {
+ _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/DeployLineaChildToParent.s.sol` around lines 45 -
62, The code treats a zero pointer as a signal to call _writeCopy, but on the
canonical/home chain a failed pointer deployment should abort; modify the logic
so that when block.chainid == homeChainId (the canonical chain) the script
requires pointer != address(0) (e.g. revert or require with a clear message like
"pointer deployment failed on home chain"), and only when not on the home chain
allow falling back to _writeCopy; update the block that currently checks pointer
== address(0) before calling _writeCopy/_writeProver accordingly, referencing
pointer, StateProverPointer, _deploy, _writeCopy, _writeProver, homeChainId and
targetChainId.
Comment on lines
+45
to
+62
| if (block.chainid == homeChainId) { | ||
| bytes memory pointerCreationCode = | ||
| abi.encodePacked(type(StateProverPointer).creationCode, abi.encode(owner)); | ||
| pointer = _deploy(pointerCreationCode, bytes32(targetChainId)); | ||
| if (pointer != address(0)) { | ||
| if (StateProverPointer(pointer).implementationAddress() == address(0)) { | ||
| // This will only work if `msg.sender` is the owner of the pointer. | ||
| StateProverPointer(pointer).setImplementationAddress(address(prover)); | ||
| } | ||
| } | ||
| } | ||
| vm.stopBroadcast(); | ||
|
|
||
| if (pointer == address(0)) { | ||
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover); | ||
| } else { | ||
| _writeProver(_chainName(targetChainId), address(pointer), prover); | ||
| } |
There was a problem hiding this comment.
Guard canonical path: pointer failure should not fall through to copy writes.
At Line 58, the persistence branch depends only on pointer == address(0). On homeChainId, this can record a copy even when pointer deployment failed.
Suggested fix
- if (pointer == address(0)) {
- _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover);
- } else {
- _writeProver(_chainName(targetChainId), address(pointer), prover);
- }
+ if (block.chainid == homeChainId) {
+ if (pointer == address(0)) {
+ revert DeploymentFailed();
+ }
+ _writeProver(_chainName(targetChainId), address(pointer), prover);
+ } else {
+ _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/DeployOptimismChildToParent.s.sol` around lines 45
- 62, The current persistence logic can write a copy when pointer deployment
failed on the home chain because it only checks pointer == address(0); modify
the branch so that when block.chainid == homeChainId you never call _writeCopy
on pointer failure—use a deployed flag or check the original condition (e.g.,
track bool pointerDeployed = (pointer != address(0) &&
StateProverPointer(pointer).implementationAddress() != address(0))) or
explicitly guard the final persistence with if (block.chainid == homeChainId) {
if (pointer != address(0)) _writeProver(...); } else { if (pointer ==
address(0)) _writeCopy(...); else _writeProver(...); } to ensure _writeCopy is
only called for non-home chains and _writeProver only for a valid pointer;
reference StateProverPointer, _deploy, pointer, homeChainId, targetChainId,
_writeCopy, and _writeProver.
Comment on lines
+47
to
+65
| if (block.chainid == homeChainId) { | ||
| bytes memory pointerCreationCode = | ||
| abi.encodePacked(type(StateProverPointer).creationCode, abi.encode(owner)); | ||
| pointer = _deploy(pointerCreationCode, bytes32(targetChainId)); | ||
| if (pointer != address(0)) { | ||
| if (StateProverPointer(pointer).implementationAddress() == address(0)) { | ||
| // This will only work if `msg.sender` is the owner of the pointer. | ||
| StateProverPointer(pointer).setImplementationAddress(address(prover)); | ||
| } | ||
| } | ||
| } | ||
| vm.stopBroadcast(); | ||
|
|
||
| if (pointer == address(0)) { | ||
| // If the pointer is not deployed, it means that this is a copy of the prover deployed in a different chain. | ||
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover)); | ||
| } else { | ||
| _writeProver(_chainName(targetChainId), address(pointer), address(prover)); | ||
| } |
There was a problem hiding this comment.
Avoid writing copy metadata for canonical-chain pointer deployment failures.
At Line 60, pointer == address(0) routes to _writeCopy(...). On homeChainId, pointer deployment failure should be treated as deployment failure to avoid corrupting manifest state.
Suggested fix
- if (pointer == address(0)) {
- // If the pointer is not deployed, it means that this is a copy of the prover deployed in a different chain.
- _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover));
- } else {
- _writeProver(_chainName(targetChainId), address(pointer), address(prover));
- }
+ if (block.chainid == homeChainId) {
+ if (pointer == address(0)) {
+ revert DeploymentFailed();
+ }
+ _writeProver(_chainName(targetChainId), address(pointer), address(prover));
+ } else {
+ _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/DeployOptimismParentToChild.s.sol` around lines 47
- 65, The current post-deploy branch treats pointer == address(0) as a copy and
calls _writeCopy, which can corrupt manifests when running on homeChainId;
update the logic in DeployOptimismParentToChild.s.sol (the block after
vm.stopBroadcast that references pointer, _writeCopy, and _writeProver) so that
_writeCopy is only called when pointer == address(0) AND block.chainid !=
homeChainId, and treat pointer == address(0) on homeChainId as a deployment
failure (e.g., revert or emit an error) instead of writing copy metadata; keep
the existing behavior that writes the pointer via _writeProver when pointer !=
address(0).
| import {ParentToChildProver} from "src/contracts/provers/scroll/ParentToChildProver.sol"; | ||
| import {StateProverPointer} from "src/contracts/StateProverPointer.sol"; | ||
|
|
||
| contract DeployArbitrumParentToChild is DeployBase { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Verify contract declarations and caller expectations
fd 'Deploy*ParentToChild.s.sol' scripts/deployment/provers -x sh -c 'echo "== {} =="; rg -n "^contract\\s+" "{}"'
rg -n "DeployScrollParentToChild|DeployArbitrumParentToChild" scripts/deployment/provers/deploy-parent-provers.shRepository: openintentsframework/broadcaster
Length of output: 230
🏁 Script executed:
#!/usr/bin/env bash
# Read contract declarations from both files
echo "=== DeployArbitrumParentToChild.s.sol ==="
head -20 scripts/deployment/provers/DeployArbitrumParentToChild.s.sol | cat -n
echo -e "\n=== DeployScrollParentToChild.s.sol ==="
head -20 scripts/deployment/provers/DeployScrollParentToChild.s.sol | cat -nRepository: openintentsframework/broadcaster
Length of output: 1927
Contract name does not match file/purpose.
Line 10 of DeployScrollParentToChild.s.sol incorrectly declares DeployArbitrumParentToChild instead of DeployScrollParentToChild. The file contains Scroll-specific implementation (importing from scroll/ParentToChildProver.sol, using SCROLL_CHAIN and FINALIZED_STATE_ROOTS_SLOT environment variables), making this naming mismatch a significant operational error that will break invocations targeting the correct contract by name.
Proposed fix
-contract DeployArbitrumParentToChild is DeployBase {
+contract DeployScrollParentToChild is DeployBase {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| contract DeployArbitrumParentToChild is DeployBase { | |
| contract DeployScrollParentToChild is DeployBase { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/DeployScrollParentToChild.s.sol` at line 10, The
contract declared as DeployArbitrumParentToChild is misnamed for this
Scroll-specific deployment; rename the contract to DeployScrollParentToChild
(update the contract identifier wherever used) so it matches the file and
imported Scroll-specific items (ParentToChildProver, SCROLL_CHAIN,
FINALIZED_STATE_ROOTS_SLOT) and any external callers referencing the contract
name; ensure the contract inheritance (DeployBase) and any constructor or
deployment metadata remain unchanged after renaming.
nahimterrazas
approved these changes
Mar 2, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
scripts/deployment/provers/DeployOptimismParentToChild.s.sol (1)
60-65:⚠️ Potential issue | 🟠 MajorAvoid writing copy metadata when pointer deployment fails on canonical chain.
This still treats
pointer == address(0)as a copy path universally, which can corrupt manifests when running onhomeChainId.Proposed fix
- if (pointer == address(0)) { - // If the pointer is not deployed, it means that this is a copy of the prover deployed in a different chain. - _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover)); - } else { - _writeProver(_chainName(targetChainId), address(pointer), address(prover)); - } + if (block.chainid == homeChainId) { + if (pointer == address(0)) { + revert DeploymentFailed(); + } + _writeProver(_chainName(targetChainId), address(pointer), address(prover)); + } else { + _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deployment/provers/DeployOptimismParentToChild.s.sol` around lines 60 - 65, The current logic treats pointer == address(0) as a copy deployment universally and calls _writeCopy, which corrupts manifests when running on the canonical (home) chain; change the branch so that if pointer == address(0) you only call _writeCopy when the current deployment is NOT for homeChainId, and when running on homeChainId treat pointer == address(0) as a failed deployment (emit an error or revert/exit) instead of writing copy metadata; update the conditional around pointer, using homeChainId/targetChainId/_chainName(...) and the same _writeCopy/_writeProver calls to ensure copies are only written for non-home chains.scripts/deployment/provers/deploy-parent-provers.sh (1)
1-2:⚠️ Potential issue | 🟠 MajorEnable strict/fail-fast shell mode.
Without strict mode, one failed deployment step can still allow later steps and leave partial state.
Proposed fix
#!/usr/bin/env bash +set -euo pipefail +IFS=$'\n\t'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deployment/provers/deploy-parent-provers.sh` around lines 1 - 2, Add strict/fail-fast shell mode to the top of the deploy-parent-provers.sh script: after the existing shebang line enable "set -euo pipefail" and set a safe IFS (e.g. IFS=$'\n\t') so any command failure, unset variable, or failed pipeline aborts the script; update the top of the script (in deploy-parent-provers.sh) to include these settings before any other logic or commands run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/deployment/provers/deploy-parent-provers.sh`:
- Around line 3-51: The parent prover deploy scripts call vm.envAddress("OWNER")
so require OWNER to be passed explicitly to each forge invocation; update every
forge script call (DeployArbitrumParentToChild.s.sol,
DeployLineaParentToChild.s.sol, DeployScrollParentToChild.s.sol,
DeployOptimismParentToChild.s.sol, DeployZkSyncParentToChild.s.sol) to include
OWNER="$OWNER" in the exported env variables before the forge script command so
the scripts no longer rely on implicit shell state.
In `@scripts/deployment/provers/DeployZkSyncChildToParent.s.sol`:
- Around line 58-62: When pointer == address(0) you must treat a missing
canonical-chain pointer on the home chain as a failure instead of calling
_writeCopy; update the conditional around pointer in
DeployZkSyncChildToParent.s.sol so that if (pointer == address(0)) you first
check whether targetChainId (or the runtime chain id being the home chain)
equals homeChainId and if so revert or emit a failure (clear error message),
otherwise proceed to call _writeCopy(_chainName(homeChainId),
_chainName(targetChainId), prover); leave the existing _writeProver(...) branch
unchanged for non-zero pointer values.
In `@scripts/deployment/provers/DeployZkSyncParentToChild.s.sol`:
- Around line 63-68: When pointer == address(0) we must not assume a copy; only
write copy metadata when the zero pointer is observed for a non-home chain.
Change the branch so that if (pointer == address(0)) you first check that
homeChainId != targetChainId before calling _writeCopy(_chainName(homeChainId),
_chainName(targetChainId), address(prover)); otherwise treat it as a deployment
failure (e.g., revert or skip writing) so you do not persist incorrect copy
metadata for homeChainId; keep the existing _writeProver(...) call for the
non-zero pointer path.
---
Duplicate comments:
In `@scripts/deployment/provers/deploy-parent-provers.sh`:
- Around line 1-2: Add strict/fail-fast shell mode to the top of the
deploy-parent-provers.sh script: after the existing shebang line enable "set
-euo pipefail" and set a safe IFS (e.g. IFS=$'\n\t') so any command failure,
unset variable, or failed pipeline aborts the script; update the top of the
script (in deploy-parent-provers.sh) to include these settings before any other
logic or commands run.
In `@scripts/deployment/provers/DeployOptimismParentToChild.s.sol`:
- Around line 60-65: The current logic treats pointer == address(0) as a copy
deployment universally and calls _writeCopy, which corrupts manifests when
running on the canonical (home) chain; change the branch so that if pointer ==
address(0) you only call _writeCopy when the current deployment is NOT for
homeChainId, and when running on homeChainId treat pointer == address(0) as a
failed deployment (emit an error or revert/exit) instead of writing copy
metadata; update the conditional around pointer, using
homeChainId/targetChainId/_chainName(...) and the same _writeCopy/_writeProver
calls to ensure copies are only written for non-home chains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68609f9f-da1c-4d9a-8887-4116ff1d6a09
📒 Files selected for processing (13)
deployments/arbitrum-sepolia.jsondeployments/ethereum-sepolia.jsondeployments/linea-sepolia.jsondeployments/optimism-sepolia.jsondeployments/scroll-sepolia.jsondeployments/zksync-sepolia.jsonfoundry.tomlscripts/deployment/provers/DeployOptimismParentToChild.s.solscripts/deployment/provers/DeployZkSyncChildToParent.s.solscripts/deployment/provers/DeployZkSyncParentToChild.s.solscripts/deployment/provers/deploy-child-provers.shscripts/deployment/provers/deploy-parent-provers.shsnapshots/verifyBroadcastMessage.json
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/deployment/provers/deploy-child-provers.sh
- foundry.toml
- deployments/linea-sepolia.json
- deployments/arbitrum-sepolia.json
- deployments/optimism-sepolia.json
Comment on lines
+3
to
+51
| OUTBOX=$ARBITRUM_SEPOLIA_OUTBOX \ | ||
| ROOTS_SLOT=$ARBITRUM_SEPOLIA_ROOTS_SLOT \ | ||
| HOME_CHAIN_ID=$ETHEREUM_SEPOLIA_CHAIN_ID \ | ||
| TARGET_CHAIN_ID=$ARBITRUM_SEPOLIA_CHAIN_ID \ | ||
| forge script scripts/deployment/provers/DeployArbitrumParentToChild.s.sol \ | ||
| --rpc-url "$RPC_URL" \ | ||
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | ||
| --broadcast | ||
|
|
||
|
|
||
| ROLLUP=$LINEA_SEPOLIA_ROLLUP \ | ||
| STATE_ROOT_HASHES_SLOT=$LINEA_SEPOLIA_STATE_ROOT_HASHES_SLOT \ | ||
| HOME_CHAIN_ID=$ETHEREUM_SEPOLIA_CHAIN_ID \ | ||
| TARGET_CHAIN_ID=$LINEA_SEPOLIA_CHAIN_ID \ | ||
| forge script scripts/deployment/provers/DeployLineaParentToChild.s.sol \ | ||
| --rpc-url "$RPC_URL" \ | ||
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | ||
| --broadcast | ||
|
|
||
|
|
||
| SCROLL_CHAIN=$SCROLL_CHAIN_SEPOLIA \ | ||
| FINALIZED_STATE_ROOTS_SLOT=$SCROLL_SEPOLIA_FINALIZED_STATE_ROOT_SLOT \ | ||
| HOME_CHAIN_ID=$ETHEREUM_SEPOLIA_CHAIN_ID \ | ||
| TARGET_CHAIN_ID=$SCROLL_SEPOLIA_CHAIN_ID \ | ||
| forge script scripts/deployment/provers/DeployScrollParentToChild.s.sol \ | ||
| --rpc-url "$RPC_URL" \ | ||
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | ||
| --broadcast | ||
|
|
||
|
|
||
| ANCHOR_STATE_REGISTRY=$OPTIMISM_SEPOLIA_ANCHOR_STATE_REGISTRY \ | ||
| ANCHOR_GAME_SLOT=$OPTIMISM_SEPOLIA_ANCHOR_GAME_SLOT \ | ||
| HOME_CHAIN_ID=$ETHEREUM_SEPOLIA_CHAIN_ID \ | ||
| TARGET_CHAIN_ID=$OPTIMISM_SEPOLIA_CHAIN_ID \ | ||
| forge script scripts/deployment/provers/DeployOptimismParentToChild.s.sol \ | ||
| --rpc-url "$RPC_URL" \ | ||
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | ||
| --broadcast | ||
|
|
||
| GATEWAY_ZK_CHAIN=$ZKSYNC_SEPOLIA_GATEWAY_ZK_CHAIN \ | ||
| L2_LOGS_ROOT_HASH_SLOT=$ZKSYNC_SEPOLIA_L2_LOGS_ROOT_HASH_SLOT \ | ||
| CHILD_CHAIN_ID=$ZKSYNC_SEPOLIA_CHAIN_ID \ | ||
| GATEWAY_CHAIN_ID=$ZKSYNC_SEPOLIA_GATEWAY_CHAIN_ID \ | ||
| HOME_CHAIN_ID=$ETHEREUM_SEPOLIA_CHAIN_ID \ | ||
| TARGET_CHAIN_ID=$ZKSYNC_SEPOLIA_CHAIN_ID \ | ||
| forge script scripts/deployment/provers/DeployZkSyncParentToChild.s.sol \ | ||
| --rpc-url "$RPC_URL" \ | ||
| --private-key "$DEPLOYER_PRIVATE_KEY" \ | ||
| --broadcast |
There was a problem hiding this comment.
Make OWNER an explicit required input for all forge calls.
All parent prover scripts consume vm.envAddress("OWNER"); relying on implicit exported shell state is fragile and can fail at runtime.
Proposed fix
#!/usr/bin/env bash
set -euo pipefail
IFS=$'\n\t'
+
+: "${OWNER:?OWNER is required}"
+export OWNER🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/deploy-parent-provers.sh` around lines 3 - 51, The
parent prover deploy scripts call vm.envAddress("OWNER") so require OWNER to be
passed explicitly to each forge invocation; update every forge script call
(DeployArbitrumParentToChild.s.sol, DeployLineaParentToChild.s.sol,
DeployScrollParentToChild.s.sol, DeployOptimismParentToChild.s.sol,
DeployZkSyncParentToChild.s.sol) to include OWNER="$OWNER" in the exported env
variables before the forge script command so the scripts no longer rely on
implicit shell state.
Comment on lines
+58
to
+62
| if (pointer == address(0)) { | ||
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover); | ||
| } else { | ||
| _writeProver(_chainName(targetChainId), address(pointer), prover); | ||
| } |
There was a problem hiding this comment.
Handle canonical-chain pointer absence as failure, not copy.
If this runs on homeChainId, pointer == address(0) should not fall through to _writeCopy(...); that records wrong state.
Proposed fix
- if (pointer == address(0)) {
- _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover);
- } else {
- _writeProver(_chainName(targetChainId), address(pointer), prover);
- }
+ if (block.chainid == homeChainId) {
+ if (pointer == address(0)) {
+ revert DeploymentFailed();
+ }
+ _writeProver(_chainName(targetChainId), address(pointer), prover);
+ } else {
+ _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (pointer == address(0)) { | |
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover); | |
| } else { | |
| _writeProver(_chainName(targetChainId), address(pointer), prover); | |
| } | |
| if (block.chainid == homeChainId) { | |
| if (pointer == address(0)) { | |
| revert DeploymentFailed(); | |
| } | |
| _writeProver(_chainName(targetChainId), address(pointer), prover); | |
| } else { | |
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/DeployZkSyncChildToParent.s.sol` around lines 58 -
62, When pointer == address(0) you must treat a missing canonical-chain pointer
on the home chain as a failure instead of calling _writeCopy; update the
conditional around pointer in DeployZkSyncChildToParent.s.sol so that if
(pointer == address(0)) you first check whether targetChainId (or the runtime
chain id being the home chain) equals homeChainId and if so revert or emit a
failure (clear error message), otherwise proceed to call
_writeCopy(_chainName(homeChainId), _chainName(targetChainId), prover); leave
the existing _writeProver(...) branch unchanged for non-zero pointer values.
Comment on lines
+63
to
+68
| if (pointer == address(0)) { | ||
| // If the pointer is not deployed, it means that this is a copy of the prover deployed in a different chain. | ||
| _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover)); | ||
| } else { | ||
| _writeProver(_chainName(targetChainId), address(pointer), address(prover)); | ||
| } |
There was a problem hiding this comment.
Do not write copy metadata when canonical pointer deployment fails.
On homeChainId, pointer == address(0) is a deployment failure, not a copy scenario. Current flow can persist incorrect deployment state.
Proposed fix
- if (pointer == address(0)) {
- // If the pointer is not deployed, it means that this is a copy of the prover deployed in a different chain.
- _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover));
- } else {
- _writeProver(_chainName(targetChainId), address(pointer), address(prover));
- }
+ if (block.chainid == homeChainId) {
+ if (pointer == address(0)) {
+ revert DeploymentFailed();
+ }
+ _writeProver(_chainName(targetChainId), address(pointer), address(prover));
+ } else {
+ _writeCopy(_chainName(homeChainId), _chainName(targetChainId), address(prover));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/provers/DeployZkSyncParentToChild.s.sol` around lines 63 -
68, When pointer == address(0) we must not assume a copy; only write copy
metadata when the zero pointer is observed for a non-home chain. Change the
branch so that if (pointer == address(0)) you first check that homeChainId !=
targetChainId before calling _writeCopy(_chainName(homeChainId),
_chainName(targetChainId), address(prover)); otherwise treat it as a deployment
failure (e.g., revert or skip writing) so you do not persist incorrect copy
metadata for homeChainId; keep the existing _writeProver(...) call for the
non-zero pointer path.
frangio
reviewed
Mar 9, 2026
frangio
reviewed
Mar 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
scripts/deployment/DeployBase.s.sol (1)
204-209:⚠️ Potential issue | 🔴 CriticalHandle proxy deployment failure before returning
predictedAddress.Line 207 captures
success, but Line 209 returnspredictedAddressunconditionally. A failed proxy call still yields a non-zero address here, so callers can persist an undeployed contract as if deployment succeeded.Suggested patch
function _deploy(bytes memory creationCode, bytes32 salt) internal returns (address) { address predictedAddress = Create2.computeAddress(salt, keccak256(creationCode), DEPLOYMENT_PROXY); - bool success = LowLevelCall.callNoReturn(DEPLOYMENT_PROXY, abi.encodePacked(salt, creationCode)); + if (predictedAddress.code.length == 0) { + bool success = LowLevelCall.callNoReturn(DEPLOYMENT_PROXY, abi.encodePacked(salt, creationCode)); + if (!success || predictedAddress.code.length == 0) { + revert DeploymentFailed(); + } + } return predictedAddress; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deployment/DeployBase.s.sol` around lines 204 - 209, In _deploy, don't return predictedAddress when the proxy call failed: after calling LowLevelCall.callNoReturn(DEPLOYMENT_PROXY, abi.encodePacked(salt, creationCode)) store the boolean result in success (already present) and assert or revert on failure (e.g., require(success, "DEPLOYMENT_PROXY call failed")) before returning predictedAddress so callers cannot treat an undeployed address as deployed; update _deploy to check success and revert with a clear message referencing DEPLOYMENT_PROXY/LowLevelCall.callNoReturn if success is false.scripts/deployment/deploy.sh (1)
3-5:⚠️ Potential issue | 🟠 MajorFail fast before sourcing
.env.A missing/unreadable
.envor any later sub-script failure still lets this orchestration continue into the next chains with unset values.Suggested patch
#!/usr/bin/env bash +set -euo pipefail + +if [[ ! -f .env ]]; then + echo "Missing .env file" >&2 + exit 1 +fi + set -a source .env set +a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deployment/deploy.sh` around lines 3 - 5, Add a fail-fast guard before sourcing .env: enable strict failure (e.g., set -euo pipefail) and verify the file is present and readable, then only export when sourcing; for example check that ".env" exists and is readable and exit with a clear message if not, then run set -a; source .env; set +a. Reference the existing symbols "set -a", "source .env", and "set +a" when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 8: The .env.example currently contains a real Ethereum address for the
OWNER variable; replace the hardcoded value for OWNER with an obviously fake
placeholder or leave it blank (e.g., OWNER=YOUR_OWNER_ADDRESS or OWNER=) so
operators must set their own address rather than accidentally inheriting
0xCc2ACDd7eF3e9841d42E00199D4cF5bcdCa77a7e; update the OWNER entry accordingly
and ensure no other real secrets/addresses remain in .env.example.
In `@scripts/deployment/deploy.sh`:
- Around line 49-71: The script repeatedly invokes deploy-parent-provers.sh with
different RPC_URLs (RPC_URL="$ETHEREUM_RPC_URL"
./scripts/deployment/provers/deploy-parent-provers.sh then repeated for
ARBITRUM_RPC_URL, LINEA_RPC_URL, etc.), which replays the full parent-to-child
prover matrix against incorrect chains; change the file so you call
./scripts/deployment/provers/deploy-parent-provers.sh only once with the
Ethereum RPC (use RPC_URL="$ETHEREUM_RPC_URL"
./scripts/deployment/provers/deploy-parent-provers.sh), remove the subsequent
repeated invocations/echo blocks for Arbitrum, Linea, Scroll, Optimism, and
ZkSync, and keep a single descriptive echo for the Ethereum deployment.
In `@scripts/deployment/DeployBase.s.sol`:
- Around line 142-147: The _isBufferDeployed function is checking the wrong JSON
path; replace the vm.keyExistsJson call that uses ".buffers.<chainKey>" with one
that checks under "contracts.buffer" (e.g. vm.keyExistsJson(json,
string.concat(".contracts.buffer.", chainKey)) if buffers are namespaced by
chainKey, or vm.keyExistsJson(json, ".contracts.buffer") if the manifest stores
a single buffer entry); update the vm.keyExistsJson call inside
_isBufferDeployed accordingly to reference contracts.buffer instead of .buffers.
---
Duplicate comments:
In `@scripts/deployment/deploy.sh`:
- Around line 3-5: Add a fail-fast guard before sourcing .env: enable strict
failure (e.g., set -euo pipefail) and verify the file is present and readable,
then only export when sourcing; for example check that ".env" exists and is
readable and exit with a clear message if not, then run set -a; source .env; set
+a. Reference the existing symbols "set -a", "source .env", and "set +a" when
applying the change.
In `@scripts/deployment/DeployBase.s.sol`:
- Around line 204-209: In _deploy, don't return predictedAddress when the proxy
call failed: after calling LowLevelCall.callNoReturn(DEPLOYMENT_PROXY,
abi.encodePacked(salt, creationCode)) store the boolean result in success
(already present) and assert or revert on failure (e.g., require(success,
"DEPLOYMENT_PROXY call failed")) before returning predictedAddress so callers
cannot treat an undeployed address as deployed; update _deploy to check success
and revert with a clear message referencing
DEPLOYMENT_PROXY/LowLevelCall.callNoReturn if success is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4787772f-d4ac-49f9-9ca7-31ed800fe61c
📒 Files selected for processing (14)
.env.exampledeployments/arbitrum-sepolia.jsondeployments/ethereum-sepolia.jsondeployments/linea-sepolia.jsondeployments/optimism-sepolia.jsondeployments/scroll-sepolia.jsondeployments/zksync-sepolia.jsonscripts/deployment/DeployBase.s.solscripts/deployment/block-hash-pusher/DeployPushers.s.solscripts/deployment/block-hash-pusher/deploy-pushers.shscripts/deployment/deploy.shscripts/deployment/provers/deploy-child-provers.shscripts/deployment/provers/deploy-parent-provers.shscripts/generate-optimism-test-payloads.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/deployment/provers/deploy-child-provers.sh
- deployments/ethereum-sepolia.json
- scripts/deployment/provers/deploy-parent-provers.sh
- scripts/deployment/block-hash-pusher/DeployPushers.s.sol
- deployments/scroll-sepolia.json
- scripts/deployment/block-hash-pusher/deploy-pushers.sh
| LINEA_RPC_URL= | ||
| SCROLL_RPC_URL= | ||
|
|
||
| OWNER=0xCc2ACDd7eF3e9841d42E00199D4cF5bcdCa77a7e |
There was a problem hiding this comment.
Remove the real OWNER value from the example file.
Copying this file verbatim will make deployments assign ownership to 0xCc2ACDd7eF3e9841d42E00199D4cF5bcdCa77a7e. Leave the sample blank or replace it with an obvious placeholder so operators must set their own address.
Suggested patch
-OWNER=0xCc2ACDd7eF3e9841d42E00199D4cF5bcdCa77a7e
+OWNER=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OWNER=0xCc2ACDd7eF3e9841d42E00199D4cF5bcdCa77a7e | |
| OWNER= |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 8, The .env.example currently contains a real Ethereum
address for the OWNER variable; replace the hardcoded value for OWNER with an
obviously fake placeholder or leave it blank (e.g., OWNER=YOUR_OWNER_ADDRESS or
OWNER=) so operators must set their own address rather than accidentally
inheriting 0xCc2ACDd7eF3e9841d42E00199D4cF5bcdCa77a7e; update the OWNER entry
accordingly and ensure no other real secrets/addresses remain in .env.example.
Comment on lines
+49
to
+71
| # Ethereum | ||
| echo "Deploying ParentToChildProver contracts on Ethereum..." | ||
| RPC_URL="$ETHEREUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | ||
|
|
||
| # Arbitrum | ||
| echo "Deploying ParentToChildProver contracts on Arbitrum..." | ||
| RPC_URL="$ARBITRUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | ||
|
|
||
| # Linea | ||
| echo "Deploying ParentToChildProver contracts on Linea..." | ||
| RPC_URL="$LINEA_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | ||
|
|
||
| # Scroll | ||
| echo "Deploying ParentToChildProver contracts on Scroll..." | ||
| RPC_URL="$SCROLL_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | ||
|
|
||
| # Optimism | ||
| echo "Deploying ParentToChildProver contracts on Optimism..." | ||
| RPC_URL="$OPTIMISM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | ||
|
|
||
| # ZkSync | ||
| echo "Deploying ParentToChildProver contracts on ZkSync..." | ||
| RPC_URL="$ZKSYNC_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh |
There was a problem hiding this comment.
Call deploy-parent-provers.sh only once with the Ethereum RPC.
scripts/deployment/provers/deploy-parent-provers.sh already deploys the full parent-to-child prover matrix using its single RPC_URL. Re-invoking it on Arbitrum, Linea, Scroll, Optimism, and ZkSync replays that whole bundle against the wrong chain endpoints.
Suggested patch
# Ethereum
echo "Deploying ParentToChildProver contracts on Ethereum..."
RPC_URL="$ETHEREUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh
-# Arbitrum
-echo "Deploying ParentToChildProver contracts on Arbitrum..."
-RPC_URL="$ARBITRUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh
-
-# Linea
-echo "Deploying ParentToChildProver contracts on Linea..."
-RPC_URL="$LINEA_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh
-
-# Scroll
-echo "Deploying ParentToChildProver contracts on Scroll..."
-RPC_URL="$SCROLL_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh
-
-# Optimism
-echo "Deploying ParentToChildProver contracts on Optimism..."
-RPC_URL="$OPTIMISM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh
-
-# ZkSync
-echo "Deploying ParentToChildProver contracts on ZkSync..."
-RPC_URL="$ZKSYNC_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Ethereum | |
| echo "Deploying ParentToChildProver contracts on Ethereum..." | |
| RPC_URL="$ETHEREUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | |
| # Arbitrum | |
| echo "Deploying ParentToChildProver contracts on Arbitrum..." | |
| RPC_URL="$ARBITRUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | |
| # Linea | |
| echo "Deploying ParentToChildProver contracts on Linea..." | |
| RPC_URL="$LINEA_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | |
| # Scroll | |
| echo "Deploying ParentToChildProver contracts on Scroll..." | |
| RPC_URL="$SCROLL_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | |
| # Optimism | |
| echo "Deploying ParentToChildProver contracts on Optimism..." | |
| RPC_URL="$OPTIMISM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | |
| # ZkSync | |
| echo "Deploying ParentToChildProver contracts on ZkSync..." | |
| RPC_URL="$ZKSYNC_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh | |
| # Ethereum | |
| echo "Deploying ParentToChildProver contracts on Ethereum..." | |
| RPC_URL="$ETHEREUM_RPC_URL" ./scripts/deployment/provers/deploy-parent-provers.sh |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/deploy.sh` around lines 49 - 71, The script repeatedly
invokes deploy-parent-provers.sh with different RPC_URLs
(RPC_URL="$ETHEREUM_RPC_URL"
./scripts/deployment/provers/deploy-parent-provers.sh then repeated for
ARBITRUM_RPC_URL, LINEA_RPC_URL, etc.), which replays the full parent-to-child
prover matrix against incorrect chains; change the file so you call
./scripts/deployment/provers/deploy-parent-provers.sh only once with the
Ethereum RPC (use RPC_URL="$ETHEREUM_RPC_URL"
./scripts/deployment/provers/deploy-parent-provers.sh), remove the subsequent
repeated invocations/echo blocks for Arbitrum, Linea, Scroll, Optimism, and
ZkSync, and keep a single descriptive echo for the Ethereum deployment.
Collaborator
Author
There was a problem hiding this comment.
This is intended, as we are also deploying copies
There was a problem hiding this comment.
@luiz-lvj, understood! Thanks for clarifying — invoking deploy-parent-provers.sh per chain is intentional so that each network receives its own copy of the prover contracts, with the RPC_URL targeting the correct endpoint per deployment. The idempotency guards (_isProverDeployed, _isCopyDeployed) in DeployBase ensure no duplicate deployments happen. I'll note this for future reviews.
(´• ω •`) ฅ
✏️ Learnings added
Learnt from: luiz-lvj
Repo: openintentsframework/broadcaster PR: 50
File: scripts/deployment/deploy.sh:49-71
Timestamp: 2026-03-11T20:05:25.604Z
Learning: In `scripts/deployment/deploy.sh`, `deploy-parent-provers.sh` is intentionally invoked multiple times — once per chain (Ethereum, Arbitrum, Linea, Scroll, Optimism, ZkSync) — each with its respective `RPC_URL`. This is by design to deploy prover copies on each chain. The scripts use `_isProverDeployed` and `_isCopyDeployed` guards in `DeployBase.s.sol` to prevent duplicate deployments.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+142
to
+147
| function _isBufferDeployed(string memory chainKey) internal returns (bool) { | ||
| string memory path = _path(); | ||
| _ensureRootScaffold(path); | ||
| string memory json = vm.readFile(path); | ||
| return vm.keyExistsJson(json, string.concat(".buffers.", chainKey)); | ||
| } |
There was a problem hiding this comment.
Check the buffer at .contracts.buffer, not .buffers.<chainKey>.
The manifests added in this PR store buffers under contracts.buffer. As written, _isBufferDeployed will always return false and can trigger duplicate buffer deployments.
Suggested patch
- function _isBufferDeployed(string memory chainKey) internal returns (bool) {
+ function _isBufferDeployed(string memory) internal returns (bool) {
string memory path = _path();
_ensureRootScaffold(path);
string memory json = vm.readFile(path);
- return vm.keyExistsJson(json, string.concat(".buffers.", chainKey));
+ return vm.keyExistsJson(json, ".contracts.buffer");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/deployment/DeployBase.s.sol` around lines 142 - 147, The
_isBufferDeployed function is checking the wrong JSON path; replace the
vm.keyExistsJson call that uses ".buffers.<chainKey>" with one that checks under
"contracts.buffer" (e.g. vm.keyExistsJson(json,
string.concat(".contracts.buffer.", chainKey)) if buffers are namespaced by
chainKey, or vm.keyExistsJson(json, ".contracts.buffer") if the manifest stores
a single buffer entry); update the vm.keyExistsJson call inside
_isBufferDeployed accordingly to reference contracts.buffer instead of .buffers.
frangio
approved these changes
Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores