chore: suggest using errors.New instead of fmt.Errorf with no parameters #1723
Conversation
Signed-off-by: liuchuan <[email protected]>
WalkthroughReplaced several fmt.Errorf(...) calls with errors.New(...) across three files, adding the errors import where needed. Error messages and control flow remain unchanged; no public APIs were modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (4)
rollup/internal/controller/relayer/l2_relayer_sanity.go (3)
66-66: Replace remaining fmt.Errorf calls without formatting with errors.New for consistencyThere are still several fmt.Errorf sites that don’t use formatting directives. Converting them improves clarity and follows the PR objective.
Apply this diff:
--- a/rollup/internal/controller/relayer/l2_relayer_sanity.go +++ b/rollup/internal/controller/relayer/l2_relayer_sanity.go @@ - return nil, fmt.Errorf("failed to type assert version to uint8") + return nil, errors.New("failed to type assert version to uint8") @@ - return nil, fmt.Errorf("failed to type assert parentBatchHash to [32]uint8") + return nil, errors.New("failed to type assert parentBatchHash to [32]uint8") @@ - return nil, fmt.Errorf("failed to type assert lastBatchHash to [32]uint8") + return nil, errors.New("failed to type assert lastBatchHash to [32]uint8") @@ - return fmt.Errorf("no batches to validate") + return errors.New("no batches to validate") @@ - return fmt.Errorf("chunk is nil") + return errors.New("chunk is nil") @@ - return fmt.Errorf("no blobs provided") + return errors.New("no blobs provided")Also applies to: 71-71, 77-77, 166-166, 289-289, 340-340
169-176: Avoid a potential index-out-of-range panic when the first batch has zero chunksfirstChunk := batchesToValidate[0].Chunks[0] can panic if the first batch has an empty Chunks slice. You do check for empty chunks later per-batch, but this early access precedes that guard.
Minimal guard:
@@ - // Get previous chunk for continuity check - firstChunk := batchesToValidate[0].Chunks[0] + // Get previous chunk for continuity check + if len(batchesToValidate[0].Chunks) == 0 { + return fmt.Errorf("batch %d has no chunks", batchesToValidate[0].Batch.Index) + } + firstChunk := batchesToValidate[0].Chunks[0]
4-4: Refactor static fmt.Errorf calls to errors.NewA repo-wide scan uncovered 14 instances where fmt.Errorf is used with a plain string literal (no formatting directives). These should be replaced with errors.New("…") for clarity and to avoid unnecessary formatting overhead. Please update each occurrence (adding an
import "errors"if needed):
- rollup/internal/controller/relayer/full_recovery.go:243
- rollup/internal/controller/relayer/l2_relayer_sanity.go:66, 71, 77, 166, 289, 340
- rollup/internal/controller/permissionless_batches/minimal_recovery.go:183
- rollup/internal/controller/relayer/l2_relayer.go:296, 1039, 1269
- coordinator/internal/logic/provertask/prover_task.go:197
- coordinator/internal/controller/api/l2_syncer.go:30
- bridge-history-api/internal/controller/fetcher/l1_fetcher.go:60
Example:
- return fmt.Errorf("no finalized batches to process") + return errors.New("no finalized batches to process")rollup/cmd/permissionless_batches/app/app.go (1)
55-67: Optional: extract config validation into a helper for readabilityNot required for this PR, but action is getting dense. Consider a small validateRecoveryConfig(cfg) helper to keep action focused on orchestration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
rollup/cmd/permissionless_batches/app/app.go(2 hunks)rollup/internal/controller/relayer/l2_relayer_sanity.go(2 hunks)rollup/internal/controller/sender/transaction_signer.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/cmd/permissionless_batches/app/app.go (1)
rollup/internal/config/recovery.go (1)
RecoveryConfig(3-14)
🔇 Additional comments (4)
rollup/internal/controller/relayer/l2_relayer_sanity.go (2)
4-4: LGTM: importing errors is appropriate for static error valuesThe file still uses fmt for wrapped/parameterized errors elsewhere, so both imports are necessary.
22-22: LGTM: use errors.New for a static messageSwitching from fmt.Errorf to errors.New here is correct and aligns with the PR goal. No behavior change.
rollup/cmd/permissionless_batches/app/app.go (1)
5-5: LGTM: consistent use of errors.New for static validation errorsThese config sanity checks all return static error strings, so switching to errors.New is correct and aligns with the PR objective. fmt remains needed elsewhere for wrapped/parameterized errors and printing.
Also applies to: 57-57, 61-61, 63-63, 66-66
rollup/internal/controller/sender/transaction_signer.go (1)
5-5: LGTM: swapped to errors.New where no formatting is used
- Importing errors is necessary and retained alongside fmt for wrapped/parameterized errors.
- Using errors.New for the empty signer address and unknown signer type paths is correct and matches the PR goal.
Also applies to: 55-55, 98-98
2 participants
chore: suggest using errors.New instead of fmt.Errorf with no parameters
Summary by CodeRabbit