-
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
Changes from 4 commits
88c746a
fe2f122
5d471b5
058248a
c8c207c
ddab5ee
06c84bb
3d96b52
9faf4dc
8da9ef2
ba09fa0
e1856ab
ad5b9ca
70c7598
f75ecbf
90ed108
25e8991
7decd50
84a6198
56c8472
eb50987
ecd1b48
85b2975
327e74c
afc1eca
5a114e8
84d319d
1589f13
80f819f
d23d13a
a857bbd
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