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: turn Handle into a trait #401

Merged
merged 3 commits into from
Jul 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
Light Client:

- Expose latest_trusted from Supervisor Handle ([#394])
- Turn `Handle` into a trait for ease of integration and testability ([#401])

[#394]: https://github.com/informalsystems/tendermint-rs/pull/394
[#401]: https://github.com/informalsystems/tendermint-rs/pull/401

## [0.14.1] (2020-06-23)

Expand Down
18 changes: 9 additions & 9 deletions light-client/examples/light_client.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
use std::collections::HashMap;
use std::{
path::{Path, PathBuf},
time::Duration,
};

use gumdrop::Options;

use tendermint_light_client::supervisor::{Handle as _, Instance, Supervisor};
use tendermint_light_client::{
components::{
clock::SystemClock,
Expand All @@ -11,18 +20,9 @@ use tendermint_light_client::{
peer_list::PeerList,
state::State,
store::{sled::SledStore, LightStore},
supervisor::{Instance, Supervisor},
types::{Height, PeerId, Status, Time, TrustThreshold},
};

use gumdrop::Options;

use std::collections::HashMap;
use std::{
path::{Path, PathBuf},
time::Duration,
};

#[derive(Debug, Options)]
struct CliOptions {
#[options(help = "print this help message")]
Expand Down
102 changes: 62 additions & 40 deletions light-client/src/supervisor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,39 @@ use crate::{
types::{Height, LightBlock, PeerId, Status},
};

pub trait Handle {
/// Get latest trusted block from the [`Supervisor`].
fn latest_trusted(&mut self) -> Result<Option<LightBlock>, Error>;

/// Verify to the highest block.
fn verify_to_highest(&mut self) -> Result<LightBlock, Error>;

/// Verify to the block at the given height.
fn verify_to_target(&mut self, height: Height) -> Result<LightBlock, Error>;

/// Async version of `verify_to_highest`.
///
/// The given `callback` will be called asynchronously with the
/// verification result.
fn verify_to_highest_async(
&mut self,
callback: impl FnOnce(Result<LightBlock, Error>) -> () + Send + 'static,
);

/// Async version of `verify_to_target`.
///
/// The given `callback` will be called asynchronously with the
/// verification result.
fn verify_to_target_async(
&mut self,
height: Height,
callback: impl FnOnce(Result<LightBlock, Error>) -> () + Send + 'static,
);

/// Terminate the underlying [`Supervisor`].
fn terminate(&mut self);
}
Copy link
Member

@liamsi liamsi Jul 1, 2020

Choose a reason for hiding this comment

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

Looks good to me 👍 My understanding of why making this a trait is cool is because it makes testability easier. Is there a (wip) example of such a test that would be annoying to write without this (maybe in another PR)?

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 commit a452244 shows the start, where the test code implements a mockHandle. Generally it means a reduction of the objects that need constructing. If something was dependent on the Handle before, one had to provide a PeerList, ForkDetector, EvidenceProvider to construct the Supervisor. On all the state inside had to be correct in order to produce output as expected, which means the correct state in peer(s) stores, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!


/// Input events sent by the [`Handle`]s to the [`Supervisor`]. They carry a [`Callback`] which is
/// used to communicate back the responses of the requests.
#[derive(Debug)]
Expand Down Expand Up @@ -134,8 +167,8 @@ impl Supervisor {
}

/// Create a new handle to this supervisor.
pub fn handle(&mut self) -> Handle {
Handle::new(self.sender.clone())
pub fn handle(&mut self) -> impl Handle {
SupervisorHandle::new(self.sender.clone())
}

#[pre(self.peers.primary().is_some())]
Expand Down Expand Up @@ -308,21 +341,38 @@ impl Supervisor {
}
}

/// A handle to a `Supervisor` which allows to communicate with
/// A [`Handle`] to the [`Supervisor`] which allows to communicate with
/// the supervisor across thread boundaries via message passing.
pub struct Handle {
struct SupervisorHandle {
sender: channel::Sender<HandleInput>,
}

impl Handle {
impl SupervisorHandle {
/// Crate a new handle that sends events to the supervisor via
/// the given channel. For internal use only.
fn new(sender: channel::Sender<HandleInput>) -> Self {
Self { sender }
}

/// Get latest trusted block from the [`Supervisor`].
pub fn latest_trusted(&mut self) -> Result<Option<LightBlock>, Error> {
fn verify(
&mut self,
make_event: impl FnOnce(Callback<Result<LightBlock, Error>>) -> HandleInput,
) -> Result<LightBlock, Error> {
let (sender, receiver) = channel::bounded::<Result<LightBlock, Error>>(1);

let callback = Callback::new(move |result| {
sender.send(result).unwrap();
});

let event = make_event(callback);
self.sender.send(event).unwrap();

receiver.recv().unwrap()
}
}

impl Handle for SupervisorHandle {
fn latest_trusted(&mut self) -> Result<Option<LightBlock>, Error> {
let (sender, receiver) = channel::bounded::<Result<Option<LightBlock>, Error>>(1);

let callback = Callback::new(move |result| {
Expand All @@ -338,50 +388,23 @@ impl Handle {
receiver.recv().unwrap()
}

/// Verify to the highest block.
pub fn verify_to_highest(&mut self) -> Result<LightBlock, Error> {
fn verify_to_highest(&mut self) -> Result<LightBlock, Error> {
self.verify(HandleInput::VerifyToHighest)
}

/// Verify to the block at the given height.
pub fn verify_to_target(&mut self, height: Height) -> Result<LightBlock, Error> {
fn verify_to_target(&mut self, height: Height) -> Result<LightBlock, Error> {
self.verify(|callback| HandleInput::VerifyToTarget(height, callback))
}

/// Verify either to the latest block (if `height == None`) or to a given block (if `height == Some(height)`).
fn verify(
&mut self,
make_event: impl FnOnce(Callback<Result<LightBlock, Error>>) -> HandleInput,
) -> Result<LightBlock, Error> {
let (sender, receiver) = channel::bounded::<Result<LightBlock, Error>>(1);

let callback = Callback::new(move |result| {
sender.send(result).unwrap();
});

let event = make_event(callback);
self.sender.send(event).unwrap();

receiver.recv().unwrap()
}

/// Async version of `verify_to_highest`.
///
/// The given `callback` will be called asynchronously with the
/// verification result.
pub fn verify_to_highest_async(
fn verify_to_highest_async(
&mut self,
callback: impl FnOnce(Result<LightBlock, Error>) -> () + Send + 'static,
) {
let event = HandleInput::VerifyToHighest(Callback::new(callback));
self.sender.send(event).unwrap();
}

/// Async version of `verify_to_target`.
///
/// The given `callback` will be called asynchronously with the
/// verification result.
pub fn verify_to_target_async(
fn verify_to_target_async(
xla marked this conversation as resolved.
Show resolved Hide resolved
&mut self,
height: Height,
callback: impl FnOnce(Result<LightBlock, Error>) -> () + Send + 'static,
Expand All @@ -390,8 +413,7 @@ impl Handle {
self.sender.send(event).unwrap();
}

/// Terminate the underlying supervisor.
pub fn terminate(&mut self) {
fn terminate(&mut self) {
xla marked this conversation as resolved.
Show resolved Hide resolved
let (sender, receiver) = channel::bounded::<()>(1);

let callback = Callback::new(move |_| {
Expand Down