-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor(server/v2): clean up storage use and config #22008
base: main
Are you sure you want to change the base?
Changes from 5 commits
88c746a
fe2f122
5d471b5
058248a
c8c207c
ddab5ee
06c84bb
3d96b52
9faf4dc
8da9ef2
ba09fa0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,7 @@ func (s *Server[T]) ExportSnapshotCmd() *cobra.Command { | |
} | ||
|
||
// RestoreSnapshotCmd returns a command to restore a snapshot | ||
func (s *Server[T]) RestoreSnapshotCmd(newApp serverv2.AppCreator[T]) *cobra.Command { | ||
func (s *Server[T]) RestoreSnapshotCmd(rootStore storev2.Backend) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "restore <height> <format>", | ||
Short: "Restore app state from local snapshot", | ||
|
@@ -95,8 +95,6 @@ func (s *Server[T]) RestoreSnapshotCmd(newApp serverv2.AppCreator[T]) *cobra.Com | |
} | ||
|
||
logger := log.NewLogger(cmd.OutOrStdout()) | ||
app := newApp(logger, v) | ||
rootStore := app.GetStore().(storev2.RootStore) | ||
|
||
sm, err := createSnapshotsManager(cmd, v, logger, rootStore) | ||
if err != nil { | ||
|
@@ -350,7 +348,9 @@ func (s *Server[T]) LoadArchiveCmd() *cobra.Command { | |
} | ||
} | ||
|
||
func createSnapshotsManager(cmd *cobra.Command, v *viper.Viper, logger log.Logger, store storev2.RootStore) (*snapshots.Manager, error) { | ||
func createSnapshotsManager( | ||
cmd *cobra.Command, v *viper.Viper, logger log.Logger, store storev2.Backend, | ||
) (*snapshots.Manager, error) { | ||
kocubinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
home := v.GetString(serverv2.FlagHome) | ||
snapshotStore, err := snapshots.NewStore(filepath.Join(home, "data", "snapshots")) | ||
if err != nil { | ||
|
@@ -371,7 +371,11 @@ func createSnapshotsManager(cmd *cobra.Command, v *viper.Viper, logger log.Logge | |
} | ||
} | ||
|
||
sm := snapshots.NewManager(snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)), store.GetStateCommitment().(snapshots.CommitSnapshotter), store.GetStateStorage().(snapshots.StorageSnapshotter), nil, logger) | ||
sm := snapshots.NewManager( | ||
snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)), | ||
store.GetStateCommitment().(snapshots.CommitSnapshotter), | ||
store.GetStateStorage().(snapshots.StorageSnapshotter), | ||
nil, logger) | ||
Comment on lines
+374
to
+378
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle type assertions to prevent potential panics. At lines 376-377, the code uses unchecked type assertions. The Uber Go Style Guide recommends using the two-value form to handle cases where the assertion might fail, preventing runtime panics. Modify the code to handle type assertion failures: commitSnapshotter, ok := store.GetStateCommitment().(snapshots.CommitSnapshotter)
if !ok {
return nil, fmt.Errorf("store does not implement snapshots.CommitSnapshotter")
}
storageSnapshotter, ok := store.GetStateStorage().(snapshots.StorageSnapshotter)
if !ok {
return nil, fmt.Errorf("store does not implement snapshots.StorageSnapshotter")
}
sm := snapshots.NewManager(
snapshotStore,
snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
commitSnapshotter,
storageSnapshotter,
nil,
logger,
)
Comment on lines
+374
to
+378
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prevent potential runtime panic due to unchecked type assertions In the call to store.GetStateCommitment().(snapshots.CommitSnapshotter),
store.GetStateStorage().(snapshots.StorageSnapshotter), If Consider using the comma-ok idiom to safely perform the type assertions and handle any errors appropriately. Here's how you can modify the code: + commitSnapshotter, ok := store.GetStateCommitment().(snapshots.CommitSnapshotter)
+ if !ok {
+ return nil, fmt.Errorf("store's state commitment does not implement snapshots.CommitSnapshotter")
+ }
+ storageSnapshotter, ok := store.GetStateStorage().(snapshots.StorageSnapshotter)
+ if !ok {
+ return nil, fmt.Errorf("store's state storage does not implement snapshots.StorageSnapshotter")
+ }
- sm := snapshots.NewManager(
- snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
- store.GetStateCommitment().(snapshots.CommitSnapshotter),
- store.GetStateStorage().(snapshots.StorageSnapshotter),
- nil, logger)
+ sm := snapshots.NewManager(
+ snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)),
+ commitSnapshotter,
+ storageSnapshotter,
+ nil, logger) This change ensures that if the type assertions fail, a meaningful error is returned instead of causing a panic. |
||
return sm, nil | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle unchecked type assertions to prevent potential panics
The type assertions on
s.store.GetStateStorage()
ands.store.GetStateCommitment()
are unchecked and could lead to a panic if the assertions fail. It is recommended to perform type assertion checks to handle potential failures gracefully.Apply the following changes to handle potential type assertion failures:
This change ensures that the type assertions are checked, and appropriate error handling is in place to prevent unexpected panics.
📝 Committable suggestion