Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Filters and block RPCs for the light client #5320

Merged
merged 13 commits into from
Apr 12, 2017
Merged

Filters and block RPCs for the light client #5320

merged 13 commits into from
Apr 12, 2017

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Mar 28, 2017

Adds minimal implementations of log/filter RPCs for the light client.
Doesn't fetch any headers from network, just bails if they're not available locally.

@rphmeier rphmeier added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust. labels Mar 28, 2017
@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Apr 5, 2017
use futures::stream::{self, Stream};
use util::H2048;

// early exit for "to" block before "from" block.
Copy link
Contributor

@gavofyork gavofyork Apr 6, 2017

Choose a reason for hiding this comment

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

comment doesn't accurately describe logic below it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should now.

BlockId::Latest | BlockId::Pending => {
let best = self.client.best_block_header();
let chain_info = self.client.chain_info();
if best.number() != chain_info.best_block_number || best.hash() != chain_info.best_block_hash {
Copy link
Contributor

Choose a reason for hiding this comment

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

am i missing something here? looks to just be comparing self.client.chain_info() with self.client.best_block_header()...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, seems like it got mangled a bit when I was running over that part of the code.

BlockId::Number(x) => Some(x),
};

if block_number(filter.to_block) < block_number(filter.from_block) {
Copy link
Contributor

@gavofyork gavofyork Apr 7, 2017

Choose a reason for hiding this comment

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

does this really play well with Option's ordering? what if to_block is None? is it not better to fail ealy if either are None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's sort of a trade-off, but actually neither from_block nor to_block is ever a hash where these are invoked, so these are both always Some.

The alternative would be to pass a rpc::Filter instead of an EthcoreFilter, but then the logic for setting the to and from blocks would have to be duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

would suggest using either an alternative type which doesn't given the option of providing a Hash, or otherwise bomb early with an Err if either is None .

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 8, 2017
@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 10, 2017
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 12, 2017
@gavofyork gavofyork merged commit daf1495 into master Apr 12, 2017
@gavofyork gavofyork deleted the light-filters branch April 12, 2017 10:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants