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

Use CosmosSdk as the chain type if omitted, for backward compatibility #3787

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

romac
Copy link
Member

@romac romac commented Jan 16, 2024

This PR changed the ChainConfig type to an enum to allow for settings which are specific to a type of chain (eg. Penumbra, Namada, CosmosSdk), but that had the effect of making the type key in the [[chain]] config required. So when we release this as is, it will require all operators to update their config to add type = "CosmosSdk".


To avoid breaking operators' configurations, we need to make the CosmosSdk the default value for the type setting.

Doing this requires a bit of hack, due to serde's lack of support for specifying a default variant when a tag is not provided.

See the doc comment on the ChainConfig struct for more details.

The idea is to use an untagged enum, but to ensure that each underlying variant has a type field which can only be deserialized from a single value. For this, the mono state crate provides a MustBe! macro which expands into a type that can only be deserialized from the given string, eg. MustBe!("Hello") can only be deserialized from the string Hello.

With this in place, we can make one of the variant the default one by annotating its type field with #[serde(default)].

In a nutshell:

use monostate::MustBe;
use serde::{Deserialize, Serialize};

#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)]
pub struct CosmosSdkConfig {
    #[serde(default)]
    r#type: MustBe!("CosmosSdk"),

    id: String,
}

#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)]
pub struct NamadaConfig {
    r#type: MustBe!("Namada"),

    id: String,
}

#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)]
#[serde(untagged)]
enum ChainConfig {
    CosmosSdk(CosmosSdkConfig),
    Namada(NamadaConfig),
}

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

Doing this requires a bit of hack, due to serde's lack of support
for specifying a default variant when a tag is not provided.

See the doc comment on the `ChainConfig` struct for more details.
@romac romac merged commit baef139 into master Jan 18, 2024
12 checks passed
@romac romac deleted the romac/default-chain-type branch January 18, 2024 13:53
@romac romac mentioned this pull request Jan 18, 2024
11 tasks
romac added a commit that referenced this pull request Jan 22, 2024
Error messages when deserializing an invalid chain config have gotten
a lot worse since #3787.

For example, when mistyping the `gas_multiplier` field as `gas_multiplie` in the per-chain config:

```
data did not match any variant of untagged enum ChainConfig for key `chains` at line 424 column 1
```

After this commit (same message as before #3787):

```
invalid CosmosSdk config: unknown field `gas_multiplie`, expected one of `id`, `rpc_addr`, `grpc_addr`, `event_source`,
`rpc_timeout`, `trusted_node`, `account_prefix`, `key_name`, `key_store_type`, `key_store_folder`, `store_prefix`,
`default_gas`, `max_gas`, `genesis_restart`, `gas_adjustment`, `gas_multiplier`, `fee_granter`, `max_msg_num`,
`max_tx_size`, `max_grpc_decoding_size`, `query_packets_chunk_size`, `clock_drift`, `max_block_time`, `trusting_period`,
`client_refresh_rate`, `ccv_consumer_chain`, `memo_prefix`, `sequential_batch_tx`, `proof_specs`, `trust_threshold`,
`gas_price`, `packet_filter`, `address_type`, `extension_options`, `compat_mode`, `clear_interval`
for key `chains` at line 424 column 1
```

For this, we now use a custom deserializer for ChainConfig instead of
relying on an untagged enum + `monostate::MustBe`.
romac added a commit that referenced this pull request Jan 22, 2024
Error messages when deserializing an invalid chain config have gotten
a lot worse since #3787.

For example, when mistyping the `gas_multiplier` field as `gas_multiplie` in the per-chain config:

```
data did not match any variant of untagged enum ChainConfig for key `chains` at line 424 column 1
```

After this commit (same message as before #3787):

```
invalid CosmosSdk config: unknown field `gas_multiplie`, expected one of `id`, `rpc_addr`, `grpc_addr`, `event_source`,
`rpc_timeout`, `trusted_node`, `account_prefix`, `key_name`, `key_store_type`, `key_store_folder`, `store_prefix`,
`default_gas`, `max_gas`, `genesis_restart`, `gas_adjustment`, `gas_multiplier`, `fee_granter`, `max_msg_num`,
`max_tx_size`, `max_grpc_decoding_size`, `query_packets_chunk_size`, `clock_drift`, `max_block_time`, `trusting_period`,
`client_refresh_rate`, `ccv_consumer_chain`, `memo_prefix`, `sequential_batch_tx`, `proof_specs`, `trust_threshold`,
`gas_price`, `packet_filter`, `address_type`, `extension_options`, `compat_mode`, `clear_interval`
for key `chains` at line 424 column 1
```

For this, we now use a custom deserializer for ChainConfig instead of
relying on an untagged enum + `monostate::MustBe`.
romac added a commit that referenced this pull request Jan 22, 2024
Error messages when deserializing an invalid chain config have gotten a lot worse since #3787.

For example, when mistyping the `gas_multiplier` field as `gas_multiplie` in the per-chain config:

```
data did not match any variant of untagged enum ChainConfig for key `chains` at line 424 column 1
```

After this commit (same message as before #3787):

```
invalid CosmosSdk config: unknown field `gas_multiplie`, expected one of `id`, `rpc_addr`, `grpc_addr`, `event_source`,
`rpc_timeout`, `trusted_node`, `account_prefix`, `key_name`, `key_store_type`, `key_store_folder`, `store_prefix`,
`default_gas`, `max_gas`, `genesis_restart`, `gas_adjustment`, `gas_multiplier`, `fee_granter`, `max_msg_num`,
`max_tx_size`, `max_grpc_decoding_size`, `query_packets_chunk_size`, `clock_drift`, `max_block_time`, `trusting_period`,
`client_refresh_rate`, `ccv_consumer_chain`, `memo_prefix`, `sequential_batch_tx`, `proof_specs`, `trust_threshold`,
`gas_price`, `packet_filter`, `address_type`, `extension_options`, `compat_mode`, `clear_interval`
for key `chains` at line 424 column 1
```

For this, we now use a custom deserializer for ChainConfig instead of relying on an untagged enum.
romac added a commit that referenced this pull request Jan 22, 2024
Error messages when deserializing an invalid chain config have gotten a lot worse since #3787.

For example, when mistyping the `gas_multiplier` field as `gas_multiplie` in the per-chain config:

```
data did not match any variant of untagged enum ChainConfig for key `chains` at line 424 column 1
```

After this commit (same message as before #3787):

```
invalid CosmosSdk config: unknown field `gas_multiplie`, expected one of `id`, `rpc_addr`, `grpc_addr`, `event_source`,
`rpc_timeout`, `trusted_node`, `account_prefix`, `key_name`, `key_store_type`, `key_store_folder`, `store_prefix`,
`default_gas`, `max_gas`, `genesis_restart`, `gas_adjustment`, `gas_multiplier`, `fee_granter`, `max_msg_num`,
`max_tx_size`, `max_grpc_decoding_size`, `query_packets_chunk_size`, `clock_drift`, `max_block_time`, `trusting_period`,
`client_refresh_rate`, `ccv_consumer_chain`, `memo_prefix`, `sequential_batch_tx`, `proof_specs`, `trust_threshold`,
`gas_price`, `packet_filter`, `address_type`, `extension_options`, `compat_mode`, `clear_interval`
for key `chains` at line 424 column 1
```

For this, we now use a custom deserializer for ChainConfig instead of relying on an untagged enum.
romac added a commit that referenced this pull request Jan 22, 2024
Error messages when deserializing an invalid chain config have gotten a lot worse since #3787.

For example, when mistyping the `gas_multiplier` field as `gas_multiplie` in the per-chain config:

```
data did not match any variant of untagged enum ChainConfig for key `chains` at line 424 column 1
```

After this commit (same message as before #3787):

```
invalid CosmosSdk config: unknown field `gas_multiplie`, expected one of `id`, `rpc_addr`, `grpc_addr`, `event_source`,
`rpc_timeout`, `trusted_node`, `account_prefix`, `key_name`, `key_store_type`, `key_store_folder`, `store_prefix`,
`default_gas`, `max_gas`, `genesis_restart`, `gas_adjustment`, `gas_multiplier`, `fee_granter`, `max_msg_num`,
`max_tx_size`, `max_grpc_decoding_size`, `query_packets_chunk_size`, `clock_drift`, `max_block_time`, `trusting_period`,
`client_refresh_rate`, `ccv_consumer_chain`, `memo_prefix`, `sequential_batch_tx`, `proof_specs`, `trust_threshold`,
`gas_price`, `packet_filter`, `address_type`, `extension_options`, `compat_mode`, `clear_interval`
for key `chains` at line 424 column 1
```

For this, we now use a custom deserializer for ChainConfig instead of relying on an untagged enum.
romac added a commit that referenced this pull request Jan 23, 2024
* Improve error message when deserializing an invalid chain config

Error messages when deserializing an invalid chain config have gotten a lot worse since #3787.

For example, when mistyping the `gas_multiplier` field as `gas_multiplie` in the per-chain config:

```
data did not match any variant of untagged enum ChainConfig for key `chains` at line 424 column 1
```

After this commit (same message as before #3787):

```
invalid CosmosSdk config: unknown field `gas_multiplie`, expected one of `id`, `rpc_addr`, `grpc_addr`, `event_source`,
`rpc_timeout`, `trusted_node`, `account_prefix`, `key_name`, `key_store_type`, `key_store_folder`, `store_prefix`,
`default_gas`, `max_gas`, `genesis_restart`, `gas_adjustment`, `gas_multiplier`, `fee_granter`, `max_msg_num`,
`max_tx_size`, `max_grpc_decoding_size`, `query_packets_chunk_size`, `clock_drift`, `max_block_time`, `trusting_period`,
`client_refresh_rate`, `ccv_consumer_chain`, `memo_prefix`, `sequential_batch_tx`, `proof_specs`, `trust_threshold`,
`gas_price`, `packet_filter`, `address_type`, `extension_options`, `compat_mode`, `clear_interval`
for key `chains` at line 424 column 1
```

For this, we now use a custom deserializer for ChainConfig instead of relying on an untagged enum.

* Remove `monostate::MustBe!` hack

* Fix relayer REST mock test

* Remove outdated changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants