server/v2 start commands and helpers

[julienrbrt]

Currently, the SDK provides helpers functions for applications to use for starting the app and cometbft:

• cosmos-sdk/server/util.go

  Line 327 in defab1a

  func AddCommands[T types.Application](rootCmd *cobra.Command, appCreator types.AppCreator[T], addStartFlags types.ModuleInitFlags) {

That function is then used by the app developer, who does not need to care about wiring a start command.

• cosmos-sdk/simapp/simd/cmd/commands.go

  Line 54 in b304cf7

  server.AddCommands(rootCmd, newApp, func(startCmd *cobra.Command) {})

We should provide a start command for the comet server (equivalent of https://github.com/cosmos/cosmos-sdk/blob/defab1a1a1520cd6dd0d80768d345560d7c99663/server/start.go) and add a helper in server/v2 to combine all server commands (equivalent server.AddCommands).

Additionally, the SDK provides another helper for config management:

cosmos-sdk/server/util.go

Line 102 in defab1a

func InterceptConfigsPreRunHandler(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig interface{}, cmtConfig *cmtcfg.Config) error {

• Abstract the config creation (equivalent server.InterceptConfigsPreRunHandler). This helper should create the config when it doesn't exist and provide a viper with all the config values.

[kocubinski]

One challenge with this work is maintaining compatibility in command handlers when a server command context is created in server v2 vs v1. See usages of server.GetServerContextFromCmd (30 usages in the SDK).

This fn expects server v1 Context to be set under v1 ServerContextKey and will err/panic unless we treat this condition carefully. We talked briefly on Thursday an proposed that we do roughly the following:

1. Audit usages of GetServerContextFromCmd in the SDK to determine which of the context fields are actually used from this struc. We suspect many usages are expecting a viper instance.
2. Transition from one contextKey defined in server/v1 to many defined in core, e.g. ViperContextKey for the viper object. This separates server v1 / v2 concerns from anywhere but server packages.
   2a) Consider if the cmtconfig.Config struct is really needed in this context, which fields are we actually using? It should be avoided in server v2.
3. Refactor all server v1 code and GetServerContextFromCmd usages to suit.

All this work is done in server v1 in preparation for implementing server v2 command handler which will use the same context keys. Once completed the v2 command handlers should be pretty simple.

[hieuvubk]

serverContext.Config is mainly used in genutil's cmds.
In case we want to completely remove it from server/v2, what if we get the config directly from viper since config.toml was added to both viper config and serverContext. It can simplify serverCtx of both v1 & v2.
The disadvantage of this method is that we have to access the entry of the toml file instead of calling through an object struct.
Also, Looked through config.toml & app.toml to see if using viper.Get() would duplicate keys but no.
cc @tac0turtle @kocubinski @julienrbrt

[julienrbrt]

serverContext.Config is mainly used in genutil's cmds. In case we want to completely remove it from server/v2, what if we get the config directly from viper since config.toml was added to both viper config and serverContext. It can simplify serverCtx of both v1 & v2. The disadvantage of this method is that we have to access the entry of the toml file instead of calling through an object struct. Also, Looked through config.toml & app.toml to see if using viper.Get() would duplicate keys but no. cc @tac0turtle @kocubinski @julienrbrt

Yes, this is what I was trying to convey last meeting. Just viper + logger should be sufficient. We only need to have context keys in core (for logger and viper).