Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp(ics24)!: enhancements and fixes to ChainId impl and validation #762

Merged
merged 5 commits into from
Jul 28, 2023
Merged

imp(ics24)!: enhancements and fixes to ChainId impl and validation #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8164e31
imp(ChainId): refactor abstraction and validations
Farhad-Shabani Jul 13, 2023
5457c55
misc: add unclog
Farhad-Shabani Jul 13, 2023
e822ec7
fix: clippy catches
Farhad-Shabani Jul 13, 2023
530e16f
fix: revert ChainId definition + improve checks
Farhad-Shabani Jul 28, 2023
963b7be
fix: set_revision_number
Farhad-Shabani Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/unreleased/breaking-changes/761-chain-id-enhancements-and-fixes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Enhancements and fixes to `ChainId` impls and validation.
([#761](https://github.com/cosmos/ibc-rs/issues/761))

36 changes: 15 additions & 21 deletions crates/ibc/src/clients/ics07_tendermint/client_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::prelude::*;

use core::cmp::max;
use core::convert::{TryFrom, TryInto};
use core::str::FromStr;
use core::time::Duration;

use ibc_proto::google::protobuf::Any;
Expand Down Expand Up @@ -147,13 +148,7 @@ impl ClientState {
}

pub fn validate(&self) -> Result<(), Error> {
if self.chain_id.as_str().len() > MaxChainIdLen {
return Err(Error::ChainIdTooLong {
chain_id: self.chain_id.clone(),
len: self.chain_id.as_str().len(),
max_len: MaxChainIdLen,
});
}
self.chain_id.validate_length(3, MaxChainIdLen)?;

// `TrustThreshold` is guaranteed to be in the range `[0, 1)`, but a `TrustThreshold::ZERO`
// value is invalid in this context
Expand Down Expand Up @@ -202,7 +197,7 @@ impl ClientState {
});
}

if self.latest_height.revision_number() != self.chain_id.version() {
if self.latest_height.revision_number() != self.chain_id.revision_number() {
return Err(Error::InvalidLatestHeight {
reason: "ClientState latest-height revision number must match chain-id version"
.to_string(),
Expand Down Expand Up @@ -606,7 +601,7 @@ impl TryFrom<RawTmClientState> for ClientState {
type Error = Error;

fn try_from(raw: RawTmClientState) -> Result<Self, Self::Error> {
let chain_id = ChainId::from_string(raw.chain_id.as_str());
let chain_id = ChainId::from_str(raw.chain_id.as_str())?;

let trust_level = {
let trust_level = raw
Expand Down Expand Up @@ -791,7 +786,7 @@ mod tests {
fn client_state_new() {
// Define a "default" set of parameters to reuse throughout these tests.
let default_params: ClientStateParams = ClientStateParams {
id: ChainId::default(),
id: ChainId::new("ibc", 0).unwrap(),
trust_level: TrustThreshold::ONE_THIRD,
trusting_period: Duration::new(64000, 0),
unbonding_period: Duration::new(128000, 0),
Expand Down Expand Up @@ -834,17 +829,17 @@ mod tests {
want_pass: true,
},
Test {
name: "Valid long (50 chars) chain-id".to_string(),
name: "Valid long (50 chars) chain-id that satisfies revision_number length < `u16::MAX` length".to_string(),
params: ClientStateParams {
id: ChainId::new(&"a".repeat(48), 0),
id: ChainId::new(&"a".repeat(29), 0).unwrap(),
..default_params.clone()
},
want_pass: true,
},
Test {
name: "Invalid too-long (51 chars) chain-id".to_string(),
params: ClientStateParams {
id: ChainId::new(&"a".repeat(49), 0),
id: ChainId::new(&"a".repeat(30), 0).unwrap(),
..default_params.clone()
},
want_pass: false,
Expand Down Expand Up @@ -957,7 +952,7 @@ mod tests {
fn client_state_verify_height() {
// Define a "default" set of parameters to reuse throughout these tests.
let default_params: ClientStateParams = ClientStateParams {
id: ChainId::new("ibc", 1),
id: ChainId::new("ibc", 1).unwrap(),
trust_level: TrustThreshold::ONE_THIRD,
trusting_period: Duration::new(64000, 0),
unbonding_period: Duration::new(128000, 0),
Expand Down Expand Up @@ -1095,6 +1090,7 @@ mod serde_tests {
#[cfg(any(test, feature = "mocks"))]
pub mod test_util {
use crate::prelude::*;
use core::str::FromStr;
use core::time::Duration;

use tendermint::block::Header;
Expand All @@ -1113,17 +1109,15 @@ pub mod test_util {
}

pub fn new_dummy_from_header(tm_header: Header) -> Self {
let chain_id = ChainId::from_str(tm_header.chain_id.as_str()).expect("Never fails");
Self::new(
tm_header.chain_id.to_string().into(),
chain_id.clone(),
Default::default(),
Duration::from_secs(64000),
Duration::from_secs(128000),
Duration::from_millis(3000),
Height::new(
ChainId::chain_version(tm_header.chain_id.as_str()),
u64::from(tm_header.height),
)
.expect("Never fails"),
Height::new(chain_id.revision_number(), u64::from(tm_header.height))
.expect("Never fails"),
Default::default(),
Default::default(),
AllowUpdate {
Expand All @@ -1138,7 +1132,7 @@ pub mod test_util {
pub fn get_dummy_raw_tm_client_state(frozen_height: RawHeight) -> RawTmClientState {
#[allow(deprecated)]
RawTmClientState {
chain_id: ChainId::new("ibc", 0).to_string(),
chain_id: ChainId::new("ibc", 0).expect("Never fails").to_string(),
trust_level: Some(Fraction {
numerator: 1,
denominator: 3,
Expand Down
19 changes: 10 additions & 9 deletions crates/ibc/src/clients/ics07_tendermint/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use crate::prelude::*;

use crate::core::ics02_client::error::ClientError;
use crate::core::ics24_host::identifier::{ChainId, ClientId};
use crate::core::ics24_host::identifier::{ClientId, IdentifierError};
use crate::Height;

use core::time::Duration;
Expand All @@ -17,12 +17,8 @@ use tendermint_light_client_verifier::Verdict;
/// The main error type
#[derive(Debug, Display)]
pub enum Error {
/// chain-id is (`{chain_id}`) is too long, got: `{len}`, max allowed: `{max_len}`
ChainIdTooLong {
chain_id: ChainId,
len: usize,
max_len: usize,
},
/// invalid identifier: `{0}`
InvalidIdentifier(IdentifierError),
/// invalid header, failed basic validation: `{reason}`, error: `{error}`
InvalidHeader {
reason: String,
Expand Down Expand Up @@ -99,14 +95,13 @@ pub enum Error {
MisbehaviourHeadersBlockHashesEqual,
/// headers are not at same height and are monotonically increasing
MisbehaviourHeadersNotAtSameHeight,
/// invalid raw client id: `{client_id}`
InvalidRawClientId { client_id: String },
}

#[cfg(feature = "std")]
impl std::error::Error for Error {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match &self {
Self::InvalidIdentifier(e) => Some(e),
Self::InvalidHeader { error: e, .. } => Some(e),
Self::InvalidTendermintTrustThreshold(e) => Some(e),
Self::InvalidRawHeader(e) => Some(e),
Expand All @@ -124,6 +119,12 @@ impl From<Error> for ClientError {
}
}

impl From<IdentifierError> for Error {
fn from(e: IdentifierError) -> Self {
Self::InvalidIdentifier(e)
}
}

pub(crate) trait IntoResult<T, E> {
fn into_result(self) -> Result<T, E>;
}
Expand Down
7 changes: 5 additions & 2 deletions crates/ibc/src/clients/ics07_tendermint/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use crate::prelude::*;
use alloc::string::ToString;
use core::fmt::{Display, Error as FmtError, Formatter};
use core::str::FromStr;

use bytes::Buf;
use ibc_proto::google::protobuf::Any;
Expand Down Expand Up @@ -54,7 +55,9 @@ impl Header {

pub fn height(&self) -> Height {
Height::new(
ChainId::chain_version(self.signed_header.header.chain_id.as_str()),
ChainId::from_str(self.signed_header.header.chain_id.as_str())
.expect("chain id")
.revision_number(),
u64::from(self.signed_header.header.height),
)
.expect("malformed tendermint header domain type has an illegal height of 0")
Expand Down Expand Up @@ -89,7 +92,7 @@ impl Header {
}

pub fn verify_chain_id_version_matches_height(&self, chain_id: &ChainId) -> Result<(), Error> {
if self.height().revision_number() != chain_id.version() {
if self.height().revision_number() != chain_id.revision_number() {
return Err(Error::MismatchHeaderChainId {
given: self.signed_header.header.chain_id.to_string(),
expected: chain_id.to_string(),
Expand Down
9 changes: 3 additions & 6 deletions crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,15 @@ impl TryFrom<RawMisbehaviour> for Misbehaviour {
type Error = Error;

fn try_from(raw: RawMisbehaviour) -> Result<Self, Self::Error> {
let client_id = raw
.client_id
.parse()
.map_err(|_| Error::InvalidRawClientId {
client_id: raw.client_id.clone(),
})?;
let client_id = raw.client_id.parse()?;

let header1: Header = raw
.header_1
.ok_or_else(|| Error::InvalidRawMisbehaviour {
reason: "missing header1".into(),
})?
.try_into()?;

let header2: Header = raw
.header_2
.ok_or_else(|| Error::InvalidRawMisbehaviour {
Expand Down
30 changes: 16 additions & 14 deletions crates/ibc/src/core/ics02_client/handler/update_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,10 @@ mod tests {
let client_id = ClientId::new(tm_client_type(), 0).unwrap();
let client_height = Height::new(1, 20).unwrap();
let update_height = Height::new(1, 21).unwrap();
let chain_id_b = ChainId::new("mockgaiaB", 1);
let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap();

let mut ctx = MockContext::new(
ChainId::new("mockgaiaA", 1),
ChainId::new("mockgaiaA", 1).unwrap(),
HostType::Mock,
5,
Height::new(1, 1).unwrap(),
Expand Down Expand Up @@ -254,10 +254,10 @@ mod tests {
let client_id = ClientId::new(tm_client_type(), 0).unwrap();
let client_height = Height::new(1, 20).unwrap();
let update_height = Height::new(1, 21).unwrap();
let chain_id_b = ChainId::new("mockgaiaB", 1);
let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap();

let mut ctx = MockContext::new(
ChainId::new("mockgaiaA", 1),
ChainId::new("mockgaiaA", 1).unwrap(),
HostType::Mock,
5,
Height::new(1, 1).unwrap(),
Expand Down Expand Up @@ -301,8 +301,8 @@ mod tests {
let client_id = ClientId::new(tm_client_type(), 0).unwrap();
let client_height = Height::new(1, 20).unwrap();

let ctx_a_chain_id = ChainId::new("mockgaiaA", 1);
let ctx_b_chain_id = ChainId::new("mockgaiaB", 1);
let ctx_a_chain_id = ChainId::new("mockgaiaA", 1).unwrap();
let ctx_b_chain_id = ChainId::new("mockgaiaB", 1).unwrap();
let start_height = Height::new(1, 11).unwrap();

let mut ctx_a = MockContext::new(ctx_a_chain_id, HostType::Mock, 5, start_height)
Expand Down Expand Up @@ -353,10 +353,12 @@ mod tests {

let tm_block = downcast!(block.clone() => HostBlock::SyntheticTendermint).unwrap();

let chain_id = ChainId::from_str(tm_block.header().chain_id.as_str()).unwrap();

let client_state = {
#[allow(deprecated)]
let raw_client_state = RawTmClientState {
chain_id: ChainId::from(tm_block.header().chain_id.to_string()).to_string(),
chain_id: chain_id.to_string(),
trust_level: Some(Fraction {
numerator: 1,
denominator: 3,
Expand All @@ -366,7 +368,7 @@ mod tests {
max_clock_drift: Some(Duration::from_millis(3000).into()),
latest_height: Some(
Height::new(
ChainId::chain_version(tm_block.header().chain_id.as_str()),
chain_id.revision_number(),
u64::from(tm_block.header().height),
)
.unwrap()
Expand Down Expand Up @@ -426,7 +428,7 @@ mod tests {
let chain_start_height = Height::new(1, 11).unwrap();

let ctx = MockContext::new(
ChainId::new("mockgaiaA", 1),
ChainId::new("mockgaiaA", 1).unwrap(),
HostType::Mock,
5,
chain_start_height,
Expand All @@ -439,7 +441,7 @@ mod tests {
);

let ctx_b = MockContext::new(
ChainId::new("mockgaiaB", 1),
ChainId::new("mockgaiaB", 1).unwrap(),
HostType::SyntheticTendermint,
5,
client_height,
Expand Down Expand Up @@ -565,11 +567,11 @@ mod tests {
let client_id = ClientId::new(tm_client_type(), 0).unwrap();
let client_height = Height::new(1, 20).unwrap();
let misbehaviour_height = Height::new(1, 21).unwrap();
let chain_id_b = ChainId::new("mockgaiaB", 1);
let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap();

// Create a mock context for chain-A with a synthetic tendermint light client for chain-B
let mut ctx_a = MockContext::new(
ChainId::new("mockgaiaA", 1),
ChainId::new("mockgaiaA", 1).unwrap(),
HostType::Mock,
5,
Height::new(1, 1).unwrap(),
Expand Down Expand Up @@ -626,11 +628,11 @@ mod tests {
let client_id = ClientId::new(tm_client_type(), 0).unwrap();
let client_height = Height::new(1, 20).unwrap();
let misbehaviour_height = Height::new(1, 21).unwrap();
let chain_id_b = ChainId::new("mockgaiaB", 1);
let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap();

// Create a mock context for chain-A with a synthetic tendermint light client for chain-B
let mut ctx_a = MockContext::new(
ChainId::new("mockgaiaA", 1),
ChainId::new("mockgaiaA", 1).unwrap(),
HostType::Mock,
5,
Height::new(1, 1).unwrap(),
Expand Down
2 changes: 1 addition & 1 deletion crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ mod tests {

let ctx_default = MockContext::default();
let ctx_new = MockContext::new(
ChainId::new("mockgaia", latest_height.revision_number()),
ChainId::new("mockgaia", latest_height.revision_number()).unwrap(),
HostType::Mock,
max_history_size,
latest_height,
Expand Down
2 changes: 1 addition & 1 deletion crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ mod tests {
};

let ctx_new = MockContext::new(
ChainId::new("mockgaia", 0),
ChainId::new("mockgaia", 0).unwrap(),
HostType::Mock,
max_history_size,
host_chain_height,
Expand Down
Loading