diff --git a/.changelog/unreleased/breaking-changes/1296-decouple-host-timestamp-from-packet-timout.md b/.changelog/unreleased/breaking-changes/1296-decouple-host-timestamp-from-packet-timout.md new file mode 100644 index 000000000..1cd397934 --- /dev/null +++ b/.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)) diff --git a/.changelog/unreleased/breaking-changes/180-decouple-timestamp-definition-from-tendermint-time.md b/.changelog/unreleased/breaking-changes/180-decouple-timestamp-definition-from-tendermint-time.md new file mode 100644 index 000000000..2af2109c4 --- /dev/null +++ b/.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)) diff --git a/.changelog/unreleased/bug-fixes/1306-fix-timestamp-nanoseconds-when-tm-time-brings-neg-values.md b/.changelog/unreleased/bug-fixes/1306-fix-timestamp-nanoseconds-when-tm-time-brings-neg-values.md new file mode 100644 index 000000000..c86874872 --- /dev/null +++ b/.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)) diff --git a/Makefile b/Makefile index c3323b2cd..18de26650 100644 --- a/Makefile +++ b/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. diff --git a/ci/no-std-check/Cargo.lock b/ci/no-std-check/Cargo.lock index baad9f1a6..01ea501f8 100644 --- a/ci/no-std-check/Cargo.lock +++ b/ci/no-std-check/Cargo.lock @@ -1247,7 +1247,6 @@ version = "0.53.0" dependencies = [ "base64 0.22.1", "displaydoc", - "hex", "ibc-core-client", "ibc-core-host-types", "ibc-primitives", diff --git a/cosmwasm/ibc-clients/cw-context/src/context/client_ctx.rs b/cosmwasm/ibc-clients/cw-context/src/context/client_ctx.rs index 414217590..87de4af28 100644 --- a/cosmwasm/ibc-clients/cw-context/src/context/client_ctx.rs +++ b/cosmwasm/ibc-clients/cw-context/src/context/client_ctx.rs @@ -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); diff --git a/cosmwasm/ibc-clients/cw-context/src/context/custom_ctx.rs b/cosmwasm/ibc-clients/cw-context/src/context/custom_ctx.rs index 54829d8e4..c8e8979b3 100644 --- a/cosmwasm/ibc-clients/cw-context/src/context/custom_ctx.rs +++ b/cosmwasm/ibc-clients/cw-context/src/context/custom_ctx.rs @@ -21,10 +21,7 @@ where fn host_timestamp(&self) -> Result { 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) } diff --git a/cosmwasm/ibc-clients/ics07-tendermint/src/tests/helper.rs b/cosmwasm/ibc-clients/ics07-tendermint/src/tests/helper.rs index f7a4daf6c..64f084ce3 100644 --- a/cosmwasm/ibc-clients/ics07-tendermint/src/tests/helper.rs +++ b/cosmwasm/ibc-clients/ics07-tendermint/src/tests/helper.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"), diff --git a/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs b/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs index 06bd48027..cc311a2ca 100644 --- a/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs +++ b/ibc-apps/ics20-transfer/types/src/msgs/transfer.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 for MsgTransfer { type Error = TokenTransferError; fn try_from(raw_msg: RawMsgTransfer) -> Result { - 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))?; diff --git a/ibc-apps/ics721-nft-transfer/types/src/msgs/transfer.rs b/ibc-apps/ics721-nft-transfer/types/src/msgs/transfer.rs index 357723f61..b35a5c1cc 100644 --- a/ibc-apps/ics721-nft-transfer/types/src/msgs/transfer.rs +++ b/ibc-apps/ics721-nft-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 for MsgTransfer { type Error = NftTransferError; fn try_from(raw_msg: RawMsgTransfer) -> Result { - 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))?; diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index d32225f4e..becb9b8bb 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -150,12 +150,6 @@ pub fn consensus_state_status( host_timestamp: &Timestamp, trusting_period: Duration, ) -> Result { - 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. diff --git a/ibc-clients/ics07-tendermint/src/client_state/execution.rs b/ibc-clients/ics07-tendermint/src/client_state/execution.rs index 5d4fadde5..084d0441c 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/execution.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/execution.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 diff --git a/ibc-clients/ics07-tendermint/src/client_state/misbehaviour.rs b/ibc-clients/ics07-tendermint/src/client_state/misbehaviour.rs index 65fd175c5..ea40fa717 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/misbehaviour.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/misbehaviour.rs @@ -84,7 +84,7 @@ pub fn verify_misbehaviour_header( 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) diff --git a/ibc-clients/ics07-tendermint/src/client_state/update_client.rs b/ibc-clients/ics07-tendermint/src/client_state/update_client.rs index ec5d88c36..03da355ff 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/update_client.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/update_client.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 diff --git a/ibc-clients/ics07-tendermint/src/consensus_state.rs b/ibc-clients/ics07-tendermint/src/consensus_state.rs index e87778f37..3f64ce085 100644 --- a/ibc-clients/ics07-tendermint/src/consensus_state.rs +++ b/ibc-clients/ics07-tendermint/src/consensus_state.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") } } diff --git a/ibc-clients/ics07-tendermint/types/src/error.rs b/ibc-clients/ics07-tendermint/types/src/error.rs index c4e25e094..264e398a0 100644 --- a/ibc-clients/ics07-tendermint/types/src/error.rs +++ b/ibc-clients/ics07-tendermint/types/src/error.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 diff --git a/ibc-clients/ics07-tendermint/types/src/header.rs b/ibc-clients/ics07-tendermint/types/src/header.rs index 97b421ff3..6ee8069f0 100644 --- a/ibc-clients/ics07-tendermint/types/src/header.rs +++ b/ibc-clients/ics07-tendermint/types/src/header.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 { + self.signed_header + .header + .time + .try_into() + .map_err(Error::InvalidHeaderTimestamp) } pub fn height(&self) -> Height { diff --git a/ibc-core/ics02-client/types/src/error.rs b/ibc-core/ics02-client/types/src/error.rs index 14e86274e..dddd67a95 100644 --- a/ibc-core/ics02-client/types/src/error.rs +++ b/ibc-core/ics02-client/types/src/error.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, } } diff --git a/ibc-core/ics03-connection/types/src/error.rs b/ibc-core/ics03-connection/types/src/error.rs index 478b2d537..e71d5426c 100644 --- a/ibc-core/ics03-connection/types/src/error.rs +++ b/ibc-core/ics03-connection/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}` diff --git a/ibc-core/ics04-channel/src/handler/recv_packet.rs b/ibc-core/ics04-channel/src/handler/recv_packet.rs index 8c0cd25a1..aa22d3ec9 100644 --- a/ibc-core/ics04-channel/src/handler/recv_packet.rs +++ b/ibc-core/ics04-channel/src/handler/recv_packet.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(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()); } diff --git a/ibc-core/ics04-channel/src/handler/send_packet.rs b/ibc-core/ics04-channel/src/handler/send_packet.rs index 895ad9457..514ffb89d 100644 --- a/ibc-core/ics04-channel/src/handler/send_packet.rs +++ b/ibc-core/ics04-channel/src/handler/send_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()); } diff --git a/ibc-core/ics04-channel/types/Cargo.toml b/ibc-core/ics04-channel/types/Cargo.toml index 0ee9080c9..5cff829ce 100644 --- a/ibc-core/ics04-channel/types/Cargo.toml +++ b/ibc-core/ics04-channel/types/Cargo.toml @@ -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 = [ diff --git a/ibc-core/ics04-channel/types/src/commitment.rs b/ibc-core/ics04-channel/types/src/commitment.rs index 8066b13b6..ece5fe641 100644 --- a/ibc-core/ics04-channel/types/src/commitment.rs +++ b/ibc-core/ics04-channel/types/src/commitment.rs @@ -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> 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()); } diff --git a/ibc-core/ics04-channel/types/src/error.rs b/ibc-core/ics04-channel/types/src/error.rs index 2b7b760ef..7042411d3 100644 --- a/ibc-core/ics04-channel/types/src/error.rs +++ b/ibc-core/ics04-channel/types/src/error.rs @@ -1,16 +1,18 @@ //! Defines the main channel, port, and packet error types use displaydoc::Display; -use ibc_core_client_types::{error as client_error, Height}; +use ibc_core_client_types::error::ClientError; +use ibc_core_client_types::Height; use ibc_core_connection_types::error as connection_error; use ibc_core_host_types::error::IdentifierError; use ibc_core_host_types::identifiers::{ChannelId, ConnectionId, PortId, Sequence}; use ibc_primitives::prelude::*; -use ibc_primitives::{ParseTimestampError, Timestamp}; +use ibc_primitives::{Timestamp, TimestampError}; use super::channel::Counterparty; use super::timeout::TimeoutHeight; use crate::channel::State; +use crate::timeout::TimeoutTimestamp; use crate::Version; #[derive(Debug, Display)] @@ -47,10 +49,10 @@ pub enum ChannelError { /// Verification fails for the packet with the sequence number `{sequence}`, error: `{client_error}` PacketVerificationFailed { sequence: Sequence, - client_error: client_error::ClientError, + client_error: ClientError, }, /// Error verifying channel state error: `{0}` - VerifyChannelFailed(client_error::ClientError), + VerifyChannelFailed(ClientError), /// String `{value}` cannot be converted to packet sequence, error: `{error}` InvalidStringAsSequence { value: String, @@ -111,7 +113,7 @@ pub enum PacketError { PacketTimeoutNotReached { timeout_height: TimeoutHeight, chain_height: Height, - timeout_timestamp: Timestamp, + timeout_timestamp: TimeoutTimestamp, chain_timestamp: Timestamp, }, /// Packet acknowledgement exists for the packet with the sequence `{sequence}` @@ -136,10 +138,10 @@ pub enum PacketError { ZeroPacketSequence, /// packet data bytes cannot be empty ZeroPacketData, - /// invalid timeout height for the packet - InvalidTimeoutHeight, - /// Invalid packet timeout timestamp value error: `{0}` - InvalidPacketTimestamp(ParseTimestampError), + /// invalid timeout height with error: `{0}` + InvalidTimeoutHeight(ClientError), + /// Invalid timeout timestamp with error: `{0}` + InvalidTimeoutTimestamp(TimestampError), /// missing timeout MissingTimeout, /// invalid identifier error: `{0}` @@ -182,6 +184,12 @@ impl From for PacketError { } } +impl From for PacketError { + fn from(err: TimestampError) -> Self { + Self::InvalidTimeoutTimestamp(err) + } +} + #[cfg(feature = "std")] impl std::error::Error for PacketError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { diff --git a/ibc-core/ics04-channel/types/src/events/mod.rs b/ibc-core/ics04-channel/types/src/events/mod.rs index ee83a4028..798210a82 100644 --- a/ibc-core/ics04-channel/types/src/events/mod.rs +++ b/ibc-core/ics04-channel/types/src/events/mod.rs @@ -5,7 +5,6 @@ mod packet_attributes; use ibc_core_host_types::identifiers::{ChannelId, ConnectionId, PortId, Sequence}; use ibc_primitives::prelude::*; -use ibc_primitives::Timestamp; use tendermint::abci; use self::channel_attributes::{ @@ -24,6 +23,7 @@ use super::timeout::TimeoutHeight; use super::Version; use crate::error::ChannelError; use crate::packet::Packet; +use crate::timeout::TimeoutTimestamp; /// Channel event types corresponding to ibc-go's channel events: /// https://github.com/cosmos/ibc-go/blob/c4413c5877f9ef883494da1721cb18caaba7f7f5/modules/core/04-channel/types/events.go#L52-L72 @@ -633,7 +633,7 @@ impl SendPacket { &self.timeout_height_attr_on_b.timeout_height } - pub fn timeout_timestamp_on_b(&self) -> &Timestamp { + pub fn timeout_timestamp_on_b(&self) -> &TimeoutTimestamp { &self.timeout_timestamp_attr_on_b.timeout_timestamp } @@ -744,7 +744,7 @@ impl ReceivePacket { &self.timeout_height_attr_on_b.timeout_height } - pub fn timeout_timestamp_on_b(&self) -> &Timestamp { + pub fn timeout_timestamp_on_b(&self) -> &TimeoutTimestamp { &self.timeout_timestamp_attr_on_b.timeout_timestamp } @@ -859,7 +859,7 @@ impl WriteAcknowledgement { &self.timeout_height_attr_on_b.timeout_height } - pub fn timeout_timestamp_on_b(&self) -> &Timestamp { + pub fn timeout_timestamp_on_b(&self) -> &TimeoutTimestamp { &self.timeout_timestamp_attr_on_b.timeout_timestamp } @@ -964,7 +964,7 @@ impl AcknowledgePacket { &self.timeout_height_attr_on_b.timeout_height } - pub fn timeout_timestamp_on_b(&self) -> &Timestamp { + pub fn timeout_timestamp_on_b(&self) -> &TimeoutTimestamp { &self.timeout_timestamp_attr_on_b.timeout_timestamp } @@ -1065,7 +1065,7 @@ impl TimeoutPacket { &self.timeout_height_attr_on_b.timeout_height } - pub fn timeout_timestamp_on_b(&self) -> &Timestamp { + pub fn timeout_timestamp_on_b(&self) -> &TimeoutTimestamp { &self.timeout_timestamp_attr_on_b.timeout_timestamp } diff --git a/ibc-core/ics04-channel/types/src/events/packet_attributes.rs b/ibc-core/ics04-channel/types/src/events/packet_attributes.rs index 2366ae71e..a15a31220 100644 --- a/ibc-core/ics04-channel/types/src/events/packet_attributes.rs +++ b/ibc-core/ics04-channel/types/src/events/packet_attributes.rs @@ -6,14 +6,13 @@ use core::str; use derive_more::From; use ibc_core_host_types::identifiers::{ChannelId, ConnectionId, PortId, Sequence}; use ibc_primitives::prelude::*; -use ibc_primitives::Timestamp; use subtle_encoding::hex; use tendermint::abci; use crate::acknowledgement::Acknowledgement; use crate::channel::Order; use crate::error::ChannelError; -use crate::timeout::TimeoutHeight; +use crate::timeout::{TimeoutHeight, TimeoutTimestamp}; const PKT_SEQ_ATTRIBUTE_KEY: &str = "packet_sequence"; const PKT_DATA_ATTRIBUTE_KEY: &str = "packet_data"; @@ -113,7 +112,7 @@ impl From for abci::EventAttribute { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[derive(Clone, Debug, From, PartialEq, Eq)] pub struct TimeoutTimestampAttribute { - pub timeout_timestamp: Timestamp, + pub timeout_timestamp: TimeoutTimestamp, } impl From for abci::EventAttribute { diff --git a/ibc-core/ics04-channel/types/src/packet.rs b/ibc-core/ics04-channel/types/src/packet.rs index 5caeb4eb3..d07746b77 100644 --- a/ibc-core/ics04-channel/types/src/packet.rs +++ b/ibc-core/ics04-channel/types/src/packet.rs @@ -2,12 +2,12 @@ use ibc_core_client_types::Height; use ibc_core_host_types::identifiers::{ChannelId, PortId, Sequence}; use ibc_primitives::prelude::*; -use ibc_primitives::Expiry::Expired; use ibc_primitives::Timestamp; use ibc_proto::ibc::core::channel::v1::{Packet as RawPacket, PacketState as RawPacketState}; use super::timeout::TimeoutHeight; use crate::error::PacketError; +use crate::timeout::TimeoutTimestamp; /// Enumeration of proof carrying ICS4 message, helper for relayer. #[derive(Clone, Debug, PartialEq, Eq)] @@ -80,7 +80,7 @@ pub struct Packet { )] pub data: Vec, pub timeout_height_on_b: TimeoutHeight, - pub timeout_timestamp_on_b: Timestamp, + pub timeout_timestamp_on_b: TimeoutTimestamp, } struct PacketData<'a>(&'a [u8]); @@ -140,8 +140,7 @@ impl Packet { pub fn timed_out(&self, dst_chain_ts: &Timestamp, dst_chain_height: Height) -> bool { let height_timed_out = self.timeout_height_on_b.has_expired(dst_chain_height); - let timestamp_timed_out = self.timeout_timestamp_on_b.is_set() - && dst_chain_ts.check_expiry(&self.timeout_timestamp_on_b) == Expired; + let timestamp_timed_out = self.timeout_timestamp_on_b.has_expired(dst_chain_ts); height_timed_out || timestamp_timed_out } @@ -184,13 +183,9 @@ impl TryFrom for Packet { // revision_number as soon as the chain starts, // `{revision_number: old_rev + 1, revision_height: 1}` // should be used. - let packet_timeout_height: TimeoutHeight = raw_pkt - .timeout_height - .try_into() - .map_err(|_| PacketError::InvalidTimeoutHeight)?; + let packet_timeout_height: TimeoutHeight = raw_pkt.timeout_height.try_into()?; - let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_pkt.timeout_timestamp) - .map_err(PacketError::InvalidPacketTimestamp)?; + let timeout_timestamp_on_b: TimeoutTimestamp = raw_pkt.timeout_timestamp.into(); // Packet timeout height and packet timeout timestamp cannot both be unset. if !packet_timeout_height.is_set() && !timeout_timestamp_on_b.is_set() { diff --git a/ibc-core/ics04-channel/types/src/timeout.rs b/ibc-core/ics04-channel/types/src/timeout/height.rs similarity index 95% rename from ibc-core/ics04-channel/types/src/timeout.rs rename to ibc-core/ics04-channel/types/src/timeout/height.rs index e20ccf0d8..43dfe3bf3 100644 --- a/ibc-core/ics04-channel/types/src/timeout.rs +++ b/ibc-core/ics04-channel/types/src/timeout/height.rs @@ -2,11 +2,12 @@ use core::fmt::{Display, Error as FmtError, Formatter}; -use ibc_core_client_types::error::ClientError; use ibc_core_client_types::Height; use ibc_primitives::prelude::*; use ibc_proto::ibc::core::client::v1::Height as RawHeight; +use crate::error::PacketError; + /// Indicates a consensus height on the destination chain after which the packet /// will no longer be processed, and will instead count as having timed-out. /// @@ -85,7 +86,7 @@ impl TimeoutHeight { } impl TryFrom for TimeoutHeight { - type Error = ClientError; + type Error = PacketError; // Note: it is important for `revision_number` to also be `0`, otherwise // packet commitment proofs will be incorrect (proof construction in @@ -95,14 +96,16 @@ impl TryFrom for TimeoutHeight { if raw_height.revision_number == 0 && raw_height.revision_height == 0 { Ok(TimeoutHeight::Never) } else { - let height: Height = raw_height.try_into()?; + let height = raw_height + .try_into() + .map_err(PacketError::InvalidTimeoutHeight)?; Ok(TimeoutHeight::At(height)) } } } impl TryFrom> for TimeoutHeight { - type Error = ClientError; + type Error = PacketError; fn try_from(maybe_raw_height: Option) -> Result { match maybe_raw_height { @@ -137,13 +140,13 @@ impl Display for TimeoutHeight { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { match self { TimeoutHeight::At(timeout_height) => write!(f, "{timeout_height}"), - TimeoutHeight::Never => write!(f, "no timeout"), + TimeoutHeight::Never => write!(f, "no timeout height"), } } } #[cfg(feature = "serde")] -mod tests { +mod serialize { use serde::{Deserialize, Serialize}; use super::TimeoutHeight; diff --git a/ibc-core/ics04-channel/types/src/timeout/mod.rs b/ibc-core/ics04-channel/types/src/timeout/mod.rs new file mode 100644 index 000000000..e07e1204d --- /dev/null +++ b/ibc-core/ics04-channel/types/src/timeout/mod.rs @@ -0,0 +1,5 @@ +mod height; +mod timestamp; + +pub use height::TimeoutHeight; +pub use timestamp::TimeoutTimestamp; diff --git a/ibc-core/ics04-channel/types/src/timeout/timestamp.rs b/ibc-core/ics04-channel/types/src/timeout/timestamp.rs new file mode 100644 index 000000000..86ef4328a --- /dev/null +++ b/ibc-core/ics04-channel/types/src/timeout/timestamp.rs @@ -0,0 +1,211 @@ +use core::fmt::{Display, Error as FmtError, Formatter}; +use core::ops::{Add, Sub}; +use core::time::Duration; + +use ibc_primitives::prelude::*; +use ibc_primitives::Timestamp; + +use crate::error::PacketError; + +/// Indicates a timestamp on the destination chain after which the packet will +/// no longer be processed, and will instead count as having timed-out. +/// +/// The IBC protocol represents timestamps as u64 Unix timestamps in +/// nanoseconds. A protocol value of 0 indicates that the timestamp is not set. +/// In this case, we use the explicit `Never` variant to distinguish the absence +/// of a timeout when converting from a zero protobuf value. +#[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 = "schema", derive(schemars::JsonSchema))] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq)] +pub enum TimeoutTimestamp { + Never, + At(Timestamp), +} + +impl TimeoutTimestamp { + /// Creates a new timeout timestamp from a given nanosecond value. + pub fn from_nanoseconds(nanoseconds: u64) -> Self { + Self::from(nanoseconds) + } + + /// Returns the timestamp in nanoseconds, where 0 indicates the absence + /// of a timeout. + pub fn nanoseconds(&self) -> u64 { + match self { + Self::At(timestamp) => timestamp.nanoseconds(), + Self::Never => 0, + } + } + + /// Returns `true` if the timeout timestamp is set. + pub fn is_set(&self) -> bool { + match self { + TimeoutTimestamp::At(_) => true, + TimeoutTimestamp::Never => false, + } + } + + /// Returns a timeout timestamp that never expires. + pub fn no_timeout() -> Self { + Self::Never + } + + /// Check if a timestamp is *strictly past* the timeout timestamp, and thus + /// is deemed expired. + pub fn has_expired(&self, timestamp: &Timestamp) -> bool { + match self { + Self::At(timeout_timestamp) => timestamp > timeout_timestamp, + // When there's no timeout, timestamps are never expired + Self::Never => false, + } + } +} + +impl From for TimeoutTimestamp { + fn from(timestamp: Timestamp) -> Self { + TimeoutTimestamp::At(timestamp) + } +} + +// Note: As any `u64` value is interpreted as a Unix timestamp in nanoseconds +// when used for timeouts, implementing `From` is appropriate. +impl From for TimeoutTimestamp { + fn from(timestamp: u64) -> Self { + if timestamp == 0 { + TimeoutTimestamp::Never + } else { + TimeoutTimestamp::At(Timestamp::from_nanoseconds(timestamp)) + } + } +} + +impl Display for TimeoutTimestamp { + fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { + match self { + TimeoutTimestamp::At(timeout_timestamp) => write!(f, "{timeout_timestamp}"), + TimeoutTimestamp::Never => write!(f, "no timeout timestamp"), + } + } +} + +impl Add for TimeoutTimestamp { + type Output = Result; + + fn add(self, rhs: Duration) -> Self::Output { + match self { + TimeoutTimestamp::At(timestamp) => { + let new_timestamp = timestamp.add(rhs)?; + Ok(TimeoutTimestamp::At(new_timestamp)) + } + TimeoutTimestamp::Never => Err(PacketError::MissingTimeout), + } + } +} + +impl Sub for TimeoutTimestamp { + type Output = Result; + + fn sub(self, rhs: Duration) -> Self::Output { + match self { + TimeoutTimestamp::At(timestamp) => { + let new_timestamp = timestamp.sub(rhs)?; + Ok(TimeoutTimestamp::At(new_timestamp)) + } + TimeoutTimestamp::Never => Err(PacketError::MissingTimeout), + } + } +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + + fn never() -> TimeoutTimestamp { + TimeoutTimestamp::Never + } + + fn some(t: u64) -> TimeoutTimestamp { + TimeoutTimestamp::At(Timestamp::from_nanoseconds(t)) + } + + use super::*; + #[rstest] + #[case::zero_plus_zero(some(0), 0, some(0))] // NOTE: zero timestamp is 1970-01-01T00:00:00Z + #[case::zero_plus_one(some(0), 1, some(1))] + #[case::some_plus_zero(some(123456), 0, some(123456))] + #[case::some_plus_one(some(123456), 1, some(123457))] + fn test_timeout_add_arithmetic( + #[case] timeout_timestamp: TimeoutTimestamp, + #[case] duration: u64, + #[case] expect: TimeoutTimestamp, + ) { + let duration = Duration::from_nanos(duration); + let result = timeout_timestamp + duration; + assert_eq!(result.unwrap(), expect); + } + + #[rstest] + #[case::never(never(), 0)] + #[case::never_plus_one(never(), 1)] + fn test_invalid_timeout_add_arithmetic( + #[case] timeout_timestamp: TimeoutTimestamp, + #[case] duration: u64, + ) { + let duration = Duration::from_nanos(duration); + let result = timeout_timestamp + duration; + assert!(result.is_err()); + } + + #[rstest] + #[case::zero_minus_zero(some(0), 0, some(0))] + #[case::some_minus(some(123456), 123456, some(0))] + #[case::some_minus_zero(some(123456), 0, some(123456))] + #[case::some_minus_one(some(123456), 1, some(123455))] + fn test_timeout_sub_arithmetic( + #[case] timeout_timestamp: TimeoutTimestamp, + #[case] duration: u64, + #[case] expect: TimeoutTimestamp, + ) { + let duration = Duration::from_nanos(duration); + let result = timeout_timestamp - duration; + assert_eq!(result.unwrap(), expect); + } + + #[rstest] + #[case::never(never(), 0)] + #[case::never_minus_one(never(), 1)] + #[case::zero_minus_one(some(0), 1)] + fn test_invalid_sub_arithmetic( + #[case] timeout_timestamp: TimeoutTimestamp, + #[case] duration: u64, + ) { + let duration = Duration::from_nanos(duration); + let result = timeout_timestamp - duration; + assert!(result.is_err()); + } + + #[cfg(feature = "serde")] + #[rstest::rstest] + #[case::never(TimeoutTimestamp::Never)] + #[case::at_zero(TimeoutTimestamp::At(Timestamp::from_nanoseconds(0)))] + #[case::at_some(TimeoutTimestamp::At(Timestamp::from_nanoseconds(123456)))] + #[case::at_u64_max(TimeoutTimestamp::At(Timestamp::from_nanoseconds(u64::MAX)))] + fn test_timeout_timestamp_serde(#[case] timeout_timestamp: TimeoutTimestamp) { + let serialized = serde_json::to_string(&timeout_timestamp).unwrap(); + let deserialized: TimeoutTimestamp = serde_json::from_str(&serialized).unwrap(); + + assert_eq!(timeout_timestamp, deserialized); + } +} diff --git a/ibc-core/ics25-handler/types/src/events.rs b/ibc-core/ics25-handler/types/src/events.rs index d6a72140b..aa62ea813 100644 --- a/ibc-core/ics25-handler/types/src/events.rs +++ b/ibc-core/ics25-handler/types/src/events.rs @@ -8,7 +8,7 @@ use ibc_core_connection_types::{error as connection_error, events as ConnectionE use ibc_core_host_types::error::IdentifierError; use ibc_core_router_types::event::ModuleEvent; use ibc_primitives::prelude::*; -use ibc_primitives::ParseTimestampError; +use ibc_primitives::TimestampError; use tendermint::abci; /// All error variants related to IBC events @@ -25,7 +25,7 @@ pub enum Error { /// channel error: `{0}` Channel(channel_error::ChannelError), /// parsing timestamp error: `{0}` - Timestamp(ParseTimestampError), + Timestamp(TimestampError), /// incorrect event type: `{event}` IncorrectEventType { event: String }, /// module event cannot use core event types: `{event:?}` diff --git a/ibc-primitives/Cargo.toml b/ibc-primitives/Cargo.toml index 0afd83407..2c56cd6de 100644 --- a/ibc-primitives/Cargo.toml +++ b/ibc-primitives/Cargo.toml @@ -38,6 +38,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 = [ diff --git a/ibc-primitives/src/types/timestamp.rs b/ibc-primitives/src/types/timestamp.rs index fb38515c4..7551db5e7 100644 --- a/ibc-primitives/src/types/timestamp.rs +++ b/ibc-primitives/src/types/timestamp.rs @@ -2,153 +2,87 @@ use core::fmt::{Display, Error as FmtError, Formatter}; use core::hash::Hash; -use core::num::ParseIntError; +use core::num::{ParseIntError, TryFromIntError}; use core::ops::{Add, Sub}; use core::str::FromStr; use core::time::Duration; use displaydoc::Display; +use ibc_proto::google::protobuf::Timestamp as RawTimestamp; +use ibc_proto::Protobuf; +#[cfg(feature = "serde")] +use serde::{Deserialize, Serialize}; use tendermint::Time; -use time::OffsetDateTime; +use time::error::ComponentRange; +use time::macros::offset; +use time::{OffsetDateTime, PrimitiveDateTime}; use crate::prelude::*; pub const ZERO_DURATION: Duration = Duration::from_secs(0); -/// A new type wrapper over `Option