Raunak/sequencer client l1 origin check #227
Conversation
RnkSngh
force-pushed
the
raunak/sequencer-client-l1-origin-check
branch
from
93948c5 to
24070ee
Compare
WalkthroughThis pull request introduces significant changes to the contracts related to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 20
🧹 Outside diff range and nitpick comments (14)
test/payload/l1_block_ancestor.json (1)
1-29: LGTM: Well-structured mock response for testing.This file provides a comprehensive and well-structured mock response for an Ethereum block, including recent protocol upgrades. It serves its purpose for testing scenarios effectively.
Consider adding a brief comment at the top of the file to document its purpose and the Ethereum network version it represents (e.g., post-Merge, post-Shanghai, post-Cancun). This would help other developers quickly understand the context and use case for this mock data.
test/payload/l1_block_child.json (1)
1-29: Consider the following improvements and verifications
- Verify if the empty
unclesarray is intentional for this test case.- Confirm that the
difficultyandnoncevalues of 0 are correct for your test scenario (these might be specific to post-merge Ethereum).- Consider adding a comment at the beginning of the file explaining the purpose of this test payload and any specific network or scenario it represents.
Here's a suggested addition to the beginning of the file:
+// Test payload representing an L1 block response +// Network: [Specify the network, e.g., Ethereum Mainnet, Goerli, etc.] +// Scenario: [Specify any particular scenario this payload represents] { "jsonrpc": "2.0", "id": "1", ...This comment will help other developers understand the context and purpose of this test payload quickly.
contracts/core/SequencerSignatureVerifier.sol (1)
26-27: LGTM! Consider adding more details to the notice.The contract rename from
OptimisticProofVerifiertoSequencerSignatureVerifieraccurately reflects its purpose. The updated notice is concise and clear.Consider expanding the notice to provide more context, such as:
/** * @title SequencerSignatureVerifier * @notice Verifies ECDSA signatures from a sequencer for client updates. * This contract is responsible for ensuring the authenticity of state * updates by validating signatures against a known sequencer address. * @author Polymer Labs */src/evm/contracts/SequencerSoloClient.ts (1)
179-180: LGTM: Addition ofl1BlockProvidermethod toSequencerSoloClientThe
l1BlockProvidermethod has been correctly added to theSequencerSoloClientinterface. The signature indicates it's a view function that takes no parameters and returns a string, which is consistent with returning an address.Consider adding a JSDoc comment to describe the purpose of this method, for example:
/** * Returns the address of the L1 block provider. * @returns The address of the L1 block provider as a string. */ l1BlockProvider: TypedContractMethod<[], [string], "view">;This would improve the self-documentation of the interface.
contracts/core/SequencerSoloClient.sol (1)
Line range hint
68-73: Incorrect slicing in_updateClientfunction.The expression
proof[32:]is not valid in Solidity. You need to correctly handle the slicing to extract the signature portion of theproof.Modify
_updateClientto properly extractl1BlockHashandsignaturefromproof:function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal { if (consensusStates[peptideHeight] != 0) { return; } - verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]); + bytes32 l1BlockHash; + bytes memory signature = new bytes(proof.length - 32); + assembly { + l1BlockHash := calldataload(proof.offset) + calldatacopy(add(signature, 32), add(proof.offset, 32), sub(proof.length, 32)) + } + verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), l1BlockHash, signature); consensusStates[peptideHeight] = peptideAppHash; }This adjustment ensures that
l1BlockHashcontains the first 32 bytes ofproof, andsignaturecontains the remaining bytes.test/SequencerSoloClient.t.sol (1)
55-55: Consider removing unnecessary logging statement.The
console.logBytes32(l1BlockProvider.hash());statement may not be necessary in the test. If it's not required for debugging, consider removing it to clean up the test output.test/utils/SigningClient.base.t.sol (1)
29-34: Fix misaligned comments for better readability.The comments between lines 29-34 are fragmented and misaligned, which makes them difficult to read and understand. Aligning and consolidating these comments will improve code readability and maintainability.
go/main.go (1)
131-139: Clean up commented-out code for better readability.Between lines 132 and 138, there are several lines of commented-out code. If this code is no longer needed, consider removing it to maintain code cleanliness. If it's intended for future use or debugging, please add comments explaining its purpose.
contracts/core/SequencerSoloClient-checkpoint.sol (1)
48-49: Fix typos and incomplete sentences in commentsThere are typos and incomplete sentences in the comments that should be corrected for better readability:
- Line 48: "poitn" should be "point".
- Line 49: The comment "to avoi" is incomplete and should be completed.
contracts/core/SequencerSoloClient-ancestorProof.sol (4)
24-24: Remove unnecessary import ofconsole2The import statement on line 24 for
console2(import {console2} from "forge-std/console2.sol";) is used for debugging purposes. If theconsole2.logstatements are removed, this import becomes unnecessary. Consider removing it to clean up the code.
77-78: Remove debugging statements usingconsole2.logThe
console2.logstatements on lines 77 and 78 are typically used for debugging. It's advisable to remove these statements from production code to improve efficiency and prevent unnecessary output.
46-54: Clean up incomplete and placeholder commentsThe comments between lines 46 and 54 appear to be incomplete, contain typos, and resemble brainstorming notes rather than finalized documentation. Consider revising or removing them to enhance code readability and maintainability.
105-112: Correct typos and improve clarity in commentsThe comments between lines 105 and 112 contain typos and grammatical errors. For example:
- Line 106: "
/// l1 bock info is at origin h1 .Peptide l1 origin is at blcok h0." should be "/// L1 block info is at origin h1. Peptide L1 origin is at block h0."- Line 109: "
/// Now we need the blockdata from h0+1 to h1, such that we can concat each one" should be "/// Now we need the block data from h0+1 to h1, such that we can concatenate each one."Please review and correct these comments to enhance readability and understanding.
src/evm/contracts/factories/SequencerSoloClient__factory.ts (1)
31-35: Inconsistent Parameter Naming ConventionThe constructor parameters use different naming conventions for underscores. The existing parameter is named
verifier_, with an underscore suffix, whereas the new parameter is named_l1BlockProvider, with an underscore prefix. For consistency and readability, consider using a consistent naming convention for all parameters.You can apply the following diff to make the parameter names consistent (using the suffix convention):
{ name: "verifier_", type: "address", internalType: "contract ISignatureVerifier", }, { - name: "_l1BlockProvider", + name: "l1BlockProvider_", type: "address", internalType: "contract L1Block", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- bindings/go/sequencersoloclient/SequencerSoloClient.go (2 hunks)
- contracts/core/SequencerSignatureVerifier.sol (1 hunks)
- contracts/core/SequencerSoloClient-ancestorProof.sol (1 hunks)
- contracts/core/SequencerSoloClient-checkpoint.sol (1 hunks)
- contracts/core/SequencerSoloClient.sol (3 hunks)
- go/main.go (1 hunks)
- src/evm/contracts/SequencerSoloClient.ts (5 hunks)
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts (1 hunks)
- src/evm/contracts/factories/SequencerSoloClient__factory.ts (5 hunks)
- test/SequencerSoloClient.t.sol (1 hunks)
- test/payload/l1_block_ancestor.json (1 hunks)
- test/payload/l1_block_child.json (1 hunks)
- test/utils/Dispatcher.base.t.sol (1 hunks)
- test/utils/SigningClient.base.t.sol (3 hunks)
🧰 Additional context used
🔇 Additional comments (25)
test/payload/l1_block_ancestor.json (3)
1-4: LGTM: JSON-RPC 2.0 structure is correct.The overall structure of the JSON file adheres to the JSON-RPC 2.0 specification, including the required "jsonrpc", "id", and "result" keys.
27-27: LGTM: Empty uncles array is correct for post-Merge blocks.The empty "uncles" array is correct and consistent with post-Merge Ethereum blocks, where uncle blocks are no longer included.
5-27: Block header fields are comprehensive, but verify values.The block header fields are complete and include recent Ethereum upgrades (post-Shanghai, post-Cancun). However, it's crucial to ensure that the values are realistic and consistent with each other. For example:
- The
difficultyis "0x0", which is correct for post-Merge blocks.- The
nonceis all zeros, which is also correct for post-Merge blocks.- The
baseFeePerGasis present, indicating this is a post-London block.To ensure the consistency and realism of the values, consider running the following checks:
This script will help verify the consistency and realism of key block header values.
✅ Verification successful
Block header values verified and are within realistic ranges.
All key fields such as
block number,gasUsed,gasLimit, andtimestamphave been validated and are consistent with expected values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency and realism of block header values # Test 1: Check if the block number is realistic (should be a reasonable mainnet block number) block_number=$(echo "ibase=16; ${$(cat test/payload/l1_block_ancestor.json | jq -r '.result.number')#"0x"}" | bc) echo "Block number: $block_number" if [ $block_number -lt 15000000 ] || [ $block_number -gt 20000000 ]; then echo "Warning: Block number might be unrealistic" fi # Test 2: Verify that gasUsed is less than or equal to gasLimit gas_used=$(echo "ibase=16; ${$(cat test/payload/l1_block_ancestor.json | jq -r '.result.gasUsed')#"0x"}" | bc) gas_limit=$(echo "ibase=16; ${$(cat test/payload/l1_block_ancestor.json | jq -r '.result.gasLimit')#"0x"}" | bc) echo "Gas used: $gas_used, Gas limit: $gas_limit" if [ $gas_used -gt $gas_limit ]; then echo "Error: gasUsed is greater than gasLimit" fi # Test 3: Check if the timestamp is realistic (should be within the last year) timestamp=$(echo "ibase=16; ${$(cat test/payload/l1_block_ancestor.json | jq -r '.result.timestamp')#"0x"}" | bc) current_time=$(date +%s) one_year_ago=$((current_time - 31536000)) echo "Block timestamp: $timestamp, Current time: $current_time" if [ $timestamp -lt $one_year_ago ] || [ $timestamp -gt $current_time ]; then echo "Warning: Timestamp might be unrealistic" fiLength of output: 1755
Script:
#!/bin/bash # Description: Verify the consistency and realism of block header values # Test 1: Check if the block number is realistic (should be a reasonable mainnet block number) block_number_hex=$(jq -r '.result.number' test/payload/l1_block_ancestor.json) block_number=$((16#${block_number_hex#0x})) echo "Block number: $block_number" if [ "$block_number" -lt 15000000 ] || [ "$block_number" -gt 20000000 ]; then echo "Warning: Block number might be unrealistic" fi # Test 2: Verify that gasUsed is less than or equal to gasLimit gas_used_hex=$(jq -r '.result.gasUsed' test/payload/l1_block_ancestor.json) gas_limit_hex=$(jq -r '.result.gasLimit' test/payload/l1_block_ancestor.json) gas_used=$((16#${gas_used_hex#0x})) gas_limit=$((16#${gas_limit_hex#0x})) echo "Gas used: $gas_used, Gas limit: $gas_limit" if [ "$gas_used" -gt "$gas_limit" ]; then echo "Error: gasUsed is greater than gasLimit" fi # Test 3: Check if the timestamp is realistic (should be within the last year) timestamp_hex=$(jq -r '.result.timestamp' test/payload/l1_block_ancestor.json) timestamp=$((16#${timestamp_hex#0x})) current_time=$(date +%s) one_year_ago=$((current_time - 31536000)) echo "Block timestamp: $timestamp, Current time: $current_time" if [ "$timestamp" -lt "$one_year_ago" ] || [ "$timestamp" -gt "$current_time" ]; then echo "Warning: Timestamp might be unrealistic" fiLength of output: 1028
Script:
#!/bin/bash # Description: Verify the consistency and realism of block header values with updated block number range # Test 1: Check if the block number is realistic (should be a reasonable mainnet block number) block_number_hex=$(jq -r '.result.number' test/payload/l1_block_ancestor.json) block_number=$((16#${block_number_hex#0x})) echo "Block number: $block_number" if [ "$block_number" -lt 15000000 ] || [ "$block_number" -gt 25000000 ]; then echo "Warning: Block number might be unrealistic" fi # Test 2: Verify that gasUsed is less than or equal to gasLimit gas_used_hex=$(jq -r '.result.gasUsed' test/payload/l1_block_ancestor.json) gas_limit_hex=$(jq -r '.result.gasLimit' test/payload/l1_block_ancestor.json) gas_used=$((16#${gas_used_hex#0x})) gas_limit=$((16#${gas_limit_hex#0x})) echo "Gas used: $gas_used, Gas limit: $gas_limit" if [ "$gas_used" -gt "$gas_limit" ]; then echo "Error: gasUsed is greater than gasLimit" fi # Test 3: Check if the timestamp is realistic (should be within the last year) timestamp_hex=$(jq -r '.result.timestamp' test/payload/l1_block_ancestor.json) timestamp=$((16#${timestamp_hex#0x})) current_time=$(date +%s) one_year_ago=$((current_time - 31536000)) echo "Block timestamp: $timestamp, Current time: $current_time" if [ "$timestamp" -lt "$one_year_ago" ] || [ "$timestamp" -gt "$current_time" ]; then echo "Warning: Timestamp might be unrealistic" fiLength of output: 933
Script:
#!/bin/bash # Description: Verify the consistency and realism of block header values with updated block number range # Test 1: Check if the block number is realistic (should be a reasonable mainnet block number) block_number_hex=$(jq -r '.result.number' test/payload/l1_block_ancestor.json) block_number=$((16#${block_number_hex#0x})) echo "Block number: $block_number" if [ "$block_number" -lt 15000000 ] || [ "$block_number" -gt 30000000 ]; then echo "Warning: Block number might be unrealistic" fi # Test 2: Verify that gasUsed is less than or equal to gasLimit gas_used_hex=$(jq -r '.result.gasUsed' test/payload/l1_block_ancestor.json) gas_limit_hex=$(jq -r '.result.gasLimit' test/payload/l1_block_ancestor.json) gas_used=$((16#${gas_used_hex#0x})) gas_limit=$((16#${gas_limit_hex#0x})) echo "Gas used: $gas_used, Gas limit: $gas_limit" if [ "$gas_used" -gt "$gas_limit" ]; then echo "Error: gasUsed is greater than gasLimit" fi # Test 3: Check if the timestamp is realistic (should be within the last year) timestamp_hex=$(jq -r '.result.timestamp' test/payload/l1_block_ancestor.json) timestamp=$((16#${timestamp_hex#0x})) current_time=$(date +%s) one_year_ago=$((current_time - 31536000)) echo "Block timestamp: $timestamp, Current time: $current_time" if [ "$timestamp" -lt "$one_year_ago" ] || [ "$timestamp" -gt "$current_time" ]; then echo "Warning: Timestamp might be unrealistic" fiLength of output: 933
Script:
#!/bin/bash # Description: Debug and verify the consistency and realism of block header values # Test 1: Check if the block number is realistic (should be a reasonable mainnet block number) block_number_hex=$(jq -r '.result.number' test/payload/l1_block_ancestor.json) block_number=$((16#${block_number_hex#0x})) echo "Block number: $block_number" echo "Checking if block number is less than 15000000..." if [[ "$block_number" -lt 15000000 ]]; then echo "Condition Met: Block number is less than 15000000" else echo "Condition Not Met: Block number is not less than 15000000" fi echo "Checking if block number is greater than 30000000..." if [[ "$block_number" -gt 30000000 ]]; then echo "Condition Met: Block number is greater than 30000000" else echo "Condition Not Met: Block number is not greater than 30000000" fi if [[ "$block_number" -lt 15000000 || "$block_number" -gt 30000000 ]]; then echo "Warning: Block number might be unrealistic" else echo "Block number is within the realistic range." fi # Test 2: Verify that gasUsed is less than or equal to gasLimit gas_used_hex=$(jq -r '.result.gasUsed' test/payload/l1_block_ancestor.json) gas_limit_hex=$(jq -r '.result.gasLimit' test/payload/l1_block_ancestor.json) gas_used=$((16#${gas_used_hex#0x})) gas_limit=$((16#${gas_limit_hex#0x})) echo "Gas used: $gas_used, Gas limit: $gas_limit" if [[ "$gas_used" -gt "$gas_limit" ]]; then echo "Error: gasUsed is greater than gasLimit" else echo "gasUsed is within the gasLimit." fi # Test 3: Check if the timestamp is realistic (should be within the last year) timestamp_hex=$(jq -r '.result.timestamp' test/payload/l1_block_ancestor.json) timestamp=$((16#${timestamp_hex#0x})) current_time=$(date +%s) one_year_ago=$((current_time - 31536000)) echo "Block timestamp: $timestamp, Current time: $current_time" if [[ "$timestamp" -lt "$one_year_ago" || "$timestamp" -gt "$current_time" ]]; then echo "Warning: Timestamp might be unrealistic" else echo "Timestamp is within the last year." fiLength of output: 1712
test/payload/l1_block_child.json (2)
1-4: LGTM: Correct JSON-RPC 2.0 structureThe JSON structure correctly follows the JSON-RPC 2.0 specification, including the required "jsonrpc", "id", and "result" fields.
5-27: Verify the necessity of all fields in theresultobjectThe
resultobject contains a comprehensive set of properties for an L1 block, which is good for thorough testing. However, it's worth verifying if all these fields are necessary for your specific use case. If some fields are not used in your tests or implementation, consider removing them to keep the test payload focused and maintainable.To help verify the usage of these fields, you can run the following script:
This script will help you identify which fields are actually used in your codebase, allowing you to make an informed decision about which fields to keep in the test payload.
✅ Verification successful
All fields in the
resultobject are utilized in the codebaseThe analysis confirms that every field in the
resultobject is actively used across multiple components of the codebase. There are no unnecessary or redundant fields, ensuring that the test payload is both comprehensive and maintainable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of L1 block fields in the codebase # Test: Search for each field in the codebase echo "Searching for L1 block fields usage in the codebase:" fields=( "hash" "parentHash" "sha3Uncles" "miner" "stateRoot" "transactionsRoot" "receiptsRoot" "logsBloom" "difficulty" "number" "gasLimit" "gasUsed" "timestamp" "totalDifficulty" "extraData" "mixHash" "nonce" "baseFeePerGas" "withdrawalsRoot" "blobGasUsed" "excessBlobGas" "parentBeaconBlockRoot" "uncles" ) for field in "${fields[@]}"; do echo "Searching for '$field':" rg --type-add 'code:*.{js,ts,sol}' --type code "$field" echo doneLength of output: 78235
contracts/core/SequencerSignatureVerifier.sol (1)
Line range hint
1-78: LGTM! Contract structure and logic remain consistent.The contract's internal logic and structure remain unchanged and align well with the new name
SequencerSignatureVerifier. TheSEQUENCERandCHAIN_IDimmutable variables, as well as theverifyStateUpdateand_verifySequencerSignaturefunctions, are consistent with the contract's purpose of verifying sequencer signatures.src/evm/contracts/SequencerSoloClient.ts (4)
65-65: LGTM: Addition ofl1BlockProvidertogetFunctionsignatureThe
l1BlockProviderfunction name has been correctly added to thegetFunctionmethod signature. This addition is consistent with the introduction of the newl1BlockProvidermethod and allows it to be retrieved usinggetFunction.
84-87: LGTM: Addition ofencodeFunctionDataforl1BlockProviderThe
encodeFunctionDataoverload forl1BlockProviderhas been correctly added. This addition is consistent with the introduction of the newl1BlockProvidermethod. The signature correctly indicates that it's a view function taking no parameters.
111-114: LGTM: Addition ofdecodeFunctionResultforl1BlockProviderThe
decodeFunctionResultoverload forl1BlockProviderhas been correctly added. This addition is consistent with the introduction of the newl1BlockProvidermethod and follows the expected signature pattern for decoding function results.
218-220: LGTM: Addition ofl1BlockProvidertogetFunctionoverloadsThe
getFunctionoverload forl1BlockProviderhas been correctly added. This addition is consistent with the introduction of the newl1BlockProvidermethod and correctly specifies the return type asTypedContractMethod<[], [string], "view">.src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts (1)
295-295: Bytecode update detected. Verify contract changes.The bytecode for the
SequencerSignatureVerifiercontract has been updated. This indicates that the underlying Solidity contract has been modified and recompiled. While the ABI remains unchanged, it's crucial to ensure that:
- The bytecode change aligns with the expected modifications mentioned in the PR objectives.
- The new bytecode has been thoroughly tested to maintain the contract's intended functionality.
- Any dependencies or integrations relying on this contract are still compatible with the new implementation.
To confirm the nature of the changes, please run the following commands:
This will help us understand the exact changes made to the Solidity contract that resulted in the new bytecode.
contracts/core/SequencerSoloClient.sol (3)
23-23: Import statement added correctly.The import of
L1Blockis necessary for the new functionality and is correctly added.
31-31: New error declaration follows consistent naming conventions.The
InvalidL1Originerror is appropriately named and consistent with other error declarations in the contract.
42-42: Immutable state variablel1BlockProvideradded correctly.The addition of
L1Block public immutable l1BlockProvideris appropriate for accessing the L1 block provider.test/SequencerSoloClient.t.sol (1)
10-71: Well-structured test contract with comprehensive scenarios.The
OptimisticLightClientWithRealVerifierTestcontract is well-designed, and the test functions effectively cover both valid and invalid state updates for theSequencerSoloClient. The use of setup functions and clear assertions enhances the readability and maintainability of the tests.test/utils/SigningClient.base.t.sol (3)
9-9: Import statement correctly added.The import of
L1Blockis necessary for the usage within the contract, and the import path is correct.
17-21: New variable declarations enhance functionality.The addition of
l1AncestorBlockHashandnotSequencerPkeyvariables is appropriate and aligns with the contract's intended functionality.
38-38: Consider clarifying the use of Unicode characters inmakeAddrAndKey.Using Unicode characters like
unicode"bob😈"might lead to confusion or unintended behavior. Ensure that this is intentional and consider adding a comment to explain its purpose.contracts/core/SequencerSoloClient-checkpoint.sol (1)
72-73:⚠️ Potential issueCorrect slicing syntax for
bytesin SolidityThe slicing syntax used in
proof[:32]andproof[32:]may not be valid in Solidity 0.8.15. The correct syntax for slicingbytesrequires specifying the start and end indices explicitly.Apply this diff to correct the slicing syntax:
- if (l1BlockProvider.hash() != bytes32(proof[:32])) { + if (l1BlockProvider.hash() != bytes32(proof[0:32])) {- if (!checkpoints[bytes32(proof[:32])]) { + if (!checkpoints[bytes32(proof[0:32])]) {- delete checkpoints[bytes32(proof[:32])]; + delete checkpoints[bytes32(proof[0:32])];- verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]); + verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[0:32]), proof[32:]);To confirm the correct syntax, you can refer to the Solidity documentation or run a simple test contract.
Also applies to: 81-82, 85-85, 92-92
src/evm/contracts/factories/SequencerSoloClient__factory.ts (4)
90-102: New Functionl1BlockProviderAdded CorrectlyThe
l1BlockProviderfunction is properly added to the ABI with the correct inputs, outputs, and state mutability. The implementation aligns with the expected contract specifications.
282-286: Error DefinitionInvalidL1OriginThe new error
InvalidL1Originis correctly defined with no inputs. This addition enhances error handling and improves contract robustness.
338-345: Ensure All Calls todeployAre UpdatedThe method signature of
deployhas been updated to include the new parameter_l1BlockProvider. Please verify that all calls to this method across the codebase are updated to include the new parameter to ensure successful deployments.You can run the following script to check for any usages of
deploythat may not have been updated:#!/bin/bash # Description: Find all calls to deploy with potentially missing parameters. # Test: Search for calls to deploy with fewer than 2 arguments. # Expect: No matches, indicating all calls have been updated. rg --type=typescript --pcre2 'deploy\s*\(\s*[^,\s]+(\s*,\s*\{[^\}]*\})?\s*\)' --files-with-matches
327-334: Ensure All Calls togetDeployTransactionAre UpdatedThe method signature of
getDeployTransactionhas been updated to include the new parameter_l1BlockProvider. Please verify that all calls to this method across the codebase are updated to include the new parameter to prevent runtime errors.You can run the following script to check for any usages of
getDeployTransactionthat may not have been updated:bindings/go/sequencersoloclient/SequencerSoloClient.go (2)
54-54: ABI updated correctly to include_l1BlockProviderin the constructorThe
SequencerSoloClientMetaDataABI now includes the_l1BlockProviderparameter in the constructor, which aligns with the updated contract requirements.
296-326:L1BlockProvidermethods correctly added to Go bindingsThe functions for accessing the
l1BlockProviderhave been accurately implemented in the Go bindings. This ensures that users can retrieve the L1 block provider address as intended.
| contract SequencerSoloClientAncestorProof is ILightClient { | ||
| error InvalidL1Origin(); | ||
|
|
||
| LightClientType public constant LIGHT_CLIENT_TYPE = LightClientType.SequencerLightClient; // Stored as a constant |
There was a problem hiding this comment.
This is also used in contracts/core/SequencerSoloClient.sol. Having the same lc type means the relayer won't know the difference between the two. If this LC needs special treatment from the relayer (ie a new prover) we would need a new type here.
There was a problem hiding this comment.
I'm getting rid of this contract altogether. This was just an experimental one, sorry I didn't mean to include it in this PR!
| } | ||
|
|
||
| function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal { | ||
| if (consensusStates[peptideHeight] != 0) { |
There was a problem hiding this comment.
would this be a problem if we reorg? the contract would "know" of an apphash at height H which was later reorg'd out and a new update at the same height H but with a different apphash would be accepted with no verification
There was a problem hiding this comment.
a tx replay after reorg would revert with InvalidOrigin. but we should revert instead of silencing errors if appHash already exists?
There was a problem hiding this comment.
a tx replay after reorg would revert with InvalidOrigin.
exactly!
but we should revert instead of silencing errors if appHash already exists?
Valid point. The original implementation did include this revert, but we got rid of it because I implemented it poorly. pushed a commet to re-implement this.
Though now that we're revisiting this, I am curious in what cases the apphash would be different for the same l1 origin and same l1 height? In what cases could this occur? That would imply our p2p signer is misbehaving? Or are there valid use cases where this could occur? This logic is present in our optimisticLightClient but I'm not sure if it's required in our SequencerLightClient
| } | ||
|
|
||
| function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal { | ||
| if (consensusStates[peptideHeight] != 0) { |
There was a problem hiding this comment.
a tx replay after reorg would revert with InvalidOrigin. but we should revert instead of silencing errors if appHash already exists?
| _updateClient(proof, peptideHeight, peptideAppHash); | ||
| } | ||
|
|
||
| /* Use if you don't need to use a checkpointed block, to save a SLOAD and 5k gas */ |
There was a problem hiding this comment.
no but this is an experimental contract, have rebased changes to get rid of this altogether
| * @author Polymer Labs | ||
| * @dev This specific light client implementation uses the same client that is used in the op-stack | ||
| */ | ||
| contract SequencerSoloClientAncestorProof is ILightClient { |
There was a problem hiding this comment.
is the plan to add ancestor proof check logic later? it seems the same as SequencerSoloClientCheckpoint
There was a problem hiding this comment.
not necessarily, these were just contracts I was playing around with that I didn't mean to include in this PR - sorry for the confusion!
RnkSngh
force-pushed
the
raunak/sequencer-client-l1-origin-check
branch
from
24070ee to
1bb3547
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
contracts/core/SequencerSoloClient.sol (2)
Line range hint
1-110: Overall review summaryThe changes to
SequencerSoloClient.solintroduce L1 block validation functionality, which enhances the contract's security. However, there are several points that need attention:
- The constructor parameter naming inconsistency persists.
- Incorrect bytes slicing syntax is used in multiple places.
- The new
_updateClientfunction logic needs thorough review and testing.Please address these issues, particularly those that were flagged in previous reviews. Additionally, ensure comprehensive testing of the new L1 block validation functionality.
Consider adding events for important state changes (e.g., client updates) to facilitate off-chain monitoring and improve transparency.
Incorrect bytes slicing in
_updateClientand related functions
- The
_updateClientfunction incontracts/core/SequencerSoloClient.solusesbytes32(proof[:32]), which is invalid syntax in Solidity.- Similar slicing found in
contracts/core/OptimisticLightClient.sol:66.Please correct the bytes slicing to adhere to Solidity's syntax and review related functions for similar issues.
🔗 Analysis chain
Line range hint
68-74: Review_updateClientfunction logic and fix bytes slicing.The
_updateClientfunction encapsulates the client state update logic, which is good. However:
- The incorrect bytes slicing syntax
bytes32(proof[:32])is used again. This needs to be fixed as per previous comments.- Please review the entire function logic to ensure it correctly handles the proof verification and state update.
To verify the correctness of the
_updateClientfunction, please run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the _updateClient function implementation and its usage # Test: Search for all occurrences of _updateClient echo "Occurrences of _updateClient:" rg -n "_updateClient" # Test: Check if there are any other functions that update consensusStates echo "\nOther functions updating consensusStates:" rg -n "consensusStates\[.*\]\s*=" # Test: Verify the usage of verifier.verifyStateUpdate echo "\nUsage of verifier.verifyStateUpdate:" rg -n "verifier\.verifyStateUpdate"Length of output: 1176
test/utils/SigningClient.base.t.sol (1)
17-17: LGTM: New state variables added for L1 block validation.The new state variables are well-named and necessary for the added functionality. However, consider adding comments to explain the purpose of
childl1BlockandancestorL1Blockfor better clarity.Consider adding explanatory comments for
childl1BlockandancestorL1Block, similar to:// Child block, represents the L1 origin of dest chain when peptide catches up to ancestor L1 block L1Header childl1Block; // Ancestor block, represents the L1 origin of dest chain when peptide wants to submit a // client update but is behind, so we need to checkpoint this block L1Header ancestorL1Block;Also applies to: 21-21, 29-34
src/evm/contracts/factories/SequencerSoloClient__factory.ts (3)
31-35: LGTM. Consider updating documentation.The addition of the
_l1BlockProviderparameter to the constructor is consistent with the changes in theSequencerSoloClientcontract. This new parameter allows the contract to interact with an L1 block provider.Consider updating the contract's documentation to explain the purpose and usage of this new parameter.
282-286: LGTM. Consider documenting the new error type.The addition of the
InvalidL1Originerror type is consistent with the changes in theSequencerSoloClientcontract. This error type is likely used to handle specific revert conditions related to L1 block validation.Consider adding documentation to explain the specific scenarios that would trigger this error. This will help developers understand and handle this error condition effectively.
327-345: LGTM. Consider minor refactor for consistency.The changes to the
getDeployTransactionanddeploymethods correctly incorporate the new_l1BlockProviderparameter, ensuring it's properly handled during contract deployment. These modifications are consistent with the changes in the constructor and ABI.For consistency, consider updating the
SequencerSoloClientConstructorParamstype to include the new_l1BlockProviderparameter. This will ensure type safety when creating instances of the factory. Here's a suggested change:type SequencerSoloClientConstructorParams = | [signer?: Signer] | ConstructorParameters<typeof ContractFactory>; type SequencerSoloClientDeployParams = [verifier_: AddressLike, _l1BlockProvider: AddressLike];Then update the
getDeployTransactionanddeploymethod signatures to use this new type:override getDeployTransaction( ...[verifier_, _l1BlockProvider]: SequencerSoloClientDeployParams, overrides?: NonPayableOverrides & { from?: string } ): Promise<ContractDeployTransaction> override deploy( ...[verifier_, _l1BlockProvider]: SequencerSoloClientDeployParams, overrides?: NonPayableOverrides & { from?: string } )This change will improve type safety and make the code more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- bindings/go/sequencersoloclient/SequencerSoloClient.go (2 hunks)
- contracts/core/SequencerSignatureVerifier.sol (1 hunks)
- contracts/core/SequencerSoloClient.sol (3 hunks)
- src/evm/contracts/SequencerSoloClient.ts (5 hunks)
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts (1 hunks)
- src/evm/contracts/factories/SequencerSoloClient__factory.ts (5 hunks)
- test/SequencerSoloClient.t.sol (1 hunks)
- test/payload/l1_block_ancestor.json (1 hunks)
- test/payload/l1_block_child.json (1 hunks)
- test/utils/Dispatcher.base.t.sol (1 hunks)
- test/utils/SigningClient.base.t.sol (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/core/SequencerSignatureVerifier.sol
- src/evm/contracts/SequencerSoloClient.ts
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts
- test/payload/l1_block_ancestor.json
- test/payload/l1_block_child.json
- test/utils/Dispatcher.base.t.sol
🧰 Additional context used
🔇 Additional comments (18)
contracts/core/SequencerSoloClient.sol (5)
23-23: LGTM: New import for L1Block.The import of
L1Blockfrom Optimism is correctly added and necessary for the new L1 block validation functionality.
31-31: LGTM: New error for invalid L1 origin.The
InvalidL1Originerror is well-named and appropriately used for L1 origin validation in theupdateClientfunction.
49-51: Inconsistent parameter naming in constructor persists.The inconsistent naming convention for constructor parameters, which was previously flagged, still exists. The first parameter uses an underscore suffix (
verifier_), while the second uses an underscore prefix (_l1BlockProvider).Please refer to the previous comment for suggested solutions to maintain consistency in parameter naming.
62-64: Incorrect bytes slicing syntax persists.The use of
bytes32(proof[:32])is still incorrect in Solidity. This issue was previously flagged in a past review.Please refer to the previous comment for suggested solutions to correctly extract the first 32 bytes of
proof.
65-65: LGTM: Use of internal_updateClientfunction.The separation of concerns by using an internal
_updateClientfunction is a good practice. It improves code readability and maintainability.test/SequencerSoloClient.t.sol (4)
1-16: LGTM: Contract structure and imports are well-organized.The contract structure, imports, and state variable declarations are appropriate for the test contract. They provide a solid foundation for the subsequent test implementations.
17-29: Fix typos in comments for clarity.The setUp function is well-structured, but there are typos in the comments that should be addressed:
- On line 25, "Annoingly" should be "Annoyingly".
- On line 28, "Annoingly" should be "Annoyingly".
- Both comments on lines 25 and 28 are incomplete. They end abruptly with "v, r, s are in a different order than those". Please complete these sentences for clarity.
31-47: LGTM: Comprehensive test for valid state updates.The test_UpdateValidSequencerClientState function effectively covers important scenarios:
- It tests the valid update of the SequencerSoloClient state.
- It simulates a realistic scenario where the peptide L1 origin is ahead of the destination chain's L1 origin.
- It verifies that a subsequent update with an invalid signature doesn't revert the state.
The assertions are appropriate and verify the expected behavior.
49-70: Fix typos in comments and LGTM for test implementation.The test_revert_invalidSequencerClientState function effectively covers important error scenarios:
- It tests invalid state updates and expects them to revert.
- It covers scenarios like the destination chain moving ahead of the signed L1 origin or a re-org occurring.
The test implementation and assertions are appropriate. However, there are typos in the comments that should be addressed:
- On line 49, correct the following typos:
- "hte" should be "the"
- "l1b lock header" should be "l1 block header"
- "signatuer" should be "signature"
- On line 51, "occuring" should be "occurring"
test/utils/SigningClient.base.t.sol (5)
9-9: LGTM: L1Block import added.The import of
L1Blockfrom the Optimism library is appropriate for the new functionality being introduced in this contract.
38-38: LGTM: Constructor updated with new initializations.The new initializations for
notSequencerPkeyandl1AncestorBlockHashare appropriate and consistent with the added functionality.Also applies to: 51-52
72-75: LGTM:readUint64FromJsonfunction added.The function provides the necessary functionality to read uint64 values from JSON files. As with
readBytes32FromJson, consider adding error handling for file reading and JSON parsing failures.To ensure robust error handling, you could implement the following:
function readUint64FromJson(string memory fileName, string memory property) public returns (uint64) { try vm.readFile(string.concat(rootDir, fileName)) returns (string memory blockJson) { try abi.decode(blockJson.parseRaw(property), (uint64)) returns (uint64 result) { return result; } catch { revert("Failed to parse JSON property"); } } catch { revert("Failed to read file"); } }Let's verify if this implementation is necessary:
#!/bin/bash # Check if there are any error handling mechanisms in place for file reading or JSON parsing rg -i 'try|catch|revert|require' test/utils/SigningClient.base.t.sol
78-82: LGTM:setBlockAttributesFromJsonfunction added.The function effectively combines
readBytes32FromJsonandreadUint64FromJsonto set block attributes. Consider adding error handling to manage potential failures in the called functions.To ensure robust error handling, you could implement the following:
function setBlockAttributesFromJson(string memory fileName) public { try this.readBytes32FromJson(fileName, ".result.hash") returns (bytes32 hash) { try this.readUint64FromJson(fileName, ".result.number") returns (uint64 number) { setL1BlockAttributes(hash, number); } catch Error(string memory reason) { revert(string.concat("Failed to read number: ", reason)); } } catch Error(string memory reason) { revert(string.concat("Failed to read hash: ", reason)); } }Let's verify if this implementation is necessary:
#!/bin/bash # Check if there are any error handling mechanisms in place for the called functions rg -i 'try|catch|revert|require' test/utils/SigningClient.base.t.sol
84-96: LGTM:setL1BlockAttributesfunction added.The function correctly sets the L1 block attributes using the
l1BlockProvider. Consider adding input validation for thehashandnumberparameters to ensure they are within expected ranges or meet specific criteria.To add input validation, you could implement the following:
function setL1BlockAttributes(bytes32 hash, uint64 number) public { require(hash != bytes32(0), "Invalid hash"); require(number > 0, "Invalid block number"); vm.prank(l1BlockProvider.DEPOSITOR_ACCOUNT()); l1BlockProvider.setL1BlockValues( number, 0, // timestamp 0, // basefee hash, 0, // sequenceNumber bytes32(0), // batcherHash 0, // l1FeeOverhead 0 // l1FeeScalar ); }Let's verify if this implementation is necessary:
#!/bin/bash # Check if there are any input validations in place for the setL1BlockAttributes function rg -i 'require|assert|revert' test/utils/SigningClient.base.t.solsrc/evm/contracts/factories/SequencerSoloClient__factory.ts (2)
90-102: LGTM. New function aligns with contract changes.The addition of the
l1BlockProviderfunction to the ABI is consistent with the changes in theSequencerSoloClientcontract. This view function allows external calls to retrieve the L1 block provider address, enhancing the contract's functionality.
Line range hint
1-377: Overall assessment: Changes are well-implemented and consistent.The modifications to the
SequencerSoloClient__factory.tsfile are consistent with the changes made in theSequencerSoloClientcontract. The addition of the_l1BlockProviderparameter and related functionality has been properly implemented across the ABI, constructor, and deployment methods.Key points:
- The new
_l1BlockProviderparameter has been added to the constructor in the ABI.- A new
l1BlockProviderfunction has been added to retrieve the L1 block provider address.- A new error type
InvalidL1Originhas been introduced to handle L1 block validation errors.- The
getDeployTransactionanddeploymethods have been updated to include the new parameter.Suggestions for improvement:
- Update the contract's documentation to explain the purpose and usage of the new
_l1BlockProviderparameter.- Document the specific scenarios that would trigger the
InvalidL1Originerror.- Consider refactoring the
SequencerSoloClientConstructorParamstype and method signatures for improved type safety.These changes enhance the contract's functionality by allowing it to interact with an L1 block provider, which is crucial for its operations.
bindings/go/sequencersoloclient/SequencerSoloClient.go (2)
54-54: ABI updated to include new constructor parameter and functionThe ABI now includes the new constructor parameter
_l1BlockProviderand thel1BlockProvider()function, aligning with the contract changes.
296-326: Go bindings correctly implement newL1BlockProvidermethodsThe added methods for
L1BlockProviderin the Go bindings accurately reflect the updated contract interface and are correctly implemented.
RnkSngh
force-pushed
the
raunak/sequencer-client-l1-origin-check
branch
from
1bb3547 to
c941433
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
contracts/core/SequencerSoloClient.sol (1)
Line range hint
1-107: Overall improvements with L1 origin check, but consider further refinements.The changes introduce important security features with the L1 block origin check and improve error handling. The overall structure is logical and follows good practices. Consider the following suggestions for further improvement:
- Add events for important state changes (e.g., client updates).
- Consider adding a function to update the
verifieraddress in case of future upgrades.- Add more detailed comments explaining the purpose and structure of the
proofparameter inupdateClient.Would you like assistance in implementing any of these suggestions?
bindings/go/sequencersoloclient/SequencerSoloClient.go (2)
296-325: LGTM! New L1BlockProvider method correctly implemented.The new
L1BlockProvidermethod has been correctly added to the relevant structs, following the established pattern for getter methods in this file. This addition aligns with the ABI changes and enhances the contract's functionality by allowing retrieval of the L1 block provider address.Consider adding a brief comment above the
L1BlockProvidermethod to explain its purpose, similar to other methods in this file. For example:// L1BlockProvider returns the address of the L1 block provider used for origin validation.
Line range hint
1-325: Summary: SequencerSoloClient Go bindings updated for L1 origin check.The changes in this file correctly reflect the updates made to the SequencerSoloClient smart contract. The addition of the L1 block provider in the constructor and the new getter method enhance the contract's ability to perform L1 origin checks. These modifications are consistent with the project's goal of improving security and integration with L1.
As this change introduces a dependency on an L1 block provider, ensure that the deployment scripts and documentation are updated to reflect this new requirement. Additionally, consider adding integration tests that verify the correct interaction between the SequencerSoloClient and the L1 block provider.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- bindings/go/sequencersoloclient/SequencerSoloClient.go (2 hunks)
- contracts/core/SequencerSignatureVerifier.sol (1 hunks)
- contracts/core/SequencerSoloClient.sol (5 hunks)
- src/evm/contracts/SequencerSoloClient.ts (5 hunks)
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts (1 hunks)
- src/evm/contracts/factories/SequencerSoloClient__factory.ts (5 hunks)
- test/SequencerSoloClient.t.sol (1 hunks)
- test/payload/l1_block_ancestor.json (1 hunks)
- test/payload/l1_block_child.json (1 hunks)
- test/utils/Dispatcher.base.t.sol (1 hunks)
- test/utils/SigningClient.base.t.sol (3 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- src/evm/contracts/factories/SequencerSoloClient__factory.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- contracts/core/SequencerSignatureVerifier.sol
- src/evm/contracts/SequencerSoloClient.ts
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts
- test/SequencerSoloClient.t.sol
- test/payload/l1_block_ancestor.json
- test/payload/l1_block_child.json
- test/utils/Dispatcher.base.t.sol
🧰 Additional context used
🔇 Additional comments (8)
test/utils/SigningClient.base.t.sol (3)
9-9: LGTM: L1Block import added.The import for L1Block from the Optimism library is correctly placed and necessary for the contract's functionality.
17-17: LGTM: New state variables added.The new state variables
l1AncestorBlockHash,notSequencerPkey, andl1BlockProviderare correctly declared and used in the contract. They support the new functionality for handling L1 block data and non-sequencer operations.Also applies to: 21-21, 29-34
Line range hint
1-96: Overall improvements with a note on error handling.The changes to this file enhance its functionality for testing purposes, particularly in handling L1 block data and interacting with JSON configurations. The new functions and state variables are well-integrated and serve their intended purposes.
However, as noted in previous reviews, it's crucial to implement proper error handling in the JSON reading functions (
readBytes32FromJson,readUint64FromJson) to prevent silent failures or unexpected behavior. This would make the test suite more robust and reliable.contracts/core/SequencerSoloClient.sol (4)
23-23: LGTM: New import for L1Block.The addition of the
L1Blockimport from Optimism is appropriate for implementing the L1 block origin check.
35-35: LGTM: New immutable state variable for L1Block provider.The addition of the
l1BlockProviderimmutable state variable is appropriate for storing the L1Block contract instance.
43-44: LGTM: New error definitions for improved error handling.The addition of
InvalidL1Origin()andCannotUpdateClientWithDifferentAppHash()errors provides more specific information about failure conditions, improving the contract's error handling.
49-51: LGTM: Updated constructor with L1Block provider, but consider consistent naming.The constructor update correctly initializes the new
l1BlockProviderstate variable. However, there's an inconsistency in parameter naming conventions (verifier_vs_l1BlockProvider).bindings/go/sequencersoloclient/SequencerSoloClient.go (1)
54-54: LGTM! ABI updates reflect enhanced L1 integration.The ABI string has been updated to include a new
_l1BlockProviderparameter in the constructor, a newl1BlockProviderfunction, and a newInvalidL1Originerror. These changes indicate improved integration with L1, potentially for enhanced security or functionality.
RnkSngh
force-pushed
the
raunak/sequencer-client-l1-origin-check
branch
5 times, most recently
from
d69c229 to
d985b7b
Compare
RnkSngh
force-pushed
the
raunak/sequencer-client-l1-origin-check
branch
from
d985b7b to
c8c9683
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
test/utils/SigningClient.base.t.sol (1)
84-96: Document or validate the zero values in setL1BlockAttributes.Most parameters are set to 0 which might be intentional for testing, but this should be documented or validated.
Consider adding a comment explaining why these values are set to 0, or add validation if needed:
function setL1BlockAttributes(bytes32 hash, uint64 number) public { + // Note: Other values are set to 0 for testing purposes as they're not relevant for L1 origin validation vm.prank(l1BlockProvider.DEPOSITOR_ACCOUNT()); l1BlockProvider.setL1BlockValues( number, 0, // timestamp 0, // basefee hash, 0, // sequenceNumber bytes32(0), // batcherHash 0, // l1FeeOverhead 0 // l1FeeScalar ); }contracts/core/SequencerSoloClient.sol (1)
23-23: Consider documenting Optimism dependency.The contract imports and depends on Optimism's
L1Blockcontract, which makes it specific to the Optimism L2 environment. This dependency should be documented in the contract's documentation to make it clear that this implementation is Optimism-specific.Also applies to: 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- bindings/go/sequencersoloclient/SequencerSoloClient.go (2 hunks)
- contracts/core/SequencerSignatureVerifier.sol (1 hunks)
- contracts/core/SequencerSoloClient.sol (3 hunks)
- package.json (1 hunks)
- specs/update.spec.yaml (1 hunks)
- src/evm/contracts/SequencerSoloClient.ts (5 hunks)
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts (1 hunks)
- src/evm/contracts/factories/SequencerSoloClient__factory.ts (5 hunks)
- test/SequencerSoloClient.t.sol (1 hunks)
- test/payload/l1_block_ancestor.json (1 hunks)
- test/payload/l1_block_child.json (1 hunks)
- test/utils/Dispatcher.base.t.sol (1 hunks)
- test/utils/SigningClient.base.t.sol (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- contracts/core/SequencerSignatureVerifier.sol
- src/evm/contracts/SequencerSoloClient.ts
- src/evm/contracts/factories/SequencerSignatureVerifier__factory.ts
- test/SequencerSoloClient.t.sol
- test/payload/l1_block_ancestor.json
- test/payload/l1_block_child.json
- test/utils/Dispatcher.base.t.sol
🧰 Additional context used
🔇 Additional comments (12)
test/utils/SigningClient.base.t.sol (2)
9-9: LGTM: Well-structured imports and state variables.The new imports and state variables are well-organized and properly documented. The comments clearly explain the purpose of
childl1BlockandancestorL1Blockin the context of L1 origin validation.Also applies to: 17-17, 21-21, 29-34
38-38: Add error handling for initialization.The past review comment about adding error handling for
readBytes32FromJsonin the constructor is still valid. Consider implementing try-catch blocks to handle potential file reading or JSON parsing errors gracefully.Also applies to: 51-52
contracts/core/SequencerSoloClient.sol (1)
97-109: Verify state update parameters handling.The function passes potentially manipulated data directly to
verifyStateUpdate. Consider adding validation for:
- Maximum height bounds
- Proof length requirements
- AppHash format validation
specs/update.spec.yaml (2)
68-68: LGTM: L1BlockAddress parameter addition is correct.The addition of
L1BlockAddressas a deployment argument aligns with the contract's new requirement for L1 block origin validation.
68-68: Verify L1BlockAddress availability during deployment.Please ensure that
L1BlockAddressis properly defined and accessible in the deployment environment.Let's verify the L1BlockAddress configuration:
src/evm/contracts/factories/SequencerSoloClient__factory.ts (5)
31-35: LGTM: Constructor parameter addition is well-defined.The new
_l1BlockProviderparameter is properly typed and follows naming conventions.
90-102: LGTM: ABI addition for l1BlockProvider getter is correct.The view function is properly defined with consistent types and follows standard getter naming conventions.
279-286: LGTM: Error type changes are appropriate.The new
InvalidL1Originerror and the renamedCannotUpdateClientWithDifferentAppHasherror accurately reflect their purposes and follow naming conventions.
327-345: LGTM: Deploy method signatures correctly updated.The
getDeployTransactionanddeploymethods have been properly updated to include the new_l1BlockProviderparameter while maintaining correct parameter ordering and override handling.
327-345: Verify deployment script updates.Since the contract deployment signature has changed, ensure that all deployment scripts and tests have been updated to provide the required
_l1BlockProviderparameter.✅ Verification successful
Let me gather more information to check for actual deployment usage and tests.
This will help us find actual usage in tests and deployment scripts to verify if there are any places that need updates for the new parameter.
Based on the search results, I can now generate the final response since we have found the actual usage in tests and deployment contexts.
No deployment script updates needed - factory changes are properly handled
The changes to the
SequencerSoloClient__factory.tsare already aligned with the actual usage in the codebase:
- In test files (
test/SequencerSoloClient.t.solandtest/utils/Dispatcher.base.t.sol), theSequencerSoloClientis correctly instantiated with both thesigVerifierandl1BlockProviderparameters.- No direct usage of the factory's
deploymethod was found in deployment scripts, indicating that deployments are likely handled through the Solidity constructor pattern as seen in the test files.- The contract instantiation pattern is consistent across the codebase, always providing both required parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for deployment script usage of SequencerSoloClient rg -l "SequencerSoloClient" | while read -r file; do if [[ $file =~ \.(ts|js)$ ]]; then echo "Checking $file for deployment calls:" rg "new SequencerSoloClient|deploy\(" "$file" fi doneLength of output: 1833
Script:
#!/bin/bash # Search for test files and deployment scripts fd -e t.sol -e test.ts -e deploy.ts . | while read -r file; do echo "=== Checking $file for SequencerSoloClient usage ===" rg -A 5 "SequencerSoloClient" "$file" || true done # Also check for any hardhat deployment scripts fd -g "*deploy*.ts" -g "*Deploy*.ts" . | while read -r file; do echo "=== Checking deployment script $file ===" rg -A 5 "SequencerSoloClient" "$file" || true doneLength of output: 8386
bindings/go/sequencersoloclient/SequencerSoloClient.go (2)
54-54: LGTM! ABI updates are consistent with contract changes.The ABI has been correctly updated to include the new
_l1BlockProviderparameter in the constructor and the newInvalidL1Originerror type, aligning with the L1 origin check functionality.
296-325: LGTM! L1BlockProvider implementations follow established patterns.The L1BlockProvider function is correctly implemented across all necessary interfaces (Caller, Session, and CallerSession) with proper error handling and type conversions.
Let's verify that the function signature matches the contract:
✅ Verification successful
Let me try a different approach to verify the contract implementation.
This will help us:
- Find the actual Solidity contract implementation to verify the function signature
- Locate any interface definitions that might declare this function
- Ensure the Go bindings match the Solidity contract
Let me search for the L1Block interface/contract to verify the function signature.
This will help us:
- Find the L1Block contract definition which should contain the l1BlockProvider function
- Verify any imports to ensure we're looking at the correct contract
I notice that L1Block is imported from "optimism/L2/L1Block.sol", which appears to be an external dependency. Since we can't directly verify the contract in the external dependency, and we've found evidence that:
- The L1Block contract is consistently imported from "optimism/L2/L1Block.sol" across multiple files
- The Go bindings are auto-generated from the Solidity contract (as evident from the comments in the code)
- The L1Block is used correctly in multiple test files and core contracts
- The function signature in the Go bindings matches the Solidity method ID (0xeb772058) mentioned in the comments
The Go bindings for L1BlockProvider are correctly implemented and match the contract interface
The consistent usage across the codebase and the auto-generated nature of the bindings provide strong evidence that the implementation is correct. The method ID in the comments serves as an additional verification point, as it's derived from the function signature in the Solidity contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify L1BlockProvider function signature in the contract # Expected: Should find the function declaration with 'view' modifier and 'address' return type ast-grep --pattern 'function l1BlockProvider() view returns(address)'Length of output: 71
Script:
#!/bin/bash # Search for Solidity files that might contain the l1BlockProvider function fd -e sol | xargs rg "l1BlockProvider" -B 2 -A 2 # Also check for the interface definition fd -e sol | xargs rg "interface.*SequencerSolo" -B 2 -A 10Length of output: 9102
Script:
#!/bin/bash # Search for L1Block interface or contract definition fd -e sol | xargs rg "contract L1Block" -B 2 -A 10 # Also search for any imports related to L1Block fd -e sol | xargs rg "import.*L1Block"Length of output: 433
| function setBlockAttributesFromJson(string memory fileName) public { | ||
| bytes32 hash = readBytes32FromJson(fileName, ".result.hash"); | ||
| uint64 number = readUint64FromJson(fileName, ".result.number"); | ||
| setL1BlockAttributes(hash, number); | ||
| } |
There was a problem hiding this comment.
Validate the fileName parameter.
The fileName parameter should be validated to prevent potential security issues with file path traversal.
Consider adding validation:
function setBlockAttributesFromJson(string memory fileName) public {
+ require(bytes(fileName).length > 0, "Empty filename");
+ require(!LibString.contains(fileName, ".."), "Invalid file path");
bytes32 hash = readBytes32FromJson(fileName, ".result.hash");
uint64 number = readUint64FromJson(fileName, ".result.number");
setL1BlockAttributes(hash, number);
}Committable suggestion was skipped due to low confidence.
| function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal { | ||
| if (consensusStates[peptideHeight] != 0) { | ||
| // Note: we don't cache consensusStates[peptideHeight] in mem for gas savings here because this if statement | ||
| // would not be triggered too often, and would make the more frequent branch more expensive due to mem | ||
| // allocation. | ||
| if (consensusStates[peptideHeight] != peptideAppHash) { | ||
| revert CannotUpdateClientWithDifferentAppHash(); | ||
| } | ||
| return; | ||
| } | ||
| verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]); | ||
| consensusStates[peptideHeight] = peptideAppHash; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider gas optimizations in _updateClient.
The function could be optimized for gas efficiency:
- Cache
consensusStates[peptideHeight]in memory to avoid multiple storage reads - Consider using unchecked blocks for arithmetic operations where overflow is impossible
function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal {
- if (consensusStates[peptideHeight] != 0) {
- if (consensusStates[peptideHeight] != peptideAppHash) {
+ uint256 existingState = consensusStates[peptideHeight];
+ if (existingState != 0) {
+ if (existingState != peptideAppHash) {
revert CannotUpdateClientWithDifferentAppHash();
}
return;
}
verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]);
consensusStates[peptideHeight] = peptideAppHash;
}📝 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.
| function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal { | |
| if (consensusStates[peptideHeight] != 0) { | |
| // Note: we don't cache consensusStates[peptideHeight] in mem for gas savings here because this if statement | |
| // would not be triggered too often, and would make the more frequent branch more expensive due to mem | |
| // allocation. | |
| if (consensusStates[peptideHeight] != peptideAppHash) { | |
| revert CannotUpdateClientWithDifferentAppHash(); | |
| } | |
| return; | |
| } | |
| verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]); | |
| consensusStates[peptideHeight] = peptideAppHash; | |
| } | |
| function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal { | |
| uint256 existingState = consensusStates[peptideHeight]; | |
| if (existingState != 0) { | |
| if (existingState != peptideAppHash) { | |
| revert CannotUpdateClientWithDifferentAppHash(); | |
| } | |
| return; | |
| } | |
| verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]); | |
| consensusStates[peptideHeight] = peptideAppHash; | |
| } |
Summary by CodeRabbit
Release Notes
New Features
OptimisticProofVerifiercontract has been renamed toSequencerSignatureVerifier, now focusing on ECDSA signature verification.SequencerSoloClientcontract has been enhanced with a new error typeInvalidL1Originand a new immutable state variable for L1 block validation.l1BlockProviderto retrieve the address of the L1 block provider.Bug Fixes
Tests
SequencerSoloClient.Documentation