-
Notifications
You must be signed in to change notification settings - Fork 230
refactor(apps): rollback cmd updates #2744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
Claude finished @julienrbrt's task —— View job Code Review Complete ✅I've performed a comprehensive code review of this PR. Overall, this is a well-executed refactoring that successfully removes unnecessary complexity and improves the rollback functionality. Summary of ChangesThis PR accomplishes two main goals:
Detailed Review✅ Positive Findings1. Code Quality & Architecture
2. Error Handling Improvements ⭐
3. State Management Improvements ⭐
4. Testing
5. Dependency Management
🔍 Areas for Consideration1. Error Handling in Rollback Commands In both if err := headerStore.Start(goCtx); err != nil {
return err // evm/single - good
}
defer headerStore.Stop(goCtx) // Stop errors ignored Recommendation: Consider capturing and logging Stop errors, especially if they might indicate resource leaks: defer func() {
if stopErr := headerStore.Stop(goCtx); stopErr != nil {
fmt.Printf("Warning: failed to stop header store: %v\n", stopErr)
}
}() 2. User Message Clarity ℹ️ Minor The success message at Current flow: fmt.Printf("Rolled back ev-node state to height %d\n", height)
if syncNode {
fmt.Println("Restart the node with the `--clear-cache` flag")
}
return errs // May contain errors Recommendation: Consider only printing success message if if errs == nil {
fmt.Printf("Rolled back ev-node state to height %d\n", height)
if syncNode {
fmt.Println("Restart the node with the `--clear-cache` flag")
}
}
return errs 3. Error Context in EVM Rollback In evm/single (good): if err := headerStore.Start(goCtx); err != nil {
return fmt.Errorf("failed to start header store: %w", err)
} testapp (could be improved): if err := headerStore.Start(goCtx); err != nil {
return err // Less context
} Recommendation: Add error wrapping in testapp for consistency. 🎯 Security & Robustness✅ No security concerns identified
📊 Testing Coverage✅ Well-tested changes
🎨 Code Style✅ Follows Go best practices
📝 Documentation✅ Comments and function docs are clear and accurate Final AssessmentOverall: LGTM with minor suggestions ✅ This is a solid refactoring that:
The suggestions above are minor improvements and not blockers. The PR is ready to merge in its current state. Files Reviewed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2744 +/- ##
==========================================
- Coverage 62.17% 62.16% -0.01%
==========================================
Files 79 79
Lines 8497 8485 -12
==========================================
- Hits 5283 5275 -8
- Misses 2721 2723 +2
+ Partials 493 487 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* main: feat(store)!: add batching for atomicity (#2746) refactor(apps): rollback cmd updates (#2744) chore: add makefile for tools (#2743) chore: fix markdown lint (#2742) build(deps): Bump the all-go group across 5 directories with 6 updates (#2738) refactor(block): improve cancellation (#2741) chore: make the prompt go oriented (#2739) perf(block): use `sync/atomic` instead of mutexes (#2735)
Overview
With the fix from #2736, the
--skip-p2p-stores
flag isn't needed.Additionally, this bump go-header to the latest changes from celestiaorg/go-header#347