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

Re-enable tests in ics02_client and foreign_client #535

Merged
merged 29 commits into from
Jan 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ec00b02
ClientId constructor; re-enabled ClientCreate tests
adizere Jan 19, 2021
6518cb8
Improved ics18; Fixed cfg cargo annotation.
adizere Jan 19, 2021
14c5cac
Merge branch 'master' into adi/469_tests
adizere Jan 19, 2021
8ce3bb6
Prep for fixing ForeignClient tests.
adizere Jan 19, 2021
f0227bf
Merge branch 'master' into adi/469_tests
vitorenesduarte Jan 21, 2021
85e7e46
Enable tests in the foreign_client module
vitorenesduarte Jan 21, 2021
9966edf
Refer to const event types in tests
vitorenesduarte Jan 21, 2021
5c37cc5
Reuse Height parsing
vitorenesduarte Jan 21, 2021
9355d4f
Add comment
vitorenesduarte Jan 21, 2021
4092026
fmt
vitorenesduarte Jan 21, 2021
ba4a39d
Make clippy happy
vitorenesduarte Jan 21, 2021
e0c7383
Make clippy happy
vitorenesduarte Jan 21, 2021
9f6f639
add TODOs
vitorenesduarte Jan 22, 2021
6658b10
add TODO
vitorenesduarte Jan 22, 2021
90b64e4
More idiomatic methods in ibc::mock::context::MockContext
vitorenesduarte Jan 22, 2021
59f2036
fix TODO
vitorenesduarte Jan 22, 2021
25be8ae
Only create events in handlers
vitorenesduarte Jan 22, 2021
0741e75
Improve assert in build_create_client_and_send
vitorenesduarte Jan 22, 2021
b991020
Remove ibc::handler::Event
vitorenesduarte Jan 22, 2021
6ada439
Make clippy happy
vitorenesduarte Jan 22, 2021
e338044
Merge branch 'adi/469_tests' of https://github.com/informalsystems/ib…
vitorenesduarte Jan 22, 2021
da8c37a
Fix extract_connection_id
vitorenesduarte Jan 22, 2021
03c2469
Merge branch 'master' into adi/469_tests
vitorenesduarte Jan 22, 2021
4ffe9bb
address some TODOs
vitorenesduarte Jan 22, 2021
935742f
Check IBCEvent produced in create_client
vitorenesduarte Jan 22, 2021
1db5815
Check IBCEvent produced in test_tm_create_client_ok
vitorenesduarte Jan 22, 2021
84885c7
update CHANGELOG
vitorenesduarte Jan 25, 2021
e0db63c
relayer: improve tests in foreign_client
vitorenesduarte Jan 25, 2021
c5f2af9
fmt
vitorenesduarte Jan 25, 2021
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
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@ Cargo.lock
.idea

# Ignore VisualStudio
.vscode
.vscode

# Ignore chain's data
data
8 changes: 7 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
- [relayer-cli]
- Replace `ChannelConfig` in `Channel::new` ([#511])
- Add `packet-send` CLI ([#470])
- UX improvements for relayer txs ([#536, 540])
- UX improvements for relayer txs ([#536, #540])

- [relayer]
- Performance improvements ([#514], [#537])
Expand All @@ -34,6 +34,11 @@
- [modules]
- Fix for storing `ClientType` upon 'create-client' ([#513])

### BREAKING CHANGES:

- [modules]
- The `ibc::handler::Event` is removed and handlers now produce `ibc::events::IBCEvent`s ([#535])

[#94]: https://github.com/informalsystems/ibc-rs/issues/94
[#306]: https://github.com/informalsystems/ibc-rs/issues/306
[#470]: https://github.com/informalsystems/ibc-rs/issues/470
Expand All @@ -50,6 +55,7 @@
[#536]: https://github.com/informalsystems/ibc-rs/issues/536
[#537]: https://github.com/informalsystems/ibc-rs/issues/537
[#540]: https://github.com/informalsystems/ibc-rs/issues/540
[#535]: https://github.com/informalsystems/ibc-rs/issues/535


## v0.0.6
Expand Down
21 changes: 10 additions & 11 deletions modules/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use std::collections::HashMap;
use std::convert::{TryFrom, TryInto};
use tendermint::block::Height;

use tendermint::abci::Event;
use tracing::warn;

/// Events types
Expand Down Expand Up @@ -70,25 +69,24 @@ pub enum IBCEvent {
}

// This is tendermint specific
pub fn from_tx_response_event(event: Event) -> Option<IBCEvent> {
pub fn from_tx_response_event(event: &tendermint::abci::Event) -> Option<IBCEvent> {
// Return the first hit we find
// Look for client event...
if let Some(client_res) = ClientEvents::try_from_tx(event.clone()) {
return Some(client_res);
// Look for connection event...
} else if let Some(conn_res) = ConnectionEvents::try_from_tx(event.clone()) {
return Some(conn_res);
if let Some(client_res) = ClientEvents::try_from_tx(event) {
Some(client_res)
} else if let Some(conn_res) = ConnectionEvents::try_from_tx(event) {
Some(conn_res)
} else if let Some(chan_res) = ChannelEvents::try_from_tx(event) {
return Some(chan_res);
Some(chan_res)
} else {
None
}

None
}

impl IBCEvent {
pub fn to_json(&self) -> String {
serde_json::to_string(self).unwrap()
}

pub fn height(&self) -> Height {
match self {
IBCEvent::NewBlock(bl) => bl.height,
Expand All @@ -102,6 +100,7 @@ impl IBCEvent {
_ => unimplemented!(),
}
}

pub fn set_height(&mut self, height: ICSHeight) {
match self {
IBCEvent::SendPacketChannel(ev) => {
Expand Down
66 changes: 9 additions & 57 deletions modules/src/handler.rs
Original file line number Diff line number Diff line change
@@ -1,61 +1,13 @@
use crate::events::IBCEvent;
use std::marker::PhantomData;

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Attribute {
key: String,
value: String,
}

impl Attribute {
pub fn new(key: String, value: String) -> Self {
Self { key, value }
}

pub fn value(&self) -> String {
self.value.clone()
}

pub fn key(&self) -> String {
self.key.clone()
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub enum EventType {
Message,
Custom(String),
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Event {
Copy link
Member Author

Choose a reason for hiding this comment

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

If you wish, I suggest once we're done with this we do a follow-up PR to update ADR003 events section, making it clear that handlers are using the strongly-typed events, not these loosely-typed events.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we open an issue for that? It's probably better if someone familiar with that ADR does the change.

pub tpe: EventType,
pub attributes: Vec<Attribute>,
}

impl Event {
pub fn new(tpe: EventType, attrs: Vec<(String, String)>) -> Self {
Self {
tpe,
attributes: attrs
.into_iter()
.map(|(k, v)| Attribute::new(k, v))
.collect(),
}
}

/// Returns a vector containing the values within all attributes of this event
pub fn attribute_values(&self) -> Vec<String> {
self.attributes.iter().map(|a| a.value.clone()).collect()
}
}

pub type HandlerResult<T, E> = Result<HandlerOutput<T>, E>;

#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
pub struct HandlerOutput<T> {
pub result: T,
pub log: Vec<String>,
pub events: Vec<Event>,
pub events: Vec<IBCEvent>,
}

impl<T> HandlerOutput<T> {
Expand All @@ -64,10 +16,10 @@ impl<T> HandlerOutput<T> {
}
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
#[derive(Clone, Debug, Default)]
pub struct HandlerOutputBuilder<T> {
log: Vec<String>,
events: Vec<Event>,
events: Vec<IBCEvent>,
marker: PhantomData<T>,
}

Expand All @@ -89,13 +41,13 @@ impl<T> HandlerOutputBuilder<T> {
self.log.push(log.into());
}

pub fn with_events(mut self, events: impl Into<Vec<Event>>) -> Self {
self.events.append(&mut events.into());
pub fn with_events(mut self, mut events: Vec<IBCEvent>) -> Self {
self.events.append(&mut events);
self
}

pub fn emit(&mut self, event: impl Into<Event>) {
self.events.push(event.into());
pub fn emit(&mut self, event: IBCEvent) {
self.events.push(event);
}

pub fn with_result(self, result: T) -> HandlerOutput<T> {
Expand Down
16 changes: 13 additions & 3 deletions modules/src/ics02_client/client_type.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use super::error;
use std::fmt;

use serde_derive::{Deserialize, Serialize};

use super::error;

/// Type of the client, depending on the specific consensus algorithm.
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub enum ClientType {
Expand All @@ -17,11 +20,17 @@ impl ClientType {
Self::Tendermint => "07-tendermint",

#[cfg(any(test, feature = "mocks"))]
Self::Mock => "mock",
Self::Mock => "9999-mock",
}
}
}

impl fmt::Display for ClientType {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "ClientType({})", self.as_string())
}
}

impl std::str::FromStr for ClientType {
type Err = error::Error;

Expand All @@ -39,9 +48,10 @@ impl std::str::FromStr for ClientType {

#[cfg(test)]
mod tests {
use super::ClientType;
use std::str::FromStr;

use super::ClientType;

#[test]
fn parse_tendermint_client_type() {
let client_type = ClientType::from_str("07-tendermint");
Expand Down
29 changes: 17 additions & 12 deletions modules/src/ics02_client/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
use crate::ics02_client::client_def::{AnyClientState, AnyConsensusState};
use crate::ics02_client::client_type::ClientType;
use crate::ics02_client::error::Error;
use crate::ics02_client::handler::ClientResult::{Create, Update};
use crate::ics02_client::handler::{ClientEvent, ClientResult};
use crate::ics02_client::handler::ClientResult::{self, Create, Update};
use crate::ics24_host::identifier::ClientId;
use crate::Height;

Expand All @@ -15,25 +14,28 @@ pub trait ClientReader {
fn client_type(&self, client_id: &ClientId) -> Option<ClientType>;
fn client_state(&self, client_id: &ClientId) -> Option<AnyClientState>;
fn consensus_state(&self, client_id: &ClientId, height: Height) -> Option<AnyConsensusState>;

/// Returns a natural number, counting how many clients have been created thus far.
/// The value of this counter should increase only via method `ClientKeeper::increase_client_counter`.
fn client_counter(&self) -> u64;
}

/// Defines the write-only part of ICS2 (client functions) context.
pub trait ClientKeeper {
fn store_client_result(
&mut self,
handler_res: ClientResult,
) -> Result<Vec<ClientEvent>, Error> {
fn store_client_result(&mut self, handler_res: ClientResult) -> Result<(), Error> {
match handler_res {
Create(res) => {
let client_id = self.next_client_id();
let client_id = res.client_id.clone();

self.store_client_type(client_id.clone(), res.client_type)?;
self.store_client_state(client_id.clone(), res.client_state.clone())?;
self.store_consensus_state(
client_id.clone(),
client_id,
res.client_state.latest_height(),
res.consensus_state,
)?;
Ok(vec![ClientEvent::ClientCreated(client_id)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Events were being created both here and at the handlers. Now they're only created at the handlers.

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks better & cleaner!

Just to note that In future (or in other handlers) we may find it useful to create events both from the process method of the handlers, as well as from the ClientKeeper. Then we would collect both of these vectors of events into a single one in ICS26::dispatch. But that's not for now..

self.increase_client_counter();
Ok(())
}
Update(res) => {
self.store_client_state(res.client_id.clone(), res.client_state.clone())?;
Expand All @@ -42,13 +44,11 @@ pub trait ClientKeeper {
res.client_state.latest_height(),
res.consensus_state,
)?;
Ok(vec![ClientEvent::ClientUpdated(res.client_id)])
Ok(())
}
}
}

fn next_client_id(&mut self) -> ClientId;

/// Called upon successful client creation
fn store_client_type(
&mut self,
Expand All @@ -70,4 +70,9 @@ pub trait ClientKeeper {
height: Height,
consensus_state: AnyConsensusState,
) -> Result<(), Error>;

/// Called upon client creation.
/// Increases the counter which keeps track of how many clients have been created.
/// Should never fail.
fn increase_client_counter(&mut self);
}
3 changes: 3 additions & 0 deletions modules/src/ics02_client/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ pub enum Kind {
#[error("unknown client type: {0}")]
UnknownClientType(String),

#[error("Client identifier constructor failed for type {0} with counter {1}")]
ClientIdentifierConstructor(ClientType, u64),

#[error("client already exists: {0}")]
ClientAlreadyExists(ClientId),

Expand Down
Loading