feat(core-client)!: replace store_update_{time,height} by metadata up…

…date (#972) * {store,delete}_update_meta * client_update_meta * log * clippy * error * review * fix unclog Signed-off-by: Farhad Shabani <[email protected]> --------- Signed-off-by: Farhad Shabani <[email protected]> Co-authored-by: Farhad Shabani <[email protected]>
cosmos · Feb 1, 2024 · 0c1e8a9 · 0c1e8a9
1 parent ba7b90c
commit 0c1e8a9
Show file tree

Hide file tree

Showing 15 changed files with 104 additions and 238 deletions.
diff --git a/.changelog/unreleased/breaking-changes/973-update-meta.md b/.changelog/unreleased/breaking-changes/973-update-meta.md
@@ -0,0 +1,6 @@
+- Merge client update time and height modification method pairs into
+  one, that is replace
+  a) client_update_{time,height} by update_meta,
+  b) store_update_{time,height} by store_update_meta and
+  c) delete_update_{time,height} by delete_update_meta.
+  ([\#972](https://github.com/cosmos/ibc-rs/pull/972))
diff --git a/docs/architecture/adr-005-handlers-redesign.md b/docs/architecture/adr-005-handlers-redesign.md
@@ -40,14 +40,14 @@ function updateClient(
 
     clientState.VerifyClientMessage(clientMessage)
     // Validation END
-    
+
     // Execution START
     foundMisbehaviour := clientState.CheckForMisbehaviour(clientMessage)
     if foundMisbehaviour {
         clientState.UpdateStateOnMisbehaviour(clientMessage)
     }
-    else {    
-        clientState.UpdateState(clientMessage) 
+    else {
+        clientState.UpdateState(clientMessage)
     }
     // Execution END
 }
@@ -99,13 +99,13 @@ pub trait Host {
     /// An error type that can represent all host errors.
     type Error;
 
-    /// The Host's key-value store that must provide access to IBC paths. 
+    /// The Host's key-value store that must provide access to IBC paths.
     type KvStore: IbcStore<Self::Error>;
 
     /// An event logging facility.
     type EventLogger: EventLogger<Event=Event<DefaultIbcTypes>>;
 
-    /// Methods to access the store (ro & rw). 
+    /// Methods to access the store (ro & rw).
     fn store(&self) -> &Self::KvStore;
     fn store_mut(&mut self) -> &mut Self::KvStore;
 
@@ -373,11 +373,9 @@ trait ValidationContext {
     /// A hashing function for packet commitments
     fn hash(&self, value: Vec<u8>) -> Vec<u8>;
 
-    /// Returns the time when the client state for the given [`ClientId`] was updated with a header for the given [`Height`]
-    fn client_update_time(&self, client_id: &ClientId, height: Height) -> Result<Timestamp, Error>;
-
-    /// Returns the height when the client state for the given [`ClientId`] was updated with a header for the given [`Height`]
-    fn client_update_height(&self, client_id: &ClientId, height: Height) -> Result<Height, Error>;
+    /// Returns the time and height when the client state for the
+    /// given [`ClientId`] was updated with a header for the given [`Height`]
+    fn update_meta(&self, client_id: &ClientId, height: Height) -> Result<(Timestamp, Height), Error>;
 
     /// Returns a counter on the number of channel ids that have been created thus far.
     /// The value of this counter should increase only via method
@@ -427,22 +425,14 @@ trait ExecutionContext {
     fn increase_client_counter(&mut self);
 
     /// Called upon successful client update.
-    /// Implementations are expected to use this to record the specified time as the time at which
-    /// this update (or header) was processed.
-    fn store_update_time(
-        &mut self,
-        client_id: ClientId,
-        height: Height,
-        timestamp: Timestamp,
-    ) -> Result<(), Error>;
-
-    /// Called upon successful client update.
-    /// Implementations are expected to use this to record the specified height as the height at
-    /// at which this update (or header) was processed.
-    fn store_update_height(
+    ///
+    /// Implementations are expected to use this to record the specified time
+    /// and height as the time at which this update (or header) was processed.
+    fn store_update_meta(
         &mut self,
         client_id: ClientId,
         height: Height,
+        host_timestamp: Timestamp,
         host_height: Height,
     ) -> Result<(), Error>;
 
@@ -550,16 +540,11 @@ pub trait Host {
 
     /// Methods currently in `ClientKeeper`
     fn increase_client_counter(&mut self);
-    fn store_update_time(
-        &mut self,
-        client_id: ClientId,
-        height: Height,
-        timestamp: Timestamp,
-    ) -> Result<(), Error>;
-    fn store_update_height(
+    fn store_update_meta(
         &mut self,
         client_id: ClientId,
         height: Height,
+        host_timestamp: Timestamp,
         host_height: Height,
     ) -> Result<(), Error>;
 
@@ -574,8 +559,7 @@ pub trait Host {
     /// Methods currently in `ChannelReader`
     fn connection_channels(&self, cid: &ConnectionId) -> Result<Vec<(PortId, ChannelId)>, Error>;
     fn hash(&self, value: Vec<u8>) -> Vec<u8>;
-    fn client_update_time(&self, client_id: &ClientId, height: Height) -> Result<Timestamp, Error>;
-    fn client_update_height(&self, client_id: &ClientId, height: Height) -> Result<Height, Error>;
+    fn update_meta(&self, client_id: &ClientId, height: Height) -> Result<(Timestamp, Height), Error>;
     fn channel_counter(&self) -> Result<u64, Error>;
     fn max_expected_time_per_block(&self) -> Duration;
     fn block_delay(&self, delay_period_time: Duration) -> u64;
@@ -626,7 +610,7 @@ pub trait StoreSerde {
     /// Serialize to canonical binary representation
     fn serialize(self) -> Vec<u8>;
 
-    /// Deserialize from bytes 
+    /// Deserialize from bytes
     fn deserialize(value: &[u8]) -> Self;
 }
 

diff --git a/ibc-clients/ics07-tendermint/src/client_state.rs b/ibc-clients/ics07-tendermint/src/client_state.rs
@@ -336,8 +336,7 @@ where
             ),
             tm_consensus_state.into(),
         )?;
-        ctx.store_update_time(client_id.clone(), self.latest_height(), host_timestamp)?;
-        ctx.store_update_height(client_id.clone(), self.latest_height(), host_height)?;
+        ctx.store_update_meta(client_id, self.latest_height(), host_timestamp, host_height)?;
 
         Ok(())
     }
@@ -387,8 +386,7 @@ where
                 ClientStatePath::new(client_id),
                 ClientState::from(new_client_state).into(),
             )?;
-            ctx.store_update_time(client_id.clone(), header_height, host_timestamp)?;
-            ctx.store_update_height(client_id.clone(), header_height, host_height)?;
+            ctx.store_update_meta(client_id, header_height, host_timestamp, host_height)?;
         }
 
         Ok(vec![header_height])
@@ -483,8 +481,7 @@ where
             ),
             TmConsensusState::from(new_consensus_state).into(),
         )?;
-        ctx.store_update_time(client_id.clone(), latest_height, host_timestamp)?;
-        ctx.store_update_height(client_id.clone(), latest_height, host_height)?;
+        ctx.store_update_meta(client_id, latest_height, host_timestamp, host_height)?;
 
         Ok(latest_height)
     }

diff --git a/ibc-clients/ics07-tendermint/src/client_state/update_client.rs b/ibc-clients/ics07-tendermint/src/client_state/update_client.rs
@@ -218,13 +218,9 @@ impl ClientState {
 
             if tm_consensus_state_expiry > host_timestamp {
                 break;
-            } else {
-                let client_id = client_id.clone();
-
-                ctx.delete_consensus_state(client_consensus_state_path)?;
-                ctx.delete_update_time(client_id.clone(), height)?;
-                ctx.delete_update_height(client_id, height)?;
             }
+            ctx.delete_consensus_state(client_consensus_state_path)?;
+            ctx.delete_update_meta(client_id, height)?;
         }
 
         Ok(())

diff --git a/ibc-clients/ics07-tendermint/types/src/error.rs b/ibc-clients/ics07-tendermint/types/src/error.rs
@@ -74,10 +74,8 @@ pub enum Error {
     NotEnoughTrustedValsSigned { reason: VotingPowerTally },
     /// verification failed: `{detail}`
     VerificationError { detail: LightClientErrorDetail },
-    /// Processed time for the client `{client_id}` at height `{height}` not found
-    ProcessedTimeNotFound { client_id: ClientId, height: Height },
-    /// Processed height for the client `{client_id}` at height `{height}` not found
-    ProcessedHeightNotFound { client_id: ClientId, height: Height },
+    /// Processed time or height for the client `{client_id}` at height `{height}` not found
+    UpdateMetaDataNotFound { client_id: ClientId, height: Height },
     /// The given hash of the validators does not matches the given hash in the signed header. Expected: `{signed_header_validators_hash}`, got: `{validators_hash}`
     MismatchValidatorsHashes {
         validators_hash: Hash,

diff --git a/ibc-core/ics02-client/context/src/context.rs b/ibc-core/ics02-client/context/src/context.rs
@@ -12,19 +12,13 @@ use super::consensus_state::ConsensusState;
 /// [crate::client_state::ClientStateValidation] must
 /// inherit from this trait.
 pub trait ClientValidationContext {
-    /// Returns the time when the client state for the given [`ClientId`] was updated with a header for the given [`Height`]
-    fn client_update_time(
+    /// Returns the time and height when the client state for the given
+    /// [`ClientId`] was updated with a header for the given [`Height`]
+    fn update_meta(
         &self,
         client_id: &ClientId,
-        height: &Height,
-    ) -> Result<Timestamp, ContextError>;
-
-    /// Returns the height when the client state for the given [`ClientId`] was updated with a header for the given [`Height`]
-    fn client_update_height(
-        &self,
-        client_id: &ClientId,
-        height: &Height,
-    ) -> Result<Height, ContextError>;
+        height: Height,
+    ) -> Result<(Timestamp, Height), ContextError>;
 }
 
 /// Defines the methods that all client `ExecutionContext`s (precisely the
@@ -61,40 +55,27 @@ pub trait ClientExecutionContext: Sized {
     ) -> Result<(), ContextError>;
 
     /// Called upon successful client update.
-    /// Implementations are expected to use this to record the specified time as the time at which
-    /// this update (or header) was processed.
-    fn store_update_time(
+    ///
+    /// Implementations are expected to use this to record the specified time
+    /// and height as the time at which this update (or header) was processed.
+    fn store_update_meta(
         &mut self,
-        client_id: ClientId,
+        client_id: &ClientId,
         height: Height,
         host_timestamp: Timestamp,
-    ) -> Result<(), ContextError>;
-
-    /// Called upon successful client update.
-    /// Implementations are expected to use this to record the specified height as the height at
-    /// at which this update (or header) was processed.
-    fn store_update_height(
-        &mut self,
-        client_id: ClientId,
-        height: Height,
         host_height: Height,
     ) -> Result<(), ContextError>;
 
-    /// Delete the update time associated with the client at the specified height. This update
-    /// time should be associated with a consensus state through the specified height.
+    /// Delete the update time and height associated with the client at the
+    /// specified height.
+    ///
+    /// This update time should be associated with a consensus state through the
+    /// specified height.
     ///
     /// Note that this timestamp is determined by the host.
-    fn delete_update_time(
-        &mut self,
-        client_id: ClientId,
-        height: Height,
-    ) -> Result<(), ContextError>;
-
-    /// Delete the update height associated with the client at the specified height. This update
-    /// time should be associated with a consensus state through the specified height.
-    fn delete_update_height(
+    fn delete_update_meta(
         &mut self,
-        client_id: ClientId,
+        client_id: &ClientId,
         height: Height,
     ) -> Result<(), ContextError>;
 }
diff --git a/ibc-core/ics02-client/types/src/error.rs b/ibc-core/ics02-client/types/src/error.rs
@@ -26,10 +26,8 @@ pub enum ClientError {
     ClientStateAlreadyExists { client_id: ClientId },
     /// consensus state not found at: `{client_id}` at height `{height}`
     ConsensusStateNotFound { client_id: ClientId, height: Height },
-    /// Processed time for the client `{client_id}` at height `{height}` not found
-    ProcessedTimeNotFound { client_id: ClientId, height: Height },
-    /// Processed height for the client `{client_id}` at height `{height}` not found
-    ProcessedHeightNotFound { client_id: ClientId, height: Height },
+    /// Processed time or height for the client `{client_id}` at height `{height}` not found
+    UpdateMetaDataNotFound { client_id: ClientId, height: Height },
     /// header verification failed with reason: `{reason}`
     HeaderVerificationFailure { reason: String },
     /// failed to build trust threshold from fraction: `{numerator}`/`{denominator}`

diff --git a/ibc-core/ics03-connection/src/delay.rs b/ibc-core/ics03-connection/src/delay.rs
@@ -19,19 +19,16 @@ where
 
     // Fetch the latest time and height that the counterparty client was updated on the host chain.
     let client_id = connection_end.client_id();
-    let last_client_update_time = ctx
+    let last_client_update = ctx
         .get_client_validation_context()
-        .client_update_time(client_id, &packet_proof_height)?;
-    let last_client_update_height = ctx
-        .get_client_validation_context()
-        .client_update_height(client_id, &packet_proof_height)?;
+        .update_meta(client_id, packet_proof_height)?;
 
     // Fetch the connection delay time and height periods.
     let conn_delay_time_period = connection_end.delay_period();
     let conn_delay_height_period = ctx.block_delay(&conn_delay_time_period);
 
     // Verify that the current host chain time is later than the last client update time
-    let earliest_valid_time = (last_client_update_time + conn_delay_time_period)
+    let earliest_valid_time = (last_client_update.0 + conn_delay_time_period)
         .map_err(ConnectionError::TimestampOverflow)?;
     if current_host_time < earliest_valid_time {
         return Err(ContextError::ConnectionError(
@@ -43,7 +40,7 @@ where
     }
 
     // Verify that the current host chain height is later than the last client update height
-    let earliest_valid_height = last_client_update_height.add(conn_delay_height_period);
+    let earliest_valid_height = last_client_update.1.add(conn_delay_height_period);
     if current_host_height < earliest_valid_height {
         return Err(ContextError::ConnectionError(
             ConnectionError::NotEnoughBlocksElapsed {

diff --git a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs
@@ -350,8 +350,12 @@ where
             new_consensus_state.into(),
         )?;
         ctx.store_client_state(ClientStatePath::new(client_id), new_client_state.into())?;
-        ctx.store_update_time(client_id.clone(), header_height, ctx.host_timestamp()?)?;
-        ctx.store_update_height(client_id.clone(), header_height, ctx.host_height()?)?;
+        ctx.store_update_meta(
+            client_id,
+            header_height,
+            ctx.host_timestamp()?,
+            ctx.host_height()?,
+        )?;
 
         Ok(vec![header_height])
     }
@@ -395,8 +399,7 @@ where
         let host_timestamp = ctx.host_timestamp()?;
         let host_height = ctx.host_height()?;
 
-        ctx.store_update_time(client_id.clone(), latest_height, host_timestamp)?;
-        ctx.store_update_height(client_id.clone(), latest_height, host_height)?;
+        ctx.store_update_meta(client_id, latest_height, host_timestamp, host_height)?;
 
         Ok(latest_height)
     }