Skip to content

Commit

Permalink
Improve error message when deserializing an invalid chain config (#3803)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
romac authored Jan 23, 2024
1 parent 091fa7a commit d9c939f
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 54 deletions.

This file was deleted.

22 changes: 0 additions & 22 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/relayer-cli/src/chain_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ where
};

Ok(ChainConfig::CosmosSdk(CosmosSdkConfig {
r#type: Default::default(),
id: chain_data.chain_id,
rpc_addr: rpc_data.rpc_address,
grpc_addr: grpc_address,
Expand Down
2 changes: 1 addition & 1 deletion crates/relayer-rest/tests/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ where
Err(e) => panic!("got an error: {e}"),
});

tokio::time::sleep(Duration::from_millis(500)).await;
tokio::time::sleep(Duration::from_millis(200)).await;

let response = reqwest::get(&format!("http://127.0.0.1:{port}{path}"))
.await
Expand Down
1 change: 0 additions & 1 deletion crates/relayer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ strum = { version = "0.25", features = ["derive"] }
tokio-stream = "0.1.14"
once_cell = "1.19.0"
tracing-subscriber = { version = "0.3.14", features = ["fmt", "env-filter", "json"] }
monostate = "0.1.11"

[dependencies.byte-unit]
version = "4.0.19"
Expand Down
6 changes: 0 additions & 6 deletions crates/relayer/src/chain/cosmos/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use core::time::Duration;
use std::path::PathBuf;

use byte_unit::Byte;
use monostate::MustBe;
use serde_derive::{Deserialize, Serialize};
use tendermint_rpc::Url;

Expand All @@ -24,11 +23,6 @@ pub mod error;
#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct CosmosSdkConfig {
/// The type of this chain, must be "CosmosSdk"
/// This is the default if not specified.
#[serde(default)]
pub r#type: MustBe!("CosmosSdk"),

/// The chain's network identifier
pub id: ChainId,

Expand Down
58 changes: 44 additions & 14 deletions crates/relayer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -618,18 +618,14 @@ pub enum EventSourceMode {
},
}

// NOTE:
// To work around a limitation of serde, which does not allow
// to specify a default variant if not tag is present,
// every underlying chain config MUST have a field `r#type` of
// type `monotstate::MustBe!("VariantName")`.
// NOTE: To work around a limitation of serde, which does not allow
// to specify a default variant if not tag is present, we use
// a custom Deserializer impl.
//
// For chains other than CosmosSdk, this field MUST NOT be annotated
// with `#[serde(default)]`.
//
// See https://github.com/serde-rs/serde/issues/2231
#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
#[serde(untagged)]
// IMPORTANT: Do not forget to update the `Deserializer` impl
// below when adding a new chain type.
#[derive(Clone, Debug, PartialEq, Serialize)]
#[serde(tag = "type")]
pub enum ChainConfig {
CosmosSdk(CosmosSdkConfig),
}
Expand Down Expand Up @@ -704,6 +700,41 @@ impl ChainConfig {
}
}

// /!\ Update me when adding a new chain type!
impl<'de> Deserialize<'de> for ChainConfig {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
let mut value = serde_json::Value::deserialize(deserializer)?;

// Remove the `type` key from the TOML value in order for deserialization to work,
// otherwise it would fail with: `unknown field `type`.
let type_value = value
.as_object_mut()
.ok_or_else(|| serde::de::Error::custom("invalid chain config, must be a table"))?
.remove("type")
.unwrap_or_else(|| serde_json::json!("CosmosSdk"));

let type_str = type_value
.as_str()
.ok_or_else(|| serde::de::Error::custom("invalid chain type, must be a string"))?;

match type_str {
"CosmosSdk" => CosmosSdkConfig::deserialize(value)
.map(Self::CosmosSdk)
.map_err(|e| serde::de::Error::custom(format!("invalid CosmosSdk config: {e}"))),

//
// <-- Add new chain types here -->
//
chain_type => Err(serde::de::Error::custom(format!(
"unknown chain type: {chain_type}",
))),
}
}
}

/// Attempt to load and parse the TOML config file as a `Config`.
pub fn load(path: impl AsRef<Path>) -> Result<Config, Error> {
let config_toml = std::fs::read_to_string(&path).map_err(Error::io)?;
Expand Down Expand Up @@ -779,7 +810,6 @@ mod tests {

use super::{load, parse_gas_prices, store_writer};
use crate::config::GasPrice;
use monostate::MustBe;
use test_log::test;

#[test]
Expand Down Expand Up @@ -850,8 +880,8 @@ mod tests {
let config = load(path).expect("could not parse config");

match config.chains[0] {
super::ChainConfig::CosmosSdk(ref config) => {
assert_eq!(config.r#type, MustBe!("CosmosSdk"));
super::ChainConfig::CosmosSdk(_) => {
// all good
}
}
}
Expand Down
1 change: 0 additions & 1 deletion tools/test-framework/src/types/single/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ impl FullNode {
};

Ok(config::ChainConfig::CosmosSdk(CosmosSdkConfig {
r#type: Default::default(),
id: self.chain_driver.chain_id.clone(),
rpc_addr: Url::from_str(&self.chain_driver.rpc_address())?,
grpc_addr: Url::from_str(&self.chain_driver.grpc_address())?,
Expand Down

0 comments on commit d9c939f

Please sign in to comment.