-
Notifications
You must be signed in to change notification settings - Fork 326
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
Change config format to scope configs by type #3636
Comments
Agreed, this would indeed make more sense than the monolithic config we have now. We're a bit swamped in Q4 so I can't promise we'll manage to get to it this year unfortunately. We'll keep you posted! |
We'll definitely consider structuring config files with the Hermes SDK in this way 👍 |
I'd be happy to send a PR, we'll need something like this for our Hermes support in order to not have a bunch of meaningless options, so it'd be just as well to send it upstream. |
That would be amazing! 🙌 |
Work towards informalsystems#3636 This turns out to be a very good place to start to discover requirements for Hermes to operate in a more multi-chain way, since it allows discovering which parts of the code access Cosmos-specific data to do Cosmos-specific logic, and which parts are required for general relaying. The only change to the existing config format so far is the change in semantics of the `type` field, which now must be `CosmosSdk`. I did not preserve the existing logic and tests that parsing different variations of casing and dashes are supported, because I don't think this is particularly valuable to support. So far, only the core `ibc-relayer` crate is updated; the cli is not, because it's not necessarily clear how the cli should change to accomodate non-SDK chains.
Some work on this here: #3644 The tests in the core relayer crate all pass, but the cli doesn't build. There are some issues with the cli baking in assumptions about the SDK that I'm not sure of the best way to resolve. Once the CI runs, I can leave those comments. |
* wip: make config an endpoint-specific enum Work towards #3636 This turns out to be a very good place to start to discover requirements for Hermes to operate in a more multi-chain way, since it allows discovering which parts of the code access Cosmos-specific data to do Cosmos-specific logic, and which parts are required for general relaying. The only change to the existing config format so far is the change in semantics of the `type` field, which now must be `CosmosSdk`. I did not preserve the existing logic and tests that parsing different variations of casing and dashes are supported, because I don't think this is particularly valuable to support. So far, only the core `ibc-relayer` crate is updated; the cli is not, because it's not necessarily clear how the cli should change to accomodate non-SDK chains. * propagate changes into relayer-cli and add questions * relayer-cli: propagate enum chain config changes * relayer: add `ChainConfig::set_key_name` * test-framework: begin propagating config changes * relayer-cli: momentarily allow irrefutable patterns * relayer: run cargo fmt * integration-tests: propagate config enum api changes * integration-tests: fix errors with `ica` feature * integration-tests: fix errors with `ics29` feature * integration-tests: fix errors with `fee-grant` feature * integration-tests: fix errors with `mbt` feature * integration-tests: fix errors with `ordered` feature * relayer-cli: fix conflict introduced by merge * integration-test: fix simulation: * hermes: various linting fixes * hermes: include chain type in default config file * hermes: add chain type to all config examples * ci: update misbehavior config * relayer-rest(tests): add chain type to mock config * ci(misbehavior): edit config of forked chain * relayer(config): remove dead code * relayer: lift keys to `ChainConfig::list_keys` * relayer: move config validation to `ibc_relayer::config` * relayer: move cosmos config to `chain::cosmos::config` * relayer: add `chain::cosmos::config` module * relayer-cli(chain-registry): update path to `CosmosSdkConfig` * ci(misbehavior-ics): add chain type to config file * relayer(evidence): exit early if chain type is not cosmos sdk * relayer(cosmos): refactor cosmos-specific config validation * Add changelog entries --------- Co-authored-by: Erwan <erwanor@penumbralabs.xyz> Co-authored-by: Romain Ruetschi <romain@informal.systems>
Summary
Currently, Hermes'
ChainConfig
is one monolithic structure with many options. Although it has atype
field specifying the chain type, it's not possible to have different config options depending on the chain type. To fix this, and ease the way into Hermes support for non-SDK chains, it would help to restructure the config format so that configuration options are scoped by chain type.As an example of the kind of problem this creates, consider the changes in penumbra-zone#3 . This option is Penumbra-specific, so it should only be present when configuring a Penumbra chain -- it doesn't make sense that all other places where the config is used should have to include a Penumbra-specific option. And many of the options in the existing
ChainConfig
are actually specific to the Cosmos SDK, and are ignored by our PenumbraChainEndpoint
implementation.Instead, if the config were an enum:
The enum can be nicely represented in a TOML config file using Serde attributes, in particular using Serde's
tag
attribute. To see an example of how this works in practice, look at the authorization policy configurations for Penumbra'sSoftKms
software key-management service, documented here and implemented here:In the config file, the enumeration is collapsed into the
type
ormethod
tags, so the user doesn't need to consider nested enums, but the code can have options scoped to the specific backend provider.Doing this would be a small and incremental step towards multi-chain support for Hermes.
Acceptance Criteria
Hermes' config format is an enumeration with config options that can be set per chain type.
For Admin Use
The text was updated successfully, but these errors were encountered: