-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Remove CometBFT Config from ServerContext #20311
Changes from 13 commits
7a6f8aa
620509e
67e13d1
16c293b
217c539
c46359d
251fa0c
95b2c29
467428e
b9a7278
4dcf9e6
e8d9242
5e1df8d
77c3bc1
081cc22
0b8808e
6c9992d
f63a54e
ef68e88
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 |
---|---|---|
@@ -1,6 +1,8 @@ | ||
package snapshot | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/spf13/cobra" | ||
|
||
"cosmossdk.io/log" | ||
|
@@ -17,13 +19,17 @@ | |
Args: cobra.NoArgs, | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
ctx := server.GetServerContextFromCmd(cmd) | ||
cfg, ok := ctx.GetConfig().(server.CometConfig) | ||
if !ok { | ||
return fmt.Errorf("Can not convert cometbft config") | ||
} | ||
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. Consider handling the type assertion failure more gracefully. Instead of returning an error immediately, you might log this incident or attempt a recovery strategy, depending on the application's criticality and design. |
||
|
||
height, err := cmd.Flags().GetInt64("height") | ||
if err != nil { | ||
return err | ||
} | ||
|
||
home := ctx.Config.RootDir | ||
home := cfg.RootDir | ||
db, err := openDB(home, server.GetAppDBBackend(ctx.Viper)) | ||
if err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||
package snapshot | ||||||
|
||||||
import ( | ||||||
"fmt" | ||||||
"path/filepath" | ||||||
"strconv" | ||||||
|
||||||
|
@@ -22,6 +23,10 @@ | |||||
Args: cobra.ExactArgs(2), | ||||||
RunE: func(cmd *cobra.Command, args []string) error { | ||||||
ctx := server.GetServerContextFromCmd(cmd) | ||||||
cfg, ok := ctx.GetConfig().(server.CometConfig) | ||||||
if !ok { | ||||||
return fmt.Errorf("Can not convert cometbft config") | ||||||
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. Error messages should start with a lowercase letter. - return fmt.Errorf("Can not convert cometbft config")
+ return fmt.Errorf("cannot convert cometbft config") According to Go conventions, error strings should not be capitalized unless beginning with proper nouns or acronyms. Committable suggestion
Suggested change
|
||||||
} | ||||||
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. Consider handling the type assertion failure more gracefully. Instead of returning an error immediately, you might log this incident or attempt a recovery strategy, depending on the application's criticality and design. |
||||||
|
||||||
height, err := strconv.ParseUint(args[0], 10, 64) | ||||||
if err != nil { | ||||||
|
@@ -32,7 +37,7 @@ | |||||
return err | ||||||
} | ||||||
|
||||||
home := ctx.Config.RootDir | ||||||
home := cfg.RootDir | ||||||
db, err := openDB(home, server.GetAppDBBackend(ctx.Viper)) | ||||||
if err != nil { | ||||||
return err | ||||||
|
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. We don't want anything comet specific in core. I would just replace this whole file by type LoggerContextKey struct{}
type ConfigContextKey struct{} |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,44 @@ | ||||||||||||||||
package context | ||||||||||||||||
|
||||||||||||||||
import ( | ||||||||||||||||
"fmt" | ||||||||||||||||
|
||||||||||||||||
"github.com/spf13/cobra" | ||||||||||||||||
"github.com/spf13/viper" | ||||||||||||||||
|
||||||||||||||||
"cosmossdk.io/log" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
type ContextKey string | ||||||||||||||||
|
||||||||||||||||
const ServerContextKey ContextKey = "server.context" | ||||||||||||||||
|
||||||||||||||||
type ServerContext interface { | ||||||||||||||||
GetLogger() log.Logger | ||||||||||||||||
GetViper() *viper.Viper | ||||||||||||||||
GetConfig() CometConfig | ||||||||||||||||
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. Ensure that the - GetConfig() CometConfig
+ GetConfig() interface{} Returning a specific type from an interface method can reduce flexibility. It's better to return an interface type and perform type assertions where this method is called. Committable suggestion
Suggested change
|
||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
type BaseConfig interface { | ||||||||||||||||
DBDir() string | ||||||||||||||||
GenesisFile() string | ||||||||||||||||
NodeKeyFile() string | ||||||||||||||||
PrivValidatorKeyFile() string | ||||||||||||||||
PrivValidatorStateFile() string | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
type CometConfig interface { | ||||||||||||||||
BaseConfig | ||||||||||||||||
CheckDeprecated() []string | ||||||||||||||||
SetRoot(root string) CometConfig | ||||||||||||||||
ValidateBasic() error | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
func GetServerContextFromCmd(cmd *cobra.Command) ServerContext { | ||||||||||||||||
if v := cmd.Context().Value(ServerContextKey); v != nil { | ||||||||||||||||
fmt.Println("serverCtxPtr", v) | ||||||||||||||||
serverCtxPtr := v.(ServerContext) | ||||||||||||||||
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. Use type assertion safely to avoid potential panics. - serverCtxPtr := v.(ServerContext)
+ serverCtxPtr, ok := v.(ServerContext)
+ if !ok {
+ return nil // or handle the error appropriately
+ } Direct type assertions without checking can lead to runtime panics if the assertion fails. It's safer to use the comma-ok idiom to assert types. Committable suggestion
Suggested change
|
||||||||||||||||
return serverCtxPtr | ||||||||||||||||
} | ||||||||||||||||
return nil | ||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -27,13 +27,18 @@ application. | |||||
RunE: func(cmd *cobra.Command, args []string) error { | ||||||
ctx := GetServerContextFromCmd(cmd) | ||||||
|
||||||
db, err := OpenDB(ctx.Config.RootDir, GetAppDBBackend(ctx.Viper)) | ||||||
config, ok := ctx.GetConfig().(CometConfig) | ||||||
if !ok { | ||||||
return fmt.Errorf("Can not convert cometbft config") | ||||||
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. Error messages should start with a lowercase letter. - return fmt.Errorf("Can not convert cometbft config")
+ return fmt.Errorf("cannot convert cometbft config") Following Go conventions, error strings should not be capitalized unless they begin with proper nouns or acronyms. Committable suggestion
Suggested change
|
||||||
} | ||||||
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. Consider handling the type assertion failure more gracefully. Instead of returning an error immediately, you might log this incident or attempt a recovery strategy, depending on the application's criticality and design. |
||||||
|
||||||
db, err := OpenDB(config.RootDir, GetAppDBBackend(ctx.Viper)) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
app := appCreator(ctx.Logger, db, nil, ctx.Viper) | ||||||
// rollback CometBFT state | ||||||
height, hash, err := cmtcmd.RollbackState(ctx.Config, removeBlock) | ||||||
height, hash, err := cmtcmd.RollbackState(config.Config, removeBlock) | ||||||
if err != nil { | ||||||
return fmt.Errorf("failed to rollback CometBFT state: %w", err) | ||||||
} | ||||||
|
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.
Error messages should start with a lowercase letter.
As per Go conventions, error strings should not be capitalized unless they begin with proper nouns or acronyms.
Committable suggestion