Conversation
📝 WalkthroughWalkthroughAdds Optimism-specific block hash synchronization contracts: OptimismBuffer (L2 receiver) and OptimismPusher (L1 sender) with cross-domain messenger integration, plus mock and test infrastructure for validating cross-chain messaging behavior. Changes
Sequence DiagramsequenceDiagram
participant L1Pusher as L1: OptimismPusher
participant L1Messenger as L1: CrossDomainMessenger
participant L2Messenger as L2: CrossDomainMessenger
participant L2Buffer as L2: OptimismBuffer
L1Pusher->>L1Pusher: buildBlockHashArray()
L1Pusher->>L1Pusher: encodeReceiveHashesCall()
L1Pusher->>L1Messenger: sendMessage(L2Buffer, calldata, gasLimit)
Note over L1Messenger: Stores message for relaying
L1Messenger->>L2Messenger: relayMessage(L2Buffer, L1Pusher, calldata)
L2Messenger->>L2Messenger: setTransientSender(L1Pusher)
L2Messenger->>L2Buffer: receiveHashes(firstBlockNumber, hashes)
L2Buffer->>L2Buffer: validateSender == L2Messenger
L2Buffer->>L2Buffer: validateXDomainSender == pusher
L2Buffer->>L2Buffer: _receiveHashes() - persist hashes
L2Buffer->>L2Buffer: emit BlockHashesPushed
L2Messenger->>L2Messenger: clearTransientSender()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (5)
src/contracts/block-hash-pusher/optimism/OptimismBuffer.sol (1)
42-49:receiveHashesaccess-control pattern is correct.Checking
msg.sender == l2CrossDomainMessengerCachedbefore callingxDomainMessageSender()is the standard Optimism two-step guard—the first prevents anyone from spoofing the cross-domain sender, and the second ensures the relayed message actually originated from_pusher.One nit: Line 43 routes through the public
l2CrossDomainMessenger()getter to buildl2CrossDomainMessengerCached. Using the immutable_l2CrossDomainMessengerdirectly is cheaper and avoids the extra dispatch.♻️ Proposed refactor
-ICrossDomainMessenger l2CrossDomainMessengerCached = ICrossDomainMessenger(l2CrossDomainMessenger()); +ICrossDomainMessenger l2CrossDomainMessengerCached = ICrossDomainMessenger(_l2CrossDomainMessenger);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/block-hash-pusher/optimism/OptimismBuffer.sol` around lines 42 - 49, The receiveHashes function currently calls the public getter l2CrossDomainMessenger() to obtain the messenger, adding an unnecessary external call; replace that with the immutable storage value _l2CrossDomainMessenger directly (use ICrossDomainMessenger(_l2CrossDomainMessenger)) when constructing l2CrossDomainMessengerCached in receiveHashes to save gas and avoid extra dispatch while retaining the same access-control checks (msg.sender comparison and xDomainMessageSender() verification).test/block-hash-pusher/optimism/OptimismBuffer.t.sol (2)
22-23:uint8 firstBlockNumberseverely limits fuzz coverage to 0–255.Real-world block numbers are in the billions. If
BaseBuffer._receiveHashescontains any range-dependent validation (e.g., overflow guards, mapping key constraints), this narrow range will never surface them. Consider a wider type with a non-zero lower bound:♻️ Proposed widened range
-function testFuzz_receiveHashes(uint16 batchSize, uint8 firstBlockNumber) public { - vm.assume(batchSize > 0 && batchSize <= 8191); +function testFuzz_receiveHashes(uint16 batchSize, uint256 firstBlockNumber) public { + vm.assume(batchSize > 0 && batchSize <= 8191); + vm.assume(firstBlockNumber <= type(uint256).max - batchSize); // prevent overflow in _receiveHashes🤖 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 too narrow (uint8) and should be widened to cover realistic block ranges; change the testFuzz_receiveHashes signature to use a larger integer type (e.g., uint32 or uint64) and add a vm.assume to constrain it to a non-zero lower bound and a reasonable upper bound (for example vm.assume(firstBlockNumber > 0 && firstBlockNumber <= SOME_MAX)) so BaseBuffer._receiveHashes range-dependent behavior is exercised; update any uses of firstBlockNumber in testFuzz_receiveHashes accordingly.
94-104: Fuzz test missingtestFuzz_prefix;vm.expectRevert()is overly broad.The function accepts an
address notPusherparameter, making Foundry treat it as a fuzz test, but the name starts withtest_rather than the conventionaltestFuzz_. Additionally,vm.expectRevert()with no arguments catches any revert—at minimum assert on the"Message sending failed"string (the revert the mock produces) to guard against accidentally passing due to an unrelated revert.♻️ Proposed fixes
-/// forge-config: default.allow_internal_expect_revert = true -function test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher(address notPusher) public { +/// forge-config: default.allow_internal_expect_revert = true +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("Message sending failed"); 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 test function to start with the fuzz prefix (e.g., change test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher to testFuzz_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher) so Foundry treats it as a fuzz test, and tighten the revert assertion by replacing the broad vm.expectRevert() with an assertion that matches the mock revert payload (e.g., vm.expectRevert(bytes("Message sending failed"))) before calling mockOpCrosschainDomainMessenger.relayMessage on the OptimismBuffer instance to ensure the failure is the expected "Message sending failed" from the mock.src/contracts/block-hash-pusher/optimism/OptimismPusher.sol (1)
42-47: Decodel2TransactionDatabefore_buildBlockHashArrayto fail fast.Line 45 decodes
l2TransactionDataafter the expensive_buildBlockHashArraycall (which reads multiple block hashes via theBLOCKHASHopcode). Ifl2TransactionDatais malformed or empty, the call reverts after all that work. Swapping the order makes an invalid input revert cheaply upfront.♻️ Proposed refactor
+ OptimismL2Transaction memory l2Transaction = abi.decode(l2TransactionData, (OptimismL2Transaction)); + bytes32[] memory blockHashes = _buildBlockHashArray(firstBlockNumber, batchSize); bytes memory l2Calldata = abi.encodeCall(IBuffer.receiveHashes, (firstBlockNumber, blockHashes)); - OptimismL2Transaction memory l2Transaction = abi.decode(l2TransactionData, (OptimismL2Transaction)); - ICrossDomainMessenger(l1CrossDomainMessengerProxy()).sendMessage(buffer, l2Calldata, l2Transaction.gasLimit);🤖 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` around lines 42 - 47, Decode l2TransactionData into OptimismL2Transaction before calling _buildBlockHashArray so malformed or empty input fails fast; specifically move the abi.decode(l2TransactionData, (OptimismL2Transaction)) call ahead of the call to _buildBlockHashArray(firstBlockNumber, batchSize), then construct blockHashes, l2Calldata and proceed to ICrossDomainMessenger(l1CrossDomainMessengerProxy()).sendMessage(buffer, l2Calldata, l2Transaction.gasLimit) as before.test/block-hash-pusher/mocks/MockOpCrosschainDomainMessenger.sol (1)
26-29: Consider bubbling up the original revert data for better test diagnostics.When the inner
_target.callfails,returnDatais captured but immediately discarded in favor of a hardcoded string revert. This masks the actual revert reason (e.g.,DomainMessageSenderMismatch,InvalidSender), making failed test assertions harder to diagnose.♻️ Proposed refactor to propagate the original revert
(bool callSuccess, bytes memory returnData) = _target.call(_message); if (!callSuccess) { - revert("Message sending failed"); + assembly { + revert(add(returnData, 32), mload(returnData)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/block-hash-pusher/mocks/MockOpCrosschainDomainMessenger.sol` around lines 26 - 29, The current mock swallows the original revert reason after calling _target.call by throwing a fixed string; update the failure branch in MockOpCrosschainDomainMessenger so that when callSuccess is false it reverts with the original returnData (e.g., using assembly revert(add(returnData, 32), mload(returnData))) to propagate the exact revert payload from _target.call, and optionally fall back to a generic message only if returnData is empty.
🤖 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: Fix the NatSpec typo in the contract title: update the `@title` comment
from "OPtimismPusher" to "OptimismPusher" in the OptimismPusher contract header
(the top-of-file NatSpec for the contract named OptimismPusher) so the
documented title matches the contract name; simply edit the `@title` line to the
correct capitalization.
In `@test/block-hash-pusher/optimism/OptimismPusher.t.sol`:
- Around line 33-35: The test calls optimismPusher.pushHashes with offsets
block.number - 1000 and block.number - 8000 which under a fork will pass
bytes32(0) for each hash because BLOCKHASH only returns non-zero for the last
256 blocks; update the test to either (A) use offsets within 256 blocks of the
fork tip (e.g., replace -1000 and -8000 with values <=256) when you need real
block hashes, or (B) if you only want to validate messaging behavior, add a
clear comment above the optimismPusher.pushHashes calls stating that older
hashes will be zero on a fork and the test intentionally validates messaging
only; locate and edit the optimismPusher.pushHashes calls and the containing
test to apply one of these fixes.
---
Nitpick comments:
In `@src/contracts/block-hash-pusher/optimism/OptimismBuffer.sol`:
- Around line 42-49: The receiveHashes function currently calls the public
getter l2CrossDomainMessenger() to obtain the messenger, adding an unnecessary
external call; replace that with the immutable storage value
_l2CrossDomainMessenger directly (use
ICrossDomainMessenger(_l2CrossDomainMessenger)) when constructing
l2CrossDomainMessengerCached in receiveHashes to save gas and avoid extra
dispatch while retaining the same access-control checks (msg.sender comparison
and xDomainMessageSender() verification).
In `@src/contracts/block-hash-pusher/optimism/OptimismPusher.sol`:
- Around line 42-47: Decode l2TransactionData into OptimismL2Transaction before
calling _buildBlockHashArray so malformed or empty input fails fast;
specifically move the abi.decode(l2TransactionData, (OptimismL2Transaction))
call ahead of the call to _buildBlockHashArray(firstBlockNumber, batchSize),
then construct blockHashes, l2Calldata and proceed to
ICrossDomainMessenger(l1CrossDomainMessengerProxy()).sendMessage(buffer,
l2Calldata, l2Transaction.gasLimit) as before.
In `@test/block-hash-pusher/mocks/MockOpCrosschainDomainMessenger.sol`:
- Around line 26-29: The current mock swallows the original revert reason after
calling _target.call by throwing a fixed string; update the failure branch in
MockOpCrosschainDomainMessenger so that when callSuccess is false it reverts
with the original returnData (e.g., using assembly revert(add(returnData, 32),
mload(returnData))) to propagate the exact revert payload from _target.call, and
optionally fall back to a generic message only if returnData is empty.
In `@test/block-hash-pusher/optimism/OptimismBuffer.t.sol`:
- Around line 22-23: The fuzz parameter firstBlockNumber is too narrow (uint8)
and should be widened to cover realistic block ranges; change the
testFuzz_receiveHashes signature to use a larger integer type (e.g., uint32 or
uint64) and add a vm.assume to constrain it to a non-zero lower bound and a
reasonable upper bound (for example vm.assume(firstBlockNumber > 0 &&
firstBlockNumber <= SOME_MAX)) so BaseBuffer._receiveHashes range-dependent
behavior is exercised; update any uses of firstBlockNumber in
testFuzz_receiveHashes accordingly.
- Around line 94-104: Rename the test function to start with the fuzz prefix
(e.g., change
test_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher to
testFuzz_receiveHashes_reverts_if_xDomainMessageSender_does_not_match_pusher) so
Foundry treats it as a fuzz test, and tighten the revert assertion by replacing
the broad vm.expectRevert() with an assertion that matches the mock revert
payload (e.g., vm.expectRevert(bytes("Message sending failed"))) before calling
mockOpCrosschainDomainMessenger.relayMessage on the OptimismBuffer instance to
ensure the failure is the expected "Message sending failed" from the mock.
| 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 @title NatSpec: 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, Fix
the NatSpec typo in the contract title: update the `@title` comment from
"OPtimismPusher" to "OptimismPusher" in the OptimismPusher contract header (the
top-of-file NatSpec for the contract named OptimismPusher) so the documented
title matches the contract name; simply edit the `@title` line to the correct
capitalization.
Comment on lines
+33
to
+35
| optimismPusher.pushHashes(buffer, block.number - 1000, 10, l2TransactionData); | ||
|
|
||
| optimismPusher.pushHashes(buffer, block.number - 8000, 20, l2TransactionData); |
There was a problem hiding this comment.
block.number - 1000 and block.number - 8000 will always push zero hashes on a fork.
The EVM BLOCKHASH opcode only returns a non-zero value for the 256 most recent blocks. Any block older than that yields bytes32(0). The fork test therefore pushes arrays of zero hashes for those two calls, which doesn't test real block hash integrity. If the intent is to validate the messaging flow only, this is fine — but a comment would clarify the limitation. If real hashes are needed, stay within 256 blocks of the fork tip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/block-hash-pusher/optimism/OptimismPusher.t.sol` around lines 33 - 35,
The test calls optimismPusher.pushHashes with offsets block.number - 1000 and
block.number - 8000 which under a fork will pass bytes32(0) for each hash
because BLOCKHASH only returns non-zero for the last 256 blocks; update the test
to either (A) use offsets within 256 blocks of the fork tip (e.g., replace -1000
and -8000 with values <=256) when you need real block hashes, or (B) if you only
want to validate messaging behavior, add a clear comment above the
optimismPusher.pushHashes calls stating that older hashes will be zero on a fork
and the test intentionally validates messaging only; locate and edit the
optimismPusher.pushHashes calls and the containing test to apply one of these
fixes.
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
1 participant
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
Tests