Support http requests #2

njgheorghita · 2021-03-03T15:01:06Z

added clap library for cli arg parsing
added some tests for cli args
added support for http requests

src/main.rs

njgheorghita · 2021-03-03T16:59:11Z

src/main.rs

+        T: Into<OsString> + Clone,
+    {
+        // TODO: things to configure:
+        //  - infura project id (not just env var?)


Any further thoughts on this? At the moment, using just an env var makes testing slightly tricky. Some other options include a config file or a cli arg (i'm not a fan of the cli arg option, it would get rather tedious imo)

I'm okay leaving it as env var only for now. I think we'll want support for that at some point. (One plausible use case is as a colocated endpoint for a web server, where env var would be the nicest way to configure IMO)

So I guess we'll just have to figure out a way to test where we can mock out the env var.

Yeah, that seems reasonable. I think once we get CI setup, it'll help illuminate the best way forward. Also, from the docs...

Because the tests are running at the same time, make sure your tests don’t depend on each other or on any shared state, including a shared environment, such as the current working directory or environment variables.

Which seems to me rules out any tests that check for the presence of an env variable. From my tinkering around any test that manually cleared and reset the env var (to check for presence) would cause every other test to simultaneously fail.

Yeah, I think if we want to keep the env var long term, we would read the value in main() and pass it into the inner function call. Then, all layers beneath it are just tested with supplied arguments.

njgheorghita · 2021-03-03T17:05:17Z

First rust pr! So I'm sure there's plenty of room for improvement here, but it's ready for a first pass 👀

njgheorghita · 2021-03-04T17:17:14Z

ping @carver for 👀 ... not sure why, but you're not showing up in the Reviewers section at the top right of the pr. But maybe that's because this repo is still so fresh

carver · 2021-03-04T23:37:35Z

Thanks!

I assume you ran rustfmt or something. (I may not have) Just wondering why the IPC code is showing up as changed.

carver · 2021-03-04T23:38:03Z

First rust pr! So I'm sure there's plenty of room for improvement here, but it's ready for a first pass

Yeah, sort of the blind leading the blind here...

carver · 2021-03-04T23:52:16Z

I assume you ran rustfmt or something. (I may not have) Just wondering why the IPC code is showing up as changed.

Yeah, just confirmed that I left out the formatting last time. My bad, sorry for the extra work. Would you mind rebasing now? It will make the diff easier for me to read.

src/cli.rs

carver · 2021-03-05T00:30:46Z

A natural follow-up will be to figure out a way to do integration tests on the http and ipc servers. Maybe we can hit only the clientVersion endpoint for now, though we'll obviously want to test Infura endpoints eventually. Also, we'll probably want arbitrary backing URLs. That's important soon, so we don't hit Infura for real during tests, and later so we can support any type of endpoint (like alchemyapi.io).

(All for future PRs)

carver · 2021-03-05T17:36:57Z

Oh, also a reminder to update the README

njgheorghita · 2021-03-07T12:25:23Z

@carver Ready for another 👀

carver

Looks good!

Cool that building the raw HTTP wasn't too bad.

For some of the comments, I mention follow-up work. Would you mind opening short issues for each of them that you don't end up fixing in this PR? (Including ones I said I'd do :P )

README.md

src/cli.rs

carver · 2021-03-09T21:47:26Z

src/cli.rs

+                    .default_value("2"),
+            )
+            .get_matches_from_safe(args)
+            .unwrap_or_else(|e| panic!("Unable to parse args: {}", e));


I think this is equivalent to the simpler:

Suggested change

.unwrap_or_else(|e| panic!("Unable to parse args: {}", e));

.expect("Unable to parse args");

Notice that the expect panic includes the error message in this example: https://doc.rust-lang.org/std/result/enum.Result.html#method.expect

It turns out get_matches_from_safe has some peculiarities in terms of handling --help and --version and will panic if those are called. It felt weird to me to panic on help and version input (even though the help/version output is still displayed), so I added some handling to exit gracefully on those calls and panic for any other errors. Lmk if you feel differently about how this should be handled

Oh weird. Well I think if we're not going to do anything clever on a failure, maybe we should just use:
https://docs.rs/clap/2.33.3/clap/struct.App.html#method.get_matches_from . It seems to just have an implicit unwrap under the hood.

carver · 2021-03-09T22:06:52Z

src/cli.rs

+        println!("Launching Trin...");
+        let protocol = matches.value_of("protocol").unwrap();
+        let endpoint = matches.value_of("endpoint").unwrap();
+        let endpoint = match endpoint.parse::<u32>() {


Waiting to parse the http port until we know the protocol is http gives two benefits:

in ipc case, we can compare http_port to "7878". That can now be a reused constant in the validation and the configuration setup, instead of duplicated in a couple places.

tiny performance benefit on ipc runs

I found a clap macro (value_t!) that simplifies the code parsing/casting the cli args. How strongly do you feel about this? I see the benefit of a constant.. But, I've also tried a couple variants and haven't found a reasonably simple solution (but, my rust type-fu is weak so who knows what obvious solutions i'm missing).

since TrinConfig has defined http_port: u32, we eventually have to cast a const http_port: String = "8545" into an int when we use the default constant (in both the http & ipc cases)

the default_arg() param for the clap parser only accepts strings, so we have to do some casting if const http_port: u32 = 8545, and early tinkering has only resulted in a pandoras box of more complicated compiler errors

A performance benefit is somewhat irrelevant if a user selects ipc and a custom http_port since we error out. In the case that the user just selects ipc, we will still have to cast a const http_port: String = "8545" into an int to feed into TrinConfig

I'm happy to keep tinkering with this, just reporting my initial findings

src/cli.rs

carver · 2021-03-09T22:30:19Z

src/cli.rs

+        let obj = obj.unwrap();
+        assert!(obj.is_object());
+        assert_eq!(obj["jsonrpc"], "2.0");
+        let request_id = obj.get("id").unwrap();
+        let method = obj.get("method").unwrap();
+
+        let response = match method.as_str().unwrap() {


I know the error handling was already this way in the IPC code, but it's bad news. Sending bad input data to trin should reply with an error, instead of crashing trin.

👍 I've added this to be tackled in the refactor issue #6

src/cli.rs

njgheorghita · 2021-03-14T16:24:30Z

@carver Sorry, was a bit sporadic in wrapping this up. Just a couple of last comments if you have any thoughts, and then I'll get this merged

njgheorghita · 2021-03-17T15:23:38Z

@carver Ahh, I don't have write access to this repo - so for now I'll have to leave merging up to you if it checks out on your end

njgheorghita force-pushed the support-http branch 6 times, most recently from 9c128c9 to df818a8 Compare March 3, 2021 16:32

njgheorghita commented Mar 3, 2021

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

njgheorghita commented Mar 3, 2021

View reviewed changes

njgheorghita marked this pull request as ready for review March 3, 2021 16:59

carver reviewed Mar 5, 2021

View reviewed changes

src/cli.rs Show resolved Hide resolved

carver reviewed Mar 5, 2021

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

carver reviewed Mar 5, 2021

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

njgheorghita added 3 commits March 6, 2021 13:40

Basic http server functionality

ee913be

Add cli arg parser

800cb4b

Add some tests for cli arg parsing

7bbced5

njgheorghita force-pushed the support-http branch from df818a8 to 7bbced5 Compare March 6, 2021 13:40

Add readme

a200102

njgheorghita force-pushed the support-http branch from d28f720 to a200102 Compare March 7, 2021 12:15

njgheorghita mentioned this pull request Mar 7, 2021

Initial Integration tests for HTTP / IPC servers #4

Closed

njgheorghita changed the title ~~WIP - Support http requests~~ Support http requests Mar 7, 2021

carver approved these changes Mar 9, 2021

View reviewed changes

njgheorghita force-pushed the support-http branch from f5c7bee to 04d2256 Compare March 14, 2021 15:42

PR feedback

422fafd

njgheorghita force-pushed the support-http branch from 04d2256 to 422fafd Compare March 14, 2021 16:25

Refactor error handling on clap cli parser

47b2141

njgheorghita mentioned this pull request Mar 29, 2021

Support custom ipc paths #8

Merged

carver merged commit ec83b0e into ethereum:master Apr 5, 2021

njgheorghita mentioned this pull request Nov 12, 2021

Update message types to match wire protocol test vectors #181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support http requests #2

Support http requests #2

njgheorghita commented Mar 3, 2021 •

edited

Loading

njgheorghita Mar 3, 2021

carver Mar 5, 2021

njgheorghita Mar 7, 2021

carver Mar 9, 2021

njgheorghita commented Mar 3, 2021

njgheorghita commented Mar 4, 2021

carver commented Mar 4, 2021

carver commented Mar 4, 2021

carver commented Mar 4, 2021

carver commented Mar 5, 2021 •

edited

Loading

carver commented Mar 5, 2021

njgheorghita commented Mar 7, 2021

carver left a comment

carver Mar 9, 2021

njgheorghita Mar 14, 2021 •

edited

Loading

carver Mar 16, 2021 •

edited

Loading

carver Mar 9, 2021

njgheorghita Mar 14, 2021 •

edited

Loading

carver Mar 9, 2021

njgheorghita Mar 14, 2021

njgheorghita commented Mar 14, 2021

njgheorghita commented Mar 17, 2021

	.unwrap_or_else(\|e\| panic!("Unable to parse args: {}", e));
	.expect("Unable to parse args");

Support http requests #2

Support http requests #2

Conversation

njgheorghita commented Mar 3, 2021 • 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

njgheorghita commented Mar 3, 2021

njgheorghita commented Mar 4, 2021

carver commented Mar 4, 2021

carver commented Mar 4, 2021

carver commented Mar 4, 2021

carver commented Mar 5, 2021 • edited Loading

carver commented Mar 5, 2021

njgheorghita commented Mar 7, 2021

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

carver Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita commented Mar 14, 2021

njgheorghita commented Mar 17, 2021

njgheorghita commented Mar 3, 2021 •

edited

Loading

carver commented Mar 5, 2021 •

edited

Loading

njgheorghita Mar 14, 2021 •

edited

Loading

carver Mar 16, 2021 •

edited

Loading

njgheorghita Mar 14, 2021 •

edited

Loading