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.

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

Channel query validation & tests #163

Merged
merged 6 commits into from
Jul 23, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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: 1 addition & 2 deletions modules/src/ics03_connection/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
3 changes: 2 additions & 1 deletion modules/src/ics03_connection/exported.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -43,6 +43,7 @@ impl State {
}
}

/// Parses the State from a i32.
pub fn from_i32(nr: i32) -> Self {
match nr {
1 => Self::Init,
Expand Down
7 changes: 4 additions & 3 deletions modules/src/ics03_connection/msgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
149 changes: 137 additions & 12 deletions modules/src/ics04_channel/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,30 +22,47 @@ pub struct ChannelEnd {
impl TryFromRaw for ChannelEnd {
type RawType = RawChannel;
type Error = anomaly::Error<Kind>;

fn try_from(value: RawChannel) -> Result<Self, Self::Error> {
// 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::<Result<Vec<_>, _>>()
.map_err(|e| Kind::IdentifierError.context(e))?;

// No explicit validation is necessary for the version attribute (opaque field).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what's the point of it? :)
Is it checked in some other context?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full details on the version field are here:
https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#versioning

The version field may be checked in other contexts (e.g., during a handshake, but as far as I can tell, it is not). But the point is that this field may contain fairly arbitrary data, according to the ICS4 spec. Looking through the cosmos-sdk codebase, however, I see that there is some basic validation on this field:

https://github.com/cosmos/cosmos-sdk/blob/e6a56226a88ec4e8761b1388a3a666af627d9e2e/x/ibc/04-channel/types/channel.go#L72

I'm adding a TODO to clarify this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there might be a mistake in cosmos-sdk code. We should permit an empty version string for ChannelEnd, so basically no validation is required here.

cosmos/cosmos-sdk#6830

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,
Expand All @@ -60,6 +77,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 {
Expand Down Expand Up @@ -136,3 +158,106 @@ 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<Test> = 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 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(),
);
}
}
}
3 changes: 3 additions & 0 deletions modules/src/ics04_channel/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ pub enum Kind {

#[error("acknowledgement too long")]
AcknowledgementTooLong,

#[error("missing counterparty")]
MissingCounterparty,
}

impl Kind {
Expand Down
22 changes: 22 additions & 0 deletions modules/src/ics04_channel/exported.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ impl State {
Self::Closed => "CLOSED",
}
}

// Parses the State out from a i32.
pub fn from_i32(s: i32) -> Result<Self, error::Error> {
match s {
0 => Ok(Self::Uninitialized),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, is the Uninitialized state an implementation requirement ? Just asking because in the ics4 spec the Channel state only has

enum ChannelState { INIT, OPENTRY, OPEN, CLOSED, }

I know the Golang implementation has the uninitialized state too, so maybe the spec should be updated ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an oversight in the cosmos/ICS repo and should be fixed in ICS4. I pinged Chris about it, will update here with details!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue in cosmos/ibc#452.

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 {
Expand Down Expand Up @@ -73,6 +85,16 @@ impl Order {
Self::Ordered => "ORDERED",
}
}

// Parses the Order out from a i32.
pub fn from_i32(nr: i32) -> Result<Self, error::Error> {
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 {
Expand Down
Loading