diff --git a/modules/src/ics03_connection/connection.rs b/modules/src/ics03_connection/connection.rs index 09e5d9fb27..a12370d9ee 100644 --- a/modules/src/ics03_connection/connection.rs +++ b/modules/src/ics03_connection/connection.rs @@ -36,8 +36,7 @@ impl TryFromRaw for ConnectionEnd { .parse() .map_err(|e| Kind::IdentifierError.context(e))?, Counterparty::try_from(cp)?, - validate_versions(value.versions) - .map_err(|e| Kind::InvalidVersion.context(e))?, + value.versions, ) .unwrap(); diff --git a/modules/src/ics03_connection/exported.rs b/modules/src/ics03_connection/exported.rs index 0068fb862f..0d919487db 100644 --- a/modules/src/ics03_connection/exported.rs +++ b/modules/src/ics03_connection/exported.rs @@ -33,7 +33,7 @@ pub enum State { } impl State { - /// Yields the state as a string + /// Yields the State as a string. pub fn as_string(&self) -> &'static str { match self { Self::Uninitialized => "UNINITIALIZED", @@ -43,6 +43,7 @@ impl State { } } + /// Parses the State from a i32. pub fn from_i32(nr: i32) -> Self { match nr { 1 => Self::Init, diff --git a/modules/src/ics03_connection/msgs.rs b/modules/src/ics03_connection/msgs.rs index 28ccc88088..9cb381e7a0 100644 --- a/modules/src/ics03_connection/msgs.rs +++ b/modules/src/ics03_connection/msgs.rs @@ -437,12 +437,13 @@ mod tests { want_pass: false, }, Test { - name: "Bad destination client id, name in uppercase".to_string(), + name: "Correct destination client id with lower/upper case and special chars" + .to_string(), params: ConOpenTryParams { - counterparty_client_id: "BadClientId".to_string(), + counterparty_client_id: "ClientId_".to_string(), ..default_con_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad counterparty versions, empty versions vec".to_string(), diff --git a/modules/src/ics04_channel/channel.rs b/modules/src/ics04_channel/channel.rs index 0b2d22fd39..28e1f5816c 100644 --- a/modules/src/ics04_channel/channel.rs +++ b/modules/src/ics04_channel/channel.rs @@ -22,30 +22,48 @@ pub struct ChannelEnd { impl TryFromRaw for ChannelEnd { type RawType = RawChannel; type Error = anomaly::Error; + fn try_from(value: RawChannel) -> Result { - // Todo: Do validation of data here. This is just an example implementation for testing. - let ordering = match value.ordering { - 0 => Order::None, - 1 => Order::Unordered, - 2 => Order::Ordered, - _ => panic!("invalid order number"), + // Parse the ordering type. Propagate the error, if any, to our caller. + let chan_ordering = Order::from_i32(value.ordering)?; + + let chan_state = State::from_i32(value.state)?; + + // Pop out the Counterparty from the Option. + let counterparty = match value.counterparty { + Some(cp) => cp, + None => return Err(Kind::MissingCounterparty.into()), }; - let counterparty = value.counterparty.unwrap(); + + // Assemble the 'remote' attribute of the Channel, which represents the Counterparty. let remote = Counterparty { - port_id: PortId::from_str(counterparty.port_id.as_str()).unwrap(), - channel_id: ChannelId::from_str(counterparty.channel_id.as_str()).unwrap(), + port_id: PortId::from_str(counterparty.port_id.as_str()) + .map_err(|e| Kind::IdentifierError.context(e))?, + channel_id: ChannelId::from_str(counterparty.channel_id.as_str()) + .map_err(|e| Kind::IdentifierError.context(e))?, }; + + // Parse each item in connection_hops into a ConnectionId. let connection_hops = value .connection_hops .into_iter() - .map(|e| ConnectionId::from_str(e.as_str()).unwrap()) - .collect(); + .map(|conn_id| ConnectionId::from_str(conn_id.as_str())) + .collect::, _>>() + .map_err(|e| Kind::IdentifierError.context(e))?; + + // This field is supposed to be opaque to the core IBC protocol. Empty + // version is allowed by the specification (cf. ICS 004). No explicit validation necessary. let version = value.version; - Ok(ChannelEnd::new(ordering, remote, connection_hops, version)) + + let mut channel_end = ChannelEnd::new(chan_ordering, remote, connection_hops, version); + channel_end.set_state(chan_state); + + Ok(channel_end) } } impl ChannelEnd { + /// Creates a new ChannelEnd in state Uninitialized and other fields parametrized. pub fn new( ordering: Order, remote: Counterparty, @@ -60,6 +78,11 @@ impl ChannelEnd { version, } } + + /// Updates the ChannelEnd to assume a new State 's'. + pub fn set_state(&mut self, s: State) { + self.state = s; + } } impl Channel for ChannelEnd { @@ -136,3 +159,114 @@ impl ChannelCounterparty for Counterparty { Ok(()) } } + +#[cfg(test)] +mod tests { + use crate::ics04_channel::channel::ChannelEnd; + use crate::try_from_raw::TryFromRaw; + use ibc_proto::channel::Channel as RawChannel; + use ibc_proto::channel::Counterparty as RawCounterparty; + + #[test] + fn channel_end_try_from_raw() { + let empty_raw_channel_end = RawChannel { + state: 0, + ordering: 0, + counterparty: None, + connection_hops: vec![], + version: "".to_string(), + }; + + let cparty = RawCounterparty { + port_id: "0123456789".into(), + channel_id: "0987654321".into(), + }; + + let raw_channel_end = RawChannel { + state: 0, + ordering: 0, + counterparty: Some(cparty), + connection_hops: vec![], + version: "".to_string(), // The version is not validated. + }; + + struct Test { + name: String, + params: RawChannel, + want_pass: bool, + } + + let tests: Vec = vec![ + Test { + name: "Raw channel end with missing counterparty".to_string(), + params: empty_raw_channel_end.clone(), + want_pass: false, + }, + Test { + name: "Raw channel end with incorrect state".to_string(), + params: RawChannel { + state: -1, + ..raw_channel_end.clone() + }, + want_pass: false, + }, + Test { + name: "Raw channel end with incorrect ordering".to_string(), + params: RawChannel { + ordering: -1, + ..raw_channel_end.clone() + }, + want_pass: false, + }, + Test { + name: "Raw channel end with incorrect connection id in connection hops".to_string(), + params: RawChannel { + connection_hops: vec!["connection*".to_string()].into_iter().collect(), + ..raw_channel_end.clone() + }, + want_pass: false, + }, + Test { + name: "Raw channel end with incorrect connection id (has blank space)".to_string(), + params: RawChannel { + connection_hops: vec!["con nection".to_string()].into_iter().collect(), + ..raw_channel_end.clone() + }, + want_pass: false, + }, + Test { + name: "Raw channel end with two correct connection ids in connection hops" + .to_string(), + params: RawChannel { + connection_hops: vec!["connection1".to_string(), "connection2".to_string()] + .into_iter() + .collect(), + ..raw_channel_end.clone() + }, + want_pass: true, + }, + Test { + name: "Raw channel end with correct params".to_string(), + params: raw_channel_end.clone(), + want_pass: true, + }, + ] + .into_iter() + .collect(); + + for test in tests { + let p = test.params.clone(); + + let ce_result = ChannelEnd::try_from(p); + + assert_eq!( + test.want_pass, + ce_result.is_ok(), + "ChannelEnd::try_from() failed for test {}, \nmsg{:?} with error {:?}", + test.name, + test.params.clone(), + ce_result.err(), + ); + } + } +} diff --git a/modules/src/ics04_channel/error.rs b/modules/src/ics04_channel/error.rs index 5fb078bff1..d4c68e6fb3 100644 --- a/modules/src/ics04_channel/error.rs +++ b/modules/src/ics04_channel/error.rs @@ -28,6 +28,9 @@ pub enum Kind { #[error("acknowledgement too long")] AcknowledgementTooLong, + + #[error("missing counterparty")] + MissingCounterparty, } impl Kind { diff --git a/modules/src/ics04_channel/exported.rs b/modules/src/ics04_channel/exported.rs index 86cd7e090b..9d57138283 100644 --- a/modules/src/ics04_channel/exported.rs +++ b/modules/src/ics04_channel/exported.rs @@ -40,6 +40,18 @@ impl State { Self::Closed => "CLOSED", } } + + // Parses the State out from a i32. + pub fn from_i32(s: i32) -> Result { + match s { + 0 => Ok(Self::Uninitialized), + 1 => Ok(Self::Init), + 2 => Ok(Self::TryOpen), + 3 => Ok(Self::Open), + 4 => Ok(Self::Closed), + _ => fail!(error::Kind::UnknownState, s), + } + } } impl std::str::FromStr for State { @@ -73,6 +85,16 @@ impl Order { Self::Ordered => "ORDERED", } } + + // Parses the Order out from a i32. + pub fn from_i32(nr: i32) -> Result { + match nr { + 0 => Ok(Self::None), + 1 => Ok(Self::Unordered), + 2 => Ok(Self::Ordered), + _ => fail!(error::Kind::UnknownOrderType, nr), + } + } } impl std::str::FromStr for Order { diff --git a/modules/src/ics04_channel/msgs.rs b/modules/src/ics04_channel/msgs.rs index c31e4a1019..28d78cc721 100644 --- a/modules/src/ics04_channel/msgs.rs +++ b/modules/src/ics04_channel/msgs.rs @@ -619,11 +619,19 @@ mod tests { want_pass: true, }, Test { - name: "Bad port, non-alpha".to_string(), + name: "Correct port identifier".to_string(), params: OpenInitParams { port_id: "p34".to_string(), ..default_params.clone() }, + want_pass: true, + }, + Test { + name: "Incorrect port identifier, slash (separator) prohibited".to_string(), + params: OpenInitParams { + port_id: "p34/".to_string(), + ..default_params.clone() + }, want_pass: false, }, Test { @@ -651,7 +659,7 @@ mod tests { want_pass: false, }, Test { - name: "Bad connection hops".to_string(), + name: "Bad connection hops (conn id too short, must be 10 chars)".to_string(), params: OpenInitParams { connection_hops: vec!["conn124".to_string()].into_iter().collect(), ..default_params.clone() @@ -732,12 +740,12 @@ mod tests { want_pass: true, }, Test { - name: "Bad port, name non-alpha".to_string(), + name: "Correct port".to_string(), params: OpenTryParams { port_id: "p34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad port, name too short".to_string(), @@ -756,12 +764,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad channel, name non-alpha".to_string(), + name: "Correct channel identifier".to_string(), params: OpenTryParams { channel_id: "channelid34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad channel, name too short".to_string(), @@ -804,12 +812,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad connection hops, non-alpha connection id".to_string(), + name: "Correct connection hops (connection id)".to_string(), params: OpenTryParams { connection_hops: vec!["connection124".to_string()].into_iter().collect(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad connection hops, connection id too long".to_string(), @@ -855,12 +863,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad counterparty channel, name non-alpha".to_string(), + name: "Correct counterparty channel identifier".to_string(), params: OpenTryParams { counterparty_channel_id: "channelid34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, ] .into_iter() @@ -929,12 +937,12 @@ mod tests { want_pass: true, }, Test { - name: "Bad port, name non-alpha".to_string(), + name: "Correct port identifier".to_string(), params: OpenAckParams { port_id: "p34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad port, name too short".to_string(), @@ -953,12 +961,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad channel, name non-alpha".to_string(), + name: "Correct channel identifier".to_string(), params: OpenAckParams { channel_id: "channelid34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad channel, name too short".to_string(), @@ -1052,12 +1060,12 @@ mod tests { want_pass: true, }, Test { - name: "Bad port, name non-alpha".to_string(), + name: "Correct port".to_string(), params: OpenConfirmParams { port_id: "p34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad port, name too short".to_string(), @@ -1076,12 +1084,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad channel, name non-alpha".to_string(), + name: "Correct channel identifier".to_string(), params: OpenConfirmParams { channel_id: "channelid34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad channel, name too short".to_string(), @@ -1162,12 +1170,12 @@ mod tests { want_pass: true, }, Test { - name: "Bad port, name non-alpha".to_string(), + name: "Correct port".to_string(), params: CloseInitParams { port_id: "p34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad port, name too short".to_string(), @@ -1186,12 +1194,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad channel, name non-alpha".to_string(), + name: "Correct channel identifier".to_string(), params: CloseInitParams { channel_id: "channelid34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad channel, name too short".to_string(), @@ -1262,12 +1270,12 @@ mod tests { want_pass: true, }, Test { - name: "Bad port, name non-alpha".to_string(), + name: "Correct port".to_string(), params: CloseConfirmParams { port_id: "p34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad port, name too short".to_string(), @@ -1286,12 +1294,12 @@ mod tests { want_pass: false, }, Test { - name: "Bad channel, name non-alpha".to_string(), + name: "Correct channel identifier".to_string(), params: CloseConfirmParams { channel_id: "channelid34".to_string(), ..default_params.clone() }, - want_pass: false, + want_pass: true, }, Test { name: "Bad channel, name too short".to_string(), diff --git a/modules/src/ics07_tendermint/client_state.rs b/modules/src/ics07_tendermint/client_state.rs index 254bbcd445..fa5f3fa6d5 100644 --- a/modules/src/ics07_tendermint/client_state.rs +++ b/modules/src/ics07_tendermint/client_state.rs @@ -103,14 +103,15 @@ mod tests { #[test] fn serialization_roundtrip_no_proof() { - let json_data = include_str!("../tests/query/serialization/client_state.json"); + let json_data = include_str!("../../tests/support/query/serialization/client_state.json"); println!("json_data: {:?}", json_data); test_serialization_roundtrip::(json_data); } #[test] fn serialization_roundtrip_with_proof() { - let json_data = include_str!("../tests/query/serialization/client_state_proof.json"); + let json_data = + include_str!("../../tests/support/query/serialization/client_state_proof.json"); println!("json_data: {:?}", json_data); test_serialization_roundtrip::(json_data); } diff --git a/modules/src/ics07_tendermint/consensus_state.rs b/modules/src/ics07_tendermint/consensus_state.rs index d20bd7f5b6..dd4ba0ab80 100644 --- a/modules/src/ics07_tendermint/consensus_state.rs +++ b/modules/src/ics07_tendermint/consensus_state.rs @@ -54,14 +54,16 @@ mod tests { #[test] fn serialization_roundtrip_no_proof() { - let json_data = include_str!("../tests/query/serialization/consensus_state.json"); + let json_data = + include_str!("../../tests/support/query/serialization/consensus_state.json"); println!("json_data: {:?}", json_data); test_serialization_roundtrip::(json_data); } #[test] fn serialization_roundtrip_with_proof() { - let json_data = include_str!("../tests/query/serialization/consensus_state_proof.json"); + let json_data = + include_str!("../../tests/support/query/serialization/consensus_state_proof.json"); println!("json_data: {:?}", json_data); test_serialization_roundtrip::(json_data); } diff --git a/modules/src/ics24_host/error.rs b/modules/src/ics24_host/error.rs index 2271a61e68..5b90d5f46e 100644 --- a/modules/src/ics24_host/error.rs +++ b/modules/src/ics24_host/error.rs @@ -16,8 +16,8 @@ pub enum ValidationKind { max: usize, }, - #[error("identifier {id} must only contain lowercase alphanumeric characters")] - NotLowerAlpha { id: String }, + #[error("identifier {id} must only contain alphanumeric characters or `.`, `_`, `+`, `-`, `#`, - `[`, `]`, `<`, `>`")] + InvalidCharacter { id: String }, #[error("identifier cannot be empty")] Empty, @@ -37,8 +37,8 @@ impl ValidationKind { } } - pub fn not_lower_alpha(id: String) -> Self { - Self::NotLowerAlpha { id } + pub fn invalid_character(id: String) -> Self { + Self::InvalidCharacter { id } } pub fn empty() -> Self { diff --git a/modules/src/ics24_host/validate.rs b/modules/src/ics24_host/validate.rs index d2a26a37fd..5deef48127 100644 --- a/modules/src/ics24_host/validate.rs +++ b/modules/src/ics24_host/validate.rs @@ -9,6 +9,7 @@ macro_rules! bail { /// Path separator (ie. forward slash '/') const PATH_SEPARATOR: char = '/'; +const VALID_SPECIAL_CHARS: &str = "._+-#[]<>"; /// Default validator function for identifiers. /// @@ -37,9 +38,15 @@ pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Valid )); } - // Check identifier is lowercase alphanumeric - if !id.chars().all(|c| c.is_alphanumeric() && c.is_lowercase()) { - bail!(ValidationKind::not_lower_alpha(id.to_string())); + // Check that the identifier comprises only valid characters: + // - Alphanumeric + // - `.`, `_`, `+`, `-`, `#` + // - `[`, `]`, `<`, `>` + if !id + .chars() + .all(|c| c.is_alphanumeric() || VALID_SPECIAL_CHARS.contains(c)) + { + bail!(ValidationKind::invalid_character(id.to_string())); } // All good! diff --git a/modules/src/tests/query/serialization/client_state.json b/modules/tests/support/query/serialization/client_state.json similarity index 100% rename from modules/src/tests/query/serialization/client_state.json rename to modules/tests/support/query/serialization/client_state.json diff --git a/modules/src/tests/query/serialization/client_state_proof.json b/modules/tests/support/query/serialization/client_state_proof.json similarity index 100% rename from modules/src/tests/query/serialization/client_state_proof.json rename to modules/tests/support/query/serialization/client_state_proof.json diff --git a/modules/src/tests/query/serialization/consensus_state.json b/modules/tests/support/query/serialization/consensus_state.json similarity index 100% rename from modules/src/tests/query/serialization/consensus_state.json rename to modules/tests/support/query/serialization/consensus_state.json diff --git a/modules/src/tests/query/serialization/consensus_state_proof.json b/modules/tests/support/query/serialization/consensus_state_proof.json similarity index 100% rename from modules/src/tests/query/serialization/consensus_state_proof.json rename to modules/tests/support/query/serialization/consensus_state_proof.json diff --git a/relayer/cli/src/commands/query/channel.rs b/relayer/cli/src/commands/query/channel.rs index 6bca582ac7..0e56d3eccc 100644 --- a/relayer/cli/src/commands/query/channel.rs +++ b/relayer/cli/src/commands/query/channel.rs @@ -116,8 +116,8 @@ impl Runnable for QueryChannelEndCmd { let res: Result = block_on(query(&chain, opts)); match res { - Ok(cs) => status_info!("channel query result: ", "{:?}", cs), - Err(e) => status_info!("channel query error", "{}", e), + Ok(cs) => status_info!("Result for channel end query: ", "{:?}", cs), + Err(e) => status_info!("Error encountered on channel end query:", "{}", e), } } } @@ -174,11 +174,19 @@ mod tests { want_pass: false, }, Test { - name: "Bad port, non-alpha".to_string(), + name: "Correct port (alphanumeric)".to_string(), params: QueryChannelEndCmd { port_id: Some("p34".to_string()), ..default_params.clone() }, + want_pass: true, + }, + Test { + name: "Incorrect port identifier (contains invalid character)".to_string(), + params: QueryChannelEndCmd { + port_id: Some("p34^".to_string()), + ..default_params.clone() + }, want_pass: false, }, Test { diff --git a/relayer/relay/src/query.rs b/relayer/relay/src/query.rs index bf376adbf2..f0b4c0538a 100644 --- a/relayer/relay/src/query.rs +++ b/relayer/relay/src/query.rs @@ -23,7 +23,7 @@ pub struct Request { /// Whether or not this path requires proof verification. /// -/// is_query_store_with_proofxpects a format like ///, +/// is_query_store_with_proof expects a format like ///, /// where queryType must be "store" and subpath must be "key" to require a proof. fn is_query_store_with_proof(_path: &TendermintPath) -> bool { false @@ -37,11 +37,11 @@ where O: Into, // Query Command configuration (opts) { // RPC Request - let request: Request = request.into(); let path = request.path.clone().unwrap(); // for the is_query_store_with_proof function - // Use the Tendermint-rs RPC client to do the query - Todo: generalize further for other type of chains + // Use the Tendermint-rs RPC client to do the query. + // Todo: generalize further for other type of chains (#157). let response = chain .rpc_client() .abci_query( @@ -57,7 +57,7 @@ where .map_err(|e| error::Kind::Rpc.context(e))?; if !response.code.is_ok() { - // Fail with response log + // Fail with response log. return Err(error::Kind::Rpc.context(response.log.to_string()).into()); } if response.value.is_empty() { @@ -65,14 +65,12 @@ where return Err(error::Kind::EmptyResponseValue.into()); } - // Verify response proof - - // Data that is not from trusted node or subspace query needs verification + // Verify response proof. + // Data that is not from trusted node or subspace query needs verification. if is_query_store_with_proof(&path) { todo!() // TODO: Verify proof } - // Deserialize response data - + // Deserialize response data. T::deserialize(response.value) }