-
Notifications
You must be signed in to change notification settings - Fork 800
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
Add client support for the lighthouse/peers
route
#4937
base: unstable
Are you sure you want to change the base?
Conversation
94113c6
to
149b28e
Compare
@@ -0,0 +1,10 @@ | |||
[package] | |||
name = "serde_utils" |
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.
We have an external package (ethereum_serde_utils
) which defines a crate called serde_utils
. I think we could either put this code there, give it a different crate name to avoid confusion, or put the Instant
serialisation as a module in the lighthouse_network
crate.
I think my preference is for the latter, as the Instant
stuff is not really Ethereum-specific, and seems a bit too hacky (using SystemTime::now
) to put in ethereum_serde_utils
. Similarly, for future serde utils, if we deem them Ethereum-relevant we can put them in the extern crate, whereas if they're Lighthouse-specific we can keep them in-tree. Wdyt?
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.
My preference is the former, although I understand your point about lack of Ethereum specificity (and concur). My issue with moving it to lighthouse_network
is that, in my opinion, that crate is already too busy and this busyness complicates downstream use of the crate outside of Lighthouse. I already have some changes that I need to open a (separate) PR for that address some of this.
and seems a bit too hacky
I also agree here, but my desire for full client-side implementation of the HTTP API overrides this. This certainly isn't a nice solution but my feeling at the moment is that having missing routes is less nice than even this.
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.
Ok cool. I'm happy with putting it in ethereum_serde_utils
. I thought it might break WASM compatibility (which seems to be something people want from our crates) but it seems it will compile fine and just panic on WASM, which is fine as it's completely optional (rust-lang/rust#48564).
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.
Oh wow, hadn't considered that WASM quirk before!
Just bumping this. Anyone know the current state? @michaelsproul @jmcph4 |
Issue Addressed
N/A
Proposed Changes
This PR implements the client-side of the (non-standard)
lighthouse/peers
HTTP API route. To achieve this, the following changes have been made:serde_utils
, undercommon
. This is to handle theDeserialize
logic forInstant
s and exists alongside other utility crates likeclap_utils
.Deserialize
derive macro to various types underlighthouse_network
(as this is where the majority of peer-related types exist)get_lighthouse_peers
function alongside the existingeth2
API methodsAdditional Info
This functionality is desirable as it allows downstream crates to contact the extended HTTP API programmatically. Desire for such functionality has already been alluded to in the codebase:
https://github.com/sigp/lighthouse/blob/stable/common/eth2/src/lighthouse.rs#L415-L421