Add support for multiple chain types in the configuration #2228

romac · 2022-05-20T11:18:29Z

Closes: #2240

Description

See the proposal on the linked issue.

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/).

Reviewer checklist:

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

ancazamfir

Looks great!!! thanks @romac !!

ancazamfir · 2022-06-01T10:32:51Z

relayer/src/upgrade_chain.rs

    opts: &UpgradePlanOptions,
 ) -> Result<TxHash, UpgradeChainError> {
    let upgrade_height = dst_chain
-        .query_chain_latest_height()
+        .query_latest_height() // FIXME(romac): Use query_chain_latest_height once added to ChainHandle


Is this going to be fixed with #2185?
A lot of the code in this function is Cosmos/Tendermint specific and should be moved in the Cosmos chain impl (when the fixme is fixed)

I agree, we should tackle this with issue 2185!

I can also issue the right call here and add the appropriate method to the handle or would you rather we do this in a follow-up PR?

let's do it in a follow up PR

relayer-cli/src/cli_utils.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

ancazamfir · 2022-06-01T10:54:01Z

Can you also add some comment in config.toml and maybe show the default value:

type = 'CosmosSdk'

adizere · 2022-06-01T10:57:39Z

Can you also add some comment in config.toml and maybe show the default value:
type = 'CosmosSdk'

I'm wondering if we should document this chain type feature already in config.toml (and release it with v1), since it's rather experimental.

ancazamfir · 2022-06-01T11:06:37Z

Can you also add some comment in config.toml and maybe show the default value:
type = 'CosmosSdk'
I'm wondering if we should document this chain type feature already in config.toml (and release it with v1), since it's rather experimental.

@romac had the same comment offline. I am ok either way. I expect this to be used when other chain types will be supported, e.g. substrate. Not sure at what stage substrate integration is now, was wondering if it would be useful for them to upstream these changes.

adizere · 2022-06-02T09:59:08Z

Not sure at what stage substrate integration is now, was wondering if it would be useful for them to upstream these changes.

Good point! Another use-case for the chain type feature is integration with basecoin-rs.

From what I can tell, we're signalling the changes here via (doc)comments and the public API here
https://github.com/informalsystems/ibc-rs/blob/22652ee2aea75cd559aedd151813dbadadd4048b/relayer/src/chain.rs#L17

Not sure how to highlight this API better for substrate or other relayer developers, so that they can find & use it with ease, but as a first approach this looks good to me! If this proves to be too obscure, then I agree we should highlight it in config.toml.

mzabaluev · 2022-06-02T10:02:49Z

Should we choose the chain type naming convention while we are at it?

"CosmosSdk" (CamelCase) vs. "cosmos-sdk" (kebab-case)

The reason for the latter is, other chain types would often have a one word name, e.g. "substrate", so capitalization of those would look annoying. Think of all those wasteful Shift keypresses.

adizere

Looks good, great work Romain!

Pinging @plafer here because the present PR with #2226 will likely conflict, so be aware of the order of merging.

adizere · 2022-06-02T10:05:37Z

relayer/src/upgrade_chain.rs

    opts: &UpgradePlanOptions,
 ) -> Result<TxHash, UpgradeChainError> {
    let upgrade_height = dst_chain
-        .query_chain_latest_height()
+        .query_latest_height() // FIXME(romac): Use query_chain_latest_height once added to ChainHandle


I agree, we should tackle this with issue 2185!

mzabaluev · 2022-06-02T10:09:53Z

On the third hand, deserialization of the chain type could just be case-insensitive. It should be if this format is also used in CLI, because 1) CLI users on Unix traditionally hate using upper case unless necessary; 2) CLI users on Windows are used to parameters being case-insensitive.

romac · 2022-06-02T11:31:48Z

On the third hand, deserialization of the chain type could just be case-insensitive.

Agreed. I'll see if I can make it both case insensitive and ignore dashes as well.

romac · 2022-06-02T14:00:45Z

On the third hand, deserialization of the chain type could just be case-insensitive.

Agreed. I'll see if I can make it both case insensitive and ignore dashes as well.

Done here: 111f3f0

…stems#2228) * Move `ChainEndpoint` trait into its own module * Add support for multiple chain types * Add changelog entry * Remove `type` entry from the default config * Update relayer-cli/src/cli_utils.rs Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Ignore case and dashes when deserializing `ChainType` * Fix test Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

romac changed the title ~~Add support for multiple chain types~~ Add support for multiple chain types in the configuration May 24, 2022

romac added 2 commits May 24, 2022 16:03

Move ChainEndpoint trait into its own module

95c78ea

Add support for multiple chain types

911d55b

romac force-pushed the romac/chain-type branch from f5f794a to 911d55b Compare May 24, 2022 14:03

Add changelog entry

a92b124

romac marked this pull request as ready for review May 25, 2022 10:34

romac requested review from adizere, seanchen1991, ancazamfir, mzabaluev and soareschen as code owners May 25, 2022 10:34

Remove type entry from the default config

65f538a

romac assigned ancazamfir May 25, 2022

Merge branch 'master' into romac/chain-type

4c06057

ancazamfir approved these changes Jun 1, 2022

View reviewed changes

ancazamfir reviewed Jun 1, 2022

View reviewed changes

relayer-cli/src/cli_utils.rs Outdated Show resolved Hide resolved

Update relayer-cli/src/cli_utils.rs

22652ee

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

adizere approved these changes Jun 2, 2022

View reviewed changes

Ignore case and dashes when deserializing ChainType

111f3f0

romac added 2 commits June 2, 2022 16:09

Merge branch 'master' into romac/chain-type

d90a4cd

Fix test

16181de

adizere merged commit 08b7f98 into master Jun 6, 2022

adizere deleted the romac/chain-type branch June 6, 2022 07:47

romac mentioned this pull request Oct 12, 2022

Parametrize chain type via configuration file #1630

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple chain types in the configuration #2228

Add support for multiple chain types in the configuration #2228

romac commented May 20, 2022 •

edited

Loading

ancazamfir left a comment

ancazamfir Jun 1, 2022

adizere Jun 2, 2022

romac Jun 2, 2022

ancazamfir Jun 3, 2022

ancazamfir commented Jun 1, 2022

adizere commented Jun 1, 2022

ancazamfir commented Jun 1, 2022 •

edited

Loading

adizere commented Jun 2, 2022 •

edited

Loading

mzabaluev commented Jun 2, 2022 •

edited

Loading

adizere left a comment

adizere Jun 2, 2022

mzabaluev commented Jun 2, 2022

romac commented Jun 2, 2022

romac commented Jun 2, 2022

Add support for multiple chain types in the configuration #2228

Add support for multiple chain types in the configuration #2228

Conversation

romac commented May 20, 2022 • edited Loading

Description

PR author checklist:

Reviewer checklist:

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir Jun 1, 2022

Choose a reason for hiding this comment

adizere Jun 2, 2022

Choose a reason for hiding this comment

romac Jun 2, 2022

Choose a reason for hiding this comment

ancazamfir Jun 3, 2022

Choose a reason for hiding this comment

ancazamfir commented Jun 1, 2022

adizere commented Jun 1, 2022

ancazamfir commented Jun 1, 2022 • edited Loading

adizere commented Jun 2, 2022 • edited Loading

mzabaluev commented Jun 2, 2022 • edited Loading

adizere left a comment

Choose a reason for hiding this comment

adizere Jun 2, 2022

Choose a reason for hiding this comment

mzabaluev commented Jun 2, 2022

romac commented Jun 2, 2022

romac commented Jun 2, 2022

romac commented May 20, 2022 •

edited

Loading

ancazamfir commented Jun 1, 2022 •

edited

Loading

adizere commented Jun 2, 2022 •

edited

Loading

mzabaluev commented Jun 2, 2022 •

edited

Loading