feat: add `--http.corsdomain` by Tirthnp · Pull Request #1305 · paradigmxyz/reth

Tirthnp · 2023-02-13T01:28:21Z

Tirthnp · 2023-02-13T01:33:45Z

I have created a draft version as it's a first pass. I just want to know if I am doing it right. I think we need to add support for --http.corsdomain * and --http.cordomain. I think we need to have an enum RpcCorsConfig. Can someone please give me some guidance on it?

onbjerg · 2023-02-13T09:39:50Z

Hey! When opening up a PR that closes an issue, you can use special words to link the PR and the issue, which auto-closes the issue when it is merged. For example:

Closes #1 or fixes #1

It helps keep the issue tracker clean 😄 Thanks for taking this on!

mattsse

great!

left some suggestions

mattsse · 2023-02-13T14:31:09Z

crates/rpc/rpc-builder/src/lib.rs

    }

+    // Confiugures the http server with cors domain
+    pub fn with_http_cors_domain(mut self, config: ServerBuilder, domains: Vec<&str>) -> Self {


I would rather have this as a separate field (cors_domans: Option<...>)and then this is created in build

Could you please elaborate on this? I am a bit confused

mattsse · 2023-02-13T14:32:12Z

bin/reth/src/node/mod.rs

+        if let Some(domains) = self.rpc.http_corsdomain.as_ref() {
+            let domains_vec = domains.split(",").collect::<Vec<&str>>();
+            let _rpc_server = reth_rpc_builder::launch(
+                ShareableDatabase::new(db.clone()),
+                reth_transaction_pool::test_utils::testing_pool(),
+                network.clone(),
+                TransportRpcModuleConfig::default()
+                    .with_http(vec![RethRpcModule::Admin, RethRpcModule::Eth]),
+                RpcServerConfig::default().with_http_cors_domain(Default::default(),domains_vec),
+            )
+            .await?;


we should leave this entirely up to the rpc builder.
It should handle Option<domain>

Tirthnp · 2023-02-16T01:30:19Z

I have made some changes as per the comments but I am not exactly sure how to test them

mattsse

thanks!

left some suggestions

mattsse · 2023-02-16T16:50:00Z

bin/reth/src/node/mod.rs

                .with_http(vec![RethRpcModule::Admin, RethRpcModule::Eth]),
-            RpcServerConfig::default().with_http(Default::default()),
+            RpcServerConfig::default()
+                .with_http(Default::default(), self.rpc.http_corsdomain.as_ref()),


the corsdomain should be configured in a separate function.
with_http should only take the server config imo.

mattsse · 2023-02-16T16:50:30Z

crates/rpc/rpc-builder/src/lib.rs

+    /// Configs for JSON-RPC that allow Corsdomains
+    http_cors_domain_server_config: Option<ServerBuilder<Stack<CorsLayer, Identity>>>,


separate for cors domain only

mattsse · 2023-02-16T16:50:58Z

crates/rpc/rpc-builder/src/lib.rs

+        if let Some(domains) = domains {
+            match domains.as_str() {
+                "*" => {
+                    let cors = CorsLayer::new()
+                        .allow_methods([Method::GET, Method::POST])
+                        .allow_origin(Any)
+                        .allow_headers(Any);
+
+                    let middleware = tower::ServiceBuilder::new().layer(cors);
+                    self.http_cors_domain_server_config =
+                        Some(config.set_middleware(middleware).http_only());
+                }


all of this needs to be set when the server is started

yeah @Tirthnp what you want here is to just set the config variable, and then in the build/start function you actually consume them

gakonst · 2023-02-18T22:12:19Z

crates/rpc/rpc-builder/src/lib.rs

+        if let Some(domains) = domains {
+            match domains.as_str() {
+                "*" => {
+                    let cors = CorsLayer::new()
+                        .allow_methods([Method::GET, Method::POST])
+                        .allow_origin(Any)
+                        .allow_headers(Any);
+
+                    let middleware = tower::ServiceBuilder::new().layer(cors);
+                    self.http_cors_domain_server_config =
+                        Some(config.set_middleware(middleware).http_only());
+                }


yeah @Tirthnp what you want here is to just set the config variable, and then in the build/start function you actually consume them

gakonst · 2023-02-18T22:14:01Z

crates/rpc/rpc-builder/src/lib.rs

    /// Configures the http server
-    pub fn with_http(mut self, config: ServerBuilder) -> Self {
-        self.http_server_config = Some(config.http_only());
+    pub fn with_http(mut self, config: ServerBuilder, domains: Option<&String>) -> Self {


The domains method should be separate, so one should be able to do with_http(config).with_cors(Some("*"))

mattsse

great progress.

mostly style nits, otherwise this is pretty close.

mattsse · 2023-02-20T12:19:47Z

crates/rpc/rpc-builder/src/lib.rs

+                match domains.as_str() {
+                    "*" => {
+                        cors = Some(CorsLayer::new()
+                            .allow_methods([Method::GET, Method::POST])
+                            .allow_origin(Any)
+                            .allow_headers(Any));
+                    } 
+                    "" => {}
+                    _ => {
+                        let domains_vec = domains.split(",").collect::<Vec<&str>>();
+
+                        let origins = domains_vec
+                            .into_iter()
+                            .map(|domain| domain.parse().unwrap())
+                            .collect::<Vec<HeaderValue>>();
+
+                        cors = Some(CorsLayer::new()
+                            .allow_methods([Method::GET, Method::POST])
+                            .allow_origin(origins)
+                            .allow_headers(Any));


please extract this to a standalone function that returns Result<Option<CorsLayer>

mattsse · 2023-02-20T12:20:25Z

crates/rpc/rpc-builder/src/lib.rs

+                        let domains_vec = domains.split(",").collect::<Vec<&str>>();
+
+                        let origins = domains_vec
+                            .into_iter()
+                            .map(|domain| domain.parse().unwrap())


can be combined into domains.split(",").into_iter()

mattsse · 2023-02-20T12:20:59Z

crates/rpc/rpc-builder/src/lib.rs

+                            .collect::<Vec<HeaderValue>>();
+


once move to function, collect into Result:

Suggested change

.collect::<Vec<HeaderValue>>();

.collect::<Result<Vec<HeaderValue>,_>();

mattsse · 2023-02-20T12:21:38Z

crates/rpc/rpc-builder/src/lib.rs

+            match cors {
+                Some(cors) => {


nit:
use if let Some() else instead of match

mattsse · 2023-02-20T12:22:55Z

crates/rpc/rpc-builder/src/lib.rs

+/// Http Servers Enum
+pub enum HttpServer {
+    /// Http server
+    Plain(Server),
+    /// Http server with cors
+    WithCors(Server<Stack<CorsLayer, Identity>>),
+}


this is okay for now, perhaps we find a better solution but for now this is ok

We could dyn dispatch

gakonst

Good with me, couple nits which we can do in follow up

gakonst · 2023-02-21T04:18:56Z

crates/rpc/rpc-builder/src/lib.rs

+                    let origins = domains
+                        .split(",")
+                        .map(|domain| domain.parse::<HeaderValue>())
+                        .collect::<Result<Vec<HeaderValue>, _>>();


Nit I'd add a CoraDomains type that impls FromStr and includes this logic, and do Option in the upstream cli code

gakonst · 2023-02-21T04:20:15Z

crates/rpc/rpc-builder/src/lib.rs

+            if let Some(cors) = cors {
+                let middleware = tower::ServiceBuilder::new().layer(cors);
+                let http_server =
+                    builder.set_middleware(middleware).build(http_socket_addr).await?;
+                server.http_local_addr = http_server.local_addr().ok();
+                server.http = Some(HttpServer::WithCors(http_server));
+            } else {
+                let http_server = builder.build(http_socket_addr).await?;
+                server.http_local_addr = http_server.local_addr().ok();
+                server.http = Some(HttpServer::Plain(http_server));
+            }


Towers middlewares are so powerful. We should investigate what else is available @mattsse may let us do nice rate limiting etc features in the node

codecov-commenter · 2023-02-21T04:32:51Z

Codecov Report

Merging #1305 (46e2bcf) into main (eb12991) will decrease coverage by 0.18%.
The diff coverage is 67.02%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
- Coverage   76.23%   76.05%   -0.18%     
==========================================
  Files         357      358       +1     
  Lines       41118    42303    +1185     
==========================================
+ Hits        31347    32175     +828     
- Misses       9771    10128     +357

Flag	Coverage Δ
integration-tests	`22.00% <9.18%> (-0.36%)`	⬇️
unit-tests	`70.49% <57.83%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bin/reth/src/node/mod.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc-builder/src/lib.rs	`65.53% <52.17%> (-0.38%)`	⬇️
bin/reth/src/args/rpc_server_args.rs	`71.64% <80.58%> (+29.70%)`	⬆️
crates/primitives/src/jsonu256.rs	`93.67% <100.00%> (+0.24%)`	⬆️
crates/primitives/src/bloom.rs	`79.04% <0.00%> (-19.14%)`	⬇️
crates/primitives/src/block.rs	`78.53% <0.00%> (-4.20%)`	⬇️
crates/net/network/src/session/mod.rs	`76.36% <0.00%> (-1.37%)`	⬇️
crates/net/network/src/eth_requests.rs	`75.96% <0.00%> (-0.23%)`	⬇️
crates/net/discv4/src/lib.rs	`66.08% <0.00%> (-0.15%)`	⬇️
crates/rpc/rpc/src/eth/api/server.rs	`98.96% <0.00%> (-0.01%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mattsse

lgtm,

please enable edits by maintainer next PR

Tirthnp · 2023-02-21T12:46:15Z

please enable edits by maintainer next PR

I will, Thanks you so much for all the help :)

first pass at adding http.corsdomain

fa0317f

Tirthnp changed the title ~~first pass at adding the http.corsdomain #1230~~ first pass at adding the http.corsdomain Feb 13, 2023

onbjerg added C-enhancement New feature or request A-rpc Related to the RPC implementation labels Feb 13, 2023

onbjerg changed the title ~~first pass at adding the http.corsdomain~~ feat: add --http.corsdomain Feb 13, 2023

mattsse requested changes Feb 13, 2023

View reviewed changes

Tirthnp added 4 commits February 14, 2023 19:59

Merge branch 'paradigmxyz:main' into main

e11ee73

Merge branch 'paradigmxyz:main' into main

75d5f25

Handling *,"" and (String) for http.corsdomain

7d750d4

Merge branch 'main' of https://github.com/paradigmxyz/reth into main

8001155

Tirthnp requested a review from mattsse February 16, 2023 01:30

fix lint errors

b9ff7b9

mattsse requested changes Feb 16, 2023

View reviewed changes

gakonst requested changes Feb 18, 2023

View reviewed changes

Tirthnp added 2 commits February 20, 2023 08:28

adding cors domain in start/build

7431973

Merge branch 'main' of https://github.com/paradigmxyz/reth into main

6b90261

mattsse requested changes Feb 20, 2023

View reviewed changes

Tirthnp added 3 commits February 20, 2023 16:42

Addressed review comments

d23fe56

fmt styling changes

12961be

adding recommended changes

46e2bcf

gakonst approved these changes Feb 21, 2023

View reviewed changes

gakonst marked this pull request as ready for review February 21, 2023 04:20

gakonst requested a review from onbjerg as a code owner February 21, 2023 04:20

mattsse approved these changes Feb 21, 2023

View reviewed changes

mattsse merged commit 42b1fc1 into paradigmxyz:main Feb 21, 2023

mattsse pushed a commit that referenced this pull request Feb 21, 2023

feat: add --http.corsdomain (#1305)

750ccfe

mattsse mentioned this pull request Feb 21, 2023

chore: cors followup changes #1480

Merged

		/// Configs for JSON-RPC that allow Corsdomains
		http_cors_domain_server_config: Option<ServerBuilder<Stack<CorsLayer, Identity>>>,

	.collect::<Vec<HeaderValue>>();
	.collect::<Result<Vec<HeaderValue>,_>();

Conversation

Tirthnp commented Feb 13, 2023 • edited by onbjerg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tirthnp commented Feb 13, 2023

Uh oh!

onbjerg commented Feb 13, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tirthnp commented Feb 16, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Tirthnp commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Tirthnp commented Feb 13, 2023 •

edited by onbjerg

Loading

codecov-commenter commented Feb 21, 2023 •

edited

Loading