Update pushHashes function signature #60
Conversation
📝 WalkthroughWalkthroughThis PR refactors the block-hash-pusher architecture by making the buffer address a dynamic parameter in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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
🤖 Fix all issues with AI agents
In `@src/contracts/block-hash-pusher/linea/LineaBuffer.sol`:
- Around line 35-42: The constructor currently validates l2MessageService_ but
not pusher_, which allows a zero-address _pusher and breaks the contract; add a
check in the constructor to revert if pusher_ == address(0) (introduce a new
error like InvalidPusherAddress() if one doesn't exist) and ensure both
l2MessageService_ and pusher_ are validated before assigning to
_l2MessageService and _pusher in the constructor of LineaBuffer.sol.
In `@src/contracts/block-hash-pusher/scroll/ScrollBuffer.sol`:
- Around line 32-39: The constructor currently validates l2ScrollMessenger_ but
not pusher_, so a zero pusher address can be stored and later cause
receiveHashes to revert with InvalidPusherAddress(); add a zero-address check
for pusher_ in the ScrollBuffer constructor (similar to ZkSyncBuffer) and revert
InvalidPusherAddress() when pusher_ == address(0) before assigning _pusher to
ensure the contract cannot be deployed with an invalid pusher; update the
constructor logic around _pusher and _l2ScrollMessenger accordingly.
🧹 Nitpick comments (4)
src/contracts/block-hash-pusher/zksync/ZkSyncBuffer.sol (1)
43-48: Note: Redundant zero check (defensive coding).The zero check in
aliasedPusher()is redundant since the constructor already prevents a zeropusher_. This is harmless defensive coding, so no change needed unless you prefer to remove it for consistency with the constructor-guaranteed invariant.src/contracts/block-hash-pusher/zksync/ZkSyncPusher.sol (1)
43-45: Consider guarding against a zero zkSync Diamond address.
A zero address would make the mailbox call fail opaquely at runtime; a constructor guard makes misconfiguration explicit.♻️ Suggested guard
contract ZkSyncPusher is BlockHashArrayBuilder, IPusher { /// `@dev` The address of the ZkSync Diamond proxy contract on L1. address private immutable _zkSyncDiamond; /// `@notice` Thrown when the L2 transaction request fails. error FailedToPushHashes(); + /// `@notice` Thrown when the zkSync Diamond address is zero. + error InvalidZkSyncDiamond(address diamond); @@ - constructor(address zkSyncDiamond_) { - _zkSyncDiamond = zkSyncDiamond_; - } + constructor(address zkSyncDiamond_) { + if (zkSyncDiamond_ == address(0)) { + revert InvalidZkSyncDiamond(zkSyncDiamond_); + } + _zkSyncDiamond = zkSyncDiamond_; + }test/block-hash-pusher/zksync/ZkSyncPusher.t.sol (1)
64-99: Add a regression test forInvalidBuffer.
Now thatbufferis user-supplied, a simple zero-address revert check would lock in the new guard.🧪 Suggested test
@@ function testFuzz_pushHashes_invalidBatchSize(uint16 batchSize) public { vm.assume(batchSize == 0 || batchSize > 8191); vm.roll(uint32(batchSize) + 1); // uint32 to avoid overflow @@ vm.expectRevert(abi.encodeWithSelector(IPusher.InvalidBatch.selector, block.number - batchSize, batchSize)); zkSyncPusher.pushHashes(buffer, block.number - batchSize, batchSize, l2TransactionData); } + + function test_pushHashes_reverts_if_buffer_zero() public { + vm.roll(2); + ZkSyncPusher zkSyncPusher = new ZkSyncPusher(mockZkSyncMailbox); + + bytes memory l2TransactionData = abi.encode( + ZkSyncPusher.ZkSyncL2Transaction({ + l2GasLimit: 1000000, + l2GasPerPubdataByteLimit: REQUIRED_L2_GAS_PRICE_PER_PUBDATA, + refundRecipient: address(0) + }) + ); + + vm.expectRevert(abi.encodeWithSelector(IPusher.InvalidBuffer.selector, address(0))); + zkSyncPusher.pushHashes(address(0), block.number - 1, 1, l2TransactionData); + }test/block-hash-pusher/linea/LineaBuffer.t.sol (1)
6-6: Remove unusedOwnableimport.The
Ownableimport is no longer used in this test file since owner-related tests were removed as part of the refactor.Suggested fix
import {Test} from "forge-std/Test.sol"; import {LineaBuffer} from "../../../src/contracts/block-hash-pusher/linea/LineaBuffer.sol"; -import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {IBuffer} from "../../../src/contracts/block-hash-pusher/interfaces/IBuffer.sol"; import {MockLineaMessageService} from "../mocks/MockLineaMessageService.sol"; import {IMessageService} from "@linea-contracts/messaging/interfaces/IMessageService.sol";
| constructor(address l2MessageService_, address pusher_) { | ||
| _l2MessageService = l2MessageService_; | ||
| _pusher = pusher_; | ||
|
|
||
| if (l2MessageService_ == address(0)) { | ||
| revert InvalidL2MessageServiceAddress(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add constructor validation for pusher_ parameter.
Same issue as in ScrollBuffer: the constructor validates l2MessageService_ but not pusher_. A zero-address pusher creates a permanently broken contract.
Proposed fix
constructor(address l2MessageService_, address pusher_) {
_l2MessageService = l2MessageService_;
_pusher = pusher_;
if (l2MessageService_ == address(0)) {
revert InvalidL2MessageServiceAddress();
}
+ if (pusher_ == address(0)) {
+ revert InvalidPusherAddress();
+ }
}🤖 Prompt for AI Agents
In `@src/contracts/block-hash-pusher/linea/LineaBuffer.sol` around lines 35 - 42,
The constructor currently validates l2MessageService_ but not pusher_, which
allows a zero-address _pusher and breaks the contract; add a check in the
constructor to revert if pusher_ == address(0) (introduce a new error like
InvalidPusherAddress() if one doesn't exist) and ensure both l2MessageService_
and pusher_ are validated before assigning to _l2MessageService and _pusher in
the constructor of LineaBuffer.sol.
| constructor(address l2ScrollMessenger_, address pusher_) { | ||
| _l2ScrollMessenger = l2ScrollMessenger_; | ||
| _pusher = pusher_; | ||
|
|
||
| if (l2ScrollMessenger_ == address(0)) { | ||
| revert InvalidL2ScrollMessengerAddress(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add constructor validation for pusher_ parameter.
The constructor validates l2ScrollMessenger_ but not pusher_. If a zero address is passed for pusher_, the contract deploys successfully but receiveHashes will always revert with InvalidPusherAddress(). This is inconsistent with ZkSyncBuffer, which validates pusher_ in its constructor.
Proposed fix
constructor(address l2ScrollMessenger_, address pusher_) {
_l2ScrollMessenger = l2ScrollMessenger_;
_pusher = pusher_;
if (l2ScrollMessenger_ == address(0)) {
revert InvalidL2ScrollMessengerAddress();
}
+ if (pusher_ == address(0)) {
+ revert InvalidPusherAddress();
+ }
}🤖 Prompt for AI Agents
In `@src/contracts/block-hash-pusher/scroll/ScrollBuffer.sol` around lines 32 -
39, The constructor currently validates l2ScrollMessenger_ but not pusher_, so a
zero pusher address can be stored and later cause receiveHashes to revert with
InvalidPusherAddress(); add a zero-address check for pusher_ in the ScrollBuffer
constructor (similar to ZkSyncBuffer) and revert InvalidPusherAddress() when
pusher_ == address(0) before assigning _pusher to ensure the contract cannot be
deployed with an invalid pusher; update the constructor logic around _pusher and
_l2ScrollMessenger accordingly.
frangio
left a comment
frangio
left a comment
There was a problem hiding this comment.
Can Buffer state variables be made immutable?
This PR changes the pushHashes function signatures on
IPusherin order to receive the buffer as a parameter. This change allows for removal of theOwnabledependency on the Buffers.Summary by CodeRabbit
Documentation
Refactor
pushHashes()to require explicit buffer address as first parameter across all pusher implementations.bufferAddress()getter function across pusher contracts.✏️ Tip: You can customize this high-level summary in your review settings.