-
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 #3644
Conversation
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.
#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize)] | ||
/// Types of chains the relayer can relay to and from | ||
pub enum ChainType { | ||
/// Chains based on the Cosmos SDK | ||
CosmosSdk, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gone entirely. Rather than having the config store an enum of chain types, the config is an enum, so that it can have different subtypes for different configs.
// TODO: extract Tendermint-related configs into a separate substructure | ||
// that can be used both by CosmosSdkConfig and configs for nonSDK chains. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the right move here is to split out the Tendermint-related fields on the config into a Tendermint-specific config structure. Then that structure can be a field on both CosmosSdkConfig
and other Tendermint-using chains (like Penumbra or Namada).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
fn bootstrap(config: ChainConfig, rt: Arc<TokioRuntime>) -> Result<Self, Error> { | ||
#[allow(irrefutable_let_patterns)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Currently, the pattern is irrefutable, but it won't be in the future, and that should be handled now)
fn get_key(&mut self) -> Result<Self::SigningKeyPair, Error> { | ||
// Get the key from key seed file | ||
let key_pair = self | ||
.keybase() | ||
.get_key(&self.config.key_name) | ||
.map_err(|e| Error::key_not_found(self.config().key_name.clone(), e))?; | ||
|
||
Ok(key_pair) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf #3641
crates/relayer/src/config.rs
Outdated
@@ -31,7 +31,7 @@ use ibc_relayer_types::core::ics23_commitment::specs::ProofSpecs; | |||
use ibc_relayer_types::core::ics24_host::identifier::{ChainId, ChannelId, PortId}; | |||
use ibc_relayer_types::timestamp::ZERO_DURATION; | |||
|
|||
use crate::chain::ChainType; | |||
//use crate::chain::ChainType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, will clean up after next round of discussion
crates/relayer/src/config.rs
Outdated
/* | ||
pub fn chain_type() -> ChainType { | ||
ChainType::CosmosSdk | ||
} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto above cleanup comment
#[serde(tag = "type")] | ||
pub enum ChainConfig { | ||
CosmosSdk(CosmosSdkConfig), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the issue description for #3636, this will produce TOML like
[[chains]]
type = "CosmosSdk"
id = "chain_id"
rpc_addr = "http://127.0.0.1:26657/"
grpc_addr = "http://127.0.0.1:8080/"
The only difference from the current format is that the type
field is now mandatory, and it must be CosmosSdk
, not cosmos-sdk
or cOsMo-sSdk
or any other variation. I don't think that supporting arbitrary variations is a worthwhile complexity vs ergonomics tradeoff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that supporting arbitrary variations is a worthwhile complexity vs ergonomics tradeoff.
Agreed! Looks good otherwise!
crates/relayer/src/config.rs
Outdated
impl ChainConfig { | ||
pub fn id(&self) -> &ChainId { | ||
match self { | ||
Self::CosmosSdk(config) => &config.id, | ||
} | ||
} | ||
|
||
pub fn packet_filter(&self) -> &PacketFilter { | ||
match self { | ||
Self::CosmosSdk(config) => &config.packet_filter, | ||
} | ||
} | ||
|
||
pub fn max_block_time(&self) -> Duration { | ||
match self { | ||
Self::CosmosSdk(config) => config.max_block_time, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the attributes that are used by Hermes outside of the Cosmos backend.
All of the other attributes are only used inside the Cosmos backend, so this is effectively a way to discover which parts of the configuration data is used by the relayer engine, and which parts are used only by the chain backend.
All other backends' configuration objects will need these fields.
An alternative representation would be to have the ChainConfig
contain these common fields unconditionally, AND an inner enum with only the non-common configuration data. But I think this is actually worse, because now the backends can't cleanly model their own data: they need access to common fields like the chain id, and those fields would live in an overarching ChainConfig
object. With the current structure, the common fields would be duplicated across different objects (the PenumbraConfig
would have an id
field as well), but this actually makes the code more modular, because each backend can just pass around and work with one object it defines (the cosmos logic can work with a CosmosSdkConfig
, the penumbra logic can work with a PenumbraConfig
, and so on).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you have done it currently sounds like a good way to me to enforce that each config type provides this information, so we can leave it like this.
@@ -287,9 +287,10 @@ impl KeyRing<Ed25519KeyPair> { | |||
} | |||
} | |||
|
|||
// Why is this not a method on `ChainConfig`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason this isn't a method on ChainConfig
, rather than a bare function defined in this other module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the whole story about keys is a bit messy right now, as we have seen elsewhere. Agreed that this should be backend-specific.
I was planning to leave self-review comments on the build failures that reveal issues with the CLI interface, but that will need to wait for the CI to run. |
crates/relayer/src/config.rs
Outdated
|
||
#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)] | ||
#[serde(deny_unknown_fields)] | ||
pub struct CosmosSdkConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should move to the cosmos/
backend but I left it for now for ease of initial commentary/review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to leave it here for the review, we can move it later on.
I thought it'd be possible to add comments to check failures, but this doesn't seem to be the case. Instead I put them inline. |
crates/relayer-cli/src/config.rs
Outdated
// TODO: why are these not methods on the config object? | ||
// why are they defined only in the relayer-cli crate? | ||
if let ChainConfig::CosmosSdk(cosmos_c) = c { | ||
validate_trust_threshold(&cosmos_c.id, cosmos_c.trust_threshold)?; | ||
// Validate gas-related settings | ||
validate_gas_settings(&cosmos_c.id, cosmos_c)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't config validation code be attached to the configs in the core crate, rather than being in the cli? Happy to move it if that sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's move that function into the core crate. No good reason to have it here.
// Q: should the key name be required across chain types, meaning that | ||
// key management is common to all chain types, or should key management | ||
// be the responsibility of the backend? If key management is common | ||
// across backends, how should it be agnostic to the key type? Can it | ||
// just be an opaque byte string handled by the backend? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ties in to #3641
Perhaps the key manager should only support opaque byte strings, and let the rest be handled by the backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for taking on this whole endeavor. What you've done so far looks really good to me.
Now we need to figure out a good story for backend-specific commands, eg. keys
, as you've pointed out here and in #3641.
@@ -144,6 +144,8 @@ fn subscribe( | |||
compat_mode: CompatMode, | |||
rt: Arc<TokioRuntime>, | |||
) -> eyre::Result<Subscription> { | |||
// Q: Should this be restricted only to backends that support it, | |||
// or are all backends expected to support subscriptions? | |||
let (event_source, monitor_tx) = match &chain_config.event_source { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that should be restricted only to backends to support subscriptions, but right now Hermes requires all backends to do so. Definitely more work needed there to remove this assumption.
@@ -166,6 +168,7 @@ fn subscribe( | |||
Ok(subscription) | |||
} | |||
|
|||
// Q: why isn't this part of the CosmosSDK chain endpoint impl? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we have similar code in the CosmosSdkChain::boostrap
which ought to be refactored to use this function after we move it under the cosmos
module.
@@ -203,6 +203,8 @@ impl Runnable for TxUpdateClientCmd { | |||
|
|||
if let Some(restart_params) = self.genesis_restart_params() { | |||
if let Some(c) = config.find_chain_mut(&reference_chain_id) { | |||
// Q: How should this be handled? Should the cli command only | |||
// work on supported chain types? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we probably want such commands to restrict which types of chains they work against, either by explicitly checking that and throwing an error, or by moving them under a new sub-command per chain type, eg. this would become tx cosmos update client
. Not sure which way to go yet.
crates/relayer-cli/src/config.rs
Outdated
// TODO: why are these not methods on the config object? | ||
// why are they defined only in the relayer-cli crate? | ||
if let ChainConfig::CosmosSdk(cosmos_c) = c { | ||
validate_trust_threshold(&cosmos_c.id, cosmos_c.trust_threshold)?; | ||
// Validate gas-related settings | ||
validate_gas_settings(&cosmos_c.id, cosmos_c)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's move that function into the core crate. No good reason to have it here.
// TODO: extract Tendermint-related configs into a separate substructure | ||
// that can be used both by CosmosSdkConfig and configs for nonSDK chains. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
#[serde(tag = "type")] | ||
pub enum ChainConfig { | ||
CosmosSdk(CosmosSdkConfig), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that supporting arbitrary variations is a worthwhile complexity vs ergonomics tradeoff.
Agreed! Looks good otherwise!
crates/relayer/src/config.rs
Outdated
impl ChainConfig { | ||
pub fn id(&self) -> &ChainId { | ||
match self { | ||
Self::CosmosSdk(config) => &config.id, | ||
} | ||
} | ||
|
||
pub fn packet_filter(&self) -> &PacketFilter { | ||
match self { | ||
Self::CosmosSdk(config) => &config.packet_filter, | ||
} | ||
} | ||
|
||
pub fn max_block_time(&self) -> Duration { | ||
match self { | ||
Self::CosmosSdk(config) => config.max_block_time, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you have done it currently sounds like a good way to me to enforce that each config type provides this information, so we can leave it like this.
crates/relayer/src/config.rs
Outdated
|
||
#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)] | ||
#[serde(deny_unknown_fields)] | ||
pub struct CosmosSdkConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to leave it here for the review, we can move it later on.
@@ -287,9 +287,10 @@ impl KeyRing<Ed25519KeyPair> { | |||
} | |||
} | |||
|
|||
// Why is this not a method on `ChainConfig`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the whole story about keys is a bit messy right now, as we have seen elsewhere. Agreed that this should be backend-specific.
Maybe for now we can restrict these commands to |
I have done some work propagating the configuration changes, but I'm leaving some of the refactor items as a last step (as previously agreed above). I would like to pause and see if CI passes, tie up loose ends, before pushing on the specific TODOs (e.g. extracting the tendermint config) and doing another pass. |
This took longer than anticipated, but the branch is finally passing all integration tests. Now, we should be able to move quickly on getting the remaining items done and ready for review by tomorrow:
when this is done: and finally:
|
@romac I think this is ready for review! We can split out the tendermint config in future work since it has a small surface area, ditto for the websocket methods, for now we can just error if they are not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work 🚀 Thanks so much to both of you for seeing this through, much appreciated! 💐
@hdevalence Before I can merge this, can you please enable edits from maintainers on this PR? |
Hmm, the Github docs only talk about personal-owned forks, I'm not sure if it's possible because we made it from an org fork. But I can certainly add you to our org fork ? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork# |
@romac I can't see that button, maybe it's only visible to the PR creator, but I added you as a collaborator to the forked repository: invitations edit: ah henry's comment just loaded for me! |
Perfect, thank you both! 🙏 |
Will these config changes require updates to the Hermes guide? |
I thought they were all taken care of, but actually found three templates that we missed: #3679 |
Closes: #3636
Description
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 beCosmosSdk
. 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.PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.