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

Preparation for Light client RPC #4485

Merged
merged 41 commits into from Feb 10, 2017
Merged

Preparation for Light client RPC #4485

merged 41 commits into from Feb 10, 2017

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Feb 8, 2017

This PR gets us most of the way to RPCs for the current light client, and ports the existing full RPCs over to more generic abstractions instead of MiningBlockChainClient and MinerService.

@tomusdrw This touches a lot of code in the signer implementation (particularly to move it to futures). The tests pass, but let me know if you see anything suspicious. There will also undoubtedly be conflicts with #4477.

Key points:

  • Skeleton of eth API
  • Generic Dispatcher with FullDispatcher implementation
  • OnDemand improvements

Next up:

  • Light client transaction queue
  • OnDemand service request batching
  • LightDispatcher (fetch nonce, historic gas price via OnDemand)

Part of #2235

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 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 Feb 9, 2017
@rphmeier
Copy link
Contributor Author

rphmeier commented Feb 9, 2017

@tomusdrw Grumbles (except for the test stuff) should be addressed now.

@gavofyork
Copy link
Contributor

not very happy about the additional of boilerplate. is there nothing that can be done?

@rphmeier
Copy link
Contributor Author

rphmeier commented Feb 9, 2017

It serves a purpose: convert result-using code into future-using code. Two options:

  • use the just-written take_weakf! and pay more interleaved "boilerplate" cost with stuff like return future::err(e).boxed() instead of return Err(e)
  • write a macro to wrap the function body and compile down to the let inner = || {...} pattern used explicitly here. Still introduces the visual noise + rightwards drift of the status quo.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Feb 9, 2017

There is no additional cost with teak_weakf!, in both examples we return one .boxed() future. There is an issue to get rid of those Weak references: #3958 that would also reduce the boilerplate.

@rphmeier
Copy link
Contributor Author

rphmeier commented Feb 9, 2017

I understand there's no more runtime cost, I mean you still pay a boilerplate cost in places, just more spread out.

@rphmeier
Copy link
Contributor Author

rphmeier commented Feb 9, 2017

Ok, no more let inner pattern via take_weakf and try_bf macros.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 10, 2017
@tomusdrw tomusdrw added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Feb 10, 2017
@tomusdrw
Copy link
Collaborator

Some conflicts to resolve.

@rphmeier
Copy link
Contributor Author

rphmeier commented Feb 10, 2017

(resolved locally, but can't build after #4486)

edit: just needed #4507

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Feb 10, 2017
@rphmeier rphmeier merged commit 48ae38e into master Feb 10, 2017
@rphmeier rphmeier deleted the lightrpc branch February 10, 2017 16:15
@rphmeier rphmeier moved this from In Review to Done in Light Client Feb 10, 2017
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
Light Client
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants