fix(rollup): only enable chain-monitor in rollup-relayer #1569
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 0
🧹 Outside diff range and nitpick comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
Nil check needed for chainMonitorClient in getBatchStatusByIndex
The code review reveals that
getBatchStatusByIndexdirectly useschainMonitorClientwithout checking if it's nil. While the client is initialized whenServiceTypeL2RollupRelayer && cfg.ChainMonitor.Enabledis true, the method should include a nil check to prevent panic when the client is not initialized.Key issues:
getBatchStatusByIndexdirectly usesr.chainMonitorClientwithout a nil check- The method is called in the codebase (as seen in the search results) and could panic if the client is not initialized
🔗 Analysis chain
Line range hint
157-161: Verify the usage of chainMonitorClient in getBatchStatusByIndex.The
getBatchStatusByIndexmethod assumeschainMonitorClientexists whencfg.ChainMonitor.Enabledis true. Let's verify all call sites handle the case where the client is nil.
Let me analyze the code further to verify the usage of
chainMonitorClientin the codebase.
Let me try a different approach to get the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of getBatchStatusByIndex to verify nil checks # Search for direct method calls rg -A 5 "getBatchStatusByIndex" # Search for chain monitor client usage rg -A 5 "chainMonitorClient"Length of output: 4386
Script:
#!/bin/bash # Search for the complete implementation of getBatchStatusByIndex ast-grep --pattern 'func (r *Layer2Relayer) getBatchStatusByIndex($$$) { $$$ }'Length of output: 83
Script:
#!/bin/bash # Get the complete implementation using ripgrep with more context rg -U --multiline --multiline-dotall "func \(r \*Layer2Relayer\) getBatchStatusByIndex.*?\n}" -A 50Length of output: 7995
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rollup/internal/controller/relayer/l2_relayer.go(1 hunks)
🔇 Additional comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
Line range hint 157-161: LGTM! The change correctly implements configuration decoupling.
The conditional initialization of chainMonitorClient now properly checks both the service type and enabled flag, which aligns with the PR objective of decoupling chain-monitor configurations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1569 +/- ##
========================================
Coverage 52.06% 52.06%
========================================
Files 157 157
Lines 12472 12472
========================================
Hits 6494 6494
Misses 5418 5418
Partials 560 560
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-actions
bot
dismissed stale reviews from georgehao and johnsonjie
via
f505231
colinlyguo
deleted the
fix-rollup-relayer-only-enable-chain-monitor-in-rollup-relayer
branch
Purpose or design rationale of this PR
Yet another config decoupling PR, before this PR, will cause a nil pointer error without chain-monitor configs, after this PR gas-oracle won't need chain-monitor configs in
config.json.PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit