Add network feature to eth2#8558
Conversation
pawanjay176
left a comment
There was a problem hiding this comment.
I'm not sure about this one.
Do we have any downstream users of this crate that would need the networking types to be the right ones and not strings?
jxs
left a comment
There was a problem hiding this comment.
I'm not sure about this one.
Do we have any downstream users of this crate that would need the networking types to be the right ones and not strings?
I was the one bringing this up in the previous PR to Mac. Since we’re using a typed language, it would be better to rely on the available types instead of Strings where possible right? 😅
pawanjay176
left a comment
There was a problem hiding this comment.
Sorry, just realised that this would not matter for beacon_node build because we already compile the networking crates for it.
We need to gate the get_node_identity function too though, otherwise no feature compilation would fail with
error[E0412]: cannot find type `IdentityData` in this scope
--> common/eth2/src/lib.rs:1939:69
|
1939 | pub async fn get_node_identity(&self) -> Result<GenericResponse<IdentityData>, Error> {
| ^^^^^^^^^^^^ not found in this scope
|
note: found an item that was configured out
--> common/eth2/src/types.rs:568:12
|
566 | #[cfg(feature = "network")]
| ------------------- the item is gated behind the `network` feature
| /// `GET node/identity` | ||
| #[cfg(feature = "network")] | ||
| pub async fn get_node_identity(&self) -> Result<GenericResponse<IdentityData>, Error> { |
There was a problem hiding this comment.
Ah nice catch, fixed here
Merge Queue StatusRule:
This pull request spent 30 minutes 20 seconds in the queue, including 29 minutes 10 seconds running CI. Required conditions to merge
|
Proposed Changes
This reverts some of the changes from #8524 by adding back the typed network endpoints with an optional
networkfeature. Without thenetworkfeature, these endpoints (and associated dependencies) will not be built.This means the
enr,multiaddrandlibp2p-identitydependencies have returned but are now optional