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

Jump to bottom

Model ChannelId as newtype u64 and improve validation #2071

Merged
merged 11 commits into from
Apr 8, 2022
Merged

Model ChannelId as newtype u64 and improve validation #2071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
24c2d7b
Remodel ChannelId as newtype u64
hu55a1n1 Apr 6, 2022
72b7b76
Fix AsRef<str> usages
hu55a1n1 Apr 6, 2022
b301f05
Modify channel-id validation to not use regex
hu55a1n1 Apr 6, 2022
bbebc34
Update tests to use valid channel ids
hu55a1n1 Apr 6, 2022
99a16d9
Fix clippy errors
hu55a1n1 Apr 6, 2022
b83fa5b
Impl Clone for ChannelId
hu55a1n1 Apr 6, 2022
8f4770b
Delete unused validate_channel_identifier() function
hu55a1n1 Apr 6, 2022
1c81c0a
Custom serde for ChannelId
hu55a1n1 Apr 6, 2022
a632ed2
Add .changelog entry
hu55a1n1 Apr 6, 2022
1a6b41a
Fix failing tests
hu55a1n1 Apr 6, 2022
91c4c04
Merge remote-tracking branch 'origin/master' into hu55a1n1/2068-chan-…
hu55a1n1 Apr 7, 2022
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
1 change: 1 addition & 0 deletions .changelog/unreleased/improvements/ibc/2068-chan-id-u64.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Improve `ChannelId` validation. ([#2068](https://github.com/informalsystems/ibc-rs/issues/2068))
11 changes: 4 additions & 7 deletions ...s/src/applications/ics20_fungible_token_transfer/relay_application_logic/send_transfer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,20 @@ where
Ctx: Ics20Context,
{
let source_channel_end = ctx
.channel_end(&(msg.source_port.clone(), msg.source_channel.clone()))
.channel_end(&(msg.source_port.clone(), msg.source_channel))
.map_err(Error::ics04_channel)?;

let destination_port = source_channel_end.counterparty().port_id().clone();
let destination_channel = source_channel_end
.counterparty()
.channel_id()
.ok_or_else(|| {
Error::destination_channel_not_found(
msg.source_port.clone(),
msg.source_channel.clone(),
)
Error::destination_channel_not_found(msg.source_port.clone(), msg.source_channel)
})?;

// get the next sequence
let sequence = ctx
.get_next_sequence_send(&(msg.source_port.clone(), msg.source_channel.clone()))
.get_next_sequence_send(&(msg.source_port.clone(), msg.source_channel))
.map_err(Error::ics04_channel)?;

//TODO: Application LOGIC.
Expand All @@ -41,7 +38,7 @@ where
source_port: msg.source_port,
source_channel: msg.source_channel,
destination_port,
destination_channel: destination_channel.clone(),
destination_channel: *destination_channel,
data: vec![0],
timeout_height: msg.timeout_height,
timeout_timestamp: msg.timeout_timestamp,
Expand Down
10 changes: 5 additions & 5 deletions modules/src/clients/ics07_tendermint/client_def.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ impl ClientDef for TendermintClient {
) -> Result<(), Ics02Error> {
client_state.verify_height(height)?;

let path = ChannelEndsPath(port_id.clone(), channel_id.clone());
let path = ChannelEndsPath(port_id.clone(), *channel_id);
let value = expected_channel_end.encode_vec().unwrap();
verify_membership(client_state, prefix, proof, root, path, value)
}
Expand Down Expand Up @@ -282,7 +282,7 @@ impl ClientDef for TendermintClient {

let commitment_path = CommitmentsPath {
port_id: port_id.clone(),
channel_id: channel_id.clone(),
channel_id: *channel_id,
sequence,
};

Expand Down Expand Up @@ -319,7 +319,7 @@ impl ClientDef for TendermintClient {

let ack_path = AcksPath {
port_id: port_id.clone(),
channel_id: channel_id.clone(),
channel_id: *channel_id,
sequence,
};
verify_membership(
Expand Down Expand Up @@ -352,7 +352,7 @@ impl ClientDef for TendermintClient {
.encode(&mut seq_bytes)
.expect("buffer size too small");

let seq_path = SeqRecvsPath(port_id.clone(), channel_id.clone());
let seq_path = SeqRecvsPath(port_id.clone(), *channel_id);
verify_membership(
client_state,
connection_end.counterparty().prefix(),
Expand Down Expand Up @@ -380,7 +380,7 @@ impl ClientDef for TendermintClient {

let receipt_path = ReceiptsPath {
port_id: port_id.clone(),
channel_id: channel_id.clone(),
channel_id: *channel_id,
sequence,
};
verify_non_membership(
Expand Down
2 changes: 1 addition & 1 deletion modules/src/core/ics04_channel/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ impl From<Counterparty> for RawCounterparty {
port_id: value.port_id.as_str().to_string(),
channel_id: value
.channel_id
.map_or_else(|| "".to_string(), |v| v.as_str().to_string()),
.map_or_else(|| "".to_string(), |v| v.to_string()),
}
}
}
Expand Down
34 changes: 12 additions & 22 deletions modules/src/core/ics04_channel/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ pub trait ChannelKeeper {
fn store_channel_result(&mut self, result: ChannelResult) -> Result<(), Error> {
// The handler processed this channel & some modifications occurred, store the new end.
self.store_channel(
(result.port_id.clone(), result.channel_id.clone()),
(result.port_id.clone(), result.channel_id),
&result.channel_end,
)?;

Expand All @@ -133,18 +133,12 @@ pub trait ChannelKeeper {
// Associate also the channel end to its connection.
self.store_connection_channels(
result.channel_end.connection_hops()[0].clone(),
&(result.port_id.clone(), result.channel_id.clone()),
&(result.port_id.clone(), result.channel_id),
)?;

// Initialize send, recv, and ack sequence numbers.
self.store_next_sequence_send(
(result.port_id.clone(), result.channel_id.clone()),
1.into(),
)?;
self.store_next_sequence_recv(
(result.port_id.clone(), result.channel_id.clone()),
1.into(),
)?;
self.store_next_sequence_send((result.port_id.clone(), result.channel_id), 1.into())?;
self.store_next_sequence_recv((result.port_id.clone(), result.channel_id), 1.into())?;
self.store_next_sequence_ack((result.port_id, result.channel_id), 1.into())?;
}

Expand All @@ -155,12 +149,12 @@ pub trait ChannelKeeper {
match general_result {
PacketResult::Send(res) => {
self.store_next_sequence_send(
(res.port_id.clone(), res.channel_id.clone()),
(res.port_id.clone(), res.channel_id),
res.seq_number,
)?;

self.store_packet_commitment(
(res.port_id.clone(), res.channel_id.clone(), res.seq),
(res.port_id.clone(), res.channel_id, res.seq),
res.timeout_timestamp,
res.timeout_height,
res.data,
Expand All @@ -175,22 +169,22 @@ pub trait ChannelKeeper {
None => {
// Ordered channel
self.store_next_sequence_recv(
(res.port_id.clone(), res.channel_id.clone()),
(res.port_id.clone(), res.channel_id),
res.seq_number,
)?
}
Some(r) => {
// Unordered channel
self.store_packet_receipt(
(res.port_id.clone(), res.channel_id.clone(), res.seq),
(res.port_id.clone(), res.channel_id, res.seq),
r,
)?
}
}
}
PacketResult::WriteAck(res) => {
self.store_packet_acknowledgement(
(res.port_id.clone(), res.channel_id.clone(), res.seq),
(res.port_id.clone(), res.channel_id, res.seq),
res.ack,
)?;
}
Expand All @@ -204,7 +198,7 @@ pub trait ChannelKeeper {
//Unordered Channel
self.delete_packet_commitment((
res.port_id.clone(),
res.channel_id.clone(),
res.channel_id,
res.seq,
))?;
}
Expand All @@ -213,13 +207,9 @@ pub trait ChannelKeeper {
PacketResult::Timeout(res) => {
if let Some(c) = res.channel {
//Ordered Channel
self.store_channel((res.port_id.clone(), res.channel_id.clone()), &c)?;
self.store_channel((res.port_id.clone(), res.channel_id), &c)?;
}
self.delete_packet_commitment((
res.port_id.clone(),
res.channel_id.clone(),
res.seq,
))?;
self.delete_packet_commitment((res.port_id.clone(), res.channel_id, res.seq))?;
}
}
Ok(())
Expand Down
16 changes: 8 additions & 8 deletions modules/src/core/ics04_channel/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ impl From<Attributes> for Vec<Tag> {
if let Some(channel_id) = a.channel_id {
let channel_id = Tag {
key: CHANNEL_ID_ATTRIBUTE_KEY.parse().unwrap(),
value: channel_id.as_str().parse().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

.to_string().parse() looks like an unnecessary string conversion roundtrip. But an ABCI tag value is a UTF-8 string super-encoded as base64 for an inexplicable reason. Go figure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it seems unnecessary. But I don't think there's any other public constructor for Tag::Value. 🤔 So not sure how to fix this. IIUC, the tag value was originally designed to be opaque bytes and later it was decided that (event) indexing was required and we're now left with this base64 encoded string.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing to fix here, maybe just add a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noticed that we already have this comment above -> https://github.com/informalsystems/ibc-rs/pull/2071/files/91c4c04dd191753eeac90159d2912262dfee17e0#diff-5d54d6bafcc46a42f95443d7c324b7c79563ad1f089618a686a8eb0d54e8beb0L237-L242

/// # Note
/// The parsing of `Key`s and `Value`s never fails, because the
/// `FromStr` instance of `tendermint::abci::tag::{Key, Value}`
/// is infallible, even if it is not represented in the error type.
/// Once tendermint-rs improves the API of the `Key` and `Value` types,
/// we will be able to remove the `.parse().unwrap()` calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, nothing to fix there, it's just an observation (maybe we should have an etiquette about this 😊)

value: channel_id.to_string().parse().unwrap(),
};
attributes.push(channel_id);
}
Expand All @@ -273,7 +273,7 @@ impl From<Attributes> for Vec<Tag> {
if let Some(channel_id) = a.counterparty_channel_id {
let channel_id = Tag {
key: COUNTERPARTY_CHANNEL_ID_ATTRIBUTE_KEY.parse().unwrap(),
value: channel_id.as_str().parse().unwrap(),
value: channel_id.to_string().parse().unwrap(),
};
attributes.push(channel_id);
}
Expand Down Expand Up @@ -612,10 +612,10 @@ impl TryFrom<Attributes> for CloseInit {
Ok(CloseInit {
height: attrs.height,
port_id: attrs.port_id.clone(),
channel_id: channel_id.clone(),
channel_id: *channel_id,
connection_id: attrs.connection_id.clone(),
counterparty_port_id: attrs.counterparty_port_id.clone(),
counterparty_channel_id: attrs.counterparty_channel_id.clone(),
counterparty_channel_id: attrs.counterparty_channel_id,
})
} else {
Err(EventError::channel(Error::missing_channel_id()))
Expand Down Expand Up @@ -1112,10 +1112,10 @@ mod tests {
let attributes = Attributes {
height: Height::default(),
port_id: "test_port".parse().unwrap(),
channel_id: Some("test_channel".parse().unwrap()),
channel_id: Some("channel-0".parse().unwrap()),
connection_id: "test_connection".parse().unwrap(),
counterparty_port_id: "counterparty_test_port".parse().unwrap(),
counterparty_channel_id: Some("counterparty_test_channel".parse().unwrap()),
counterparty_channel_id: Some("channel-1".parse().unwrap()),
};
let mut abci_events = vec![];
let open_init = OpenInit::try_from(attributes.clone()).unwrap();
Expand Down Expand Up @@ -1164,9 +1164,9 @@ mod tests {
let packet = Packet {
sequence: Sequence::from(10),
source_port: "a_test_port".parse().unwrap(),
source_channel: "a_test_channel".parse().unwrap(),
source_channel: "channel-0".parse().unwrap(),
destination_port: "b_test_port".parse().unwrap(),
destination_channel: "b_test_channel".parse().unwrap(),
destination_channel: "channel-1".parse().unwrap(),
data: "test_data".as_bytes().to_vec(),
timeout_height: Height::new(1, 10),
timeout_timestamp: Timestamp::now(),
Expand Down
22 changes: 11 additions & 11 deletions modules/src/core/ics04_channel/handler/acknowledgement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,23 @@ pub fn process(
let packet = &msg.packet;

let source_channel_end =
ctx.channel_end(&(packet.source_port.clone(), packet.source_channel.clone()))?;
ctx.channel_end(&(packet.source_port.clone(), packet.source_channel))?;

if !source_channel_end.state_matches(&State::Open) {
return Err(Error::channel_closed(packet.source_channel.clone()));
return Err(Error::channel_closed(packet.source_channel));
}

let _channel_cap = ctx.authenticated_capability(&packet.source_port)?;

let counterparty = Counterparty::new(
packet.destination_port.clone(),
Some(packet.destination_channel.clone()),
Some(packet.destination_channel),
);

if !source_channel_end.counterparty_matches(&counterparty) {
return Err(Error::invalid_packet_counterparty(
packet.destination_port.clone(),
packet.destination_channel.clone(),
packet.destination_channel,
));
}

Expand All @@ -59,7 +59,7 @@ pub fn process(
// Verify packet commitment
let packet_commitment = ctx.get_packet_commitment(&(
packet.source_port.clone(),
packet.source_channel.clone(),
packet.source_channel,
packet.sequence,
))?;

Expand All @@ -83,8 +83,8 @@ pub fn process(
)?;

let result = if source_channel_end.order_matches(&Order::Ordered) {
let next_seq_ack = ctx
.get_next_sequence_ack(&(packet.source_port.clone(), packet.source_channel.clone()))?;
let next_seq_ack =
ctx.get_next_sequence_ack(&(packet.source_port.clone(), packet.source_channel))?;

if packet.sequence != next_seq_ack {
return Err(Error::invalid_packet_sequence(
Expand All @@ -95,14 +95,14 @@ pub fn process(

PacketResult::Ack(AckPacketResult {
port_id: packet.source_port.clone(),
channel_id: packet.source_channel.clone(),
channel_id: packet.source_channel,
seq: packet.sequence,
seq_number: Some(next_seq_ack.increment()),
})
} else {
PacketResult::Ack(AckPacketResult {
port_id: packet.source_port.clone(),
channel_id: packet.source_channel.clone(),
channel_id: packet.source_channel,
seq: packet.sequence,
seq_number: None,
})
Expand Down Expand Up @@ -171,7 +171,7 @@ mod tests {
Order::default(),
Counterparty::new(
packet.destination_port.clone(),
Some(packet.destination_channel.clone()),
Some(packet.destination_channel),
),
vec![ConnectionId::default()],
Version::ics20(),
Expand Down Expand Up @@ -215,7 +215,7 @@ mod tests {
.with_port_capability(packet.destination_port.clone())
.with_channel(
packet.source_port.clone(),
packet.source_channel.clone(),
packet.source_channel,
source_channel_end,
)
.with_packet_commitment(
Expand Down
15 changes: 7 additions & 8 deletions modules/src/core/ics04_channel/handler/chan_close_confirm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ pub(crate) fn process(
let mut output = HandlerOutput::builder();

// Retrieve the old channel end and validate it against the message.
let mut channel_end = ctx.channel_end(&(msg.port_id.clone(), msg.channel_id.clone()))?;
let mut channel_end = ctx.channel_end(&(msg.port_id.clone(), msg.channel_id))?;

// Validate that the channel end is in a state where it can be closed.
if channel_end.state_matches(&State::Closed) {
return Err(Error::channel_closed(msg.channel_id.clone()));
return Err(Error::channel_closed(msg.channel_id));
}

// Channel capabilities
Expand All @@ -47,8 +47,7 @@ pub(crate) fn process(
// Proof verification in two steps:
// 1. Setup: build the Channel as we expect to find it on the other party.

let expected_counterparty =
Counterparty::new(msg.port_id.clone(), Some(msg.channel_id.clone()));
let expected_counterparty = Counterparty::new(msg.port_id.clone(), Some(msg.channel_id));

let counterparty = conn.counterparty();
let ccid = counterparty.connection_id().ok_or_else(|| {
Expand Down Expand Up @@ -81,14 +80,14 @@ pub(crate) fn process(

let result = ChannelResult {
port_id: msg.port_id.clone(),
channel_id: msg.channel_id.clone(),
channel_id: msg.channel_id,
channel_id_state: ChannelIdState::Reused,
channel_cap,
channel_end,
};

let event_attributes = Attributes {
channel_id: Some(msg.channel_id.clone()),
channel_id: Some(msg.channel_id),
height: ctx.host_height(),
..Default::default()
};
Expand Down Expand Up @@ -151,7 +150,7 @@ mod tests {
Order::default(),
Counterparty::new(
msg_chan_close_confirm.port_id.clone(),
Some(msg_chan_close_confirm.channel_id.clone()),
Some(msg_chan_close_confirm.channel_id),
),
vec![conn_id.clone()],
Version::default(),
Expand All @@ -163,7 +162,7 @@ mod tests {
.with_port_capability(msg_chan_close_confirm.port_id.clone())
.with_channel(
msg_chan_close_confirm.port_id.clone(),
msg_chan_close_confirm.channel_id.clone(),
msg_chan_close_confirm.channel_id,
chan_end,
);

Expand Down
Loading