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

Quickfix: make fields in event subscription accessible publicly #312

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented Jun 9, 2020

Non-elegant quickfix: makes all event subscription fields pub s.t. they can be accessed and processed.

close #308

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGES.md

@liamsi liamsi requested a review from ancazamfir June 9, 2020 13:54
@codecov-commenter
Copy link

Codecov Report

Merging #312 into master will not change coverage.
The diff coverage is 0.0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #312   +/-   ##
======================================
  Coverage    30.5%   30.5%           
======================================
  Files         127     127           
  Lines        4580    4580           
  Branches     1416    1416           
======================================
  Hits         1398    1398           
  Misses       2215    2215           
  Partials      967     967           
Impacted Files Coverage Δ
tendermint/src/rpc/event_listener.rs 0.0% <0.0%> (ø)

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 71d7e23...cc6a4cf. Read the comment docs.

@@ -101,7 +101,8 @@ impl EventListener {
dbg!(&msg.to_string());
return Ok(None);
}

dbg!("received neither event nor generic string message:");
dbg!(&msg.to_string());
Copy link
Contributor

@ancazamfir ancazamfir Jun 9, 2020

Choose a reason for hiding this comment

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

I checked this with IBC relayer and it's "" for #311

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 that is a ping message:
https://github.com/tendermint/tendermint/blob/ba3a2dde376f708085726cd778bb361bd8c7c692/rpc/jsonrpc/server/ws_handler.go#L407

If the server doesn't drop your event subscription, you can ignore it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the server doesn't drop your event subscription, you can ignore it for now.

Yes, I am ignoring but was wondering if this could be handled more gracefully :) Anyway..this is for the other issue. Just saw you added this here and wanted to mention that for the failure I see it is empty string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I am ignoring but was wondering if this could be handled more gracefully :)

Absolutely, the client should actually play ping-pong with the server instead of treating this as an error.

Copy link
Contributor

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@liamsi liamsi merged commit eda96b4 into master Jun 9, 2020
@liamsi liamsi deleted the ismail/events_publisize_fields branch June 9, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need access to block in EventDataNewBlock
3 participants