imp(ibc)!: refactor Timestamp type and distinguish timeout timestamp (#1307)

* imp: refactor Timestamp type and distinguish timeout timestamp

* fix: timestamp tests

* fix: correct serde impls + tests

* imp: From<u64> nevel fails + tests

* fix: From<u64> for TimeoutTimestamp never fails

* fix: correct from_unix_timestamp to reject large seconds + remove From<u64> for Timestamp

* chore: apply review comments

* fix: cargo doc

* Fix typo in doc comment

* fix: remove redundant negative check in Sub impl

* tests: add timout arithmetic tests

* docs: add a comment for From<u64> for TimeoutTimestamp

* fix: revert client_processed_times signature

* chore: add changelog

---------

Co-authored-by: Sean Chen <[email protected]>

Author: Farhad-Shabani

.changelog/unreleased/breaking-changes/1296-decouple-host-timestamp-from-packet-timout.md

@@ -0,0 +1,3 @@
+- [ibc-core] Separate the packet timeout timestamp from the host `Timestamp` by
+  defining a new specific `TimeoutTimestamp`.
+  ([\#1296](https://github.com/cosmos/ibc-rs/issues/1296))

.changelog/unreleased/breaking-changes/180-decouple-timestamp-definition-from-tendermint-time.md

@@ -0,0 +1,2 @@
+- [ibc-primitive] Decouple `Timestamp` definition from `tendermint::Time`.
+  ([\#180](https://github.com/cosmos/ibc-rs/issues/180))

.changelog/unreleased/bug-fixes/1306-fix-timestamp-nanoseconds-when-tm-time-brings-neg-values.md

@@ -0,0 +1,3 @@
+- [ibc-core] Prevent `Timestamp::nanoseconds` from panicking by disallowing
+  negative values from `tendermint::Time`.
+  ([\#1306](https://github.com/cosmos/ibc-rs/issues/1306))

Makefile

@@ -14,7 +14,7 @@ lint: ## Lint the code using rustfmt, clippy and whitespace lints.
 	$(MAKE) fmt
 	$(MAKE) clippy
 	$(MAKE) lint-toml
-	$(MAKE) -C ./cosmwasm lint $@
+	$(MAKE) -C ./cosmwasm lint
 	bash ./ci/code-quality/whitespace-lints.sh
 
 fmt: ## Format the code using nightly rustfmt.

ci/no-std-check/Cargo.lock

@@ -64,9 +64,7 @@ where
             description: "time key cannot be converted to u64".to_string(),
         })?);
 
-        let timestamp = Timestamp::from_nanoseconds(time).map_err(|e| ClientError::Other {
-            description: e.to_string(),
-        })?;
+        let timestamp = Timestamp::from_nanoseconds(time);
 
         let height_key = self.client_update_height_key(height);
 

cosmwasm/ibc-clients/cw-context/src/context/client_ctx.rs

@@ -21,10 +21,7 @@ where
     fn host_timestamp(&self) -> Result<Timestamp, ContextError> {
         let time = self.env().block.time;
 
-        let host_timestamp =
-            Timestamp::from_nanoseconds(time.nanos()).map_err(|e| ClientError::Other {
-                description: e.to_string(),
-            })?;
+        let host_timestamp = Timestamp::from_nanoseconds(time.nanos());
 
         Ok(host_timestamp)
     }

cosmwasm/ibc-clients/cw-context/src/context/custom_ctx.rs

@@ -17,7 +17,7 @@ pub fn dummy_checksum() -> Binary {
 pub fn dummy_sov_consensus_state(timestamp: IbcTimestamp) -> ConsensusState {
     ConsensusState::new(
         vec![0].into(),
-        timestamp.into_tm_time().expect("Time exists"),
+        timestamp.into_tm_time(),
         // Hash of default validator set
         Hash::from_str("D6B93922C33AAEBEC9043566CB4B1B48365B1358B67C7DEF986D9EE1861BC143")
             .expect("Never fails"),

cosmwasm/ibc-clients/ics07-tendermint/src/tests/helper.rs

@@ -1,11 +1,10 @@
 //! Defines the token transfer message type
 
 use ibc_core::channel::types::error::PacketError;
-use ibc_core::channel::types::timeout::TimeoutHeight;
+use ibc_core::channel::types::timeout::{TimeoutHeight, TimeoutTimestamp};
 use ibc_core::handler::types::error::ContextError;
 use ibc_core::host::types::identifiers::{ChannelId, PortId};
 use ibc_core::primitives::prelude::*;
-use ibc_core::primitives::Timestamp;
 use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::applications::transfer::v1::MsgTransfer as RawMsgTransfer;
 use ibc_proto::Protobuf;
@@ -45,22 +44,20 @@ pub struct MsgTransfer {
     pub timeout_height_on_b: TimeoutHeight,
     /// Timeout timestamp relative to the current block timestamp.
     /// The timeout is disabled when set to 0.
-    pub timeout_timestamp_on_b: Timestamp,
+    pub timeout_timestamp_on_b: TimeoutTimestamp,
 }
 
 impl TryFrom<RawMsgTransfer> for MsgTransfer {
     type Error = TokenTransferError;
 
     fn try_from(raw_msg: RawMsgTransfer) -> Result<Self, Self::Error> {
-        let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_msg.timeout_timestamp)
-            .map_err(PacketError::InvalidPacketTimestamp)
-            .map_err(ContextError::from)?;
-
         let timeout_height_on_b: TimeoutHeight = raw_msg
             .timeout_height
             .try_into()
             .map_err(ContextError::from)?;
 
+        let timeout_timestamp_on_b: TimeoutTimestamp = raw_msg.timeout_timestamp.into();
+
         // Packet timeout height and packet timeout timestamp cannot both be unset.
         if !timeout_height_on_b.is_set() && !timeout_timestamp_on_b.is_set() {
             return Err(ContextError::from(PacketError::MissingTimeout))?;

ibc-apps/ics20-transfer/types/src/msgs/transfer.rs

@@ -1,11 +1,10 @@
 //! Defines the Non-Fungible Token Transfer message type
 
 use ibc_core::channel::types::error::PacketError;
-use ibc_core::channel::types::timeout::TimeoutHeight;
+use ibc_core::channel::types::timeout::{TimeoutHeight, TimeoutTimestamp};
 use ibc_core::handler::types::error::ContextError;
 use ibc_core::host::types::identifiers::{ChannelId, PortId};
 use ibc_core::primitives::prelude::*;
-use ibc_core::primitives::Timestamp;
 use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::applications::nft_transfer::v1::MsgTransfer as RawMsgTransfer;
 use ibc_proto::Protobuf;
@@ -45,22 +44,20 @@ pub struct MsgTransfer {
     pub timeout_height_on_b: TimeoutHeight,
     /// Timeout timestamp relative to the current block timestamp.
     /// The timeout is disabled when set to 0.
-    pub timeout_timestamp_on_b: Timestamp,
+    pub timeout_timestamp_on_b: TimeoutTimestamp,
 }
 
 impl TryFrom<RawMsgTransfer> for MsgTransfer {
     type Error = NftTransferError;
 
     fn try_from(raw_msg: RawMsgTransfer) -> Result<Self, Self::Error> {
-        let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_msg.timeout_timestamp)
-            .map_err(PacketError::InvalidPacketTimestamp)
-            .map_err(ContextError::from)?;
-
         let timeout_height_on_b: TimeoutHeight = raw_msg
             .timeout_height
             .try_into()
             .map_err(ContextError::from)?;
 
+        let timeout_timestamp_on_b: TimeoutTimestamp = raw_msg.timeout_timestamp.into();
+
         // Packet timeout height and packet timeout timestamp cannot both be unset.
         if !timeout_height_on_b.is_set() && !timeout_timestamp_on_b.is_set() {
             return Err(ContextError::from(PacketError::MissingTimeout))?;

ibc-apps/ics721-nft-transfer/types/src/msgs/transfer.rs

@@ -150,12 +150,6 @@ pub fn consensus_state_status<CS: ConsensusState>(
     host_timestamp: &Timestamp,
     trusting_period: Duration,
 ) -> Result<Status, ClientError> {
-    if !host_timestamp.is_set() {
-        return Err(ClientError::Other {
-            description: "host timestamp is none".into(),
-        });
-    }
-
     // Note: if the `duration_since()` is `None`, indicating that the latest
     // consensus state is in the future, then we don't consider the client
     // to be expired.

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

@@ -336,12 +336,7 @@ where
         let tm_consensus_state: ConsensusStateType =
             consensus_state.try_into().map_err(Into::into)?;
 
-        let host_timestamp =
-            ctx.host_timestamp()?
-                .into_tm_time()
-                .ok_or_else(|| ClientError::Other {
-                    description: String::from("host timestamp is not a valid TM timestamp"),
-                })?;
+        let host_timestamp = ctx.host_timestamp()?.into_tm_time();
 
         let tm_consensus_state_timestamp = tm_consensus_state.timestamp();
         let tm_consensus_state_expiry = (tm_consensus_state_timestamp

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

@@ -84,7 +84,7 @@ pub fn verify_misbehaviour_header<H>(
     header: &TmHeader,
     chain_id: &ChainId,
     options: &Options,
-    trusted_timestamp: Time,
+    trusted_time: Time,
     trusted_next_validator_hash: Hash,
     current_timestamp: Timestamp,
     verifier: &impl Verifier,
@@ -97,12 +97,15 @@ where
 
     // ensure trusted consensus state is within trusting period
     {
-        let duration_since_consensus_state = current_timestamp
-            .duration_since(&trusted_timestamp.into())
-            .ok_or_else(|| ClientError::InvalidConsensusStateTimestamp {
-                time1: trusted_timestamp.into(),
-                time2: current_timestamp,
-            })?;
+        let trusted_timestamp = trusted_time.try_into().expect("time conversion failed");
+
+        let duration_since_consensus_state =
+            current_timestamp.duration_since(&trusted_timestamp).ok_or(
+                ClientError::InvalidConsensusStateTimestamp {
+                    time1: trusted_timestamp,
+                    time2: current_timestamp,
+                },
+            )?;
 
         if duration_since_consensus_state >= options.trusting_period {
             return Err(Error::ConsensusStateTimestampGteTrustingPeriod {
@@ -123,15 +126,10 @@ where
             description: format!("failed to parse chain id: {e}"),
         })?;
 
-    let trusted_state = header.as_trusted_block_state(
-        tm_chain_id,
-        trusted_timestamp,
-        trusted_next_validator_hash,
-    )?;
+    let trusted_state =
+        header.as_trusted_block_state(tm_chain_id, trusted_time, trusted_next_validator_hash)?;
 
-    let current_timestamp = current_timestamp.into_tm_time().ok_or(ClientError::Other {
-        description: "host timestamp must not be zero".to_string(),
-    })?;
+    let current_timestamp = current_timestamp.into_tm_time();
 
     verifier
         .verify_misbehaviour_header(untrusted_state, trusted_state, options, current_timestamp)

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

@@ -83,12 +83,7 @@ where
             next_validators: None,
         };
 
-        let now =
-            ctx.host_timestamp()?
-                .into_tm_time()
-                .ok_or_else(|| ClientError::ClientSpecific {
-                    description: "host timestamp is not a valid TM timestamp".to_string(),
-                })?;
+        let now = ctx.host_timestamp()?.into_tm_time();
 
         // main header verification, delegated to the tendermint-light-client crate.
         verifier

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

@@ -93,6 +93,9 @@ impl ConsensusStateTrait for ConsensusState {
     }
 
     fn timestamp(&self) -> Timestamp {
-        self.0.timestamp.into()
+        self.0
+            .timestamp
+            .try_into()
+            .expect("UNIX Timestamp can't be negative")
     }
 }

ibc-clients/ics07-tendermint/src/consensus_state.rs

@@ -9,6 +9,7 @@ use ibc_core_commitment_types::error::CommitmentError;
 use ibc_core_host_types::error::IdentifierError;
 use ibc_core_host_types::identifiers::ClientId;
 use ibc_primitives::prelude::*;
+use ibc_primitives::TimestampError;
 use tendermint::{Error as TendermintError, Hash};
 use tendermint_light_client_verifier::errors::VerificationErrorDetail as LightClientErrorDetail;
 use tendermint_light_client_verifier::operations::VotingPowerTally;
@@ -58,10 +59,8 @@ pub enum Error {
     InvalidRawHeader(TendermintError),
     /// invalid raw misbehaviour: `{reason}`
     InvalidRawMisbehaviour { reason: String },
-    /// given other previous updates, header timestamp should be at most `{max}`, but was `{actual}`
-    HeaderTimestampTooHigh { actual: String, max: String },
-    /// given other previous updates, header timestamp should be at least `{min}`, but was `{actual}`
-    HeaderTimestampTooLow { actual: String, min: String },
+    /// invalid header timestamp: `{0}`
+    InvalidHeaderTimestamp(TimestampError),
     /// header revision height = `{height}` is invalid
     InvalidHeaderHeight { height: u64 },
     /// frozen height is missing

ibc-clients/ics07-tendermint/types/src/error.rs

@@ -47,8 +47,12 @@ impl Display for Header {
 }
 
 impl Header {
-    pub fn timestamp(&self) -> Timestamp {
-        self.signed_header.header.time.into()
+    pub fn timestamp(&self) -> Result<Timestamp, Error> {
+        self.signed_header
+            .header
+            .time
+            .try_into()
+            .map_err(Error::InvalidHeaderTimestamp)
     }
 
     pub fn height(&self) -> Height {

ibc-clients/ics07-tendermint/types/src/header.rs

@@ -81,8 +81,6 @@ pub enum ClientError {
     },
     /// invalid commitment proof bytes error: `{0}`
     InvalidCommitmentProof(CommitmentError),
-    /// invalid packet timeout timestamp value error: `{0}`
-    InvalidPacketTimestamp(ibc_primitives::ParseTimestampError),
     /// mismatch between client and arguments types
     ClientArgsTypeMismatch { client_type: ClientType },
     /// timestamp is invalid or missing, timestamp=`{time1}`,  now=`{time2}`
@@ -133,7 +131,6 @@ impl std::error::Error for ClientError {
             | Self::InvalidClientIdentifier(e)
             | Self::InvalidRawMisbehaviour(e) => Some(e),
             Self::InvalidCommitmentProof(e) | Self::Ics23Verification(e) => Some(e),
-            Self::InvalidPacketTimestamp(e) => Some(e),
             _ => None,
         }
     }

ibc-core/ics02-client/types/src/error.rs

@@ -5,7 +5,7 @@ use ibc_core_client_types::{error as client_error, Height};
 use ibc_core_host_types::error::IdentifierError;
 use ibc_core_host_types::identifiers::{ClientId, ConnectionId};
 use ibc_primitives::prelude::*;
-use ibc_primitives::{Timestamp, TimestampOverflowError};
+use ibc_primitives::{Timestamp, TimestampError};
 
 use crate::version::Version;
 
@@ -80,7 +80,7 @@ pub enum ConnectionError {
         earliest_valid_time: Timestamp,
     },
     /// timestamp overflowed error: `{0}`
-    TimestampOverflow(TimestampOverflowError),
+    TimestampOverflow(TimestampError),
     /// connection counter overflow error
     CounterOverflow,
     /// other error: `{description}`

ibc-core/ics03-connection/types/src/error.rs

@@ -16,7 +16,6 @@ use ibc_core_host::types::path::{
 use ibc_core_host::{ExecutionContext, ValidationContext};
 use ibc_core_router::module::Module;
 use ibc_primitives::prelude::*;
-use ibc_primitives::Expiry;
 
 pub fn recv_packet_validate<ValCtx>(ctx_b: &ValCtx, msg: MsgRecvPacket) -> Result<(), ContextError>
 where
@@ -171,7 +170,11 @@ where
     }
 
     let latest_timestamp = ctx_b.host_timestamp()?;
-    if let Expiry::Expired = latest_timestamp.check_expiry(&msg.packet.timeout_timestamp_on_b) {
+    if msg
+        .packet
+        .timeout_timestamp_on_b
+        .has_expired(&latest_timestamp)
+    {
         return Err(PacketError::LowPacketTimestamp.into());
     }
 

ibc-core/ics04-channel/src/handler/recv_packet.rs

@@ -10,7 +10,6 @@ use ibc_core_host::types::path::{
     ChannelEndPath, ClientConsensusStatePath, CommitmentPath, SeqSendPath,
 };
 use ibc_primitives::prelude::*;
-use ibc_primitives::Expiry;
 
 use crate::context::{SendPacketExecutionContext, SendPacketValidationContext};
 
@@ -81,7 +80,7 @@ pub fn send_packet_validate(
         client_val_ctx_a.consensus_state(&client_cons_state_path_on_a)?;
     let latest_timestamp = consensus_state_of_b_on_a.timestamp();
     let packet_timestamp = packet.timeout_timestamp_on_b;
-    if let Expiry::Expired = latest_timestamp.check_expiry(&packet_timestamp) {
+    if packet_timestamp.has_expired(&latest_timestamp) {
         return Err(PacketError::LowPacketTimestamp.into());
     }
 

ibc-core/ics04-channel/src/handler/send_packet.rs

@@ -43,6 +43,10 @@ tendermint = { workspace = true }
 parity-scale-codec = { workspace = true, optional = true }
 scale-info         = { workspace = true, optional = true }
 
+[dev-dependencies]
+rstest     = { workspace = true }
+serde-json = { workspace = true }
+
 [features]
 default = [ "std" ]
 std = [

ibc-core/ics04-channel/types/Cargo.toml

@@ -1,10 +1,9 @@
 //! Types and utilities related to packet commitments.
 
 use ibc_primitives::prelude::*;
-use ibc_primitives::Timestamp;
 
 use super::acknowledgement::Acknowledgement;
-use crate::timeout::TimeoutHeight;
+use crate::timeout::{TimeoutHeight, TimeoutTimestamp};
 
 /// Packet commitment
 #[cfg_attr(
@@ -87,7 +86,7 @@ impl From<Vec<u8>> for AcknowledgementCommitment {
 pub fn compute_packet_commitment(
     packet_data: &[u8],
     timeout_height: &TimeoutHeight,
-    timeout_timestamp: &Timestamp,
+    timeout_timestamp: &TimeoutTimestamp,
 ) -> PacketCommitment {
     let mut hash_input = [0; 8 * 3 + 32];
 
@@ -128,7 +127,7 @@ mod test {
         let actual = compute_packet_commitment(
             b"packet data",
             &TimeoutHeight::At(ibc_core_client_types::Height::new(42, 24).unwrap()),
-            &Timestamp::from_nanoseconds(0x42).unwrap(),
+            &TimeoutTimestamp::from(0x42),
         );
         assert_eq!(&expected[..], actual.as_ref());
     }