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

Introduce crate ibc-chain-registry and hermes config auto to automatically generate a config file #2533

Merged
merged 96 commits into from
Aug 24, 2022

Conversation

AlianBenabdallah
Copy link
Contributor

@AlianBenabdallah AlianBenabdallah commented Aug 5, 2022

Closes: #2187

Status

Ready for review

config.toml
[global]
log_level = 'info'

[mode.clients]
enabled = true
refresh = true
misbehaviour = true

[mode.connections]
enabled = false

[mode.channels]
enabled = false

[mode.packets]
enabled = true
clear_interval = 100
clear_on_start = true
tx_confirmation = false

[rest]
enabled = false
host = '127.0.0.1'
port = 3000

[telemetry]
enabled = false
host = '127.0.0.1'
port = 3001

[[chains]]
id = 'cosmoshub-4'
type = 'CosmosSdk'
rpc_addr = 'https://rpc-cosmoshub.ecostake.com/'
websocket_addr = 'ws://rpc-cosmoshub.ecostake.com/websocket'
grpc_addr = 'https://grpc-cosmoshub-ia.notional.ventures/'
rpc_timeout = '10s'
account_prefix = 'cosmos'
key_name = 'testkey1'
key_store_type = 'Test'
store_prefix = 'ibc'
default_gas = 100000
max_gas = 400000
gas_multiplier = 1.1
max_msg_num = 30
max_tx_size = 2097152
clock_drift = '5s'
max_block_time = '30s'
memo_prefix = ''


[chains.trust_threshold]
numerator = '1'
denominator = '3'

[chains.gas_price]
price = 0.1
denom = 'uatom'

[chains.packet_filter]
policy = 'allowall'

[chains.address_type]
derivation = 'cosmos'

[[chains]]
id = 'osmosis-1'
type = 'CosmosSdk'
rpc_addr = 'https://rpc-osmosis.ecostake.com/'
websocket_addr = 'ws://rpc-osmosis.ecostake.com/websocket'
grpc_addr = 'https://grpc-osmosis-ia.notional.ventures/'
rpc_timeout = '10s'
account_prefix = 'osmo'
key_name = 'testkey2'
key_store_type = 'Test'
store_prefix = 'ibc'
default_gas = 100000
max_gas = 400000
gas_multiplier = 1.1
max_msg_num = 30
max_tx_size = 2097152
clock_drift = '5s'
max_block_time = '30s'
memo_prefix = ''


[chains.trust_threshold]
numerator = '1'
denominator = '3'

[chains.gas_price]
price = 0.1
denom = 'uosmo'

[chains.packet_filter]
policy = 'allowall'

[chains.address_type]
derivation = 'cosmos'

Description

  • Introduces a new crate able to fetch data from the chain-registry. This crate is an alternative to Ocular which is not stable. Ocular can introduce dependency issues and does not verify that the websocket interface is enabled.
  • Introduces command hermes config auto [OPTIONS] --output <PATH> -chains <CHAIN_NAME_1[:<KEY1>] CHAIN_NAME_2[:<KEY2>]> [--commit <COMMIT_HASH>] which fetches data from the chain-registry and automatically generates a config file.
  • Gas settings are set to default value.

Design choices

  • If a ChainConfig can not be retrieved. The command fails and outputs and error.
  • Data is fetched from the latest commit of the chain-registry by default.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@AlianBenabdallah AlianBenabdallah requested review from adizere, romac and seanchen1991 and removed request for romac and seanchen1991 August 5, 2022 16:53
@AlianBenabdallah AlianBenabdallah self-assigned this Aug 5, 2022
@AlianBenabdallah AlianBenabdallah added O: new-feature Objective: cause to add a new feature or support O: usability Objective: cause to improve the user experience (UX) and ease using the product I: configuration Internal: related to Hermes configuration in progress labels Aug 5, 2022
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

It doesn't seem like the CLI parameter names for the hermes config auto command adhere to ADR 010. Are users supposed to be able to pass in as many chain IDs and key names as they would like? Or are users only able to supply two chain IDs and two key names?

There's also a fair bit of duplicated code. For example, AssetList::fetch and ChainData::fetch are essentially the same functions. select_healthy_rpc and select_healthy_grpc as well. While you'd typically abstract these into traits, it looks like doing that with these functions isn't currently possible due to them being async, unless we want to pull in the async-trait crate, but that doesn't really seem worth it to me.

chain-registry/src/assetlist.rs Outdated Show resolved Hide resolved
chain-registry/src/chain.rs Outdated Show resolved Hide resolved
chain-registry/src/lib.rs Outdated Show resolved Hide resolved
chain-registry/src/relayer_config.rs Outdated Show resolved Hide resolved
chain-registry/src/relayer_config.rs Outdated Show resolved Hide resolved
chain-registry/src/relayer_config.rs Outdated Show resolved Hide resolved
chain-registry/src/relayer_config.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/config/auto.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/config/auto.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/config/auto.rs Outdated Show resolved Hide resolved
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
@AlianBenabdallah
Copy link
Contributor Author

AlianBenabdallah commented Aug 5, 2022

It doesn't seem like the CLI parameter names for the hermes config auto command adhere to ADR 010.

I will rename every parameter. I also think key-names could be optionnal.

Are users supposed to be able to pass in as many chain IDs and key names as they would like? Or are users only able to supply two chain IDs and two key names?

They are able to pass in as many chain_id and key_names as they'd like.

There's also a fair bit of duplicated code. For example, AssetList::fetch and ChainData::fetch are essentially the same functions. select_healthy_rpc and select_healthy_grpc as well. While you'd typically abstract these into traits, it looks like doing that with these functions isn't currently possible due to them being async, unless we want to pull in the async-trait crate, but that doesn't really seem worth it to me.

Done.

AlianBenabdallah and others added 5 commits August 5, 2022 22:35
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
@AlianBenabdallah
Copy link
Contributor Author

AlianBenabdallah commented Aug 17, 2022

Awesome, great work Ali! Before we merge, can we also change --path to --output to avoid confusion between the path of the config file that's generated and a relay path between two blockchains?

@romac Done !

@romac romac added this to the v1.1 milestone Aug 17, 2022
@romac romac marked this pull request as draft August 18, 2022 15:12
@seanchen1991
Copy link
Contributor

@romac Now that v1 is released, can we re-open this PR and get it merged for v1.1? 🙂

@romac
Copy link
Member

romac commented Aug 22, 2022

Yep, let's re-open all the PRs in v1.1 milestone

@seanchen1991 seanchen1991 marked this pull request as ready for review August 22, 2022 15:37
@seanchen1991
Copy link
Contributor

@AlianBenabdallah Looks like there are some merge conflicts that need to be resolved, but once that's done, we can merge this 🙂

@AlianBenabdallah
Copy link
Contributor Author

It is done and ready to merge !

@seanchen1991 seanchen1991 merged commit 7683d43 into master Aug 24, 2022
@seanchen1991 seanchen1991 deleted the ali/auto_config branch August 24, 2022 17:50
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…matically generate a config file (informalsystems#2533)

* tmp

* read from chain registry

* add crate to load data from chain registry

* dependencies

* incomplete

* chain-registry crate ready

* chain-registry ready

* minor changes

* command implemented, todo: tests and doc

* added test, fixed empty grpc issue

* fmt

* minor change

* Update top level comments

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* Update relayer-cli/src/commands/config/auto.rs

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* Update chain-registry/src/relayer_config.rs

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* Update relayer-cli/src/commands/config/auto.rs

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* Update chain-registry/src/relayer_config.rs

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* Update chain-registry/src/relayer_config.rs

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* Apply suggestions from code review

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* renamed parameters

* small improvement

* fixed chain_id

* use websocketclient

* better error handling

* merge trait fetch and filename -> fetchable, use tokio::test

* wss instead of ws

* Apply suggestions from code review

Co-authored-by: Sean Chen <seanchen11235@gmail.com>

* sort error in alphabetical order

* Apply suggestions from code review

Co-authored-by: Romain Ruetschi <romain@informal.systems>

* use PathBuf, remove &str instead of String, replace unwrap by error

* fix dependency issues, better error handling

* support fetch ibc path

* Flesh our relayer_config docs a bit

* minor changes

* Document the `AutoCmd` type

* support PacketFilters + documentation

* fmt

* new tests

* fmt

* context oriented query

* different code structure

* add comments

* remove todo statement

* even more tests

* add tests for formatter

* clippy + fmt

* more and more tests

* update comment to add constraint on get_configs

* check filter len

* documentation improvements

* fmt and add is_empty to ChannelFilters

* changelog update

* Formatting nits

* More formatting nits

* auto key support

* More formatting nits

* Clean up changelog entries a bit

* Split utils.rs into fetchable.rs and constants.rs. Fixed tests + apply review

* bug fix + removed keys argument from get_config + guide/doc update

* delete example_config.toml, update log info and doc

* renamed chain-registry to ibc-chain-registry, moved relayer_config.rse to relayer-cli/chain_registry, minor code improvements

* fetch more data from chain.json

* changelog update

* remove explorers because it can be duplicated and cause an error, add more deserialization tests

* rename tests

* fix doc

* update doc

* remove keys argument, read from last commit

* change --path to --output

* Fix clippy warnings

* Skip serialiazing `proof_specs` setting if default by turning it into an `Option`

* Typo

* Fix clippy warning

* Formatting and case

* Refactor `config auto` code a bit

* Add tracing to chain-registry

* Refactor chain_registry in Hermes CLI

* Refactor `hermes_config` a little

* Revert "Refactor chain_registry in Hermes CLI"

This reverts commit 39ec5da.

* revert commit 39ec5da and code refactoring

* update guide

* fix cargo toml

* add new config params

* Remove an unneeded `?`

Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: configuration Internal: related to Hermes configuration O: new-feature Objective: cause to add a new feature or support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add initial support for Ocular to automate part of Hermes configuration
5 participants