light-node: RPC server #363

xla · 2020-06-23T13:38:08Z

Adds an RPC server to expose internal state of a light node. Besides adding the general infrastructure it provides the first endpoint under /state as described in #219.

First part of #219
Depends on #394
Depends on #403

light-node/Cargo.toml

light-client/src/supervisor.rs

xla · 2020-06-29T20:34:19Z

Supervisor changes have been split out into #394.

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 · 2020-06-30T20:07:39Z

Depends on #401

liamsi · 2020-07-06T16:57:43Z

light-node/src/rpc.rs

+        }
+    }
+
+    const LIGHTBLOCK_JSON: &str = r#"


Is that needed twice? Here and above (in light-node/examples/rpc.rs)?

The example is temporary to help with the integration work, wouldn't it to leave as is mid-term.

liamsi · 2020-07-06T17:17:43Z

light-node/src/error.rs

-    }
-}
+    #[error("i/o error: {0}")]
+    Io(String),


Just out of curiosity: why did you decide to wrap the underlying error as a string here (as opposed to using the #[from] directive)?

Because io::Error doesn't implement Clone. Which we mind not need anymore on our Kind as I refrained from returning our custom errors in the rpc server, as I wasn't able to get mapping to the jsonrpc error to work.

One way to work around the lack of Clone on io::Error is to use the anomaly::Context wrapper (as shown in the docs), which would look something like that:

diff --git a/light-node/src/error.rs b/light-node/src/error.rs index 094799b..7162c03 100644 --- a/light-node/src/error.rs +++ b/light-node/src/error.rs @@ -1,8 +1,9 @@ //! Error types -use std::io; use std::net; +use anomaly::{BoxError, Context}; + /// Error type pub type Error = anomaly::Error<Kind>; @@ -18,12 +19,15 @@ pub enum Kind { Config, /// Input/output error - #[error("i/o error: {0}")] - Io(String), + #[error("i/o error")] + Io, } -impl From<io::Error> for Kind { - fn from(err: io::Error) -> Self { - Self::Io(format!("{}", err)) +impl Kind { + /// Add additional context (i.e. include a source error and capture a backtrace). + /// You can convert the resulting `Context` into an `Error` by calling `.into()`. + pub fn context(self, source: impl Into<BoxError>) -> Context<Self> { + Context::new(self, Some(source.into())) } } + diff --git a/light-node/src/rpc.rs b/light-node/src/rpc.rs index 544dcb4..f6b190f 100644 --- a/light-node/src/rpc.rs +++ b/light-node/src/rpc.rs @@ -25,7 +25,7 @@ where AccessControlAllowOrigin::Any, ])) .start_http(&addr.parse().map_err(error::Kind::from)?) - .map_err(error::Kind::from)?; + .map_err(|e| error::Kind::Io.context(e))?; srv.wait();

Yeah was looking at the Context as we also use it in the light-client crate. You do loose the From semantics with it, correct? As in you have to know which variant to construct when mapping.

You do loose the From semantics with it, correct? As in you have to know which variant to construct when mapping.

I would say that you just have to be more explicit, rather than let the compiler pick the appropriate From instance based on the (static) type of the underlying error, which in this case is a io::Error. So you don't really lose anything aside from the From instance, which in this case is not appropriate for io::Error because of the lack of Clone bound. If io::Error was Clone, then you would use #[from] like you did for net::AddrParseError.

liamsi · 2020-07-06T17:20:22Z

light-node/examples/rpc.rs

@@ -0,0 +1,146 @@
+//! Basic example of running the RPC server. This is a temporary show-case and should be removed
+//! once integrated in the light node proper. To test the `/state` endpoint run:


liamsi · 2020-07-06T17:21:11Z

light-node/src/error.rs

-        ErrorKind::Io.context(err).into()
+impl Into<jsonrpc_core::types::Error> for Kind {
+    fn into(self) -> jsonrpc_core::types::Error {
+        todo!()


Did you leave this a todo!() on purpose?

See https://github.com/informalsystems/tendermint-rs/pull/363/files#r450373699 - I think the whole conversion can go. We don't use it at the moment.

light-node/src/rpc.rs

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

codecov-commenter · 2020-07-06T17:41:40Z

Codecov Report

Merging #363 into master will increase coverage by 0.2%.
The diff coverage is 24.3%.

@@           Coverage Diff            @@
##           master    #363     +/-   ##
========================================
+ Coverage    30.4%   30.7%   +0.2%     
========================================
  Files         130     131      +1     
  Lines        5448    5464     +16     
  Branches     1686    1695      +9     
========================================
+ Hits         1659    1678     +19     
+ Misses       2640    2603     -37     
- Partials     1149    1183     +34

Impacted Files	Coverage Δ
light-client/src/components/scheduler.rs	`47.6% <ø> (ø)`
light-node/src/error.rs	`0.0% <0.0%> (ø)`
light-node/src/rpc.rs	`36.0% <36.0%> (ø)`
light-client/src/types.rs	`47.8% <0.0%> (-7.2%)`	⬇️
light-client/src/supervisor.rs	`31.0% <0.0%> (-0.9%)`	⬇️
tendermint/src/time.rs	`47.2% <0.0%> (ø)`
tendermint/src/block/commit.rs	`54.5% <0.0%> (ø)`
tendermint/src/validator.rs	`50.0% <0.0%> (+0.9%)`	⬆️
tendermint/src/signature.rs	`37.5% <0.0%> (+4.1%)`	⬆️
... and 5 more

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 9b78fc5...3cce7af. Read the comment docs.

liamsi · 2020-07-06T17:50:36Z

light-node/Cargo.toml

@@ -7,11 +7,18 @@ publish = false

 [dependencies]
 abscissa_tokio = "0.5"
+anomaly = { version = "0.2", features = [ "serializer" ] }


@tarcieri @tony-iqlusion Mind chiming in here? It's been a while since I actively used all 3 dependencies: anomaly, thiserror, and abscissa. Does it make sense to mix those here? Doesn't make abscissa using anomaly in the same crate obsolete? I'm a bit lost here.

Error handling in Abscissa is in the middle of an overhaul to switch to eyre (or if the improvements in eyre end up being upstreamable, potentially anyhow) /cc @yaahc

I plan on eventually phasing out anomaly at this point (too many crates to manage, and there are better options now)

Good to know thanks!

It looks like there's a decent chance that the changes will get upstreamed back into anyhow, not sure how long it will be though.

liamsi

I think It's OK to merge this as is to move forward with the cli / light node tmrw.

brapse

👍

romac · 2020-07-07T09:44:47Z

Great work @xla! 🥇

xla added light-client Issues/features which involve the light client rpc labels Jun 23, 2020

xla requested a review from liamsi June 23, 2020 13:38

xla self-assigned this Jun 23, 2020

xla mentioned this pull request Jun 23, 2020

Bootstrap proxy for light-node API #329

Closed

liamsi reviewed Jun 23, 2020

View reviewed changes

light-node/Cargo.toml Show resolved Hide resolved

liamsi reviewed Jun 28, 2020

View reviewed changes

light-client/src/supervisor.rs Outdated Show resolved Hide resolved

liamsi reviewed Jun 28, 2020

View reviewed changes

light-client/src/supervisor.rs Outdated Show resolved Hide resolved

liamsi reviewed Jun 28, 2020

View reviewed changes

light-client/src/supervisor.rs Outdated Show resolved Hide resolved

xla mentioned this pull request Jun 29, 2020

light-client: expose latest_trusted on Supervisor Handle #394

Merged

5 tasks

xla force-pushed the xla/219-light-node-rpc branch from a7b39fb to e1b3918 Compare June 29, 2020 20:33

xla added 2 commits June 30, 2020 21:58

Add changes entry

874ede1

xla added 15 commits June 30, 2020 22:56

Remove async trait methods

87b02b7

Make supervisor more CSP

26e8626

Improve crate imports

ddb15f3

Merge branch 'master' into xla/398-improve-supervisor-ergonomics

f9006f2

Relax mutability on Handle

b5eb909

Provide noop default implementations

39cbfe3

Merge branch 'master' into xla/398-improve-supervisor-ergonomics

c4a9cba

Merge branch 'master' into xla/398-improve-supervisor-ergonomics

e1847d3

Update adr status

48085c8

Handle crossbeam errors honestly

d423c10

Add changes entry

49dc22a

WIP

126df4d

Mock out actual endpoints

fbc37f9

Assert actual response shapes

5715d72

Use async tests

38088a6

xla added 8 commits July 4, 2020 19:28

Remove unused import

42f8993

Start error infra

ccd4af9

Use json input for tests

ffbe895

Remove unused status endpoint

736c751

Seal and expose proper run functionality

9a7f60d

Correct address configuration

869c71b

Add temp example

68b2055

Merge branch 'master' into xla/219-light-node-rpc

f78a6a2

liamsi reviewed Jul 6, 2020

View reviewed changes

xla marked this pull request as ready for review July 6, 2020 17:12

xla requested review from romac, brapse and ebuchman July 6, 2020 17:14

liamsi reviewed Jul 6, 2020

View reviewed changes

light-node/src/rpc.rs Outdated Show resolved Hide resolved

Update light-node/src/rpc.rs

3cce7af

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

liamsi reviewed Jul 6, 2020

View reviewed changes

liamsi previously approved these changes Jul 6, 2020

View reviewed changes

xla added 2 commits July 7, 2020 08:50

Remove unused errors

c98be58

Improve error propagation

881d831

xla dismissed liamsi’s stale review via 881d831 July 7, 2020 07:00

xla mentioned this pull request Jul 7, 2020

light-client constraint Handle to Clone + Send + Sync #412

Closed

Correct wording

fdae01a

brapse approved these changes Jul 7, 2020

View reviewed changes

brapse merged commit 1b46e32 into master Jul 7, 2020

brapse deleted the xla/219-light-node-rpc branch July 7, 2020 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

light-node: RPC server #363

light-node: RPC server #363

xla commented Jun 23, 2020 •

edited

Loading

xla commented Jun 29, 2020

xla commented Jun 30, 2020

liamsi Jul 6, 2020 •

edited

Loading

xla Jul 6, 2020

liamsi Jul 6, 2020

xla Jul 6, 2020

romac Jul 7, 2020

romac Jul 7, 2020

xla Jul 7, 2020

romac Jul 7, 2020

liamsi Jul 6, 2020

liamsi Jul 6, 2020

xla Jul 6, 2020

codecov-commenter commented Jul 6, 2020

liamsi Jul 6, 2020

tarcieri Jul 6, 2020 •

edited

Loading

liamsi Jul 6, 2020

yaahc Jul 6, 2020

liamsi left a comment

brapse left a comment

romac commented Jul 7, 2020

		@@ -0,0 +1,146 @@
		//! Basic example of running the RPC server. This is a temporary show-case and should be removed
		//! once integrated in the light node proper. To test the `/state` endpoint run:

light-node: RPC server #363

light-node: RPC server #363

Conversation

xla commented Jun 23, 2020 • edited Loading

xla commented Jun 29, 2020

xla commented Jun 30, 2020

liamsi Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 6, 2020

Codecov Report

Choose a reason for hiding this comment

tarcieri Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

brapse left a comment

Choose a reason for hiding this comment

romac commented Jul 7, 2020

xla commented Jun 23, 2020 •

edited

Loading

liamsi Jul 6, 2020 •

edited

Loading

tarcieri Jul 6, 2020 •

edited

Loading