-
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
feat(server): Allow calling back into the application struct in PostSetup. #19455
Conversation
WalkthroughWalkthroughThe update introduces generics to enhance type safety and extensibility across various components of the codebase. It applies a generic type Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (10)
- client/pruning/main.go (2 hunks)
- client/snapshot/cmd.go (1 hunks)
- client/snapshot/export.go (1 hunks)
- client/snapshot/restore.go (1 hunks)
- server/cmt_cmds.go (1 hunks)
- server/rollback.go (1 hunks)
- server/start.go (10 hunks)
- server/types/app.go (1 hunks)
- server/util.go (2 hunks)
- server/util_test.go (9 hunks)
Additional comments: 24
client/snapshot/cmd.go (1)
- 11-11: The introduction of the generic type
T
fortypes.Application
in theCmd
function signature is a significant improvement, enhancing type safety and flexibility. This change allows for more generic and reusable code structures, aligning with the PR's objectives to improve the flexibility and extensibility of the application's initialization process.client/snapshot/restore.go (1)
- 18-18: The update to include a generic type
T
in theRestoreSnapshotCmd
function signature is well-implemented, enhancing type safety and flexibility. This change is consistent with the PR's goal of making the application's setup process more adaptable and promotes better code reusability.client/snapshot/export.go (1)
- 14-14: The modification to include a generic type
T
in theExportSnapshotCmd
function signature is correctly implemented, enhancing type safety and flexibility. This change aligns with the PR's objectives and promotes better code reusability and adaptability.server/rollback.go (1)
- 13-13: The update to include a generic type
T
in theNewRollbackCmd
function signature is well-implemented, enhancing type safety and flexibility. This change is consistent with the PR's goal of making the application's setup process more adaptable and promotes better code reusability.server/types/app.go (1)
- 69-69: The modification to include a generic type
T
in theAppCreator
type declaration is correctly implemented, enhancing type safety and flexibility. This change aligns with the PR's objectives and promotes better code reusability and adaptability in the application's initialization process.client/pruning/main.go (1)
- 26-26: The update to include a generic type
T
in theCmd
function signature for pruning is well-implemented, enhancing type safety and flexibility. This change is consistent with the PR's goal of making the application's setup process more adaptable and promotes better code reusability.server/cmt_cmds.go (1)
- 359-359: The introduction of the generic type
T
in theBootstrapStateCmd
function signature is a significant improvement, enhancing type safety and flexibility. This change allows for more generic and reusable code structures, aligning with the PR's objectives to improve the flexibility and extensibility of the application's initialization process.server/util_test.go (8)
- 54-54: The usage of
server.StartCmd[types.Application](nil)
introduces a generic type fortypes.Application
, enhancing type safety and flexibility. This change aligns with the PR's objective to improve the server component's extensibility. Ensure that thenil
argument passed here is intended and that any required initialization for theApplication
type is handled appropriately elsewhere in the codebase.- 133-133: Similar to the previous comment, the usage of
server.StartCmd[types.Application](nil)
here is consistent with the PR's goal. It's crucial to verify that passingnil
as the argument does not lead to unintended behavior during the execution of these tests.- 176-176: Again, the use of
server.StartCmd[types.Application](nil)
is observed. As before, ensure that this usage is consistent with the intended design and that thenil
argument does not cause issues.- 198-198: The repeated use of
server.StartCmd[types.Application](nil)
in test functions demonstrates a consistent approach to utilizing generic types across the test suite. It's important to maintain this consistency and ensure that thenil
argument is appropriate for the tests' context.- 228-228: The usage of
server.StartCmd[types.Application](nil)
here is in line with the PR's objectives. As with previous instances, confirm that thenil
argument is suitable and does not compromise the tests' integrity.- 318-318: The introduction of
server.StartCmd[types.Application](nil)
in thenewPrecedenceCommon
function further emphasizes the PR's focus on enhancing type safety and flexibility through generics. This consistent application of generics across the test suite is commendable.- 435-435: The use of
server.StartCmd[types.Application](nil)
in this context is consistent with the pattern observed throughout the file. As always, ensure that the implications of thenil
argument are fully considered and that it aligns with the test's requirements.- 475-475: The final observed usage of
server.StartCmd[types.Application](nil)
in this file. This change is part of the broader effort to introduce generics and enhance the flexibility of the server component. As with other instances, the use ofnil
should be carefully evaluated to ensure it meets the test's needs.server/util.go (2)
- 327-327: The introduction of a generic type
T
in theAddCommands
function signature enhances type safety and flexibility. This change allows the function to accept any type that implements thetypes.Application
interface, making the code more reusable and adaptable to different application types. The implementation appears correct and follows best practices for generic programming in Go.- 358-358: Similarly, the use of a generic type
T
in theAddTestnetCreatorCommand
function signature is a positive change. It increases the function's flexibility by allowing it to handle various types ofAppCreator
, which is beneficial for type safety and code reusability. The modification is implemented correctly and aligns with best practices for leveraging generics in Go.server/start.go (7)
- 111-119: The introduction of the generic type
T
inStartCmdOptions
is a significant enhancement for flexibility. However, ensure that all implementations usingStartCmdOptions
are updated to leverage generics properly. This change aligns well with the PR's goal of enhancing the server component's extensibility.- 126-127: The
StartCmd
function now accepts a generic type, which is a good use of Go's type parameters feature introduced in Go 1.18. This change allows for more type-safe and flexible application structures.- 132-132: In
StartCmdWithOptions
, the check fornil
DBOpener
and its default assignment is a good practice. It ensures that there's always a database opener function available, preventing potential nil pointer dereferences.- 221-232: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [229-292]
The separation of
startStandAlone
andstartInProcess
functions for different modes of operation is a good architectural decision. It enhances readability and maintainability. Ensure that all new configurations and flags introduced are documented and tested.
- 339-339: The conditional execution of
PostSetup
instartInProcess
is a crucial addition for extending the initialization process. It's important to ensure that all implementations ofPostSetup
handle errors gracefully and do not leave the application in an inconsistent state if an error occurs.- 600-608: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [590-605]
The
startApp
function's adaptation to support generics and the introduction oftestnetify
logic are significant changes. Ensure comprehensive testing around these changes, especially thetestnetify
logic, to prevent any unintended side effects on the application's state.
- 936-936: Adding start node flags with the introduction of generics is handled well. It's important to ensure that all new flags introduced are documented and that their usage is clear to end-users.
server/util_test.go
Outdated
@@ -20,6 +20,7 @@ import ( | |||
"github.com/cosmos/cosmos-sdk/client/flags" | |||
"github.com/cosmos/cosmos-sdk/server" | |||
"github.com/cosmos/cosmos-sdk/server/config" | |||
"github.com/cosmos/cosmos-sdk/server/types" |
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.
The import statement for types
is duplicated as both github.com/cosmos/cosmos-sdk/server/types
and servertypes "github.com/cosmos/cosmos-sdk/server/types"
. This redundancy could be eliminated to improve code clarity and maintainability.
- "github.com/cosmos/cosmos-sdk/server/types"
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
"github.com/cosmos/cosmos-sdk/server/types" |
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.
Good point from the bot
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.
@julienrbrt: Thank you for acknowledging the point! If there's anything more I can help with, please let me know.
\ (^-^) /
return cmd | ||
} | ||
|
||
func start(svrCtx *Context, clientCtx client.Context, appCreator types.AppCreator, withCmt bool, opts StartCmdOptions) error { | ||
func start[T types.Application](svrCtx *Context, clientCtx client.Context, appCreator types.AppCreator[T], withCmt bool, opts StartCmdOptions[T]) error { | ||
svrCfg, err := getAndValidateConfig(svrCtx) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
app, appCleanupFn, err := startApp(svrCtx, appCreator, opts) | ||
app, appCleanupFn, err := startApp[T](svrCtx, appCreator, opts) | ||
if err != nil { | ||
return 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.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [204-226]
The start
function's adaptation to use generics is well-implemented. However, consider adding error handling for the startStandAlone
and startInProcess
calls to ensure any errors during the startup process are appropriately managed.
- return startStandAlone[T](svrCtx, svrCfg, clientCtx, app, metrics, opts)
+ if err := startStandAlone[T](svrCtx, svrCfg, clientCtx, app, metrics, opts); err != nil {
+ return err
+ }
- return startInProcess[T](svrCtx, svrCfg, clientCtx, app, metrics, opts)
+ if err := startInProcess[T](svrCtx, svrCfg, clientCtx, app, metrics, opts); err != nil {
+ return err
+ }
|
||
// testnetify modifies both state and blockStore, allowing the provided operator address and local validator key to control the network | ||
// that the state in the data folder represents. The chainID of the local genesis file is modified to match the provided chainID. | ||
func testnetify(ctx *Context, home string, testnetAppCreator types.AppCreator, db dbm.DB, traceWriter io.WriteCloser) (types.Application, error) { | ||
func testnetify[T types.Application](ctx *Context, home string, testnetAppCreator types.AppCreator[T], db dbm.DB, traceWriter io.WriteCloser) (*T, error) { | ||
config := ctx.Config | ||
|
||
newChainID, ok := ctx.Viper.Get(KeyNewChainID).(string) |
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.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [625-932]
The InPlaceTestnetCreator
and testnetify
functions introduce complex logic for creating a testnet from the current state. It's crucial to ensure that these functions are well-documented and that their impact on the application's state is clearly understood by developers. Consider adding more inline comments explaining the steps involved, especially in the testnetify
function, to improve maintainability.
+ // Explanation of the logic and its impact on the application's state
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.
utACK!
nit: changelog entry
Curious, how do you plan on using post-setup? What do you plan on executing in that handler?
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.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- client/pruning/main.go (1 hunks)
- server/start.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- client/pruning/main.go
- server/start.go
@@ -271,6 +273,12 @@ func startStandAlone(svrCtx *Context, svrCfg serverconfig.Config, clientCtx clie | |||
return err | |||
} | |||
|
|||
if opts.PostSetup != 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.
if opts.PostSetup != nil { | |
if opts.PostSetupStandalone != 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.
gently ping on this one
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.
@itsdevbear, it should still be committed as you've forced pushed your last commit, which removed this.
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- server/start.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- server/start.go
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.
utACK.
Linter won't be happy unless you remove those duplicated imports tho.
a05b089
to
68e9430
Compare
fixed linter |
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- client/snapshot/cmd.go (1 hunks)
- client/snapshot/export.go (1 hunks)
- client/snapshot/restore.go (1 hunks)
- server/start.go (11 hunks)
Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- client/snapshot/cmd.go
- client/snapshot/export.go
- client/snapshot/restore.go
- server/start.go
babd6c7
to
ca37db5
Compare
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- client/snapshot/cmd.go (1 hunks)
- client/snapshot/export.go (1 hunks)
- client/snapshot/restore.go (1 hunks)
- server/start.go (11 hunks)
- server/util_test.go (8 hunks)
Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- client/snapshot/cmd.go
- client/snapshot/export.go
- client/snapshot/restore.go
- server/start.go
- server/util_test.go
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- server/start.go (11 hunks)
- server/util_test.go (8 hunks)
Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- server/start.go
- server/util_test.go
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit