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

Change config format to scope configs by type #3636

Closed
5 tasks
hdevalence opened this issue Sep 22, 2023 · 5 comments · Fixed by #3644
Closed
5 tasks

Change config format to scope configs by type #3636

hdevalence opened this issue Sep 22, 2023 · 5 comments · Fixed by #3644
Labels
E: non-cosmos External: related to non-Cosmos chains I: configuration Internal: related to Hermes configuration
Milestone

Comments

@hdevalence
Copy link
Contributor

hdevalence commented Sep 22, 2023

Summary

Currently, Hermes' ChainConfig is one monolithic structure with many options. Although it has a type 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 Penumbra ChainEndpoint implementation.

Instead, if the config were an enum:

  • Each "backend" can define their own configuration structure, with only the options relevant to that backend;
  • The top-level config can be an enumeration of all of the backends included in Hermes.

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's SoftKms software key-management service, documented here and implemented here:

[[kms_config.auth_policy]]
type = 'DestinationAllowList'
allowed_destination_addresses = ['penumbrav2t13vh0fkf3qkqjacpm59g23ufea9n5us45e4p5h6hty8vg73r2t8g5l3kynad87u0n9eragf3hhkgkhqe5vhngq2cw493k48c9qg9ms4epllcmndd6ly4v4dw2jcnxaxzjqnlvnw']

[[kms_config.auth_policy]]
type = 'OnlyIbcRelay'

[[kms_config.auth_policy]]
type = 'PreAuthorization'
method = 'Ed25519'
required_signatures = 1
allowed_signers = ['+Osq5OiWKos57KigDjd3XCG/YLUOSUbuBly4LBBpJTg=']

In the config file, the enumeration is collapsed into the type or method 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

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@romac
Copy link
Member

romac commented Sep 26, 2023

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!

@romac romac added I: configuration Internal: related to Hermes configuration E: non-cosmos External: related to non-Cosmos chains labels Sep 26, 2023
@seanchen1991
Copy link
Contributor

seanchen1991 commented Sep 27, 2023

We'll definitely consider structuring config files with the Hermes SDK in this way 👍

@hdevalence
Copy link
Contributor Author

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.

@romac
Copy link
Member

romac commented Sep 28, 2023

That would be amazing! 🙌

hdevalence added a commit to penumbra-zone/hermes that referenced this issue Oct 1, 2023
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.
@hdevalence
Copy link
Contributor Author

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.

romac added a commit that referenced this issue Oct 25, 2023
* 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 <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: non-cosmos External: related to non-Cosmos chains I: configuration Internal: related to Hermes configuration
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants