refactor: clean up channel creation #407
Conversation
alex-miao
requested review from
dancoombs,
dphilipson and
0xfourzerofour
as code owners
crates/builder/src/server/local.rs
Outdated
| @@ -221,7 +223,7 @@ enum ServerRequestKind { | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| struct ServerRequest { | |||
| pub struct ServerRequest { | |||
There was a problem hiding this comment.
Any thoughts on workarounds to avoid making this public?
There was a problem hiding this comment.
Lets just pass in the capacity like it was setup before.
crates/pool/src/task.rs
Outdated
| @@ -69,7 +71,7 @@ impl Task for PoolTask { | |||
| }; | |||
| let provider = eth::new_provider(&self.args.http_url, self.args.http_poll_interval)?; | |||
| let chain = Chain::new(provider, chain_settings); | |||
| let (update_sender, _) = broadcast::channel(1000); | |||
| let (update_sender, _) = broadcast::channel(CHAIN_UPDATE_CHANNEL_CAPACITY); | |||
There was a problem hiding this comment.
Thought about making this one a function param but would break the run function signature for the trait.
There was a problem hiding this comment.
I think at a minimum we should pass in CHAIN_UPDATE_CHANNEL_CAPACITY via Args
There was a problem hiding this comment.
I agree with @dancoombs passing it as an argument with a default value
|
Wasn't sure how to/if I should refactor some of the other places with channel creation, so let me know if this is incomplete. |
alex-miao
force-pushed
the
alex/channels
branch
from
80a7e5c
to
7889192
Compare
crates/builder/src/server/local.rs
Outdated
| req_sender: mpsc::Sender<ServerRequest>, | ||
| req_receiver: mpsc::Receiver<ServerRequest>, |
There was a problem hiding this comment.
So I actually think for this one its fine to just pass in the capacity, hides the actual message passing mechanism from the interface.
crates/pool/src/server/local.rs
Outdated
| @@ -30,9 +30,11 @@ pub struct LocalPoolBuilder { | |||
|
|
|||
| impl LocalPoolBuilder { | |||
| /// Create a new local pool server builder | |||
| pub fn new(request_capacity: usize, block_capacity: usize) -> Self { | |||
There was a problem hiding this comment.
Hm, I like the old implementation better than having to expose the internals of how the local pool actor passes messages. I think just exposing the capacities is fine here.
alex-miao
force-pushed
the
alex/channels
branch
from
7889192
to
62effb3
Compare
crates/pool/src/task.rs
Outdated
| @@ -69,7 +71,7 @@ impl Task for PoolTask { | |||
| }; | |||
| let provider = eth::new_provider(&self.args.http_url, self.args.http_poll_interval)?; | |||
| let chain = Chain::new(provider, chain_settings); | |||
| let (update_sender, _) = broadcast::channel(1000); | |||
| let (update_sender, _) = broadcast::channel(CHAIN_UPDATE_CHANNEL_CAPACITY); | |||
There was a problem hiding this comment.
I agree with @dancoombs passing it as an argument with a default value
Closes #356
Proposed Changes
cli