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

Add IBC events, handler and per chain monitor tasks #88

Merged
merged 8 commits into from
Jun 12, 2020
Merged

Conversation

ancazamfir
Copy link
Collaborator

@ancazamfir ancazamfir commented Jun 9, 2020

Closes: #87

Description

Starting with informalsystems/ibc-rs#59 as base, extract attributes specific to IBC events.
Add monitor tasks, one per chain, that register for Tx and Block events with tendermint nodes.
Push the IBC events to the main handler task.

There are a few things to clarify:


For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

@ancazamfir ancazamfir added this to the 0.5-5mo milestone Jun 9, 2020
@ancazamfir ancazamfir requested a review from romac as a code owner June 9, 2020 08:58
@ancazamfir ancazamfir self-assigned this Jun 9, 2020
@ancazamfir ancazamfir changed the title [WIP] Add main event handler and per chain monitor tasks [WIP] Add IBC events, handler and per chain monitor tasks Jun 9, 2020
@ancazamfir ancazamfir requested review from Shivani912 and zmanian June 9, 2020 09:04
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Just have some nitpicks and minor comments, looks good otherwise! (only reviewed it superficially, so not an endorsement w.r.t. to functionality, though nothing on that side jumped out to me :)

modules/Cargo.toml Outdated Show resolved Hide resolved
modules/src/error.rs Outdated Show resolved Hide resolved
modules/src/events.rs Outdated Show resolved Hide resolved
modules/src/events.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/client_type.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/events.rs Outdated Show resolved Hide resolved
modules/src/events.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #88 into master will decrease coverage by 3.3%.
The diff coverage is 0.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #88     +/-   ##
========================================
- Coverage    14.5%   11.1%   -3.4%     
========================================
  Files          58      67      +9     
  Lines        2219    2825    +606     
  Branches      761    1002    +241     
========================================
- Hits          322     315      -7     
- Misses       1598    2211    +613     
  Partials      299     299             
Impacted Files Coverage Δ
modules/src/error.rs 0.0% <0.0%> (ø)
modules/src/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/client_type.rs 47.6% <0.0%> (ø)
modules/src/ics02_client/events.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/events.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/msgs.rs 20.2% <ø> (-0.9%) ⬇️
modules/src/ics04_channel/events.rs 0.0% <0.0%> (ø)
modules/src/ics04_channel/msgs.rs 21.6% <ø> (-0.7%) ⬇️
modules/src/ics07_tendermint/msgs/create_client.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/msgs/update_client.rs 0.0% <0.0%> (ø)
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf63bda...b10f0b4. Read the comment docs.

@ancazamfir ancazamfir changed the title [WIP] Add IBC events, handler and per chain monitor tasks Add IBC events, handler and per chain monitor tasks Jun 12, 2020
@ancazamfir ancazamfir added I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic and removed work in progress labels Jun 12, 2020
@ancazamfir ancazamfir requested a review from milosevic June 12, 2020 06:57
modules/src/events.rs Outdated Show resolved Hide resolved
channel_to_handler: Sender<(ChainId, Vec<IBCEvent>)>,
) -> Result<Self, Box<dyn std::error::Error>> {
let mut event_listener = event_listener::EventListener::connect(rpc_addr.clone()).await?;
// TODO move them to config file(?)
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 create issue for this?

pub async fn collect_events(&mut self) -> Result<(), TMError> {
if let Some(tm_event) = self.event_listener.get_event().await? {
if let Ok(ibc_events) = relayer_modules::events::get_all_events(tm_event) {
// TODO - send_timeout()?
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 create issue for this?

Copy link
Contributor

@milosevic milosevic left a comment

Choose a reason for hiding this comment

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

Great work. I have created an issue for figuring out common format for IBC events informalsystems/ibc-rs#102.

@ancazamfir ancazamfir merged commit 147e44a into master Jun 12, 2020
@ancazamfir ancazamfir deleted the anca/monitor branch June 13, 2020 07:23
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Add main event handler and per chain monitor tasks

* Code review comments and fix clippy errors

* Fix cargo fmt

* Add command to start relayer in listen mode

* Change to use anomaly::BoxError - review comment

* Remove unused code - review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscribe to TM events and push IBC events to the relayer
5 participants