Raunak/sig verifier test #2
Conversation
|
Warning Rate limit exceeded@RnkSngh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis pull request introduces several new components to the repository, focusing on testing and proof verification for Layer 2 blockchain operations. The changes include adding a new Forge standard library submodule, creating new test contracts for receipt and sequencer proof verification, and introducing payload and proof files for Optimism and Arbitrum. The modifications aim to enhance the project's testing infrastructure and support for cross-chain proof validation. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Contracts
participant Verifier as Signature Verifier
participant Proof as Proof Loader
participant File as Payload Files
Test->>File: Load proof/receipt data
File-->>Test: Return hex/JSON data
Test->>Proof: Parse and prepare proofs
Test->>Verifier: Verify signatures/proofs
Verifier-->>Test: Return verification result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
test/ReceiptMPT.t.sol (1)
14-14: Document test data filesAdd comments explaining the source and format of the test data files (
op-proof.hex,op-receipt.hex, etc.) to improve maintainability.Also applies to: 21-21
test/utils/Base.t.sol (2)
14-14: Consider making PEPTIDE_CHAIN_ID configurableThe hardcoded chain ID might not be suitable for different testing environments. Consider making it configurable through constructor parameters or environment variables.
21-21: Improve file path handlingUsing string concatenation for file paths could be fragile. Consider creating a dedicated function for path resolution that handles path separators correctly across different operating systems.
Also applies to: 24-24
test/SequencerVerifier.t.sol (3)
11-13: Improve comment clarity for signature encodingThe current comment about signature order is unclear. Consider documenting the expected format:
// Signature format: |r (32 bytes)|s (32 bytes)|v (1 byte)| // vm.sign returns (v, r, s) but we need (r, s, v) format
19-30: Add test cases for edge casesConsider adding test cases for:
- Zero values in r, s components
- Maximum allowed values for v
- Malformed signatures with valid lengths but invalid content
50-50: Extract magic number to a constantThe
+ 1increment should be extracted to a named constant for clarity:- sigVerifier.verifyStateUpdate(peptideBlockNumber + 1, peptideAppHash, l1BlockHash, signature); + uint256 constant BLOCK_INCREMENT = 1; + sigVerifier.verifyStateUpdate(peptideBlockNumber + BLOCK_INCREMENT, peptideAppHash, l1BlockHash, signature);test/utils/Signing.base.t.sol (1)
21-21: Document domain usageThe comment about the domain being empty needs more context. Explain why it's acceptable to leave it as zero bytes and any implications for production use.
contracts/libs/ReceiptParser.sol (1)
44-51: Enhance documentation for OpL2StateProof struct.While the struct is well-structured, consider adding detailed documentation for each field to improve maintainability:
- Purpose of each proof field
- Expected format/structure of the proofs
- Relationship between these proofs in the L2 state verification process
Example documentation:
// This is the proof we use to verify the apphash (state) updates. struct OpL2StateProof { + /// @notice Merkle proof of the account state + /// @dev Array of RLP-encoded nodes forming the Merkle proof bytes[] accountProof; + /// @notice Merkle proof of the L2 output root + /// @dev Array of RLP-encoded nodes proving the output root bytes[] outputRootProof; + /// @notice Key used to identify the L2 output proposal bytes32 l2OutputProposalKey; + /// @notice Hash of the L2 block being proven bytes32 l2BlockHash; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitmodules(1 hunks)contracts/libs/ReceiptParser.sol(1 hunks)lib/forge-std(1 hunks)test/PolymerProofLib.t.sol(1 hunks)test/ReceiptMPT.t.sol(1 hunks)test/SequencerVerifier.t.sol(1 hunks)test/payload/arb-proof.hex(1 hunks)test/payload/arb-receipt-proof.hex(1 hunks)test/payload/arb-receipt.hex(1 hunks)test/payload/l1_block_0x4df537.hex(1 hunks)test/payload/l2_block_0x4b0.json(1 hunks)test/payload/op-proof.hex(1 hunks)test/payload/op-receipt-proof.hex(1 hunks)test/payload/op-receipt.hex(1 hunks)test/utils/Base.t.sol(1 hunks)test/utils/Signing.base.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- lib/forge-std
- test/PolymerProofLib.t.sol
- test/payload/arb-proof.hex
- test/payload/arb-receipt.hex
- test/payload/op-receipt.hex
- test/payload/op-proof.hex
- test/payload/l1_block_0x4df537.hex
- test/payload/arb-receipt-proof.hex
- test/payload/op-receipt-proof.hex
- test/payload/l2_block_0x4b0.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: verify-bindings
- GitHub Check: lint
🔇 Additional comments (2)
test/utils/Base.t.sol (1)
36-38: Add validation for storage manipulationThe storage write operation lacks validation. Consider adding checks:
- Verify lightClient address is not zero
- Validate proof height is within reasonable bounds
- Add error handling for storage write failures
.gitmodules (1)
10-12: Verify forge-std submodule version.The addition of forge-std is appropriate for testing. However, let's verify we're using a recent stable version.
✅ Verification successful
forge-std version is appropriate
The submodule is using a very recent commit from January 2025, which is newer than the latest release (v1.9.5). This is acceptable for a testing library, as recent versions often include important bug fixes and improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the forge-std submodule version and latest release # Get current commit hash cd lib/forge-std && git rev-parse HEAD # Get latest release tag and date git ls-remote --tags https://github.com/foundry-rs/forge-std | sort -t '/' -k 3 -V | tail -n1Length of output: 232
Script:
#!/bin/bash # Get commit details and its relationship to latest release cd lib/forge-std # Get commit date and branch git show -s --format=%ci 24a3d44659b267336c15a397f65506491d8fdf8c git branch -r --contains 24a3d44659b267336c15a397f65506491d8fdf8c # Check if commit is ahead of latest release git merge-base --is-ancestor b93cf4bc34ff214c099dc970b153f85ade8c9f66 24a3d44659b267336c15a397f65506491d8fdf8c echo $?Length of output: 480
test/ReceiptMPT.t.sol
Outdated
|
|
||
| // Testing receipt proofs are as the relayer encodes them. | ||
| contract ReceiptMPT is Base { | ||
| function setUp() public {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement setUp() function to initialize test state
The empty setUp() function should be implemented to properly initialize the test state, following Forge testing best practices.
test/ReceiptMPT.t.sol
Outdated
| function test_optimism_receipt() public { | ||
| (bytes memory receiptIdx, bytes[] memory receiptProof, bytes32 receiptRoot, bytes memory receiptRLP) = | ||
| load_receipt("/test/payload/op-proof.hex", "/test/payload/op-receipt.hex"); | ||
| assertEq(receiptRLP, MerkleTrie.get(receiptIdx, receiptProof, receiptRoot)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with negative test cases
Both test functions only verify the happy path. Consider adding test cases for:
- Invalid/malformed proofs
- Incorrect receipt indices
- Invalid root hashes
- Empty or malformed RLP data
Also applies to: 19-23
| constructor() { | ||
| (sequencer, sequencerPkey) = makeAddrAndKey("alice"); | ||
| (, notSequencerPkey) = makeAddrAndKey(unicode"bob😈"); | ||
| sigVerifier = new SequencerSignatureVerifier(sequencer, PEPTIDE_CHAIN_ID); | ||
|
|
||
| // generate the channel_proof.hex file with the following command: | ||
| // cd test-data-generator && go run ./cmd/ --type l1 > ../test/payload/l1_block_0x4df537.hex | ||
| // this is the "rlp" half-encoded header that would be sent by the relayer. this was produced | ||
| // by the test-data-generator tool. | ||
| L1Header memory l1header = abi.decode( | ||
| vm.parseBytes(vm.readFile(string.concat(rootDir, "/test/payload/l1_block_0x4df537.hex"))), (L1Header) | ||
| ); | ||
|
|
||
| l1BlockHash = keccak256(RLPWriter.writeList(l1header.header)); // Blockhash that will be signed by sequencer | ||
|
|
||
| peptideBlockNumber = 101; | ||
|
|
||
| // this happens to be the polymer height when the L2OO was updated with the output proposal | ||
| // we are using in the test | ||
| string memory l2BlockJson = vm.readFile(string.concat(rootDir, "/test/payload/l2_block_0x4b0.json")); | ||
| peptideAppHash = abi.decode(l2BlockJson.parseRaw(".result.stateRoot"), (bytes32)); | ||
|
|
||
| bytes32 payloadHash = keccak256(abi.encodePacked(peptideBlockNumber, peptideAppHash, l1BlockHash)); | ||
| hashToSign = keccak256(bytes.concat(domain, PEPTIDE_CHAIN_ID, payloadHash)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor constructor to reduce complexity
The constructor has too many responsibilities. Consider breaking it down into smaller, focused functions:
- initializeAddresses()
- loadL1Header()
- loadPeptideState()
- computeHashes()
| function readBytes32FromJson(string memory fileName, string memory property) public view returns (bytes32) { | ||
| string memory blockJson = vm.readFile(string.concat(rootDir, fileName)); | ||
| return abi.decode(blockJson.parseRaw(property), (bytes32)); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing
The JSON parsing functions should include error handling for:
- Missing files
- Invalid JSON format
- Missing properties
- Invalid property types
Example implementation:
function readBytes32FromJson(string memory fileName, string memory property) public view returns (bytes32) {
+ if (bytes(fileName).length == 0 || bytes(property).length == 0) {
+ revert("Invalid file name or property");
+ }
string memory blockJson = vm.readFile(string.concat(rootDir, fileName));
+ if (bytes(blockJson).length == 0) {
+ revert("File not found or empty");
+ }
return abi.decode(blockJson.parseRaw(property), (bytes32));
}Also applies to: 65-68
RnkSngh
force-pushed
the
raunak/sigVerifierTest
branch
2 times, most recently
from
e0e8840 to
c3f4a94
Compare
RnkSngh
force-pushed
the
raunak/sigVerifierTest
branch
from
c3f4a94 to
6db1239
Compare
PR to add sig verifier tests for contracts!
Summary by CodeRabbit
New Features
Tests
PolymerProofLib,ReceiptMPT, andSequencerVerifierChores
Documentation