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

light-client: improve supervisor ergonomics #403

Merged
merged 17 commits into from
Jul 3, 2020

Conversation

xla
Copy link
Contributor

@xla xla commented Jun 30, 2020

  • replace callbacks with channels
  • remove Callback abstraction
  • relax mutability on Handle
  • provide noop default implementations for Handle trait methods
  • handle crossbeam errors honestly
  • update ADR status
  • add changes entry

Closes #398
Implements ADR-007
Depends on #401


  • 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

xla added 5 commits June 30, 2020 21:58
As we start to depend on the surface of the `Handle` we benefit from it
being a trait that can be implemented on a per need basis. This will
result in less overhead constructing object graphs in places where we
wannt to assert behaviour of other types in remote places, i.e. the
light-node rpc server. Overall we hope for an increased ease in writing
tests on module level.

Ref #219
@xla xla added enhancement New feature or request light-client Issues/features which involve the light client labels Jun 30, 2020
@xla xla added this to the Light Node milestone Jun 30, 2020
@xla xla requested review from brapse, romac, ebuchman and liamsi June 30, 2020 21:25
@xla xla self-assigned this Jun 30, 2020
Base automatically changed from xla/219-handle-trait to master July 1, 2020 08:01
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2020

Codecov Report

Merging #403 into master will increase coverage by 0.0%.
The diff coverage is 1.8%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #403   +/-   ##
======================================
  Coverage    28.2%   28.2%           
======================================
  Files         130     129    -1     
  Lines        5383    5381    -2     
  Branches     1663    1666    +3     
======================================
  Hits         1519    1519           
+ Misses       2795    2793    -2     
  Partials     1069    1069           
Impacted Files Coverage Δ
light-client/src/errors.rs 0.0% <0.0%> (ø)
light-client/src/fork_detector.rs 0.0% <0.0%> (ø)
light-client/src/light_client.rs 31.0% <ø> (ø)
light-client/src/supervisor.rs 0.0% <0.0%> (ø)
light-client/src/predicates.rs 75.2% <100.0%> (ø)
tendermint/src/channel.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 9adad9a...1510e8b. Read the comment docs.

@xla xla marked this pull request as ready for review July 2, 2020 19:36
@xla xla requested a review from brapse July 2, 2020 19:36
@xla xla mentioned this pull request Jul 2, 2020
@@ -57,6 +60,9 @@ pub enum ErrorKind {

#[error("invalid light block: {0}")]
InvalidLightBlock(#[source] VerificationError),

#[error("internal channel disconnected")]
ChannelDisconnected,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be nice to "decorate" the error with the underlying crossbeam error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we jump through these hoops is that the crossbeam errors don't derive Deserialize and Serialize which we need for our errors. Furthermore they only carry the message that failed to send, Would question how much value that has.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I thought you can wrap errors in the error kinds which do not derive (de)serialize and still have serialization via anomaly but I might be wrong (or too tired). Doesn't sound like it's worth to further investigate here.

light-client/src/supervisor.rs Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
light-client/examples/light_client.rs Show resolved Hide resolved
light-client/src/errors.rs Show resolved Hide resolved
loop {
let event = self.receiver.recv().unwrap();
let event = self.receiver.recv().map_err(ErrorKind::from)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice improvement 😍

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Nice work 👏 👏 👏

@brapse brapse merged commit 78ff0c3 into master Jul 3, 2020
@brapse brapse deleted the xla/398-improve-supervisor-ergonomics branch July 3, 2020 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request light-client Issues/features which involve the light client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

light-client: ergonomic limitations of Callback abstraction
5 participants