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

feat: abci client type now an explicit config setting #416

Merged
merged 4 commits into from
Apr 18, 2024

Conversation

JimLarson
Copy link

@JimLarson JimLarson commented Apr 18, 2024

Description

Refs: Agoric/agoric-sdk#9224

Add an explicit entry for abci-client-type in the TOML file. The setting can be done in the file but is overridden by when the command-line flag is set.

Note that this entry is unlike the other entries in the "base config" part of the TOML file, in that there are no BaseApp.Get/SetABCIClientType() methods, option helpers, etc. This makes it more like the various start command flags that do not have explicit config fields, e.g. --with-tendermint or --trace-store. This seems appropriate for a setting that is used near the "top" of the start command.

Note that even without this change, the option can be set in the config/app.toml file. This just makes the entry part of the automatically-generated TOML file.

Note that the default value is now specified both in the flag definition and in the default config constructor, as some of the other flags are. It's confusing to think about which one takes precedence, so they should just be kept in sync.

Tested the following:

  • start with default toml file which includes and explicit entry to the default "committing" value
  • start with an illegal value - fails as expected
  • start with non-default valid value "local"
  • start with line removed from app.toml
  • override with flag set to an illegal value - fails as expected
  • override with flag set to "committing"
  • restore app.toml entry and override with a different legal value
  • override with illegal flag - still fails as expected.

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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Setting the flag still works and takes precedence.
@JimLarson JimLarson added enhancement New feature or request agoric-cosmos Agoric tags for cosmos labels Apr 18, 2024
@JimLarson JimLarson self-assigned this Apr 18, 2024
@@ -289,6 +292,7 @@ func DefaultConfig() *Config {
IAVLCacheSize: 781250, // 50 MB
IAVLDisableFastNode: false,
IAVLLazyLoading: false,
ABCIClientType: "committing", // [AGORIC]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No way to reference the "other" default value ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not where it is now - circular imports. Let me see if I can do the reference the other way - or at least add cross-reference comments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Now the options and default are defined in one place - the config package. (The server/types package seems like the better place, but it imports server/config.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh! It would help if I actually used the constant I just defined. Actually done now.

@JimLarson JimLarson merged commit e16fe33 into Agoric Apr 18, 2024
30 of 31 checks passed
@JimLarson JimLarson deleted the 9224-abci-client-ceremony branch April 18, 2024 02:37
JimLarson added a commit that referenced this pull request Apr 18, 2024
* feat: abci client type now an explicit config setting

Setting the flag still works and takes precedence.

* docs: changelog entry

* refactor: common location for valid abci client types and default

* refactor: actually use the defined default value
JeancarloBarrios pushed a commit that referenced this pull request Sep 28, 2024
* feat: abci client type now an explicit config setting

Setting the flag still works and takes precedence.

* docs: changelog entry

* refactor: common location for valid abci client types and default

* refactor: actually use the defined default value
JeancarloBarrios pushed a commit that referenced this pull request Sep 28, 2024
* feat: abci client type now an explicit config setting

Setting the flag still works and takes precedence.

* docs: changelog entry

* refactor: common location for valid abci client types and default

* refactor: actually use the defined default value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cosmos Agoric tags for cosmos enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants