Validation for ICS7 ClientState::new(). (cosmos#111)

* (cosmos#91) WIP: Added validation for ICS7 ClientState::new().

* Bit more progress on validation for ICS7 ClientState::new()

* Initialization for dummy header done. Tests done.

* Added the JSON representation of a dummy signed header.

modules/Cargo.toml

@@ -29,4 +29,4 @@ tracing = "0.1.13"
 
 [dev-dependencies]
 tokio = { version = "0.2", features = ["macros"] }
-
+subtle-encoding = { version = "0.5" }

modules/src/ics02_client/client_type.rs

@@ -2,7 +2,7 @@ use super::error;
 use anomaly::fail;
 use serde_derive::{Deserialize, Serialize};
 
-/// Type of the consensus algorithm
+/// Type of the client, depending on the specific consensus algorithm.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 pub enum ClientType {
     Tendermint = 1,

modules/src/ics02_client/state.rs

@@ -29,12 +29,13 @@ pub trait ClientState {
     /// Type of client associated with this state (eg. Tendermint)
     fn client_type(&self) -> ClientType;
 
-    /// Height of consensus state
+    /// Latest height of consensus state
     fn get_latest_height(&self) -> Height;
 
     /// Freeze status of the client
     fn is_frozen(&self) -> bool;
 
+    // TODO: It's unclear what this function is expected to achieve. Document this.
     fn verify_client_consensus_state(
         &self,
         root: &CommitmentRoot,

modules/src/ics07_tendermint/client_state.rs

@@ -1,6 +1,7 @@
 use crate::ics02_client::client_type::ClientType;
 use crate::ics23_commitment::CommitmentRoot;
 
+use crate::ics07_tendermint::error::{Error, Kind};
 use crate::ics07_tendermint::header::Header;
 use crate::ics24_host::identifier::ClientId;
 use serde_derive::{Deserialize, Serialize};
@@ -9,7 +10,7 @@ use tendermint::lite::Header as liteHeader;
 
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub struct ClientState {
-    id: String,
+    id: ClientId,
     trusting_period: Duration,
     unbonding_period: Duration,
     frozen_height: crate::Height,
@@ -23,22 +24,51 @@ impl ClientState {
         unbonding_period: Duration,
         latest_header: Header,
         frozen_height: crate::Height,
-    ) -> Self {
-        Self {
-            id,
+    ) -> Result<ClientState, Error> {
+        // Basic validation of trusting period and unbonding period: each should be non-zero.
+        if trusting_period <= Duration::new(0, 0) {
+            return Err(Kind::InvalidTrustingPeriod
+                .context("ClientState trusting period must be greater than zero")
+                .into());
+        }
+        if unbonding_period <= Duration::new(0, 0) {
+            return Err(Kind::InvalidUnboundingPeriod
+                .context("ClientState unbonding period must be greater than zero")
+                .into());
+        }
+        if trusting_period >= unbonding_period {
+            return Err(Kind::InvalidUnboundingPeriod
+                .context("ClientState trusting period must be smaller than unbonding period")
+                .into());
+        }
+
+        // Basic validation for the frozen_height parameter.
+        if frozen_height != 0 {
+            return Err(Kind::ValidationError
+                .context("ClientState cannot be frozen at creation time")
+                .into());
+        }
+
+        // Initially, no validation is needed for the `latest_header`. This has to be validated
+        // upon updating a client (see `update_client.rs` and fn
+        // `ClientState::verify_client_consensus_state`).
+
+        Ok(Self {
+            // TODO: Consider adding a specific 'IdentifierError' Kind, akin to the one in ICS04.
+            id: id.parse().map_err(|e| Kind::ValidationError.context(e))?,
             trusting_period,
             unbonding_period,
             latest_header,
             frozen_height,
-        }
+        })
     }
 }
 
 impl crate::ics02_client::state::ClientState for ClientState {
-    type ValidationError = crate::ics07_tendermint::error::Error;
+    type ValidationError = Error;
 
     fn client_id(&self) -> ClientId {
-        self.id.parse().unwrap()
+        self.id.clone()
     }
 
     fn client_type(&self) -> ClientType {
@@ -50,7 +80,8 @@ impl crate::ics02_client::state::ClientState for ClientState {
     }
 
     fn is_frozen(&self) -> bool {
-        false
+        // If 'frozen_height' is set to a non-zero value, then the client state is frozen.
+        self.frozen_height != 0
     }
 
     fn verify_client_consensus_state(
@@ -63,7 +94,11 @@ impl crate::ics02_client::state::ClientState for ClientState {
 
 #[cfg(test)]
 mod tests {
+    use crate::ics07_tendermint::client_state::ClientState;
+    use crate::ics07_tendermint::header::test_util::get_dummy_header;
+    use crate::ics07_tendermint::header::Header;
     use crate::test::test_serialization_roundtrip;
+    use std::time::Duration;
     use tendermint_rpc::endpoint::abci_query::AbciQuery;
 
     #[test]
@@ -79,4 +114,103 @@ mod tests {
         println!("json_data: {:?}", json_data);
         test_serialization_roundtrip::<AbciQuery>(json_data);
     }
+
+    #[test]
+    fn client_state_new() {
+        #[derive(Clone, Debug, PartialEq)]
+        struct ClientStateParams {
+            id: String,
+            trusting_period: Duration,
+            unbonding_period: Duration,
+            latest_header: Header,
+            frozen_height: crate::Height,
+        }
+
+        // Define a "default" set of parameters to reuse throughout these tests.
+        let default_params: ClientStateParams = ClientStateParams {
+            id: "abcdefghijkl".to_string(),
+            trusting_period: Duration::from_secs(64000),
+            unbonding_period: Duration::from_secs(128000),
+            latest_header: get_dummy_header(),
+            frozen_height: 0,
+        };
+
+        struct Test {
+            name: String,
+            params: ClientStateParams,
+            want_pass: bool,
+        }
+
+        let tests: Vec<Test> = vec![
+            Test {
+                name: "Valid parameters".to_string(),
+                params: default_params.clone(),
+                want_pass: true,
+            },
+            Test {
+                name: "Invalid client id".to_string(),
+                params: ClientStateParams {
+                    id: "9000".to_string(),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Invalid frozen height parameter (should be 0)".to_string(),
+                params: ClientStateParams {
+                    frozen_height: 1,
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Invalid unbonding period".to_string(),
+                params: ClientStateParams {
+                    unbonding_period: Duration::from_secs(0),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Invalid (too small) trusting period".to_string(),
+                params: ClientStateParams {
+                    trusting_period: Duration::from_secs(0),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Invalid (too large) trusting period w.r.t. unbonding period".to_string(),
+                params: ClientStateParams {
+                    trusting_period: Duration::from_secs(11),
+                    unbonding_period: Duration::from_secs(10),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+        ]
+        .into_iter()
+        .collect();
+
+        for test in tests {
+            let p = test.params.clone();
+
+            let cs_result = ClientState::new(
+                p.id,
+                p.trusting_period,
+                p.unbonding_period,
+                p.latest_header,
+                p.frozen_height,
+            );
+
+            assert_eq!(
+                test.want_pass,
+                cs_result.is_ok(),
+                "ClientState::new() failed for test {}, \nmsg{:?} with error {:?}",
+                test.name,
+                test.params.clone(),
+                cs_result.err(),
+            );
+        }
+    }
 }

modules/src/ics07_tendermint/error.rs

@@ -14,7 +14,7 @@ pub enum Kind {
     #[error("invalid address")]
     InvalidAddress,
 
-    #[error("invalid header, failed basic validation with its own chain-id")]
+    #[error("invalid header, failed basic validation")]
     InvalidHeader,
 
     #[error("validation error")]

modules/src/ics07_tendermint/header.rs

@@ -25,3 +25,46 @@ impl crate::ics02_client::header::Header for Header {
         self.signed_header.header.height()
     }
 }
+
+#[cfg(test)]
+pub mod test_util {
+    use crate::ics07_tendermint::header::Header;
+    use subtle_encoding::hex;
+    use tendermint::block::signed_header::SignedHeader;
+    use tendermint::validator::Info as ValidatorInfo;
+    use tendermint::validator::Set as ValidatorSet;
+    use tendermint::{vote, PublicKey};
+
+    // TODO: This should be replaced with a ::default() or ::produce().
+    // The implementation of this function comprises duplicate code (code borrowed from
+    // `tendermint-rs` for assembling a Header).
+    // See https://github.com/informalsystems/tendermint-rs/issues/381.
+    pub fn get_dummy_header() -> Header {
+        // Build a SignedHeader from a JSON file.
+        let shdr = serde_json::from_str::<SignedHeader>(include_str!(
+            "../../tests/support/signed_header.json"
+        ))
+        .unwrap();
+
+        // Build a set of validators.
+        // Below are test values inspired form `test_validator_set()` in tendermint-rs.
+        let v1: ValidatorInfo = ValidatorInfo::new(
+            PublicKey::from_raw_ed25519(
+                &hex::decode_upper(
+                    "F349539C7E5EF7C49549B09C4BFC2335318AB0FE51FBFAA2433B4F13E816F4A7",
+                )
+                .unwrap(),
+            )
+            .unwrap(),
+            vote::Power::new(281_815),
+        );
+
+        let vs = ValidatorSet::new(vec![v1]);
+
+        Header {
+            signed_header: shdr,
+            validator_set: vs.clone(),
+            next_validator_set: vs.clone(),
+        }
+    }
+}

modules/src/ics07_tendermint/msgs/create_client.rs

@@ -5,6 +5,8 @@ use crate::ics23_commitment::CommitmentRoot;
 use crate::ics24_host::identifier::ClientId;
 use crate::tx_msg::Msg;
 
+use std::time::Duration;
+
 use serde_derive::{Deserialize, Serialize};
 use tendermint::account::Id as AccountId;
 
@@ -14,17 +16,17 @@ pub const TYPE_MSG_CREATE_CLIENT: &str = "create_client";
 pub struct MsgCreateClient {
     client_id: ClientId,
     header: Header,
-    trusting_period: std::time::Duration,
-    bonding_period: std::time::Duration,
+    trusting_period: Duration,
+    bonding_period: Duration,
     signer: AccountId,
 }
 
 impl MsgCreateClient {
     pub fn new(
         client_id: ClientId,
         header: Header,
-        trusting_period: std::time::Duration,
-        bonding_period: std::time::Duration,
+        trusting_period: Duration,
+        bonding_period: Duration,
         signer: AccountId,
     ) -> Self {
         Self {

modules/tests/support/signed_header.json

@@ -0,0 +1,48 @@
+{
+  "commit": {
+    "height": "9",
+    "round": "0",
+    "block_id": {
+      "hash": "760E050B2404A4BC661635CA552FF45876BCD927C367ADF88961E389C01D32FF",
+      "parts": {
+        "total": "1",
+        "hash": "485070D01F9543827B3F9BAF11BDCFFBFD2BDED0B63D7192FA55649B94A1D5DE"
+      }
+    },
+    "signatures": [
+      {
+        "block_id_flag": 2,
+        "validator_address": "12CC3970B3AE9F19A4B1D98BE1799F2CB923E0A3",
+        "timestamp": "2020-03-15T16:57:08.151Z",
+        "signature": "GRBX/UNaf19vs5byJfAuXk2FQ05soOHmaMFCbrNBhHdNZtFKHp6J9eFwZrrG+YCxKMdqPn2tQWAes6X8kpd1DA=="
+      }
+    ]
+  },
+  "header": {
+    "version": {
+      "block": "10",
+      "app": "0"
+    },
+    "chain_id": "cosmoshub-1",
+    "height": "15",
+    "time": "2019-03-13T23:09:34.503701042Z",
+    "num_txs": "2",
+    "total_txs": "4",
+    "last_block_id": {
+      "hash": "42C70F10EF1835CED7248114514B4EF3D06F0D7FD24F6486E3315DEE310D305C",
+      "parts": {
+        "total": "1",
+        "hash": "F51D1B8E6ED859CE23F6B0539E0101653ED4025B13DAA3E76FCC779D5FD96ABE"
+      }
+    },
+    "last_commit_hash": "C499C138BCABA4D40D68A1446F6E5DE1965E07DF17EEACE1A69C1C9B1B8AC5AB",
+    "data_hash": "AC6A27A91A6EF9057D1A33F7944DD9EDD5FC1A3CA49E04EF0801A17FF01B4412",
+    "validators_hash": "EB25B1ACF639219180EB77AFC67E75A51A7CA0D666123E514B6882EC38868652",
+    "next_validators_hash": "EB25B1ACF639219180EB77AFC67E75A51A7CA0D666123E514B6882EC38868652",
+    "consensus_hash": "29C5629148426FB74676BE07F40F2ED79674A67F5833E4C9CCBF759C9372E99C",
+    "app_hash": "0A5826D21A3B0B341C843A0E4946AC787EC9B42A7DC1BEAA344C03C43943B179",
+    "last_results_hash": "",
+    "evidence_hash": "",
+    "proposer_address": "CC05882978FC5FDD6A7721687E14C0299AE004B8"
+  }
+}