feat(mdns): emit ToSwarm::NewExternalAddrOfPeer
#5179
Conversation
justcode740
changed the title
ToSwarm::NewExternalAddrOfPeer
| @@ -347,8 +347,10 @@ where | |||
| } | |||
|
|
|||
| if !discovered.is_empty() { | |||
| let event = Event::Discovered(discovered); | |||
| return Poll::Ready(ToSwarm::GenerateEvent(event)); | |||
| return Poll::Ready(ToSwarm::NewExternalAddrOfPeer { | |||
There was a problem hiding this comment.
[question]
Sorry if this is a silly question, I'm new to both rust and the libp2p code base.
- This seems like a breaking change for any code base that currently relies on
Event::Discovered
Is my understanding of rust breaking changes right here?
Would it be worth preserving Event::Discovered here for backwards compatibility?
- This only emits the first discovered peer
Reading lines 333-345, it looks like the Poll event will keep going until it drains query_response_receiver, discovering multiple peers?
This seems like we would want to return a ToSwarm::NewExternalAddrOfPeer for every peer we discover on line 345.
Should this move up to line 345 and replace the discovered vec?
- I acknowledge that, as written, point 2 might be at odds with point 1 😅.
To have our cake, and eat it too, could 333-347 be moved out of fn poll and instead feed two receivers, one that sends Event::Discovered after all ToSwarm::NewExternalAddrOfPeers have been flushed and the other that sends each ToSwarm::NewExternalAddrOfPeer?
There was a problem hiding this comment.
yeah exactly, thanks @retrohacker!
You can see the upnp example on how to return both
Description
Resolves #5104
Notes & open questions
Change checklist