hermes icon indicating copy to clipboard operation
hermes copied to clipboard

Introduce crate `chain-registry` and `hermes config auto` to automatically generate a config file

Open AlianBenabdallah opened this issue 2 years ago • 7 comments

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 = '' proof_specs = ''' [ { "leaf_spec": { "hash": 1, "prehash_key": 0, "prehash_value": 1, "length": 1, "prefix": "AA==" }, "inner_spec": { "child_order": [ 0, 1 ], "child_size": 33, "min_prefix_length": 4, "max_prefix_length": 12, "empty_child": "", "hash": 1 }, "max_depth": 0, "min_depth": 0 }, { "leaf_spec": { "hash": 1, "prehash_key": 0, "prehash_value": 1, "length": 1, "prefix": "AA==" }, "inner_spec": { "child_order": [ 0, 1 ], "child_size": 32, "min_prefix_length": 1, "max_prefix_length": 1, "empty_child": "", "hash": 1 }, "max_depth": 0, "min_depth": 0 } ]'''

[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 = '' proof_specs = ''' [ { "leaf_spec": { "hash": 1, "prehash_key": 0, "prehash_value": 1, "length": 1, "prefix": "AA==" }, "inner_spec": { "child_order": [ 0, 1 ], "child_size": 33, "min_prefix_length": 4, "max_prefix_length": 12, "empty_child": "", "hash": 1 }, "max_depth": 0, "min_depth": 0 }, { "leaf_spec": { "hash": 1, "prehash_key": 0, "prehash_value": 1, "length": 1, "prefix": "AA==" }, "inner_spec": { "child_order": [ 0, 1 ], "child_size": 32, "min_prefix_length": 1, "max_prefix_length": 1, "empty_child": "", "hash": 1 }, "max_depth": 0, "min_depth": 0 } ]'''

[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 --path PATH --chains <CHAIN_NAME_1 CHAIN_NAME2...> [--keys <KEY_CHAIN_1 KEY_CHAIN_2...>] 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.
  • If --keys is not provided, key_name is automatically retrieved by picking the first key found at ~/.hermes/keys/chain_id.
  • --keys can be used to manually set the key_name parameter of a ChainConfig. The command does not check that the key exists.
  • Data is fetched from a specific commit of the chain-registry.

PR author checklist:

  • [x] Added changelog entry, using unclog.
  • [ ] Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • [x] Linked to GitHub issue.
  • [x] Updated code comments and documentation (e.g., docs/).
  • [x] 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 avatar Aug 05 '22 16:08 AlianBenabdallah

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 avatar Aug 05 '22 20:08 AlianBenabdallah

Great job! Some unit-tests for the the new command hermes config auto would be great.

ljoss17 avatar Aug 09 '22 12:08 ljoss17

I added path.rs which contains structs and functions to fetch IBC data from the chain repo.

Now I need to modify the API in order to incorporate this change. I'll most likely add a function which takes chain_names : &[String] as argument and returns a Vec<ChainConfig>.

AlianBenabdallah avatar Aug 09 '22 16:08 AlianBenabdallah

keys is a required argument. It populates key_name in ChainConfig.

Discussing with Luca today and we're wondering why would the keys option be required? Hermes could automatically detect in the default keystore location whether each chain has some wallets already (see keys list --chain <CHAIN_ID>), and select the first one among them. It could also output warnings if there are no wallets for the given chain, and also log to warn the user that it picked a certain wallet. Then the user could manually alter config.toml to choose a different wallet if they wish.

adizere avatar Aug 11 '22 09:08 adizere

keys is a required argument. It populates key_name in ChainConfig.

Discussing with Luca today and we're wondering why would the keys option be required? Hermes could automatically detect in the default keystore location whether each chain has some wallets already (see keys list --chain <CHAIN_ID>), and select the first one among them. It could also output warnings if there are no wallets for the given chain, and also log to warn the user that it picked a certain wallet. Then the user could manually alter config.toml to choose a different wallet if they wish.

It makes sense, I'll put it in the TODO list

AlianBenabdallah avatar Aug 11 '22 12:08 AlianBenabdallah

Maybe I should try to remove ChainConfig dependencies from the chain-registry crate.

AlianBenabdallah avatar Aug 11 '22 20:08 AlianBenabdallah

keys is a required argument. It populates key_name in ChainConfig.

Discussing with Luca today and we're wondering why would the keys option be required? Hermes could automatically detect in the default keystore location whether each chain has some wallets already (see keys list --chain <CHAIN_ID>), and select the first one among them. It could also output warnings if there are no wallets for the given chain, and also log to warn the user that it picked a certain wallet. Then the user could manually alter config.toml to choose a different wallet if they wish.

@adizere @seanchen1991 This feature is now implemented. I guess the next and final step is to add integration tests.

AlianBenabdallah avatar Aug 12 '22 16:08 AlianBenabdallah

I see how they are useful here for providing the corresponding keys in the same order, but perhaps we could improve the ux a little bit in this case by asking for CHAIN:KEY pairs? eg.

hermes config auto [OPTIONS] --path <PATH> --keys <CHAIN_1:KEY_1 CHAIN_2:KEY_2...>

The final commit incorporates this change. The command now fetches data from the last commit of the chain-registry. I think the PR is now ready to merge.

AlianBenabdallah avatar Aug 16 '22 17:08 AlianBenabdallah

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 avatar Aug 17 '22 08:08 romac

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 !

AlianBenabdallah avatar Aug 17 '22 09:08 AlianBenabdallah

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

seanchen1991 avatar Aug 22 '22 15:08 seanchen1991

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

romac avatar Aug 22 '22 15:08 romac

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

seanchen1991 avatar Aug 22 '22 15:08 seanchen1991

It is done and ready to merge !

AlianBenabdallah avatar Aug 23 '22 10:08 AlianBenabdallah