Conversation
anylots
requested review from
tomatoishealthy
and removed request for
a team
Contributor
📝 WalkthroughWalkthroughA new public view function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/contracts/l1/rollup/Rollup.sol (1)
671-679: Useexternalfor this view helper (slightly cheaper).
proveCommittedBatchStateisn’t called internally, soexternalsaves a tiny bit of gas and clarifies API intent.♻️ Proposed tweak
- function proveCommittedBatchState(bytes calldata _batchHeader, bytes calldata _batchProof) public view { + function proveCommittedBatchState(bytes calldata _batchHeader, bytes calldata _batchProof) external view {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/l1/rollup/Rollup.sol` around lines 671 - 679, The function proveCommittedBatchState is declared public view but is never called internally; change its visibility to external view to save gas and clarify the API. Update the function signature for proveCommittedBatchState to use external instead of public and confirm there are no internal callers (search for proveCommittedBatchState references) before committing; no other logic changes are needed—leave the internal helpers (_loadBatchHeader, BatchHeaderCodecV0.getBatchIndex, committedBatches check, and _verifyProof) untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/contracts/l1/rollup/Rollup.sol`:
- Around line 671-679: The function proveCommittedBatchState is declared public
view but is never called internally; change its visibility to external view to
save gas and clarify the API. Update the function signature for
proveCommittedBatchState to use external instead of public and confirm there are
no internal callers (search for proveCommittedBatchState references) before
committing; no other logic changes are needed—leave the internal helpers
(_loadBatchHeader, BatchHeaderCodecV0.getBatchIndex, committedBatches check, and
_verifyProof) untouched.
Kukoomomo
approved these changes
Feb 24, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/contracts/test/Rollup.t.sol (2)
628-630: The hardcoded 640-byte proof is never validated — simplify to a lightweight placeholder.
_mockVerifierCall()(line 593) installs a persistenthevm.mockCallonverifyAggregateProof. That mock remains active for the entire test, so the verifier call insideproveCommittedBatchStateat line 630 is intercepted and the proof bytes are never inspected. The large literal gives a misleading impression that a real ZK proof is being exercised. Every other test in this contract useshex"deadbeef"for this reason.♻️ Simplify proof literal
- bytes - memory proof = hex"ffea2d2e...00000000"; - rollup.proveCommittedBatchState(batchHeader1, proof); + rollup.proveCommittedBatchState(batchHeader1, hex"deadbeef");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/test/Rollup.t.sol` around lines 628 - 630, The test uses a large hardcoded proof bytes literal passed to rollup.proveCommittedBatchState while _mockVerifierCall installs a persistent hevm.mockCall that intercepts verifyAggregateProof, so the proof is never validated; replace the long 640-byte hex with a lightweight placeholder (e.g. hex"deadbeef") where the proof variable is defined and passed to proveCommittedBatchState, keeping the call to _mockVerifierCall, verifyAggregateProof, and proveCommittedBatchState unchanged so the test remains clear and concise.
591-631: Add negative test cases forproveCommittedBatchState.
test_proveCommittedBatchStateonly exercises the happy path. The new function contains several validation checks (committed hash existence, header consistency, verifier call) that are not covered by any failure-path test. Consider adding cases such as:
- Calling with a batch header whose hash is not in
committedBatches(should revert with a committed-hash mismatch error).- Calling after the batch has been finalized and its committed state pruned (if applicable per the contract's cleanup logic).
- Passing a header whose derived index/hash does not match the stored committed hash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/test/Rollup.t.sol` around lines 591 - 631, Add failing-path tests for proveCommittedBatchState: create tests that call rollup.proveCommittedBatchState with (1) a batch header whose hash is not in committedBatches and assert the call reverts with the committed-hash mismatch error, (2) a header whose derived index/hash does not match the stored committed hash (use a mismatched prevStateRoot/postStateRoot or a non-matching batchHeader produced by _createMatchingBatchHeader) and assert revert, and (3) a scenario where the batch was finalized/pruned (finalizeBatch then attempt proveCommittedBatchState) and assert the appropriate revert; reuse setup helpers like _mockVerifierCall and _mockMessageQueueStalled and reference functions proveCommittedBatchState, test_proveCommittedBatchState, committedBatches, batchHeader1 to locate where to add these new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/contracts/test/Rollup.t.sol`:
- Around line 628-630: The test uses a large hardcoded proof bytes literal
passed to rollup.proveCommittedBatchState while _mockVerifierCall installs a
persistent hevm.mockCall that intercepts verifyAggregateProof, so the proof is
never validated; replace the long 640-byte hex with a lightweight placeholder
(e.g. hex"deadbeef") where the proof variable is defined and passed to
proveCommittedBatchState, keeping the call to _mockVerifierCall,
verifyAggregateProof, and proveCommittedBatchState unchanged so the test remains
clear and concise.
- Around line 591-631: Add failing-path tests for proveCommittedBatchState:
create tests that call rollup.proveCommittedBatchState with (1) a batch header
whose hash is not in committedBatches and assert the call reverts with the
committed-hash mismatch error, (2) a header whose derived index/hash does not
match the stored committed hash (use a mismatched prevStateRoot/postStateRoot or
a non-matching batchHeader produced by _createMatchingBatchHeader) and assert
revert, and (3) a scenario where the batch was finalized/pruned (finalizeBatch
then attempt proveCommittedBatchState) and assert the appropriate revert; reuse
setup helpers like _mockVerifierCall and _mockMessageQueueStalled and reference
functions proveCommittedBatchState, test_proveCommittedBatchState,
committedBatches, batchHeader1 to locate where to add these new tests.
chengwenxi
approved these changes
Feb 25, 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.
proveBatchStatetoRollup.sol.Summary by CodeRabbit
New Features
Tests