-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: integrate chain_id and protocol_version in NetworkInfo #1029
feat: integrate chain_id and protocol_version in NetworkInfo #1029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, looks great overall, only a few nits
crates/net/rpc/src/eth/api/mod.rs
Outdated
@@ -43,17 +47,23 @@ where | |||
Client: BlockProvider + StateProviderFactory + 'static, | |||
{ | |||
/// Creates a new, shareable instance. | |||
pub fn new(client: Arc<Client>, pool: Pool) -> Self { | |||
let inner = EthApiInner { client, pool }; | |||
pub fn new(client: Arc<Client>, pool: Pool, network: NetworkHandle) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please have a look at #857
we only want traits here and not NetworkHandle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to add chain_id()
to NetworkInfo
or do you want me to create another trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep let's add this
crates/net/rpc/src/eth/api/mod.rs
Outdated
Self { inner: Arc::new(inner) } | ||
} | ||
|
||
/// Returns the inner `Client` | ||
fn client(&self) -> &Arc<Client> { | ||
&self.inner.client | ||
} | ||
|
||
/// Returns the inner `Client` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update docs
Codecov Report
@@ Coverage Diff @@
## main #1029 +/- ##
==========================================
+ Coverage 73.91% 73.95% +0.04%
==========================================
Files 300 300
Lines 32937 32954 +17
==========================================
+ Hits 24346 24372 +26
+ Misses 8591 8582 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Closes #980