Fp wd refactoring #123
Fp wd refactoring #123
Conversation
…ement utility functions for block retrieval
…e game events and remove unused helper files
… interfaces for block retrieval
… logging in Run method
WalkthroughThe pull request introduces a comprehensive refactoring of the faultproof withdrawals monitoring system, focusing on enhancing modularity and abstraction. The changes replace direct Ethereum client interactions with proxy interfaces for Layer 1 and Layer 2, introducing more flexible and testable code structures. New utility functions for block retrieval, withdrawal validation, and state management have been added, while several helper classes have been consolidated or removed to streamline the codebase. Changes
✨ Finishing Touches
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
Documentation and Community
|
![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 (17)
op-monitorism/faultproof_withdrawals/utils.go (2)
65-65: Simplify absolute value comparison for readabilityYou can simplify the comparison by storing the absolute value in the existing
diffvariable, improving readability.Apply this diff:
-diff := new(big.Int).Sub(blockTime, targetTime) -if new(big.Int).Abs(diff).Cmp(acceptablediff) <= 0 { +diff.Sub(blockTime, targetTime).Abs(diff) +if diff.Cmp(acceptablediff) <= 0 {
67-67: Enhance logging with detailed block informationTo improve debugging and monitoring, include the block number and timestamp in the log message.
Apply this diff:
-logger.Info("block number closest to target time", "block", block) +logger.Info("Found block closest to target time", "blockNumber", block.BlockNumber, "blockTime", block.BlockTime)op-monitorism/faultproof_withdrawals/validator/l2_proxy.go (1)
56-63: Differentiate error messages when dialing L2 clientsThe error messages for dialing
l2GethClientandl2NodeClientare identical, which can cause confusion during debugging. Include the client type or URL in the error messages for clarity.Apply this diff:
l2GethClient, err := ethclient.Dial(l2GethURL) if err != nil { - return nil, fmt.Errorf("failed to dial l2: %w", err) + return nil, fmt.Errorf("failed to dial l2GethClient at %s: %w", l2GethURL, err) } l2NodeClient, err := ethclient.Dial(l2NodeURL) if err != nil { - return nil, fmt.Errorf("failed to dial l2: %w", err) + return nil, fmt.Errorf("failed to dial l2NodeClient at %s: %w", l2NodeURL, err) }op-monitorism/faultproof_withdrawals/monitor.go (3)
37-42: Implement a cache or database for withdrawal queuesStoring withdrawals in maps is not scalable for large volumes of data. To improve scalability and traceability, consider using a cache or database.
Plan to integrate a caching mechanism or persistent storage to manage
currentWithdrawalsQueueandcurrentWithdrawalsQueueAttacksefficiently.
113-126: Enhance error resilience in fetching block heightsCurrently, the monitor exits if there's an error fetching the latest block heights. Implementing retry logic or handling transient errors can improve the monitor's resilience.
Consider adding retry mechanisms or error classification to handle transient network errors without stopping the monitor.
130-183: Refactor repeated logic in withdrawal queue processingThe processing of
currentWithdrawalsQueuecontains repeated code blocks. Refactoring these into helper functions can improve readability and maintainability.Extract repeated code segments into separate functions, such as
processInProgressGameandprocessCompletedGame, to reduce duplication.op-monitorism/faultproof_withdrawals/validator/l1_proxy.go (4)
10-13: Consider removing unused imports.The imports on lines 10-13 include packages that are not used elsewhere in the code. Removing unused imports can improve code readability and reduce potential confusion.
Apply this diff to remove unused imports:
import ( "context" "fmt" "math/big" - "github.com/ethereum-optimism/monitorism/op-monitorism/faultproof_withdrawals/bindings/dispute" - "github.com/ethereum-optimism/monitorism/op-monitorism/faultproof_withdrawals/bindings/l1" - "github.com/ethereum/go-ethereum/accounts/abi/bind" - "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/ethclient" )
131-156: Handle edge cases and errors in event processing loop.In the
GetDisputeGamesEventsmethod, if an error occurs while processing an event, the function returns immediately. This could lead to partial results and may skip processing subsequent events. Consider collecting errors and proceeding with processing, or implementing a more robust error handling strategy.Optionally, you can modify the loop to continue processing remaining events:
for _, event := range provenWithdrawalsExtension1Events { disputeGameRef, err := l1Proxy.getSubmittedProofsDataFromWithdrawalProvenExtensionEvent(&event) if err != nil { - return nil, fmt.Errorf("failed to get submitted proofs data from withdrawal proven extension1 event error:%w", err) + // Log the error and continue with the next event + fmt.Printf("Warning: %v\n", err) + continue } disputeGame, err := l1Proxy.getDisputeGameProxyFromAddress(disputeGameRef) if err != nil { - return nil, fmt.Errorf("failed to get dispute game proxy from address error:%w", err) + // Log the error and continue with the next event + fmt.Printf("Warning: %v\n", err) + continue } disputeGamesEvents = append(disputeGamesEvents, DisputeGameEvent{ EventRef: &event, DisputeGame: disputeGame, }) }
243-269: Optimize redundant connection state updates.In
getDisputeGameProxyFromAddress, thel1Proxy.ConnectionState.ProxyConnectionandl1Proxy.ConnectionState.ProxyConnectionFailedcounters are incremented multiple times within a single method. This may not accurately reflect the number of distinct connections. Consider updating these counters only when initiating new connections or handling significant operations.
337-349: Simplify error message formatting.The error message on line 342 includes redundant information by printing the block number and the error with
%d %w. The%wverb is used for wrapping errors. It's better to format the message to clearly convey the block number and the error.Apply this diff to improve the error message:
- return BlockInfo{}, fmt.Errorf("L1Proxy failed to get block by number: %d %w", blockNumber, err) + return BlockInfo{}, fmt.Errorf("failed to get block by number %d: %w", blockNumber, err)op-monitorism/faultproof_withdrawals/state.go (3)
Line range hint
85-122: Update function signature and documentation to reflect new parameters.The
NewStatefunction now accepts ametrics.Factoryparameter, but the function documentation and comments haven't been updated to reflect this change. To maintain clarity, update the comments to describe all parameters.Add documentation for the
m metrics.Factoryparameter:func NewState(logger log.Logger, nextL1Height uint64, latestL1Height uint64, latestL2Height uint64, m metrics.Factory) (*State, error) { + // m: Metrics factory for initializing Prometheus metrics
139-152: Correct logging statements for consistency.In the
LogStatemethod, the logging keys should be consistent. For example, use either camelCase or snake_case throughout. Also, ensure that all relevant fields are included in the log.Standardize the logging keys and include missing fields:
s.logger.Info("STATE:", "withdrawals_processed", fmt.Sprintf("%d", s.withdrawalsProcessed), "initial_l1_height", fmt.Sprintf("%d", s.initialL1Height), "next_l1_height", fmt.Sprintf("%d", s.nextL1Height), "latest_l1_height", fmt.Sprintf("%d", s.latestL1Height), "latest_l2_height", fmt.Sprintf("%d", s.latestL2Height), "blocks_to_process", fmt.Sprintf("%d", blockToProcess), "sync_percentage", fmt.Sprintf("%d%%", syncPercentage), "events_processed", fmt.Sprintf("%d", s.eventsProcessed), "l1_node_connection_failures", fmt.Sprintf("%d", s.l1NodeConnectionFailures), "l1_node_connection", fmt.Sprintf("%d", s.l1NodeConnection), "l2_node_connection_failures", fmt.Sprintf("%d", s.l2NodeConnectionFailures), "l2_node_connection", fmt.Sprintf("%d", s.l2NodeConnection), "latest_events_processed", fmt.Sprintf("%v", s.latestEventsProcessed), "latest_events_processed_timestamp", fmt.Sprintf("%f", s.latestEventsProcessedTimestamp), "potential_attack_on_defender_wins_games", fmt.Sprintf("%d", s.numberOfPotentialAttacksOnDefenderWinsGames), "potential_attack_on_in_progress_games", fmt.Sprintf("%d", s.numberOfPotentialAttackOnInProgressGames), "suspicious_events_on_challenger_wins_games", fmt.Sprintf("%d", s.numberOfSuspiciousEventsOnChallengerWinsGames), )
407-417: Handle potential underflow in counter delta calculations.When calculating deltas for counters, there's a risk of underflow if the current value is less than the previous value. Although unlikely, it's good practice to safeguard against this.
Modify the delta calculations to prevent underflow:
// Node Connection Failures l1NodeConnectionFailuresDelta := state.l1NodeConnectionFailures - m.previousl1NodeConnectionFailures +if state.l1NodeConnectionFailures >= m.previousl1NodeConnectionFailures { + l1NodeConnectionFailuresDelta = state.l1NodeConnectionFailures - m.previousl1NodeConnectionFailures +} else { + l1NodeConnectionFailuresDelta = 0 +}Repeat similar checks for
l2NodeConnectionFailuresDelta.op-monitorism/faultproof_withdrawals/validator.go (1)
33-37: Handle errors without terminating the entire process.In the loop within the
GetRangemethod, an error while processing one dispute game event causes the method to return immediately. Consider logging the error and continuing with other events to improve robustness.Modify the loop to handle errors gracefully:
for _, disputeGameEvent := range disputeGamesEvents { withdrawalValidation, err := v.L2Proxy.GetWithdrawalValidation(disputeGameEvent) if err != nil { - return nil, err + // Log the error and continue processing + fmt.Printf("Warning: %v\n", err) + continue } withdrawalValidations = append(withdrawalValidations, *withdrawalValidation) }op-monitorism/faultproof_withdrawals/validator/utils.go (1)
11-19: Remove commented out code.Instead of keeping commented code, it should be removed since it can be retrieved from version control if needed. This helps maintain a cleaner codebase.
op-monitorism/faultproof_withdrawals/validator/interfaces.go (2)
31-50: Consider extracting common methods to a base interface.Both L1ProxyInterface and L2ProxyInterface share several methods (LatestHeight, BlockByNumber, ChainID, Close, GetConnectionState). Consider creating a base interface to reduce duplication and improve maintainability.
type BaseProxyInterface interface { GetConnectionState() interface{} LatestHeight() (BlockInfo, error) BlockByNumber(blockNumber *big.Int) (BlockInfo, error) ChainID() (*big.Int, error) Close() } type L1ProxyInterface interface { BaseProxyInterface GetDisputeGamesEvents(start uint64, end uint64) ([]DisputeGameEvent, error) GetDisputeGameProxyUpdates(disputeGame *DisputeGame) (*DisputeGame, error) } type L2ProxyInterface interface { BaseProxyInterface GetWithdrawalValidation(disputeGameEvent DisputeGameEvent) (*WithdrawalValidationRef, error) }
56-63: Improve method documentation.The comment "Add these methods to the existing L1Proxy and L2Proxy structs" appears to be a development note. Consider replacing it with proper method documentation that describes the purpose and return values of GetConnectionState.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
op-monitorism/faultproof_withdrawals/monitor.go(2 hunks)op-monitorism/faultproof_withdrawals/state.go(10 hunks)op-monitorism/faultproof_withdrawals/utils.go(1 hunks)op-monitorism/faultproof_withdrawals/validator.go(1 hunks)op-monitorism/faultproof_withdrawals/validator/dispute_game_factory_helper.go(0 hunks)op-monitorism/faultproof_withdrawals/validator/fault_dispute_game_helper.go(0 hunks)op-monitorism/faultproof_withdrawals/validator/interfaces.go(1 hunks)op-monitorism/faultproof_withdrawals/validator/l1_proxy.go(1 hunks)op-monitorism/faultproof_withdrawals/validator/l2_proxy.go(1 hunks)op-monitorism/faultproof_withdrawals/validator/l2_to_l1_message_passer_helper.go(0 hunks)op-monitorism/faultproof_withdrawals/validator/op_node_helper.go(0 hunks)op-monitorism/faultproof_withdrawals/validator/optimism_portal2_helper.go(0 hunks)op-monitorism/faultproof_withdrawals/validator/proven_withdrawal_validator.go(0 hunks)op-monitorism/faultproof_withdrawals/validator/utils.go(1 hunks)
💤 Files with no reviewable changes (6)
- op-monitorism/faultproof_withdrawals/validator/l2_to_l1_message_passer_helper.go
- op-monitorism/faultproof_withdrawals/validator/dispute_game_factory_helper.go
- op-monitorism/faultproof_withdrawals/validator/fault_dispute_game_helper.go
- op-monitorism/faultproof_withdrawals/validator/op_node_helper.go
- op-monitorism/faultproof_withdrawals/validator/proven_withdrawal_validator.go
- op-monitorism/faultproof_withdrawals/validator/optimism_portal2_helper.go
🔇 Additional comments (8)
op-monitorism/faultproof_withdrawals/validator/l2_proxy.go (1)
146-154: Ensure context is correctly passed after refactoringAfter changing
ctxto be passed by value, verify that the context is correctly used in all RPC calls, especially inCallContextmethods.Double-check that all calls to
CallContextnow usel2Proxy.ctxwithout dereferencing and that the context propagation remains correct.op-monitorism/faultproof_withdrawals/state.go (2)
58-83: Ensure thread safety for shared metrics.The
Metricsstruct contains counters and gauges that may be updated concurrently. It's important to ensure that these metrics are thread-safe. Prometheus metrics are generally safe, but if you access or modify any shared data in custom methods, consider adding synchronization primitives.Verify that all metric updates are thread-safe and that there's no concurrent access to shared resources without proper synchronization.
325-330: Check for possible duplication of L1 and L2 node connection failure metrics.Lines 325-330 introduce counters for L1 and L2 node connection failures. Ensure that these metrics are not duplicated elsewhere and that they accurately represent the connection failures. Duplication can lead to misleading metrics.
op-monitorism/faultproof_withdrawals/validator.go (1)
10-13: Ensure interfaces are correctly implemented.The
WithdrawalValidatorstruct uses interfacesL1ProxyInterfaceandL2ProxyInterface. Verify that these interfaces are properly defined and that the methods used (GetDisputeGamesEventsandGetWithdrawalValidation) are correctly implemented.Confirm that
validator.L1ProxyInterfaceandvalidator.L2ProxyInterfaceinclude the required methods and that all implementations satisfy these interfaces.op-monitorism/faultproof_withdrawals/validator/utils.go (2)
21-25: LGTM! Well-structured BlockInfo definition.The struct is well-documented and the field types are appropriate for their purposes.
27-30: LGTM! Clear String method implementation.The method provides a clear string representation of BlockInfo with both fields properly formatted.
op-monitorism/faultproof_withdrawals/validator/interfaces.go (2)
52-54: LGTM! Good use of compile-time interface verification.The type assertions ensure that L1Proxy and L2Proxy properly implement their respective interfaces.
7-29: LGTM! Well-designed L1ProxyInterface.The interface provides a comprehensive set of methods for L1 chain interactions with clear signatures and good documentation.
Let's verify the error handling consistency across implementations:
✅ Verification successful
Error handling verified and consistent across L1ProxyInterface usage
The implementation shows consistent and robust error handling patterns:
- All interface method calls are properly checked for errors
- Errors are appropriately propagated with context
- No silent error drops found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for L1ProxyInterface method implementations to verify error handling ast-grep --pattern 'func \($_\) $_\($_\) \($_, error\) { $$$ return $_, nil }'Length of output: 84
Script:
#!/bin/bash # First, find files that might contain the implementation rg -l "L1ProxyInterface" --type go # Then look for the actual implementation rg "type \w+\s+struct.*{[\s\S]*}|func \([^)]+\) \w+\([^)]*\) \([^)]*error\)" --type go -A 5Length of output: 507
Script:
#!/bin/bash # Look for struct definitions and method implementations echo "=== Searching in validator.go ===" cat op-monitorism/faultproof_withdrawals/validator.go echo -e "\n=== Searching in utils.go ===" cat op-monitorism/faultproof_withdrawals/utils.go echo -e "\n=== Searching in monitor.go ===" cat op-monitorism/faultproof_withdrawals/monitor.goLength of output: 16182
| block, error := l1Proxy.BlockByNumber(big.NewInt(cfg.StartingL1BlockHeight)) | ||
| if error != nil { | ||
| return validator.BlockInfo{}, fmt.Errorf("failed to get block by number: %w", error) | ||
| } | ||
|
|
||
| return block, error |
There was a problem hiding this comment.
Avoid using 'error' as a variable name
Using error as a variable name shadows the built-in error type and can lead to confusion.
Apply this diff to rename the variable:
-block, error := l1Proxy.BlockByNumber(big.NewInt(cfg.StartingL1BlockHeight))
-if error != nil {
- return validator.BlockInfo{}, fmt.Errorf("failed to get block by number: %w", error)
+block, err := l1Proxy.BlockByNumber(big.NewInt(cfg.StartingL1BlockHeight))
+if err != nil {
+ return validator.BlockInfo{}, fmt.Errorf("failed to get block by number: %w", err)
}
-return block, error
+return block, err📝 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.
| block, error := l1Proxy.BlockByNumber(big.NewInt(cfg.StartingL1BlockHeight)) | |
| if error != nil { | |
| return validator.BlockInfo{}, fmt.Errorf("failed to get block by number: %w", error) | |
| } | |
| return block, error | |
| block, err := l1Proxy.BlockByNumber(big.NewInt(cfg.StartingL1BlockHeight)) | |
| if err != nil { | |
| return validator.BlockInfo{}, fmt.Errorf("failed to get block by number: %w", err) | |
| } | |
| return block, err |
| "github.com/ethereum/go-ethereum/log" | ||
| ) | ||
|
|
||
| // getBlockAtApproximateTimeBinarySearch finds the block number corresponding to the timestamp from two weeks ago using a binary search approach. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix function comment to match Go conventions
The comment for GetBlockAtApproximateTimeBinarySearch should start with the function name and provide an accurate description.
Apply this diff to correct the comment:
-// getBlockAtApproximateTimeBinarySearch finds the block number corresponding to the timestamp from two weeks ago using a binary search approach.
+// GetBlockAtApproximateTimeBinarySearch finds the block number corresponding to a timestamp from a specified number of hours in the past using a binary search approach.📝 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.
| // getBlockAtApproximateTimeBinarySearch finds the block number corresponding to the timestamp from two weeks ago using a binary search approach. | |
| // GetBlockAtApproximateTimeBinarySearch finds the block number corresponding to a timestamp from a specified number of hours in the past using a binary search approach. |
| ctx *context.Context | ||
| l2NodeClient *ethclient.Client // The op-node (consensus) client. | ||
| l2GethClient *ethclient.Client // The op-geth client. | ||
| rpc_l2Client *rpc.Client // The RPC client for the L2 node. | ||
| l2ToL1MessagePasser *l2.L2ToL1MessagePasser | ||
| ConnectionState *L2ConnectionState | ||
| } | ||
|
|
||
| type L2ConnectionState struct { | ||
| ProxyConnection uint64 | ||
| ProxyConnectionFailed uint64 | ||
| } | ||
|
|
||
| // OutputResponse represents the response structure for output-related data. | ||
| type OutputResponse struct { | ||
| Version string `json:"version"` // The version of the output. | ||
| OutputRoot string `json:"outputRoot"` // The output root associated with the response. | ||
| } | ||
|
|
||
| func NewL2Proxy(ctx *context.Context, l2GethURL string, l2NodeURL string) (*L2Proxy, error) { | ||
| connectionState := &L2ConnectionState{ | ||
| ProxyConnection: 0, | ||
| ProxyConnectionFailed: 0, | ||
| } | ||
|
|
||
| l2GethClient, err := ethclient.Dial(l2GethURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to dial l2: %w", err) | ||
| } | ||
| l2NodeClient, err := ethclient.Dial(l2NodeURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to dial l2: %w", err) | ||
| } | ||
|
|
||
| connectionState.ProxyConnection++ | ||
| l2ToL1MessagePasser, err := l2.NewL2ToL1MessagePasser(predeploys.L2ToL1MessagePasserAddr, l2GethClient) | ||
| if err != nil { | ||
| connectionState.ProxyConnectionFailed++ | ||
| return nil, fmt.Errorf("failed to bind to L2ToL1MessagePasser: %w", err) | ||
| } | ||
|
|
||
| rpc_l2Client := l2NodeClient.Client() | ||
|
|
||
| return &L2Proxy{ | ||
| l2NodeClient: l2NodeClient, | ||
| l2GethClient: l2GethClient, | ||
| rpc_l2Client: rpc_l2Client, | ||
| l2ToL1MessagePasser: l2ToL1MessagePasser, | ||
| ctx: ctx, | ||
| ConnectionState: connectionState, | ||
| }, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass context by value instead of pointer
In Go, it's idiomatic to pass context.Context by value. Storing it as a pointer can lead to unexpected behavior and complicates dereferencing.
Apply this diff:
-type L2Proxy struct {
- ctx *context.Context
+type L2Proxy struct {
+ ctx context.Context
// other fields...
}
-func NewL2Proxy(ctx *context.Context, l2GethURL string, l2NodeURL string) (*L2Proxy, error) {
+func NewL2Proxy(ctx context.Context, l2GethURL string, l2NodeURL string) (*L2Proxy, error) {
// ...
return &L2Proxy{
- ctx: ctx,
+ ctx: ctx,
// other fields...
}, nil
}And update all instances of *l2Proxy.ctx to l2Proxy.ctx throughout the file.
📝 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.
| ctx *context.Context | |
| l2NodeClient *ethclient.Client // The op-node (consensus) client. | |
| l2GethClient *ethclient.Client // The op-geth client. | |
| rpc_l2Client *rpc.Client // The RPC client for the L2 node. | |
| l2ToL1MessagePasser *l2.L2ToL1MessagePasser | |
| ConnectionState *L2ConnectionState | |
| } | |
| type L2ConnectionState struct { | |
| ProxyConnection uint64 | |
| ProxyConnectionFailed uint64 | |
| } | |
| // OutputResponse represents the response structure for output-related data. | |
| type OutputResponse struct { | |
| Version string `json:"version"` // The version of the output. | |
| OutputRoot string `json:"outputRoot"` // The output root associated with the response. | |
| } | |
| func NewL2Proxy(ctx *context.Context, l2GethURL string, l2NodeURL string) (*L2Proxy, error) { | |
| connectionState := &L2ConnectionState{ | |
| ProxyConnection: 0, | |
| ProxyConnectionFailed: 0, | |
| } | |
| l2GethClient, err := ethclient.Dial(l2GethURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to dial l2: %w", err) | |
| } | |
| l2NodeClient, err := ethclient.Dial(l2NodeURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to dial l2: %w", err) | |
| } | |
| connectionState.ProxyConnection++ | |
| l2ToL1MessagePasser, err := l2.NewL2ToL1MessagePasser(predeploys.L2ToL1MessagePasserAddr, l2GethClient) | |
| if err != nil { | |
| connectionState.ProxyConnectionFailed++ | |
| return nil, fmt.Errorf("failed to bind to L2ToL1MessagePasser: %w", err) | |
| } | |
| rpc_l2Client := l2NodeClient.Client() | |
| return &L2Proxy{ | |
| l2NodeClient: l2NodeClient, | |
| l2GethClient: l2GethClient, | |
| rpc_l2Client: rpc_l2Client, | |
| l2ToL1MessagePasser: l2ToL1MessagePasser, | |
| ctx: ctx, | |
| ConnectionState: connectionState, | |
| }, nil | |
| ctx context.Context | |
| l2NodeClient *ethclient.Client // The op-node (consensus) client. | |
| l2GethClient *ethclient.Client // The op-geth client. | |
| rpc_l2Client *rpc.Client // The RPC client for the L2 node. | |
| l2ToL1MessagePasser *l2.L2ToL1MessagePasser | |
| ConnectionState *L2ConnectionState | |
| } | |
| type L2ConnectionState struct { | |
| ProxyConnection uint64 | |
| ProxyConnectionFailed uint64 | |
| } | |
| // OutputResponse represents the response structure for output-related data. | |
| type OutputResponse struct { | |
| Version string `json:"version"` // The version of the output. | |
| OutputRoot string `json:"outputRoot"` // The output root associated with the response. | |
| } | |
| func NewL2Proxy(ctx context.Context, l2GethURL string, l2NodeURL string) (*L2Proxy, error) { | |
| connectionState := &L2ConnectionState{ | |
| ProxyConnection: 0, | |
| ProxyConnectionFailed: 0, | |
| } | |
| l2GethClient, err := ethclient.Dial(l2GethURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to dial l2: %w", err) | |
| } | |
| l2NodeClient, err := ethclient.Dial(l2NodeURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to dial l2: %w", err) | |
| } | |
| connectionState.ProxyConnection++ | |
| l2ToL1MessagePasser, err := l2.NewL2ToL1MessagePasser(predeploys.L2ToL1MessagePasserAddr, l2GethClient) | |
| if err != nil { | |
| connectionState.ProxyConnectionFailed++ | |
| return nil, fmt.Errorf("failed to bind to L2ToL1MessagePasser: %w", err) | |
| } | |
| rpc_l2Client := l2NodeClient.Client() | |
| return &L2Proxy{ | |
| l2NodeClient: l2NodeClient, | |
| l2GethClient: l2GethClient, | |
| rpc_l2Client: rpc_l2Client, | |
| l2ToL1MessagePasser: l2ToL1MessagePasser, | |
| ctx: ctx, | |
| ConnectionState: connectionState, | |
| }, nil |
| func NewL1Proxy(ctx *context.Context, l1GethURL string, optimismPortalAddress common.Address) (*L1Proxy, error) { | ||
| connectionState := &L1ConnectionState{ | ||
| ProxyConnection: 0, | ||
| ProxyConnectionFailed: 0, | ||
| } | ||
|
|
||
| l1Client, err := ethclient.Dial(l1GethURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to dial l1: %w", err) | ||
| } | ||
|
|
||
| connectionState.ProxyConnection++ | ||
| optimismPortal2, err := l1.NewOptimismPortal2(optimismPortalAddress, l1Client) | ||
| if err != nil { | ||
| connectionState.ProxyConnectionFailed++ | ||
| return nil, fmt.Errorf("failed to create new OptimismPortal2 instance error:%w", err) | ||
| } | ||
|
|
||
| return &L1Proxy{ | ||
| ctx: ctx, | ||
| l1GethClient: l1Client, | ||
| optimismPortal2: optimismPortal2, | ||
| ConnectionState: connectionState, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid passing context as a pointer.
In Go, it's idiomatic to pass context.Context by value, not by pointer. Modifying the function signature to accept a value can prevent inadvertent nil pointer dereferences and aligns with best practices.
Apply this diff to adjust the function signature and relevant struct fields:
-func NewL1Proxy(ctx *context.Context, l1GethURL string, optimismPortalAddress common.Address) (*L1Proxy, error) {
+func NewL1Proxy(ctx context.Context, l1GethURL string, optimismPortalAddress common.Address) (*L1Proxy, error) {
...
return &L1Proxy{
- ctx: ctx,
+ ctx: &ctx,
l1GethClient: l1Client,
optimismPortal2: optimismPortal2,
ConnectionState: connectionState,
}Also, update the L1Proxy struct to store ctx as a value:
type L1Proxy struct {
- ctx *context.Context
+ ctx context.Context
l1GethClient *ethclient.Client
optimismPortal2 *l1.OptimismPortal2
ConnectionState *L1ConnectionState
}Committable suggestion skipped: line range outside the PR's diff.
Description
Tests
Additional context
Metadata