feat: add snapclient implementation for Fetchclient #19893
Conversation
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
this looks amazing already
I have a few more questions and smol requests
still need to look into this in detail
crates/net/network/src/message.rs
Outdated
| PooledTransactions(NewPooledTransactionHashes), | ||
| /// All `eth` request variants. | ||
| EthRequest(PeerRequest<N>), | ||
| /// All `snap` request variants (wrapped in `PeerRequest`). |
There was a problem hiding this comment.
do we even need this additional variant if we added snap variants to peerrequest?
There was a problem hiding this comment.
You are right , agree that this is redundant
The snap requests would flow the same if we just sent them via PeerMessage::EthRequest because the inner PeerRequest discriminates the snap cases.
| if snap_enabled { | ||
| hello_message = hello_message.with_snap(); | ||
| } |
| direction, | ||
| error: Some(PendingSessionHandshakeError::Eth(err)), | ||
| let supports_snap = | ||
| multiplex_stream.shared_capabilities().iter_caps().any(|cap| cap.name() == "snap"); |
There was a problem hiding this comment.
can we add a helper type for this on sharecap?
| // Sending snap message over eth-only connection is unsupported | ||
| ( | ||
| EthRlpxConnProj::EthOnly(_) | EthRlpxConnProj::Satellite(_), | ||
| EthSnapMessage::Snap(_), | ||
| ) => Err(EthStreamError::UnsupportedMessage { | ||
| message_id: SnapMessageId::GetAccountRange as u8, |
There was a problem hiding this comment.
I believe this should fail regardless
| type Error = EthStreamError; | ||
|
|
||
| fn poll_ready(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
| delegate_call!(self.poll_ready(cx)) |
There was a problem hiding this comment.
why can we no longer use this macro?
There was a problem hiding this comment.
I think dropping it is a right call , as delegate_call! is for single-field delegation , can’t express the current per-variant behavior .
| mut self, | ||
| cap: &Capability, | ||
| handshake: F, | ||
| additional_caps: Vec<SharedCapability>, |
There was a problem hiding this comment.
We need it so the primary stream can own more than one capability (e.g., eth + snap). additional_caps tells the multiplexer to route those extra capabilities to the primary; without it, only the main cap goes to primary and snap messages would be treated as “non-primary” and misrouted.
mattsse
added
C-enhancement
2 participants
This is on issue #19882 #19883
Changes as bellow :
cc @mattsse