fix(ics02)!: modify MsgUpgradeClient to utilize CommitmentProofBytes (#748)

* fix(ics02): modify MsgUpgradeClient to utilize CommitmentProofBytes

* fix: convert upgrade_path properly into vec

Author: Farhad-Shabani

.changelog/unreleased/breaking-changes/739-modify-msg-upgrade-client.md

@@ -0,0 +1,3 @@
+- Modify `MsgUpgradeClient` struct to utilize `CommitmentProofBytes` and
+  apply some refinements around upgrade client methods and impls respectively.
+  ([#739](https://github.com/cosmos/ibc-rs/issues/739))

crates/ibc-derive/src/client_state/traits/client_state_common.rs

@@ -68,7 +68,6 @@ pub(crate) fn impl_ClientStateCommon(
     let ClientType = Imports::ClientType();
     let ClientError = Imports::ClientError();
     let Height = Imports::Height();
-    let MerkleProof = Imports::MerkleProof();
     let Path = Imports::Path();
 
     quote! {
@@ -112,8 +111,8 @@ pub(crate) fn impl_ClientStateCommon(
                 &self,
                 upgraded_client_state: #Any,
                 upgraded_consensus_state: #Any,
-                proof_upgrade_client: #MerkleProof,
-                proof_upgrade_consensus_state: #MerkleProof,
+                proof_upgrade_client: #CommitmentProofBytes,
+                proof_upgrade_consensus_state: #CommitmentProofBytes,
                 root: &#CommitmentRoot,
             ) -> core::result::Result<(), #ClientError> {
                 match self {

crates/ibc-derive/src/client_state/traits/client_state_execution.rs

@@ -39,7 +39,7 @@ pub(crate) fn impl_ClientStateExecution(
         client_state_enum_name,
         enum_variants.iter(),
         opts,
-        quote! { update_state_with_upgrade_client(cs, ctx, client_id, upgraded_client_state, upgraded_consensus_state) },
+        quote! { update_state_on_upgrade(cs, ctx, client_id, upgraded_client_state, upgraded_consensus_state) },
     );
 
     let HostClientState = client_state_enum_name;
@@ -88,7 +88,7 @@ pub(crate) fn impl_ClientStateExecution(
                 }
             }
 
-            fn update_state_with_upgrade_client(
+            fn update_state_on_upgrade(
                 &self,
                 ctx: &mut #ClientExecutionContext,
                 client_id: &#ClientId,

crates/ibc-derive/src/utils.rs

@@ -61,10 +61,6 @@ impl Imports {
         quote! {ibc_proto::google::protobuf::Any}
     }
 
-    pub fn MerkleProof() -> TokenStream {
-        quote! {ibc::core::ics23_commitment::merkle::MerkleProof}
-    }
-
     pub fn Timestamp() -> TokenStream {
         quote! {ibc::core::timestamp::Timestamp}
     }

crates/ibc/src/clients/ics07_tendermint/client_state.rs

@@ -12,7 +12,7 @@ use core::time::Duration;
 
 use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::core::client::v1::Height as RawHeight;
-use ibc_proto::ibc::core::commitment::v1::{MerklePath, MerkleProof as RawMerkleProof};
+use ibc_proto::ibc::core::commitment::v1::MerkleProof as RawMerkleProof;
 use ibc_proto::ibc::lightclients::tendermint::v1::ClientState as RawTmClientState;
 use ibc_proto::protobuf::Protobuf;
 use prost::Message;
@@ -318,12 +318,12 @@ impl ClientStateCommon for ClientState {
         &self,
         upgraded_client_state: Any,
         upgraded_consensus_state: Any,
-        proof_upgrade_client: MerkleProof,
-        proof_upgrade_consensus_state: MerkleProof,
+        proof_upgrade_client: CommitmentProofBytes,
+        proof_upgrade_consensus_state: CommitmentProofBytes,
         root: &CommitmentRoot,
     ) -> Result<(), ClientError> {
         // Make sure that the client type is of Tendermint type `ClientState`
-        let mut upgraded_tm_client_state = Self::try_from(upgraded_client_state.clone())?;
+        let upgraded_tm_client_state = Self::try_from(upgraded_client_state.clone())?;
 
         // Make sure that the consensus type is of Tendermint type `ConsensusState`
         TmConsensusState::try_from(upgraded_consensus_state.clone())?;
@@ -346,57 +346,38 @@ impl ClientStateCommon for ClientState {
             });
         };
 
-        let last_height = self.latest_height().revision_height();
-
-        // Construct the merkle path for the client state
-        let mut client_upgrade_path = upgrade_path.clone();
-        client_upgrade_path.push(UpgradeClientPath::UpgradedClientState(last_height).to_string());
-
-        let client_upgrade_merkle_path = MerklePath {
-            key_path: client_upgrade_path,
-        };
+        let upgrade_path_prefix = CommitmentPrefix::try_from(upgrade_path[0].clone().into_bytes())
+            .map_err(ClientError::InvalidCommitmentProof)?;
 
-        upgraded_tm_client_state.zero_custom_fields();
+        let last_height = self.latest_height().revision_height();
 
         let mut client_state_value = Vec::new();
         upgraded_client_state
             .encode(&mut client_state_value)
             .map_err(ClientError::Encode)?;
 
         // Verify the proof of the upgraded client state
-        proof_upgrade_client
-            .verify_membership(
-                &self.proof_specs,
-                root.clone().into(),
-                client_upgrade_merkle_path,
-                client_state_value,
-                0,
-            )
-            .map_err(ClientError::Ics23Verification)?;
-
-        // Construct the merkle path for the consensus state
-        let mut cons_upgrade_path = upgrade_path;
-        cons_upgrade_path
-            .push(UpgradeClientPath::UpgradedClientConsensusState(last_height).to_string());
-        let cons_upgrade_merkle_path = MerklePath {
-            key_path: cons_upgrade_path,
-        };
+        self.verify_membership(
+            &upgrade_path_prefix,
+            &proof_upgrade_client,
+            root,
+            Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(last_height)),
+            client_state_value,
+        )?;
 
         let mut cons_state_value = Vec::new();
         upgraded_consensus_state
             .encode(&mut cons_state_value)
             .map_err(ClientError::Encode)?;
 
         // Verify the proof of the upgraded consensus state
-        proof_upgrade_consensus_state
-            .verify_membership(
-                &self.proof_specs,
-                root.clone().into(),
-                cons_upgrade_merkle_path,
-                cons_state_value,
-                0,
-            )
-            .map_err(ClientError::Ics23Verification)?;
+        self.verify_membership(
+            &upgrade_path_prefix,
+            &proof_upgrade_consensus_state,
+            root,
+            Path::UpgradeClient(UpgradeClientPath::UpgradedClientConsensusState(last_height)),
+            cons_state_value,
+        )?;
 
         Ok(())
     }
@@ -559,16 +540,18 @@ where
     }
 
     // Commit the new client state and consensus state to the store
-    fn update_state_with_upgrade_client(
+    fn update_state_on_upgrade(
         &self,
         ctx: &mut E,
         client_id: &ClientId,
         upgraded_client_state: Any,
         upgraded_consensus_state: Any,
     ) -> Result<Height, ClientError> {
-        let upgraded_tm_client_state = Self::try_from(upgraded_client_state)?;
+        let mut upgraded_tm_client_state = Self::try_from(upgraded_client_state)?;
         let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?;
 
+        upgraded_tm_client_state.zero_custom_fields();
+
         // Construct new client state and consensus state relayer chosen client
         // parameters are ignored. All chain-chosen parameters come from
         // committed client, all client-chosen parameters come from current

crates/ibc/src/core/ics02_client/client_state.rs

@@ -12,7 +12,6 @@ use crate::core::ics02_client::ClientExecutionContext;
 use crate::core::ics23_commitment::commitment::{
     CommitmentPrefix, CommitmentProofBytes, CommitmentRoot,
 };
-use crate::core::ics23_commitment::merkle::MerkleProof;
 use crate::core::ics24_host::identifier::ClientId;
 use crate::core::ics24_host::path::Path;
 use crate::prelude::*;
@@ -72,8 +71,8 @@ pub trait ClientStateCommon {
         &self,
         upgraded_client_state: Any,
         upgraded_consensus_state: Any,
-        proof_upgrade_client: MerkleProof,
-        proof_upgrade_consensus_state: MerkleProof,
+        proof_upgrade_client: CommitmentProofBytes,
+        proof_upgrade_consensus_state: CommitmentProofBytes,
         root: &CommitmentRoot,
     ) -> Result<(), ClientError>;
 
@@ -193,7 +192,7 @@ where
     ) -> Result<(), ClientError>;
 
     // Update the client state and consensus state in the store with the upgraded ones.
-    fn update_state_with_upgrade_client(
+    fn update_state_on_upgrade(
         &self,
         ctx: &mut E,
         client_id: &ClientId,

crates/ibc/src/core/ics02_client/handler/upgrade_client.rs

@@ -10,7 +10,6 @@ use crate::core::ics02_client::consensus_state::ConsensusState;
 use crate::core::ics02_client::error::ClientError;
 use crate::core::ics02_client::events::UpgradeClient;
 use crate::core::ics02_client::msgs::upgrade_client::MsgUpgradeClient;
-use crate::core::ics23_commitment::merkle::MerkleProof;
 use crate::core::ics24_host::path::ClientConsensusStatePath;
 use crate::core::{ExecutionContext, ValidationContext};
 
@@ -58,17 +57,12 @@ where
         ));
     };
 
-    // Note: verification of proofs that unmarshalled correctly has been done
-    // while decoding the proto message into a `MsgEnvelope` domain type
-    let merkle_proof_upgrade_client = MerkleProof::from(msg.proof_upgrade_client.clone());
-    let merkle_proof_upgrade_cons_state = MerkleProof::from(msg.proof_upgrade_consensus_state);
-
     // Validate the upgraded client state and consensus state and verify proofs against the root
     old_client_state.verify_upgrade_client(
-        msg.client_state.clone(),
-        msg.consensus_state,
-        merkle_proof_upgrade_client,
-        merkle_proof_upgrade_cons_state,
+        msg.upgraded_client_state.clone(),
+        msg.upgraded_consensus_state,
+        msg.proof_upgrade_client,
+        msg.proof_upgrade_consensus_state,
         old_consensus_state.root(),
     )?;
 
@@ -83,11 +77,11 @@ where
 
     let old_client_state = ctx.client_state(&client_id)?;
 
-    let latest_height = old_client_state.update_state_with_upgrade_client(
+    let latest_height = old_client_state.update_state_on_upgrade(
         ctx.get_client_execution_context(),
         &client_id,
-        msg.client_state.clone(),
-        msg.consensus_state,
+        msg.upgraded_client_state.clone(),
+        msg.upgraded_consensus_state,
     )?;
 
     let event = IbcEvent::UpgradeClient(UpgradeClient::new(
@@ -148,8 +142,10 @@ mod tests {
         let msg_with_low_upgrade_height = MsgUpgradeClient::new_dummy(low_upgrade_height);
 
         let msg_with_unknown_upgraded_cs = MsgUpgradeClient {
-            client_state: TmClientState::new_dummy_from_header(get_dummy_tendermint_header())
-                .into(),
+            upgraded_client_state: TmClientState::new_dummy_from_header(
+                get_dummy_tendermint_header(),
+            )
+            .into(),
             ..msg_default.clone()
         };
 
@@ -204,7 +200,7 @@ mod tests {
 
                 let client_state = fxt.ctx.client_state(&fxt.msg.client_id).unwrap();
                 let msg_client_state: AnyClientState =
-                    fxt.msg.client_state.clone().try_into().unwrap();
+                    fxt.msg.upgraded_client_state.clone().try_into().unwrap();
                 assert_eq!(client_state, msg_client_state);
 
                 let consensus_state = fxt
@@ -215,7 +211,7 @@ mod tests {
                     ))
                     .unwrap();
                 let msg_consensus_state: AnyConsensusState =
-                    fxt.msg.consensus_state.clone().try_into().unwrap();
+                    fxt.msg.upgraded_consensus_state.clone().try_into().unwrap();
                 assert_eq!(consensus_state, msg_consensus_state);
             }
         };

crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs

@@ -6,7 +6,6 @@ use core::str::FromStr;
 
 use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::core::client::v1::MsgUpgradeClient as RawMsgUpgradeClient;
-use ibc_proto::ibc::core::commitment::v1::MerkleProof as RawMerkleProof;
 use ibc_proto::protobuf::Protobuf;
 
 use crate::core::ics02_client::error::{ClientError, UpgradeClientError};
@@ -24,14 +23,14 @@ pub struct MsgUpgradeClient {
     // client unique identifier
     pub client_id: ClientId,
     // Upgraded client state
-    pub client_state: Any,
+    pub upgraded_client_state: Any,
     // Upgraded consensus state, only contains enough information
     // to serve as a basis of trust in update logic
-    pub consensus_state: Any,
+    pub upgraded_consensus_state: Any,
     // proof that old chain committed to new client
-    pub proof_upgrade_client: RawMerkleProof,
+    pub proof_upgrade_client: CommitmentProofBytes,
     // proof that old chain committed to new consensus state
-    pub proof_upgrade_consensus_state: RawMerkleProof,
+    pub proof_upgrade_consensus_state: CommitmentProofBytes,
     // signer address
     pub signer: Signer,
 }
@@ -48,17 +47,12 @@ impl Protobuf<RawMsgUpgradeClient> for MsgUpgradeClient {}
 
 impl From<MsgUpgradeClient> for RawMsgUpgradeClient {
     fn from(dm_msg: MsgUpgradeClient) -> RawMsgUpgradeClient {
-        let c_bytes = CommitmentProofBytes::try_from(dm_msg.proof_upgrade_client)
-            .map_or(vec![], |c| c.into());
-        let cs_bytes = CommitmentProofBytes::try_from(dm_msg.proof_upgrade_consensus_state)
-            .map_or(vec![], |c| c.into());
-
         RawMsgUpgradeClient {
             client_id: dm_msg.client_id.to_string(),
-            client_state: Some(dm_msg.client_state),
-            consensus_state: Some(dm_msg.consensus_state),
-            proof_upgrade_client: c_bytes,
-            proof_upgrade_consensus_state: cs_bytes,
+            client_state: Some(dm_msg.upgraded_client_state),
+            consensus_state: Some(dm_msg.upgraded_consensus_state),
+            proof_upgrade_client: dm_msg.proof_upgrade_client.into(),
+            proof_upgrade_consensus_state: dm_msg.proof_upgrade_consensus_state.into(),
             signer: dm_msg.signer.to_string(),
         }
     }
@@ -90,12 +84,10 @@ impl TryFrom<RawMsgUpgradeClient> for MsgUpgradeClient {
         Ok(MsgUpgradeClient {
             client_id: ClientId::from_str(&proto_msg.client_id)
                 .map_err(ClientError::InvalidClientIdentifier)?,
-            client_state: raw_client_state,
-            consensus_state: raw_consensus_state,
-            proof_upgrade_client: RawMerkleProof::try_from(c_bytes)
-                .map_err(UpgradeClientError::InvalidUpgradeClientProof)?,
-            proof_upgrade_consensus_state: RawMerkleProof::try_from(cs_bytes)
-                .map_err(UpgradeClientError::InvalidUpgradeConsensusStateProof)?,
+            upgraded_client_state: raw_client_state,
+            upgraded_consensus_state: raw_consensus_state,
+            proof_upgrade_client: c_bytes,
+            proof_upgrade_consensus_state: cs_bytes,
             signer: proto_msg.signer.into(),
         })
     }
@@ -105,7 +97,7 @@ impl TryFrom<RawMsgUpgradeClient> for MsgUpgradeClient {
 pub mod test_util {
     use super::*;
 
-    use crate::core::ics23_commitment::commitment::test_util::get_dummy_merkle_proof;
+    use crate::core::ics23_commitment::commitment::test_util::get_dummy_commitment_proof_bytes;
     use crate::core::{ics02_client::height::Height, ics24_host::identifier::ClientId};
     use crate::mock::client_state::client_type as mock_client_type;
     use crate::mock::{
@@ -118,10 +110,11 @@ pub mod test_util {
         pub fn new_dummy(upgrade_height: Height) -> Self {
             MsgUpgradeClient {
                 client_id: ClientId::new(mock_client_type(), 0).unwrap(),
-                client_state: MockClientState::new(MockHeader::new(upgrade_height)).into(),
-                consensus_state: MockConsensusState::new(MockHeader::new(upgrade_height)).into(),
-                proof_upgrade_client: get_dummy_merkle_proof(),
-                proof_upgrade_consensus_state: get_dummy_merkle_proof(),
+                upgraded_client_state: MockClientState::new(MockHeader::new(upgrade_height)).into(),
+                upgraded_consensus_state: MockConsensusState::new(MockHeader::new(upgrade_height))
+                    .into(),
+                proof_upgrade_client: get_dummy_commitment_proof_bytes(),
+                proof_upgrade_consensus_state: get_dummy_commitment_proof_bytes(),
                 signer: get_dummy_account_id(),
             }
         }

crates/ibc/src/core/ics23_commitment/commitment.rs

@@ -194,14 +194,16 @@ impl serde::Serialize for CommitmentPrefix {
 
 #[cfg(test)]
 pub mod test_util {
+    use super::CommitmentProofBytes;
     use crate::prelude::*;
     use ibc_proto::ibc::core::commitment::v1::MerkleProof as RawMerkleProof;
     use ibc_proto::ics23::CommitmentProof;
 
-    /// Returns a dummy `RawMerkleProof`, for testing only!
-    pub fn get_dummy_merkle_proof() -> RawMerkleProof {
+    /// Returns a dummy `CommitmentProofBytes`, for testing only!
+    pub fn get_dummy_commitment_proof_bytes() -> CommitmentProofBytes {
         let parsed = CommitmentProof { proof: None };
         let mproofs: Vec<CommitmentProof> = vec![parsed];
-        RawMerkleProof { proofs: mproofs }
+        let raw_mp = RawMerkleProof { proofs: mproofs };
+        raw_mp.try_into().unwrap()
     }
 }