Switch to the new peer notify mechanism #564
Conversation
| // We don't bother subscribing to "connectivity" events because we always run identify after | ||
| // every new connection. |
There was a problem hiding this comment.
I think this is right but I'm not 100% certain. I remember remember running into some issues with this with respect to bitswap...
cc @sukunrt do you think the code here is sufficient?
There was a problem hiding this comment.
Yeah this looks fine. We run identify after connecting on every connection.
|
Apparently pubsub uses the "blank" host in tests, which doesn't support identify. |
|
Tests should be fixed by #565. |
1. Only listen for peers added and identify events. 2. Remove the old "Limited" check. Peers only show up as "Connected" if they have non-limited connections. 3. Don't bother listening for new connections directly and/or connectivity changes. We'll get a new identify event per new connection regardless. fixes #546
Stebalien
force-pushed
the
steb/new-peer-notify-mechanism
branch
from
0b97df5
to
03f77e0
Compare
|
This conflicts with #440. I'd prefer to just remove that feature because it doesn't work reliably with identify. |
|
I agree, but we should check with users of the feature. If it is in use, we might have to make some changes; maybe have a user function to test instead of directly comparing with the router protocols? |
|
Hm. You're right, we have the function and can just use that. |
This is actually kind of backwards (it tells us if the peer would be able to open a stream for us, not if we could open a steram to the peer) but it's good enough.
|
Ok, it should all work correctly-ish now. Or, well, correct enough. |
fixes #546