Check query parameters but not against the config (#78)

* Check query parameters but not against the config Fixed parameter verification order to be consistent Simplified the code * Fix error string * Added some tests * Fix clippy and test errors tendermint-rs changed the AbciQuery.value to non-option * add forgotten test fixture * attempt to fix codecov * Minor fix in error msg Co-authored-by: Shivani Joshi <[email protected]>
octopus-network · May 25, 2020 · e8cf761 · e8cf761
1 parent 8feb8dc
commit e8cf761
Show file tree

Hide file tree

Showing 8 changed files with 307 additions and 93 deletions.
diff --git a/codecov.yml b/codecov.yml
@@ -1,5 +1,6 @@
 codecov:
   require_ci_to_pass: yes
+  allow_coverage_offsets: true
 
 ignore:
 

diff --git a/modules/src/ics02_client/query.rs b/modules/src/ics02_client/query.rs
@@ -88,19 +88,12 @@ where
         query: QueryClientFullState<CLS>,
         response: AbciQuery,
     ) -> Result<Self, error::Error> {
-        match (response.value, &response.proof) {
-            (Some(value), _) => {
-                let client_state = amino_unmarshal_binary_length_prefixed(&value)?;
-
-                Ok(ClientFullStateResponse::new(
-                    query.client_id,
-                    client_state,
-                    response.proof,
-                    response.height.into(),
-                ))
-            }
-            (None, _) => Err(error::Kind::Rpc.context("Bad response").into()),
-        }
+        Ok(ClientFullStateResponse::new(
+            query.client_id,
+            amino_unmarshal_binary_length_prefixed(&response.value)?,
+            response.proof,
+            response.height.into(),
+        ))
     }
 }
 
@@ -192,18 +185,11 @@ where
         query: QueryClientConsensusState<CS>,
         response: AbciQuery,
     ) -> Result<Self, error::Error> {
-        match (response.value, &response.proof) {
-            (Some(value), _) => {
-                let consensus_state = amino_unmarshal_binary_length_prefixed(&value)?;
-
-                Ok(ConsensusStateResponse::new(
-                    query.client_id,
-                    consensus_state,
-                    response.proof,
-                    response.height.into(),
-                ))
-            }
-            (None, _) => Err(error::Kind::Rpc.context("Bad response").into()),
-        }
+        Ok(ConsensusStateResponse::new(
+            query.client_id,
+            amino_unmarshal_binary_length_prefixed(&response.value)?,
+            response.proof,
+            response.height.into(),
+        ))
     }
 }
diff --git a/modules/src/ics04_channel/msgs.rs b/modules/src/ics04_channel/msgs.rs
@@ -146,7 +146,7 @@ mod tests {
             Test {
                 name: "Bad channel, name too short".to_string(),
                 params: OpenInitParams {
-                    channel_id: "connone".to_string(),
+                    channel_id: "chshort".to_string(),
                     ..default_params.clone()
                 },
                 want_pass: false,

diff --git a/modules/src/ics04_channel/query.rs b/modules/src/ics04_channel/query.rs
@@ -79,20 +79,13 @@ impl ChannelResponse {
 
 impl IbcResponse<QueryChannel> for ChannelResponse {
     fn from_abci_response(query: QueryChannel, response: AbciQuery) -> Result<Self, error::Error> {
-        match (response.value, &response.proof) {
-            (Some(value), _) => {
-                let channel = amino_unmarshal_binary_length_prefixed(&value)?;
-
-                Ok(ChannelResponse::new(
-                    query.port_id,
-                    query.channel_id,
-                    channel,
-                    response.proof,
-                    response.height.into(),
-                ))
-            }
-            (None, _) => Err(error::Kind::Rpc.context("Bad response").into()),
-        }
+        Ok(ChannelResponse::new(
+            query.port_id,
+            query.channel_id,
+            amino_unmarshal_binary_length_prefixed(&response.value)?,
+            response.proof,
+            response.height.into(),
+        ))
     }
 }
 

diff --git a/relayer/cli/Cargo.toml b/relayer/cli/Cargo.toml
@@ -29,4 +29,4 @@ version = "0.5.2"
 
 [dev-dependencies]
 abscissa_core = { version = "0.5.2", features = ["testing"] }
-once_cell = "1.2"
+once_cell = "1.2"
diff --git a/relayer/cli/src/commands/query/channel.rs b/relayer/cli/src/commands/query/channel.rs
@@ -8,9 +8,10 @@ use relayer_modules::ics24_host::identifier::{ChannelId, PortId};
 
 use crate::commands::utils::block_on;
 use relayer::chain::tendermint::TendermintChain;
+use relayer_modules::ics24_host::error::ValidationError;
 use tendermint::chain::Id as ChainId;
 
-#[derive(Command, Debug, Options)]
+#[derive(Clone, Command, Debug, Options)]
 pub struct QueryChannelEndsCmd {
     #[options(free, help = "identifier of the chain to query")]
     chain_id: Option<ChainId>,
@@ -41,32 +42,42 @@ impl QueryChannelEndsCmd {
         &self,
         config: &Config,
     ) -> Result<(ChainConfig, QueryChannelOptions), String> {
-        match (&self.chain_id, &self.port_id, &self.channel_id) {
-            (Some(chain_id), Some(port_id), Some(channel_id)) => {
-                let chain_config = config.chains.iter().find(|c| c.id == *chain_id);
-                match chain_config {
-                    Some(chain_config) => {
-                        let opts = QueryChannelOptions {
-                            port_id: port_id.parse().unwrap(),
-                            channel_id: channel_id.parse().unwrap(),
-                            height: match self.height {
-                                Some(h) => h,
-                                None => 0 as u64,
-                            },
-                            proof: match self.proof {
-                                Some(proof) => proof,
-                                None => true,
-                            },
-                        };
-                        Ok((chain_config.clone(), opts))
-                    }
-                    _ => Err(format!("cannot find chain {} in config", chain_id)),
-                }
-            }
-            (None, _, _) => Err("missing chain identifier".to_string()),
-            (_, None, _) => Err("missing port identifier".to_string()),
-            (_, _, None) => Err("missing channel identifier".to_string()),
-        }
+        let chain_id = self
+            .chain_id
+            .ok_or_else(|| "missing chain identifier".to_string())?;
+        let chain_config = config
+            .chains
+            .iter()
+            .find(|c| c.id == chain_id)
+            .ok_or_else(|| "missing chain configuration".to_string())?;
+
+        let port_id = self
+            .port_id
+            .as_ref()
+            .ok_or_else(|| "missing port identifier".to_string())?
+            .parse()
+            .map_err(|err: ValidationError| err.to_string())?;
+
+        let channel_id = self
+            .channel_id
+            .as_ref()
+            .ok_or_else(|| "missing channel identifier".to_string())?
+            .parse()
+            .map_err(|err: ValidationError| err.to_string())?;
+
+        let opts = QueryChannelOptions {
+            port_id,
+            channel_id,
+            height: match self.height {
+                Some(h) => h,
+                None => 0 as u64,
+            },
+            proof: match self.proof {
+                Some(proof) => proof,
+                None => true,
+            },
+        };
+        Ok((chain_config.clone(), opts))
     }
 }
 
@@ -105,3 +116,112 @@ impl Runnable for QueryChannelEndsCmd {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::commands::query::channel::QueryChannelEndsCmd;
+    use relayer::config::parse;
+
+    #[test]
+    fn parse_query_ends_parameters() {
+        let default_params = QueryChannelEndsCmd {
+            chain_id: Some("ibc0".to_string().parse().unwrap()),
+            port_id: Some("transfer".to_string().parse().unwrap()),
+            channel_id: Some("testchannel".to_string().parse().unwrap()),
+            height: None,
+            proof: None,
+        };
+
+        struct Test {
+            name: String,
+            params: QueryChannelEndsCmd,
+            want_pass: bool,
+        }
+
+        let tests: Vec<Test> = vec![
+            Test {
+                name: "Good parameters".to_string(),
+                params: default_params.clone(),
+                want_pass: true,
+            },
+            Test {
+                name: "No chain specified".to_string(),
+                params: QueryChannelEndsCmd {
+                    chain_id: None,
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Chain not configured".to_string(),
+                params: QueryChannelEndsCmd {
+                    chain_id: Some("notibc0oribc1".to_string().parse().unwrap()),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "No port id specified".to_string(),
+                params: QueryChannelEndsCmd {
+                    port_id: None,
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Bad port, non-alpha".to_string(),
+                params: QueryChannelEndsCmd {
+                    port_id: Some("p34".to_string()),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "No channel id specified".to_string(),
+                params: QueryChannelEndsCmd {
+                    channel_id: None,
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Bad channel, name too short".to_string(),
+                params: QueryChannelEndsCmd {
+                    channel_id: Some("chshort".to_string()),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+        ]
+        .into_iter()
+        .collect();
+
+        let path = concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/tests/fixtures/two_chains.toml"
+        );
+
+        let config = parse(path).unwrap();
+
+        for test in tests {
+            let res = test.params.validate_options(&config);
+
+            match res {
+                Ok(_res) => {
+                    assert!(
+                        test.want_pass,
+                        "validate_options should have failed for test {}",
+                        test.name
+                    );
+                }
+                Err(err) => {
+                    assert!(
+                        !test.want_pass,
+                        "validate_options failed for test {}, \nerr {}",
+                        test.name, err
+                    );
+                }
+            }
+        }
+    }
+}