hermes
hermes copied to clipboard
Introduce crate `chain-registry` and `hermes config auto` to automatically generate a config file
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 thekey_name
parameter of aChainConfig
. 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).
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
andChainData::fetch
are essentially the same functions.select_healthy_rpc
andselect_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 beingasync
, unless we want to pull in the async-trait crate, but that doesn't really seem worth it to me.
Done.
Great job! Some unit-tests for the the new command hermes config auto
would be great.
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>
.
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.
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 alterconfig.toml
to choose a different wallet if they wish.
It makes sense, I'll put it in the TODO list
Maybe I should try to remove ChainConfig
dependencies from the chain-registry
crate.
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 alterconfig.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.
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.
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?
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 Now that v1 is released, can we re-open this PR and get it merged for v1.1? 🙂
Yep, let's re-open all the PRs in v1.1 milestone
@AlianBenabdallah Looks like there are some merge conflicts that need to be resolved, but once that's done, we can merge this 🙂
It is done and ready to merge !