Fix upgrade path check

ibc-clients/ics07-tendermint/src/client_state/common.rs

@@ -11,7 +11,9 @@
 use ibc_core_commitment_types::proto::ics23::{HostFunctionsManager, HostFunctionsProvider};
 use ibc_core_commitment_types::specs::ProofSpecs;
 use ibc_core_host::types::identifiers::ClientType;
-use ibc_core_host::types::path::{Path, PathBytes, UpgradeClientPath};
+use ibc_core_host::types::path::{
+    Path, PathBytes, UpgradeClientStatePath, UpgradeConsensusStatePath, UPGRADED_IBC_STATE,
+};
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::Any;
 use ibc_primitives::ToVec;
@@ -44,24 +46,13 @@
         proof_upgrade_consensus_state: CommitmentProofBytes,
         root: &CommitmentRoot,
     ) -> Result<(), ClientError> {
-        let last_height = self.latest_height().revision_height();
-
-        let upgrade_client_path_bytes = self.serialize_path(Path::UpgradeClient(
-            UpgradeClientPath::UpgradedClientState(last_height),
-        ))?;
-
-        let upgrade_consensus_path_bytes = self.serialize_path(Path::UpgradeClient(
-            UpgradeClientPath::UpgradedClientConsensusState(last_height),
-        ))?;
-
         verify_upgrade_client::<HostFunctionsManager>(
             self.inner(),
             upgraded_client_state,
             upgraded_consensus_state,
             proof_upgrade_client,
             proof_upgrade_consensus_state,
-            upgrade_client_path_bytes,
-            upgrade_consensus_path_bytes,
+            self.latest_height(),
             root,
         )
     }
@@ -163,8 +154,7 @@
     upgraded_consensus_state: Any,
     proof_upgrade_client: CommitmentProofBytes,
     proof_upgrade_consensus_state: CommitmentProofBytes,
-    upgrade_client_path_bytes: PathBytes,
-    upgrade_consensus_path_bytes: PathBytes,
+    last_height: Height,
     root: &CommitmentRoot,
 ) -> Result<(), ClientError> {
     // Make sure that the client type is of Tendermint type `ClientState`
@@ -186,16 +176,42 @@
         })?
     }
 
-    // Check to see if the upgrade path is set
-    let mut upgrade_path = client_state.upgrade_path.clone();
-
-    if upgrade_path.pop().is_none() {
-        return Err(ClientError::ClientSpecific {
-            description: "cannot upgrade client as no upgrade path has been set".to_string(),
-        });
+    // The client state's upgrade path vector needs to parsed into a tuple in the form
+    // of `(upgrade_path_prefix, upgrade_path)`. Given the length of the client
+    // state's upgrade path vector, the following determinations are made:
+    // 0: The tuple defaults to an empty commitment prefix and `UPGRADED_IBC_STATE` as the path.
+    // 1: The commitment prefix is left empty and the upgrade path is used as-is.
+    // 2: The commitment prefix and upgrade path are both taken as-is.
+    let upgrade_path = &client_state.upgrade_path;
+    let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() {
+        0 => (CommitmentPrefix::empty(), UPGRADED_IBC_STATE),
+        1 => (CommitmentPrefix::empty(), upgrade_path[0].as_ref()),
+        2 => (
+            upgrade_path[0].as_bytes().to_vec().into(),
+            upgrade_path[1].as_ref(),
+        ),
+        _ => {
+            return Err(ClientError::ClientSpecific {
+                description: "upgrade client failed: upgrade path is too long".to_string(),
+            })
+        }
     };
 
-    let upgrade_path_prefix = CommitmentPrefix::from(upgrade_path[0].clone().into_bytes());
+    let upgrade_client_path_bytes = Path::UpgradeClientState(UpgradeClientStatePath {
+        upgrade_path: upgrade_path.to_string(),
+        height: last_height.revision_height(),
+    })
+    .to_string()
+    .into_bytes()
+    .into();
+
+    let upgrade_consensus_path_bytes = Path::UpgradeConsensusState(UpgradeConsensusStatePath {
+        upgrade_path: upgrade_path.to_string(),
+        height: last_height.revision_height(),
+    })
+    .to_string()
+    .into_bytes()
+    .into();
 
     // Verify the proof of the upgraded client state
     verify_membership::<H>(

ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs

@@ -7,7 +7,7 @@
 
 use ibc_core_client_context::ClientValidationContext;
 use ibc_core_client_types::error::UpgradeClientError;
-use ibc_core_host_types::path::UpgradeClientPath;
+use ibc_core_host_types::path::{UpgradeClientStatePath, UpgradeConsensusStatePath};
 
 use super::Plan;
 
@@ -28,13 +28,13 @@ pub trait UpgradeValidationContext {
     /// Returns the upgraded client state at the specified upgrade path.
     fn upgraded_client_state(
         &self,
-        upgrade_path: &UpgradeClientPath,
+        upgrade_path: &UpgradeClientStatePath,
     ) -> Result<UpgradedClientStateRef<Self>, UpgradeClientError>;
 
     /// Returns the upgraded consensus state at the specified upgrade path.
     fn upgraded_consensus_state(
         &self,
-        upgrade_path: &UpgradeClientPath,
+        upgrade_path: &UpgradeConsensusStatePath,
     ) -> Result<UpgradedConsensusStateRef<Self>, UpgradeClientError>;
 }
 
@@ -50,14 +50,14 @@ pub trait UpgradeExecutionContext: UpgradeValidationContext {
     /// Stores the upgraded client state at the specified upgrade path.
     fn store_upgraded_client_state(
         &mut self,
-        upgrade_path: UpgradeClientPath,
+        upgrade_path: UpgradeClientStatePath,
         client_state: UpgradedClientStateRef<Self>,
     ) -> Result<(), UpgradeClientError>;
 
     /// Stores the upgraded consensus state at the specified upgrade path.
     fn store_upgraded_consensus_state(
         &mut self,
-        upgrade_path: UpgradeClientPath,
+        upgrade_path: UpgradeConsensusStatePath,
         consensus_state: UpgradedConsensusStateRef<Self>,
     ) -> Result<(), UpgradeClientError>;
 }

ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs

@@ -1,6 +1,6 @@
 use ibc_client_tendermint::types::ClientState as TmClientState;
 use ibc_core_client_types::error::UpgradeClientError;
-use ibc_core_host_types::path::UpgradeClientPath;
+use ibc_core_host_types::path::{UpgradeClientStatePath, UPGRADED_IBC_STATE};
 use ibc_primitives::prelude::*;
 use tendermint::abci::Event as TmEvent;
 
@@ -37,7 +37,10 @@
 
     ctx.schedule_upgrade(plan.clone())?;
 
-    let upgraded_client_state_path = UpgradeClientPath::UpgradedClientState(plan.height);
+    let upgraded_client_state_path = UpgradeClientStatePath {
+        upgrade_path: UPGRADED_IBC_STATE.to_string(),
+        height: plan.height,
+    };
 
     ctx.store_upgraded_client_state(upgraded_client_state_path, client_state.into())?;
 

ibc-core/ics24-host/types/src/path.rs

@@ -99,7 +99,8 @@
     Commitment(CommitmentPath),
     Ack(AckPath),
     Receipt(ReceiptPath),
-    UpgradeClient(UpgradeClientPath),
+    UpgradeClientState(UpgradeClientStatePath),
+    UpgradeConsensusState(UpgradeConsensusStatePath),
 }
 
 #[cfg_attr(
@@ -693,13 +694,31 @@
     derive(borsh::BorshSerialize, borsh::BorshDeserialize)
 )]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-/// Paths that are specific for client upgrades.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)]
-pub enum UpgradeClientPath {
-    #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_STATE}")]
-    UpgradedClientState(u64),
-    #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_CONSENSUS_STATE}")]
-    UpgradedClientConsensusState(u64),
+#[display(fmt = "{upgrade_path}/{height}/{UPGRADED_CLIENT_STATE}")]
+pub struct UpgradeClientStatePath {
+    pub upgrade_path: String,
+    pub height: u64,
+}
+
+#[cfg_attr(
+    feature = "parity-scale-codec",
+    derive(
+        parity_scale_codec::Encode,
+        parity_scale_codec::Decode,
+        scale_info::TypeInfo
+    )
+)]
+#[cfg_attr(
+    feature = "borsh",
+    derive(borsh::BorshSerialize, borsh::BorshDeserialize)
+)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)]
+#[display(fmt = "{upgrade_path}/{height}/{UPGRADED_CLIENT_CONSENSUS_STATE}")]
+pub struct UpgradeConsensusStatePath {
+    pub upgrade_path: String,
+    pub height: u64,
 }
 
 #[cfg_attr(
@@ -760,7 +779,8 @@
             .or_else(|| parse_commitments(&components))
             .or_else(|| parse_acks(&components))
             .or_else(|| parse_receipts(&components))
-            .or_else(|| parse_upgrades(&components))
+            .or_else(|| parse_upgrade_client_state(&components))
+            .or_else(|| parse_upgrade_consensus_state(&components))
             .ok_or(PathError::ParseFailure {
                 path: s.to_string(),
             })
@@ -1084,28 +1104,52 @@
     )
 }
 
-fn parse_upgrades(components: &[&str]) -> Option<Path> {
+fn parse_upgrade_client_state(components: &[&str]) -> Option<Path> {
     if components.len() != 3 {
         return None;
     }
 
-    let first = *components.first()?;
+    let last = *components.last()?;
 
-    if first != UPGRADED_IBC_STATE {
+    if last != UPGRADED_CLIENT_STATE {
         return None;
     }
 
-    let last = *components.last()?;
+    let upgrade_path = components.first()?.to_string();
 
-    let height = components[1].parse::<u64>().ok()?;
+    let height = u64::from_str(components[1]).ok()?;
 
-    match last {
-        UPGRADED_CLIENT_STATE => Some(UpgradeClientPath::UpgradedClientState(height).into()),
-        UPGRADED_CLIENT_CONSENSUS_STATE => {
-            Some(UpgradeClientPath::UpgradedClientConsensusState(height).into())
+    Some(
+        UpgradeClientStatePath {
+            upgrade_path,
+            height,
         }
-        _ => None,
+        .into(),
+    )
+}
+
+fn parse_upgrade_consensus_state(components: &[&str]) -> Option<Path> {
+    if components.len() != 3 {
+        return None;
+    }
+
+    let last = *components.last()?;
+
+    if last != UPGRADED_CLIENT_CONSENSUS_STATE {
+        return None;
     }
+
+    let upgrade_path = components.first()?.to_string();
+
+    let height = u64::from_str(components[1]).ok()?;
+
+    Some(
+        UpgradeConsensusStatePath {
+            upgrade_path,
+            height,
+        }
+        .into(),
+    )
 }
 
 #[cfg(test)]
@@ -1207,11 +1251,17 @@
     )]
     #[case(
         "upgradedIBCState/0/upgradedClient",
-        Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(0))
+        Path::UpgradeClientState(UpgradeClientStatePath {
+            upgrade_path: UPGRADED_IBC_STATE.to_string(),
+            height: 0,
+        })
     )]
     #[case(
         "upgradedIBCState/0/upgradedConsState",
-        Path::UpgradeClient(UpgradeClientPath::UpgradedClientConsensusState(0))
+        Path::UpgradeConsensusState(UpgradeConsensusStatePath {
+            upgrade_path: UPGRADED_IBC_STATE.to_string(),
+            height: 0,
+        })
     )]
     fn test_successful_parsing(#[case] path_str: &str, #[case] path: Path) {
         // can be parsed into Path
@@ -1419,25 +1469,30 @@
     }
 
     #[test]
-    fn test_parse_upgrades_fn() {
+    fn test_parse_upgrade_client_state_fn() {
         let path = "upgradedIBCState/0/upgradedClient";
         let components: Vec<&str> = path.split('/').collect();
 
         assert_eq!(
-            parse_upgrades(&components),
-            Some(Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(
-                0
-            ))),
+            parse_upgrade_client_state(&components),
+            Some(Path::UpgradeClientState(UpgradeClientStatePath {
+                upgrade_path: UPGRADED_IBC_STATE.to_string(),
+                height: 0,
+            })),
         );
+    }
 
+    #[test]
+    fn test_parse_upgrade_consensus_state_fn() {
         let path = "upgradedIBCState/0/upgradedConsState";
         let components: Vec<&str> = path.split('/').collect();
 
         assert_eq!(
-            parse_upgrades(&components),
-            Some(Path::UpgradeClient(
-                UpgradeClientPath::UpgradedClientConsensusState(0)
-            )),
+            parse_upgrade_consensus_state(&components),
+            Some(Path::UpgradeConsensusState(UpgradeConsensusStatePath {
+                upgrade_path: UPGRADED_IBC_STATE.to_string(),
+                height: 0,
+            })),
         )
     }
 }