[Provers M01] Proof Stale Time in Optimism#87
Merged
Conversation
📝 WalkthroughWalkthroughAdds an Optimism L1→L2 block-hash buffering flow: new Changes
Sequence Diagram(s)sequenceDiagram
participant L1 as L1 Caller
participant Pusher as OptimismPusher
participant L1Messenger as L1CrossDomainMessenger
participant L2Messenger as L2CrossDomainMessenger
participant Buffer as OptimismBuffer
participant Prover as ChildToParentProver
L1->>Pusher: pushHashes(buffer, firstBlockNumber, batchSize, l2TxData)
Pusher->>Pusher: build block-hash array & encode IBuffer.receiveHashes
Pusher->>L1Messenger: sendMessage(buffer, calldata, gasLimit)
L1Messenger->>L2Messenger: relayMessage(...)
L2Messenger->>Buffer: receiveHashes(firstBlockNumber, blockHashes)
Buffer->>Buffer: check caller == l2CrossDomainMessenger
Buffer->>Buffer: check xDomainMessageSender == pusher
Buffer->>Buffer: store hashes (emit BlockHashesPushed if written)
Prover->>Buffer: parentChainBlockHash(targetBlockNumber)
Buffer-->>Prover: return block hash
Prover->>Prover: verify target state commitment using returned hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/VerifyBroadcastMessageBenchmark.t.sol (1)
400-404:⚠️ Potential issue | 🟡 MinorStale comment: still references "L1Block predeploy".
Line 403 says "Hop 1: OP-C2P gets Ethereum block hash from L1Block predeploy" but the prover now reads from the block hash buffer.
📝 Proposed fix
- /// Hop 1: OP-C2P gets Ethereum block hash from L1Block predeploy + /// Hop 1: OP-C2P gets Ethereum block hash from block hash buffer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/VerifyBroadcastMessageBenchmark.t.sol` around lines 400 - 404, Update the stale comment in the benchmark description in VerifyBroadcastMessageBenchmark.t.sol so Hop 1 no longer says "OP-C2P gets Ethereum block hash from L1Block predeploy" but instead references reading from the block hash buffer (e.g., change the Hop 1 line to mention "reads Ethereum block hash from block hash buffer"); adjust any related phrasing in the same comment block (the route/Hop descriptions) to reflect "block hash buffer" rather than "L1Block predeploy" so the documentation matches the current prover behavior.test/provers/optimism/ChildToParentProver.t.sol (1)
13-17:⚠️ Potential issue | 🟡 MinorStale NatSpec: still references "L1Block predeploy".
The comment on Line 16 says "This prover reads L1 block hashes from the L1Block predeploy on Optimism" but the prover now reads from a block hash buffer.
📝 Proposed fix
-/** - * `@title` Optimism ChildToParentProver Test - * `@notice` Tests for the Optimism ChildToParentProver contract - * `@dev` This prover reads L1 block hashes from the L1Block predeploy on Optimism - */ +/** + * `@title` Optimism ChildToParentProver Test + * `@notice` Tests for the Optimism ChildToParentProver contract + * `@dev` This prover reads L1 block hashes from a block hash buffer on Optimism + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/provers/optimism/ChildToParentProver.t.sol` around lines 13 - 17, The NatSpec header for the Optimism ChildToParentProver test is stale: update the comment that currently says "reads L1 block hashes from the L1Block predeploy on Optimism" to reflect the current implementation (it now reads from the block hash buffer). Edit the top comment for the ChildToParentProver test to say something like "This prover reads L1 block hashes from the block hash buffer on Optimism" so the description matches the actual code paths in the ChildToParentProver tests.
🧹 Nitpick comments (5)
test/provers/optimism/ChildToParentProver.t.sol (1)
42-83: Testtest_verifyStorageSlot_broadcasterdoesn't exerciseverifyTargetStateCommitmentorgetTargetStateCommitment.The buffer hashes are pushed (Line 73) but neither
verifyTargetStateCommitmentnorgetTargetStateCommitmentis called — onlyverifyStorageSlotis tested, which is a pure function that doesn't interact with the buffer at all. Consider adding test coverage for the new buffer-based target state commitment paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/provers/optimism/ChildToParentProver.t.sol` around lines 42 - 83, The test currently pushes the block hash into the buffer but never calls the buffer-backed target state commitment paths; after buffer.receiveHashes(blockNumber, blockHashes) call into childToParentProver.verifyTargetStateCommitment(...) and childToParentProver.getTargetStateCommitment(...) (using the same blockHash and the appropriate proof data from the JSON/rlpBlockHeader or the encoded input) and add assertions that their returned commitment/verification result matches the expected commitment computed from the test vectors; insert these calls just before calling childToParentProver.verifyStorageSlot so the buffer path is exercised (referencing buffer.receiveHashes, childToParentProver.verifyTargetStateCommitment, and childToParentProver.getTargetStateCommitment).test/Receiver.t.sol (1)
286-290: Optional: inline theIBuffercast or hoistbufferto function scope.The pattern
IBuffer buffer = IBuffer(blockHashBuffer)is repeated in five OP test functions, each immediately after theblockHashBuffervariable is already in scope. SinceBufferMockis declared as plainaddress, consider either casting inline (IBuffer(blockHashBuffer).receiveHashes(...)) or declaringbufferonce at the start of each function alongsideblockHashBuffer.♻️ Example inline simplification (applies analogously to all five occurrences)
- IBuffer buffer = IBuffer(blockHashBuffer); - bytes32[] memory blockHashes = new bytes32[](1); - blockHashes[0] = blockHash; - - buffer.receiveHashes(blockNumber, blockHashes); + bytes32[] memory blockHashes = new bytes32[](1); + blockHashes[0] = blockHash; + IBuffer(blockHashBuffer).receiveHashes(blockNumber, blockHashes);Also applies to: 357-361, 430-434, 495-499, 558-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Receiver.t.sol` around lines 286 - 290, The IBuffer cast is redundantly assigned to a local variable in multiple test functions (e.g., IBuffer buffer = IBuffer(blockHashBuffer) before calling receiveHashes); either inline the cast at the call site (IBuffer(blockHashBuffer).receiveHashes(...)) or hoist a single IBuffer buffer = IBuffer(blockHashBuffer) declaration to the top of each test function where blockHashBuffer is declared so the repeated pattern is removed; update all five occurrences referencing IBuffer, blockHashBuffer, and receiveHashes accordingly.test/block-hash-pusher/optimism/OptimismBuffer.t.sol (3)
94-104: Parameterizedtest_function should followtestFuzz_naming convention; weakvm.expectRevert()on Line 101.Two issues:
Naming:
test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher(address notPusher)is a fuzz test (Foundry treats anytest*function with parameters as fuzz). ThetestFuzz_prefix signals this intent clearly and matches the convention used elsewhere in this file.Assertion strength:
vm.expectRevert()on Line 101 accepts any revert—including unexpected panics or wrong custom errors. The actual expected error fromOptimismBufferwhen the xDomain sender doesn't match isDomainMessageSenderMismatch; the test should assert that specific selector.♻️ Proposed changes
- function test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher(address notPusher) public { + function testFuzz_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher(address notPusher) public { vm.assume(notPusher != pusher); OptimismBuffer buffer = new OptimismBuffer(address(mockOpCrosschainDomainMessenger), pusher); bytes memory l2Calldata = abi.encodeCall(buffer.receiveHashes, (1, new bytes32[](1))); - vm.expectRevert(); + vm.expectRevert(abi.encodeWithSelector(OptimismBuffer.DomainMessageSenderMismatch.selector)); vm.prank(relayer); mockOpCrosschainDomainMessenger.relayMessage(address(buffer), notPusher, l2Calldata, 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/block-hash-pusher/optimism/OptimismBuffer.t.sol` around lines 94 - 104, Rename the fuzz test function test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher to start with testFuzz_ (e.g., testFuzz_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher) so it follows the file's fuzz naming convention, and replace the weak vm.expectRevert() call with a targeted revert assertion using the OptimismBuffer error selector (expectRevert(abi.encodeWithSelector(OptimismBuffer.DomainMessageSenderMismatch.selector))) before calling mockOpCrosschainDomainMessenger.relayMessage so the test asserts the specific DomainMessageSenderMismatch error instead of any revert.
22-23:uint8 firstBlockNumberartificially caps the fuzz domain to [0, 255].Real Ethereum block numbers are in the tens of millions. Any block-number-dependent logic above 255 in
receiveHashesorBaseBufferwould never be exercised by this fuzz run. The only boundary this intentionally covers isfirstBlockNumber == 0. Useuint256and tighten the assumption if the zero-check is all that matters, or widen to a representative range.♻️ Proposed change
- function testFuzz_receiveHashes(uint16 batchSize, uint8 firstBlockNumber) public { + function testFuzz_receiveHashes(uint16 batchSize, uint256 firstBlockNumber) public { vm.assume(batchSize > 0 && batchSize <= 8191);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/block-hash-pusher/optimism/OptimismBuffer.t.sol` around lines 22 - 23, The fuzz parameter firstBlockNumber is declared as uint8 which limits testing to 0..255; change the parameter in testFuzz_receiveHashes from uint8 to uint256 so block-number-dependent logic in receiveHashes and BaseBuffer can be exercised, and update the vm.assume to either only exclude negatives (e.g., keep vm.assume(batchSize > 0 && batchSize <= 8191) and add vm.assume(firstBlockNumber >= 0)) or constrain firstBlockNumber to a representative range (e.g., <= 1e8) depending on whether you only need to preserve the zero-check or want realistic block-number coverage.
42-42: Misleading test name — rename to reflect the actual duplicate-push scenario.
test_receiveHashes_does_not_emit_event_when_no_hashes_writtensounds like it covers a zero-hash batch, but the test actually verifies that replaying an identical push (all blocks already stored) suppresses theBlockHashesPushedevent. A name liketest_receiveHashes_does_not_emit_event_on_duplicate_pushis more precise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/block-hash-pusher/optimism/OptimismBuffer.t.sol` at line 42, Rename the misleading test function test_receiveHashes_does_not_emit_event_when_no_hashes_written to reflect the duplicate-push behavior being tested (e.g., test_receiveHashes_does_not_emit_event_on_duplicate_push); update the function declaration and any references or docstrings/comments inside the OptimismBuffer.t.sol test file so the name accurately describes that the test replays an identical push (all blocks already stored) and asserts that BlockHashesPushed is suppressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/block-hash-pusher/optimism/OptimismPusher.sol`:
- Line 9: Update the NatSpec title comment from "OPtimismPusher" to the correct
"OptimismPusher" in the OptimismPusher contract header comment; locate the
top-of-file NatSpec/title line (for the OptimismPusher contract) and correct the
capitalization/typo so it matches the contract name OptimismPusher.
In `@test/provers/optimism/ChildToParentProver.t.sol`:
- Line 8: The test imports the wrong chain-specific prover: change the import of
ChildToParentProver that currently points to provers/zksync/ to the
provers/optimism/ implementation so the test uses the optimism-specific contract
(update the import statement referencing ChildToParentProver to the optimism
path); this preserves the chain-specific pattern and avoids future divergence
issues.
---
Outside diff comments:
In `@test/provers/optimism/ChildToParentProver.t.sol`:
- Around line 13-17: The NatSpec header for the Optimism ChildToParentProver
test is stale: update the comment that currently says "reads L1 block hashes
from the L1Block predeploy on Optimism" to reflect the current implementation
(it now reads from the block hash buffer). Edit the top comment for the
ChildToParentProver test to say something like "This prover reads L1 block
hashes from the block hash buffer on Optimism" so the description matches the
actual code paths in the ChildToParentProver tests.
In `@test/VerifyBroadcastMessageBenchmark.t.sol`:
- Around line 400-404: Update the stale comment in the benchmark description in
VerifyBroadcastMessageBenchmark.t.sol so Hop 1 no longer says "OP-C2P gets
Ethereum block hash from L1Block predeploy" but instead references reading from
the block hash buffer (e.g., change the Hop 1 line to mention "reads Ethereum
block hash from block hash buffer"); adjust any related phrasing in the same
comment block (the route/Hop descriptions) to reflect "block hash buffer" rather
than "L1Block predeploy" so the documentation matches the current prover
behavior.
---
Nitpick comments:
In `@test/block-hash-pusher/optimism/OptimismBuffer.t.sol`:
- Around line 94-104: Rename the fuzz test function
test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher to
start with testFuzz_ (e.g.,
testFuzz_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher) so
it follows the file's fuzz naming convention, and replace the weak
vm.expectRevert() call with a targeted revert assertion using the OptimismBuffer
error selector
(expectRevert(abi.encodeWithSelector(OptimismBuffer.DomainMessageSenderMismatch.selector)))
before calling mockOpCrosschainDomainMessenger.relayMessage so the test asserts
the specific DomainMessageSenderMismatch error instead of any revert.
- Around line 22-23: The fuzz parameter firstBlockNumber is declared as uint8
which limits testing to 0..255; change the parameter in testFuzz_receiveHashes
from uint8 to uint256 so block-number-dependent logic in receiveHashes and
BaseBuffer can be exercised, and update the vm.assume to either only exclude
negatives (e.g., keep vm.assume(batchSize > 0 && batchSize <= 8191) and add
vm.assume(firstBlockNumber >= 0)) or constrain firstBlockNumber to a
representative range (e.g., <= 1e8) depending on whether you only need to
preserve the zero-check or want realistic block-number coverage.
- Line 42: Rename the misleading test function
test_receiveHashes_does_not_emit_event_when_no_hashes_written to reflect the
duplicate-push behavior being tested (e.g.,
test_receiveHashes_does_not_emit_event_on_duplicate_push); update the function
declaration and any references or docstrings/comments inside the
OptimismBuffer.t.sol test file so the name accurately describes that the test
replays an identical push (all blocks already stored) and asserts that
BlockHashesPushed is suppressed.
In `@test/provers/optimism/ChildToParentProver.t.sol`:
- Around line 42-83: The test currently pushes the block hash into the buffer
but never calls the buffer-backed target state commitment paths; after
buffer.receiveHashes(blockNumber, blockHashes) call into
childToParentProver.verifyTargetStateCommitment(...) and
childToParentProver.getTargetStateCommitment(...) (using the same blockHash and
the appropriate proof data from the JSON/rlpBlockHeader or the encoded input)
and add assertions that their returned commitment/verification result matches
the expected commitment computed from the test vectors; insert these calls just
before calling childToParentProver.verifyStorageSlot so the buffer path is
exercised (referencing buffer.receiveHashes,
childToParentProver.verifyTargetStateCommitment, and
childToParentProver.getTargetStateCommitment).
In `@test/Receiver.t.sol`:
- Around line 286-290: The IBuffer cast is redundantly assigned to a local
variable in multiple test functions (e.g., IBuffer buffer =
IBuffer(blockHashBuffer) before calling receiveHashes); either inline the cast
at the call site (IBuffer(blockHashBuffer).receiveHashes(...)) or hoist a single
IBuffer buffer = IBuffer(blockHashBuffer) declaration to the top of each test
function where blockHashBuffer is declared so the repeated pattern is removed;
update all five occurrences referencing IBuffer, blockHashBuffer, and
receiveHashes accordingly.
| import {IPusher} from "../interfaces/IPusher.sol"; | ||
| import {ICrossDomainMessenger} from "@eth-optimism/contracts/libraries/bridge/ICrossDomainMessenger.sol"; | ||
|
|
||
| /// @title OPtimismPusher |
There was a problem hiding this comment.
Typo in NatSpec title: "OPtimismPusher" → "OptimismPusher".
📝 Proposed fix
-/// `@title` OPtimismPusher
+/// `@title` OptimismPusher📝 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
| /// @title OPtimismPusher | |
| /// `@title` OptimismPusher |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/block-hash-pusher/optimism/OptimismPusher.sol` at line 9,
Update the NatSpec title comment from "OPtimismPusher" to the correct
"OptimismPusher" in the OptimismPusher contract header comment; locate the
top-of-file NatSpec/title line (for the OptimismPusher contract) and correct the
capitalization/typo so it matches the contract name OptimismPusher.
pepebndc
approved these changes
Feb 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/contracts/provers/optimism/ChildToParentProver.sol (1)
15-19: Avoid hardcoding the buffer mapping slot.
HardcodingblockHashMappingSlot = 1tightly couples this prover toBaseBuffer’s storage layout; any layout change will silently break proofs. Consider exposing the slot from the buffer contract (e.g., via an interface getter/constant) and referencing that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/optimism/ChildToParentProver.sol` around lines 15 - 19, The contract hardcodes blockHashMappingSlot = 1 which couples ChildToParentProver to BaseBuffer's storage layout; change this by removing the hardcoded constant and instead query the buffer contract for its slot at deployment or construction: add or use an interface (e.g., IBlockHashBuffer) with a public getter/constant like blockHashMappingSlot() and store the returned value in a non-constant state variable (replace the current blockHashMappingSlot constant), initialize it from blockHashBuffer in the constructor, and update any references to use the new state variable so the prover reads the actual buffer mapping slot at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/provers/optimism/ChildToParentProver.sol`:
- Around line 28-30: Add a guard in the constructor of ChildToParentProver to
ensure the _blockHashBuffer address is not zero: validate that _blockHashBuffer
!= address(0) and revert with a clear message (e.g., "blockHashBuffer: zero
address") before assigning blockHashBuffer = _blockHashBuffer; this prevents
runtime reverts when reading the buffer later from functions in
ChildToParentProver.
- Line 57: The require calls using custom errors (e.g.
require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()))
rely on the via-IR compiler pipeline; either enable --via-ir in the build config
or replace these requires with explicit checks and reverts to ensure
compatibility — for example, change the single-line require to: if
(targetStateCommitment == bytes32(0)) revert InvalidTargetStateCommitment(); and
do the same for the other similar require (the one around line 71) so both use
explicit if + revert with the custom error.
---
Nitpick comments:
In `@src/contracts/provers/optimism/ChildToParentProver.sol`:
- Around line 15-19: The contract hardcodes blockHashMappingSlot = 1 which
couples ChildToParentProver to BaseBuffer's storage layout; change this by
removing the hardcoded constant and instead query the buffer contract for its
slot at deployment or construction: add or use an interface (e.g.,
IBlockHashBuffer) with a public getter/constant like blockHashMappingSlot() and
store the returned value in a non-constant state variable (replace the current
blockHashMappingSlot constant), initialize it from blockHashBuffer in the
constructor, and update any references to use the new state variable so the
prover reads the actual buffer mapping slot at runtime.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapshots/verifyBroadcastMessage.jsonsrc/contracts/provers/optimism/ChildToParentProver.soltest/Receiver.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- snapshots/verifyBroadcastMessage.json
Comment on lines
+28
to
30
| constructor(address _blockHashBuffer, uint256 _homeChainId) { | ||
| blockHashBuffer = _blockHashBuffer; | ||
| homeChainId = _homeChainId; |
There was a problem hiding this comment.
Guard against a zero buffer address.
A zero blockHashBuffer will cause runtime reverts when reading the buffer, so it’s safer to validate in the constructor.
🔧 Proposed fix
+ error InvalidBlockHashBuffer();
+
constructor(address _blockHashBuffer, uint256 _homeChainId) {
+ if (_blockHashBuffer == address(0)) revert InvalidBlockHashBuffer();
blockHashBuffer = _blockHashBuffer;
homeChainId = _homeChainId;
}📝 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
| constructor(address _blockHashBuffer, uint256 _homeChainId) { | |
| blockHashBuffer = _blockHashBuffer; | |
| homeChainId = _homeChainId; | |
| error InvalidBlockHashBuffer(); | |
| constructor(address _blockHashBuffer, uint256 _homeChainId) { | |
| if (_blockHashBuffer == address(0)) revert InvalidBlockHashBuffer(); | |
| blockHashBuffer = _blockHashBuffer; | |
| homeChainId = _homeChainId; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/provers/optimism/ChildToParentProver.sol` around lines 28 - 30,
Add a guard in the constructor of ChildToParentProver to ensure the
_blockHashBuffer address is not zero: validate that _blockHashBuffer !=
address(0) and revert with a clear message (e.g., "blockHashBuffer: zero
address") before assigning blockHashBuffer = _blockHashBuffer; this prevents
runtime reverts when reading the buffer later from functions in
ChildToParentProver.
| homeBlockHash, rlpBlockHeader, blockHashBuffer, slot, accountProof, storageProof | ||
| ); | ||
|
|
||
| require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Solidity 0.8.30 custom errors require statement second argument
💡 Result:
In Solidity 0.8.30, require has a 3rd overload that accepts a custom error as the second argument:
error Unauthorized(address caller);
function f(address owner) external view {
require(msg.sender == owner, Unauthorized(msg.sender));
}The Solidity docs list the overload as require(bool, error). [1]
However, custom errors in require are only supported in the via-IR pipeline (introduced in Solidity 0.8.26). So if you compile without --via-ir / settings.viaIR: true, this may fail to compile; in that case use:
if (msg.sender != owner) revert Unauthorized(msg.sender);[2]
Sources:
[1] Solidity 0.8.30 docs – Expressions and Control Structures (require overloads) (docs.soliditylang.org)
[2] Solidity 0.8.26 release announcement – Custom errors support in require (via-IR only) (soliditylang.org)
Citations:
- 1: https://docs.soliditylang.org/en/v0.8.30/control-structures.html
- 2: https://soliditylang.org/blog/2024/05/21/solidity-0.8.26-release-announcement/?utm_source=openai
🏁 Script executed:
# Check the actual code at the specified lines
cat -n src/contracts/provers/optimism/ChildToParentProver.sol | sed -n '50,80p'Repository: openintentsframework/broadcaster
Length of output: 1867
🏁 Script executed:
# Check the Solidity compiler version specified in the contract
head -30 src/contracts/provers/optimism/ChildToParentProver.sol | grep -E "pragma solidity|import"Repository: openintentsframework/broadcaster
Length of output: 377
Clarify via-IR requirement for custom errors in require. Solidity 0.8.30 supports custom errors as the second argument to require(), but only when compiling with the via-IR pipeline. If this project doesn't use --via-ir, the code won't compile. Verify the build configuration or use explicit revert for guaranteed compatibility:
- require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment());
+ if (targetStateCommitment == bytes32(0)) revert InvalidTargetStateCommitment();Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/provers/optimism/ChildToParentProver.sol` at line 57, The
require calls using custom errors (e.g. require(targetStateCommitment !=
bytes32(0), InvalidTargetStateCommitment())) rely on the via-IR compiler
pipeline; either enable --via-ir in the build config or replace these requires
with explicit checks and reverts to ensure compatibility — for example, change
the single-line require to: if (targetStateCommitment == bytes32(0)) revert
InvalidTargetStateCommitment(); and do the same for the other similar require
(the one around line 71) so both use explicit if + revert with the custom error.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/contracts/provers/optimism/ChildToParentProver.sol (2)
28-31: Guard against a zero_blockHashBufferaddress.A zero address will cause runtime reverts when calling
IBuffer(blockHashBuffer).parentChainBlockHash(...)or when proof verification targets address(0). Adding a constructor check makes the failure mode explicit at deploy time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/optimism/ChildToParentProver.sol` around lines 28 - 31, The constructor currently accepts _blockHashBuffer without validation; add a guard in the constructor of ChildToParentProver to require(_blockHashBuffer != address(0), "blockHashBuffer zero") so blockHashBuffer is never set to address(0) (keep assigning blockHashBuffer = _blockHashBuffer and homeChainId = _homeChainId) to prevent runtime reverts when later calling IBuffer(blockHashBuffer).parentChainBlockHash(...) or verifying proofs against address(0).
56-56: Custom errors inrequireneed the via-IR pipeline.Already flagged —
require(condition, CustomError())syntax requires--via-ircompilation. Same applies to line 70.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/optimism/ChildToParentProver.sol` at line 56, The contract ChildToParentProver uses require(..., InvalidTargetStateCommitment()) which relies on the via-IR compilation pipeline; fix this by either (A) replacing those require(...) calls with explicit revert InvalidTargetStateCommitment() style revert statements (e.g., check targetStateCommitment and revert InvalidTargetStateCommitment() on failure) or (B) enable the --via-ir compiler flag in your build so require(condition, CustomError()) is supported; update both the require referencing InvalidTargetStateCommitment and the other require that uses a custom error in the same contract accordingly (refer to the ChildToParentProver contract and the InvalidTargetStateCommitment custom error symbol to locate the spots).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/provers/optimism/ChildToParentProver.sol`:
- Around line 48-50: The comment above the slot calculation incorrectly links to
OffchainLabs' block-hash-pusher; update the inline doc comment to reference this
repo's BaseBuffer implementation instead. Locate the comment that precedes
"uint256 slot =
uint256(SlotDerivation.deriveMapping(bytes32(BLOCK_HASH_MAPPING_SLOT),
targetBlockNumber));" and replace the external URL with the project's BaseBuffer
link (the openintentsframework/broadcaster BaseBuffer) so the reference is
consistent with SlotDerivation.deriveMapping, BLOCK_HASH_MAPPING_SLOT and
targetBlockNumber usage.
---
Duplicate comments:
In `@src/contracts/provers/optimism/ChildToParentProver.sol`:
- Around line 28-31: The constructor currently accepts _blockHashBuffer without
validation; add a guard in the constructor of ChildToParentProver to
require(_blockHashBuffer != address(0), "blockHashBuffer zero") so
blockHashBuffer is never set to address(0) (keep assigning blockHashBuffer =
_blockHashBuffer and homeChainId = _homeChainId) to prevent runtime reverts when
later calling IBuffer(blockHashBuffer).parentChainBlockHash(...) or verifying
proofs against address(0).
- Line 56: The contract ChildToParentProver uses require(...,
InvalidTargetStateCommitment()) which relies on the via-IR compilation pipeline;
fix this by either (A) replacing those require(...) calls with explicit revert
InvalidTargetStateCommitment() style revert statements (e.g., check
targetStateCommitment and revert InvalidTargetStateCommitment() on failure) or
(B) enable the --via-ir compiler flag in your build so require(condition,
CustomError()) is supported; update both the require referencing
InvalidTargetStateCommitment and the other require that uses a custom error in
the same contract accordingly (refer to the ChildToParentProver contract and the
InvalidTargetStateCommitment custom error symbol to locate the spots).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snapshots/verifyBroadcastMessage.jsonsrc/contracts/provers/optimism/ChildToParentProver.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- snapshots/verifyBroadcastMessage.json
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/contracts/provers/optimism/ChildToParentProver.sol (2)
28-30:⚠️ Potential issue | 🟡 MinorGuard against a zero
blockHashBufferaddress.
Fail fast in the constructor to avoid opaque runtime reverts later.🔧 Proposed fix
+ error InvalidBlockHashBuffer(); + constructor(address _blockHashBuffer, uint256 _homeChainId) { + if (_blockHashBuffer == address(0)) revert InvalidBlockHashBuffer(); blockHashBuffer = _blockHashBuffer; homeChainId = _homeChainId; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/optimism/ChildToParentProver.sol` around lines 28 - 30, The constructor in ChildToParentProver.sol should guard against a zero blockHashBuffer address to fail fast; update the constructor (the constructor that assigns blockHashBuffer and homeChainId) to require(_blockHashBuffer != address(0)) and revert with a clear message before assigning blockHashBuffer, so the contract cannot be deployed with a zero address and later trigger opaque runtime errors.
56-70: Confirm via-IR is enabled for custom-errorrequire, or switch to explicitrevert.
Without via-IR, Solidity 0.8.30 won’t acceptrequire(bool, error)and the build will fail.✅ Compatible alternative
- require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()); + if (targetStateCommitment == bytes32(0)) revert InvalidTargetStateCommitment(); ... - require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()); + if (targetStateCommitment == bytes32(0)) revert InvalidTargetStateCommitment();#!/bin/bash set -euo pipefail rg -n "viaIR|via-ir|via_ir" -g "foundry.toml" -g "hardhat.config.*" -g "*.json" rg -n "solc" -g "foundry.toml" -g "hardhat.config.*" -g "*.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/optimism/ChildToParentProver.sol` around lines 56 - 70, The custom-error form of require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()) is not compatible without via-IR; either enable via-IR in the compiler config or replace those require(...) calls with explicit checks and reverts: e.g., in the constructor and in getTargetStateCommitment(uint256 input) (look for require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()) and the other require/use in getTargetStateCommitment), change to if (targetStateCommitment == bytes32(0)) revert InvalidTargetStateCommitment(); so the code compiles on Solidity 0.8.30 without via-IR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/contracts/provers/optimism/ChildToParentProver.sol`:
- Around line 28-30: The constructor in ChildToParentProver.sol should guard
against a zero blockHashBuffer address to fail fast; update the constructor (the
constructor that assigns blockHashBuffer and homeChainId) to
require(_blockHashBuffer != address(0)) and revert with a clear message before
assigning blockHashBuffer, so the contract cannot be deployed with a zero
address and later trigger opaque runtime errors.
- Around line 56-70: The custom-error form of require(targetStateCommitment !=
bytes32(0), InvalidTargetStateCommitment()) is not compatible without via-IR;
either enable via-IR in the compiler config or replace those require(...) calls
with explicit checks and reverts: e.g., in the constructor and in
getTargetStateCommitment(uint256 input) (look for require(targetStateCommitment
!= bytes32(0), InvalidTargetStateCommitment()) and the other require/use in
getTargetStateCommitment), change to if (targetStateCommitment == bytes32(0))
revert InvalidTargetStateCommitment(); so the code compiles on Solidity 0.8.30
without via-IR.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/contracts/provers/linea/ChildToParentProver.solsrc/contracts/provers/optimism/ChildToParentProver.solsrc/contracts/provers/scroll/ChildToParentProver.solsrc/contracts/provers/zksync/ChildToParentProver.soltest/provers/optimism/ChildToParentProver.t.sol
✅ Files skipped from review due to trivial changes (2)
- src/contracts/provers/zksync/ChildToParentProver.sol
- src/contracts/provers/scroll/ChildToParentProver.sol
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
2 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.
This PR changes the Optimism
ChildToParentProverto use a pusher/buffer system instead of the native predeploy on OP chains.Summary by CodeRabbit
New Features
Tests
Chores