-
Notifications
You must be signed in to change notification settings - Fork 941
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
refactor(swarm): express dial logic linearly #3253
Conversation
With this refactoring, we are not executing certain checks more thoroughly. For example, we make all addresses to be dialed unique and filter out the ones we listen on. Previously, those was only true for the addresses coming from I think the current version is more correct so I pushed a hack that fixes the test. Alternatively, we could also change the test (and perhaps remove the other code that checks for self dials?). |
No longer dialing duplicate addresses would fix #3209. Not doing it everywhere but in |
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.
All in all a nice clean-up. Thanks @thomaseizinger. Happy my ugly deeply nested method is gone.
I don't think this patch fixed #3209 because we are doing the deduplication before we ensure the address has a Let's re-open #3209 for now and confirm with a test that it is actually fixed. |
Description
Previously, the logic within
Swarm::dial
involved fairly convolutedmatch
expressions. This patch refactors this function to use new utility functions introduced onDialOpts
to handle one concern at a time.This has the advantage that we are covering slightly more cases now. Because we are parsing the
PeerId
only once at the top, checks like banning will now also act on dials that specify thePeerId
as part of the/p2p
protocol.Notes
I am touching this function as part of #3099 and found it hard to integrate the new callbacks into it.
Links to any relevant issues
Open Questions
Change checklist