Skip to content
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

Server TOML config template can get out of sync with config.go #20097

Open
1 task done
gibson042 opened this issue Apr 19, 2024 · 4 comments
Open
1 task done

Server TOML config template can get out of sync with config.go #20097

gibson042 opened this issue Apr 19, 2024 · 4 comments

Comments

@gibson042
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

server/config/toml.go includes contents that are similar to those of server/config/config.go, but any synchronization between the two locations is manual so there are undesirable differences (and some undesirable similarities, such as using the CamelCase Go names to comment on TOML settings like halt-height).

To remedy this, I would recommend programmatically deriving the TOML template from the Go config structs and their doc comments in a build step. Here's a proof of concept that parses Go source text and emits TOML template text: https://go.dev/play/p/W74nda-DQKZ

Cosmos SDK Version

main

How to reproduce?

No response

@julienrbrt
Copy link
Member

julienrbrt commented Apr 19, 2024

Yes 100% agree, we are going to use struct annotations and github.com/pelletier/go-toml/v2 library for server/v2: https://github.com/cosmos/cosmos-sdk/blob/605b724/server/v2/api/grpc/config.go#L19-L34
I don't think it makes sense to refactor the server v0 config.

@julienrbrt julienrbrt removed the T:Bug label Apr 19, 2024
@julienrbrt
Copy link
Member

julienrbrt commented Apr 19, 2024

Could you please point me where you see a missing field between the config.go and the toml.go? I cannot find any.
Removing the bug label as it is more a suggestion / refactor request than a bug.

@julienrbrt julienrbrt changed the title [Bug]: server TOML config template is out of sync with config.go Server TOML config template can get out of sync with config.go Apr 19, 2024
@gibson042
Copy link
Contributor Author

Could you please point me where you see a missing field between the config.go and the toml.go? I cannot find any. Removing the bug label as it is more a suggestion / refactor request than a bug.

I don't see any missing fields, but I do see bugs:

  • Many TOML comments use Go field names. Example:
    # HaltHeight contains a non-zero block height at which a node will gracefully
    # halt and shutdown that can be used to assist upgrades and testing.
    #
    # Note: Commitment of state will be attempted on the corresponding block.
    halt-height = {{ .BaseConfig.HaltHeight }}
    (HaltHeight vs. halt-height)
  • Many TOML comments are missing from Go doc comments. Examples:
    • # These are applied if and only if the pruning strategy is custom.
      pruning-keep-recent = "{{ .BaseConfig.PruningKeepRecent }}"
      pruning-interval = "{{ .BaseConfig.PruningInterval }}"
      vs.
      Pruning string `mapstructure:"pruning"`
      PruningKeepRecent string `mapstructure:"pruning-keep-recent"`
      PruningInterval string `mapstructure:"pruning-interval"`
    • # AppDBBackend defines the database backend type to use for the application and snapshots DBs.
      # An empty string indicates that a fallback will be used.
      # The fallback is the db_backend value set in CometBFT's config.toml.
      app-db-backend = "{{ .BaseConfig.AppDBBackend }}"
      vs.
      // AppDBBackend defines the type of Database to use for the application and snapshots databases.
      // An empty string indicates that the CometBFT config's DBBackend value should be used.
      AppDBBackend string `mapstructure:"app-db-backend"`
    • # List of kv store keys to stream out via gRPC.
      # The store key names MUST match the module's StoreKey name.
      #
      # Example:
      # ["acc", "bank", "gov", "staking", "mint"[,...]]
      # ["*"] to expose all keys.
      keys = [{{ range .Streaming.ABCI.Keys }}{{ printf "%q, " . }}{{end}}]
      # The plugin name used for streaming via gRPC.
      # Streaming is only enabled if this is set.
      # Supported plugins: abci
      plugin = "{{ .Streaming.ABCI.Plugin }}"
      # stop-node-on-err specifies whether to stop the node on message delivery error.
      stop-node-on-err = {{ .Streaming.ABCI.StopNodeOnErr }}
      vs.
      ABCIListenerConfig struct {
      Keys []string `mapstructure:"keys"`
      Plugin string `mapstructure:"plugin"`
      StopNodeOnErr bool `mapstructure:"stop-node-on-err"`
      }

@julienrbrt
Copy link
Member

Could you please point me where you see a missing field between the config.go and the toml.go? I cannot find any. Removing the bug label as it is more a suggestion / refactor request than a bug.

I don't see any missing fields, but I do see bugs:

  • Many TOML comments use Go field names. Example:

    # HaltHeight contains a non-zero block height at which a node will gracefully
    # halt and shutdown that can be used to assist upgrades and testing.
    #
    # Note: Commitment of state will be attempted on the corresponding block.
    halt-height = {{ .BaseConfig.HaltHeight }}
    (HaltHeight vs. halt-height)

  • Many TOML comments are missing from Go doc comments. Examples:

    • # These are applied if and only if the pruning strategy is custom.
      pruning-keep-recent = "{{ .BaseConfig.PruningKeepRecent }}"
      pruning-interval = "{{ .BaseConfig.PruningInterval }}"
      vs.
      Pruning string `mapstructure:"pruning"`
      PruningKeepRecent string `mapstructure:"pruning-keep-recent"`
      PruningInterval string `mapstructure:"pruning-interval"`

    • # AppDBBackend defines the database backend type to use for the application and snapshots DBs.
      # An empty string indicates that a fallback will be used.
      # The fallback is the db_backend value set in CometBFT's config.toml.
      app-db-backend = "{{ .BaseConfig.AppDBBackend }}"
      vs.
      // AppDBBackend defines the type of Database to use for the application and snapshots databases.
      // An empty string indicates that the CometBFT config's DBBackend value should be used.
      AppDBBackend string `mapstructure:"app-db-backend"`

    • # List of kv store keys to stream out via gRPC.
      # The store key names MUST match the module's StoreKey name.
      #
      # Example:
      # ["acc", "bank", "gov", "staking", "mint"[,...]]
      # ["*"] to expose all keys.
      keys = [{{ range .Streaming.ABCI.Keys }}{{ printf "%q, " . }}{{end}}]
      # The plugin name used for streaming via gRPC.
      # Streaming is only enabled if this is set.
      # Supported plugins: abci
      plugin = "{{ .Streaming.ABCI.Plugin }}"
      # stop-node-on-err specifies whether to stop the node on message delivery error.
      stop-node-on-err = {{ .Streaming.ABCI.StopNodeOnErr }}
      vs.
      ABCIListenerConfig struct {
      Keys []string `mapstructure:"keys"`
      Plugin string `mapstructure:"plugin"`
      StopNodeOnErr bool `mapstructure:"stop-node-on-err"`
      }

Right, I see. We could fix those inconsistencies indeed regarding the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 Backlog
Development

No branches or pull requests

2 participants