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

Add tests to verify AbciEvent match emitted events #363

Merged
merged 8 commits into from
Jan 24, 2023
Merged

Add tests to verify AbciEvent match emitted events #363

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
392155c
Verify AbciEvent keys match emitted events
Farhad-Shabani Jan 20, 2023
b2839f4
Add changelog entry
Farhad-Shabani Jan 20, 2023
9d0021d
Add expected value checks
Farhad-Shabani Jan 20, 2023
ffa0bbc
Hardcode attribute key constants
Farhad-Shabani Jan 23, 2023
817a440
Remove pub before channel attr key consts
Farhad-Shabani Jan 23, 2023
dc20ce7
Hardcode expected values
Farhad-Shabani Jan 24, 2023
13b434a
Fix version const
Farhad-Shabani Jan 24, 2023
2354ef5
Move away from default + Fix header issue
Farhad-Shabani Jan 24, 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
2 changes: 2 additions & 0 deletions .changelog/unreleased/improvements/163-verify-ibc-to-abci-event.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Add tests to verify `AbciEvent` match the expected Ibc events
([#163](https://github.com/cosmos/ibc-rs/issues/163)).
98 changes: 98 additions & 0 deletions crates/ibc/src/core/ics02_client/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,3 +390,101 @@ impl From<UpgradeClient> for abci::Event {
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::mock::client_state::client_type as mock_client_type;
use crate::mock::header::MockHeader;
use ibc_proto::google::protobuf::Any;
use tendermint::abci::Event as AbciEvent;

#[test]
fn ibc_to_abci_client_events() {
struct Test {
kind: IbcEventType,
event: AbciEvent,
expected_keys: Vec<&'static str>,
expected_values: Vec<String>,
}

let client_type = mock_client_type();
let client_id = ClientId::new(client_type.clone(), 0).unwrap();
let consensus_height = Height::new(0, 10).unwrap();
let consensus_heights = vec![Height::new(0, 5).unwrap(), Height::new(0, 7).unwrap()];
let header: Any = MockHeader::new(consensus_height).into();
let expected_keys = vec![
"client_id",
"client_type",
"consensus_height",
"consensus_heights",
"header",
];
let expected_values = vec![
client_id.to_string(),
"9999-mock".to_string(),
consensus_height.to_string(),
consensus_heights
.iter()
.map(|h| h.to_string())
.collect::<Vec<_>>()
.join(","),
String::from_utf8(hex::encode(header.clone().value)).unwrap(),
];
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't give me the confidence that we're constructing the events in the exact same way as ibc-go. We're still using our own objects for the expected_values; we want hardcoded strings here too.

The best way to write this test is to

  1. run ibc-go's EmitCreateClientEvent() with a given set of arguments
  2. print the output events to the console
  3. copy/paste the strings to construct expected_values
  4. Recreate the same arguments in (1) for client_type, client_id, etc

Repeat for the connection/channel events tests.

Make sure to use the main branch for ibc-go; the "consensus_heights" attribute doesn't exist yet in 5.0.1, and I made the mistake of adding it to ibc-rs.

Copy link
Member Author

@Farhad-Shabani Farhad-Shabani Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should be fine now dc20ce7
Just an issue I came across... Is it ok to have a default implementation for the ClientId (here)?
Therefore, the core module becomes chain-specific depending on a specific client. I think we should discard it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes let's discard it; and we have more Default implementations like that that we should discard at the same time. Can you create a new issue? Should be low priority though

Copy link
Member Author

Choose a reason for hiding this comment

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

of course


let tests: Vec<Test> = vec![
Test {
kind: IbcEventType::CreateClient,
event: CreateClient::new(client_id.clone(), client_type.clone(), consensus_height)
.into(),
expected_keys: expected_keys[0..3].to_vec(),
expected_values: expected_values[0..3].to_vec(),
},
Test {
kind: IbcEventType::UpdateClient,
event: UpdateClient::new(
client_id.clone(),
client_type.clone(),
consensus_height,
consensus_heights,
header,
)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values.clone(),
},
Test {
kind: IbcEventType::UpgradeClient,
event: UpgradeClient::new(client_id.clone(), client_type.clone(), consensus_height)
.into(),
expected_keys: expected_keys[0..3].to_vec(),
expected_values: expected_values[0..3].to_vec(),
},
Test {
kind: IbcEventType::ClientMisbehaviour,
event: ClientMisbehaviour::new(client_id, client_type).into(),
expected_keys: expected_keys[0..2].to_vec(),
expected_values: expected_values[0..2].to_vec(),
},
];

for t in tests {
assert_eq!(t.event.kind, t.kind.as_str());
assert_eq!(t.expected_keys.len(), t.event.attributes.len());
for (i, key) in t.expected_keys.iter().enumerate() {
assert_eq!(
t.event.attributes[i].key, *key,
"key mismatch for {:?}",
t.kind
);
}
for (i, value) in t.expected_values.iter().enumerate() {
assert_eq!(
t.event.attributes[i].value, *value,
"value mismatch for {:?}",
t.kind
);
}
}
}
}
104 changes: 104 additions & 0 deletions crates/ibc/src/core/ics03_connection/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,3 +281,107 @@ impl From<OpenConfirm> for abci::Event {
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::mock::client_state::client_type as mock_client_type;
use tendermint::abci::Event as AbciEvent;

#[test]
fn ibc_to_abci_connection_events() {
struct Test {
kind: IbcEventType,
event: AbciEvent,
expected_keys: Vec<&'static str>,
expected_values: Vec<String>,
}

let conn_id_on_a = ConnectionId::default();
let client_id_on_a = ClientId::default();
let conn_id_on_b = ConnectionId::new(1);
let client_id_on_b = ClientId::new(mock_client_type(), 0).unwrap();
let expected_keys = vec![
"connection_id",
"client_id",
"counterparty_client_id",
"counterparty_connection_id",
];
let expected_values = vec![
conn_id_on_a.to_string(),
client_id_on_a.to_string(),
client_id_on_b.to_string(),
conn_id_on_b.to_string(),
];

let tests: Vec<Test> = vec![
Test {
kind: IbcEventType::OpenInitConnection,
event: OpenInit::new(
conn_id_on_a.clone(),
client_id_on_a.clone(),
client_id_on_b.clone(),
)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values
.iter()
.enumerate()
.map(|(i, v)| if i == 3 { "".to_string() } else { v.clone() })
.collect(),
},
Test {
kind: IbcEventType::OpenTryConnection,
event: OpenTry::new(
conn_id_on_b.clone(),
client_id_on_b.clone(),
conn_id_on_a.clone(),
client_id_on_a.clone(),
)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values.iter().rev().cloned().collect(),
},
Test {
kind: IbcEventType::OpenAckConnection,
event: OpenAck::new(
conn_id_on_a.clone(),
client_id_on_a.clone(),
conn_id_on_b.clone(),
client_id_on_b.clone(),
)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values.clone(),
},
Test {
kind: IbcEventType::OpenConfirmConnection,
event: OpenConfirm::new(conn_id_on_b, client_id_on_b, conn_id_on_a, client_id_on_a)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values.iter().rev().cloned().collect(),
},
];

for t in tests {
assert_eq!(t.kind.as_str(), t.event.kind);
assert_eq!(t.expected_keys.len(), t.event.attributes.len());
for (i, key) in t.expected_keys.iter().enumerate() {
assert_eq!(
t.event.attributes[i].key,
*key,
"key mismatch for {:?}",
t.kind.as_str()
);
}
for (i, value) in t.expected_values.iter().enumerate() {
assert_eq!(
t.event.attributes[i].value,
*value,
"value mismatch for {:?}",
t.kind.as_str()
);
}
}
}
}
146 changes: 146 additions & 0 deletions crates/ibc/src/core/ics04_channel/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,3 +1058,149 @@ impl TryFrom<TimeoutPacket> for abci::Event {
})
}
}

#[cfg(test)]
mod tests {
use super::*;
use tendermint::abci::Event as AbciEvent;

#[test]
fn ibc_to_abci_channel_events() {
struct Test {
kind: IbcEventType,
event: AbciEvent,
expected_keys: Vec<&'static str>,
expected_values: Vec<String>,
}

let port_id = PortId::default();
let channel_id = ChannelId::default();
let connection_id = ConnectionId::default();
let counterparty_port_id = PortId::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use default() here too; we can just use new(). I don't like the use of default() in tests as they make it harder to verify correctness quickly (I need to go check the implementation for default() for each of these), for little gain.

let counterparty_channel_id = ChannelId::new(1);
let version = Version::default();
let expected_keys = vec![
"port_id",
"channel_id",
"counterparty_port_id",
"counterparty_channel_id",
"connection_id",
"version",
];
let expected_values = vec![
port_id.to_string(),
channel_id.to_string(),
counterparty_port_id.to_string(),
counterparty_channel_id.to_string(),
connection_id.to_string(),
version.to_string(),
];

let tests: Vec<Test> = vec![
Test {
kind: IbcEventType::OpenInitChannel,
event: OpenInit::new(
port_id.clone(),
channel_id.clone(),
counterparty_port_id.clone(),
connection_id.clone(),
version.clone(),
)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values
.iter()
.enumerate()
.map(|(i, v)| if i == 3 { "".to_string() } else { v.clone() })
.collect(),
},
Test {
kind: IbcEventType::OpenTryChannel,
event: OpenTry::new(
port_id.clone(),
channel_id.clone(),
counterparty_port_id.clone(),
counterparty_channel_id.clone(),
connection_id.clone(),
version,
)
.into(),
expected_keys: expected_keys.clone(),
expected_values: expected_values.clone(),
},
Test {
kind: IbcEventType::OpenAckChannel,
event: OpenAck::new(
port_id.clone(),
channel_id.clone(),
counterparty_port_id.clone(),
counterparty_channel_id.clone(),
connection_id.clone(),
)
.into(),
expected_keys: expected_keys[0..5].to_vec(),
expected_values: expected_values[0..5].to_vec(),
},
Test {
kind: IbcEventType::OpenConfirmChannel,
event: OpenConfirm::new(
port_id.clone(),
channel_id.clone(),
counterparty_port_id.clone(),
counterparty_channel_id.clone(),
connection_id.clone(),
)
.into(),
expected_keys: expected_keys[0..5].to_vec(),
expected_values: expected_values[0..5].to_vec(),
},
Test {
kind: IbcEventType::CloseInitChannel,
event: CloseInit::new(
port_id.clone(),
channel_id.clone(),
counterparty_port_id.clone(),
counterparty_channel_id.clone(),
connection_id.clone(),
)
.into(),
expected_keys: expected_keys[0..5].to_vec(),
expected_values: expected_values[0..5].to_vec(),
},
Test {
kind: IbcEventType::CloseConfirmChannel,
event: CloseConfirm::new(
port_id,
channel_id,
counterparty_port_id,
counterparty_channel_id,
connection_id,
)
.into(),
expected_keys: expected_keys[0..5].to_vec(),
expected_values: expected_values[0..5].to_vec(),
},
];

for t in tests {
assert_eq!(t.kind.as_str(), t.event.kind);
assert_eq!(t.expected_keys.len(), t.event.attributes.len());
for (i, key) in t.expected_keys.iter().enumerate() {
assert_eq!(
t.event.attributes[i].key,
*key,
"key mismatch for {:?}",
t.kind.as_str()
);
}
for (i, value) in t.expected_values.iter().enumerate() {
assert_eq!(
t.event.attributes[i].value,
*value,
"value mismatch for {:?}",
t.kind.as_str()
);
}
}
}
}
3 changes: 2 additions & 1 deletion crates/ibc/src/core/ics04_channel/events/channel_attributes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ const CHANNEL_ID_ATTRIBUTE_KEY: &str = "channel_id";
const PORT_ID_ATTRIBUTE_KEY: &str = "port_id";
/// This attribute key is public so that OpenInit can use it to convert itself
/// to an `AbciEvent`
pub const COUNTERPARTY_CHANNEL_ID_ATTRIBUTE_KEY: &str = "counterparty_channel_id";
pub(super) const COUNTERPARTY_CHANNEL_ID_ATTRIBUTE_KEY: &str = "counterparty_channel_id";
const COUNTERPARTY_PORT_ID_ATTRIBUTE_KEY: &str = "counterparty_port_id";
const VERSION_ATTRIBUTE_KEY: &str = "version";

#[cfg_attr(
feature = "parity-scale-codec",
derive(
Expand Down