feat: custom dialer addr sorter #792
Conversation
vasco-santos
force-pushed
the
feat/custom-dialer-multiaddr-sorter
branch
2 times, most recently
from
203d3de to
fa4c480
Compare
vasco-santos
force-pushed
the
feat/custom-dialer-multiaddr-sorter
branch
2 times, most recently
from
dfdd29d to
5fdf959
Compare
src/dialer/index.js
Outdated
| @@ -120,7 +124,16 @@ class Dialer { | |||
| this.peerStore.addressBook.add(id, multiaddrs) | |||
| } | |||
|
|
|||
| let knownAddrs = this.peerStore.addressBook.getMultiaddrsForPeer(id) || [] | |||
| let knownAddrs = this.addressSorter( | |||
| this.peerStore.addressBook.get(id) || [] | |||
There was a problem hiding this comment.
Any reason you switched away from getMultiaddrsForPeer? The address sorter should be able to handle /p2p on a multiaddr and avoids the need for the map logic here.
There was a problem hiding this comment.
Yes! I want the sorter to get an Address and not a multiaddr. With this, we have the ability to sort with other data, like the .isCertified boolean.
There was a problem hiding this comment.
The downside is having to do the /p2p logic which is embedded in the getMultiaddrsForPeer. I suggest I move that logic to libp2p-utils and reuse it here. What do you think? Any naming suggestions?
There was a problem hiding this comment.
Ah, right. What if we just extend getMultiaddrsForPeer to take an optional sorter? Then we can keep that logic isolated and allow different systems to use different sorters as needed.
There was a problem hiding this comment.
that seems good! I will change that as we will only use this with the getMultiaddrsForPeer
src/circuit/auto-relay.js
Outdated
| // Get peer known addresses and sort them per public addresses first | ||
| // Sorting addresses to public first | ||
| const remoteAddrs = this._addressSorter( | ||
| this._peerStore.addressBook.get(connection.remotePeer) || [] |
There was a problem hiding this comment.
Use getMultiaddrsForPeer? This should also avoid the if check below for the p2p protoName.
src/dialer/utils.js
Outdated
|
|
||
| const isPrivate = require('libp2p-utils/src/multiaddr/is-private') | ||
|
|
||
| // TODO: Move to libp2p-utils? |
There was a problem hiding this comment.
Yeah, I think this would be useful for other modules that would avoid needing to use libp2p as a dependency.
test/dialing/direct.spec.js
Outdated
| @@ -176,14 +177,36 @@ describe('Dialing (direct, WebSockets)', () => { | |||
| .and.to.have.property('code', ErrorCodes.ERR_TIMEOUT) | |||
| }) | |||
|
|
|||
| it('should sort addresses on dial', async () => { | |||
| sinon.spy(dialerUtils, 'sortPublicAddressesFirst') | |||
There was a problem hiding this comment.
It would be good to actually verify the dial order here, we dont check anything after connectToPeer. Just pass a stub to addressSorter and return a different order than addressBook returns. We should be able to spy on the transportManager and ensure the addresses are executed in the same order addressSorter returns.
vasco-santos
force-pushed
the
feat/custom-dialer-multiaddr-sorter
branch
2 times, most recently
from
735973f to
6a686c6
Compare
vasco-santos
force-pushed
the
feat/custom-dialer-multiaddr-sorter
branch
from
eabd04a to
f691d9b
Compare
Co-authored-by: Jacob Heun <jacobheun@gmail.com>
|
CI is green and suggestions added! 🎉 |
* feat: custom dialer addr sorter * chore: use libp2p utils sorter via addressBook getMultiaddrsForPeer * chore: use new libp2p utils * chore: apply suggestions from code review Co-authored-by: Jacob Heun <jacobheun@gmail.com> Co-authored-by: Jacob Heun <jacobheun@gmail.com>
* feat: custom dialer addr sorter * chore: use libp2p utils sorter via addressBook getMultiaddrsForPeer * chore: use new libp2p utils * chore: apply suggestions from code review Co-authored-by: Jacob Heun <jacobheun@gmail.com> Co-authored-by: Jacob Heun <jacobheun@gmail.com>
This PR adds a custom dialer addr sorter via config. It is used both by the dialer and the autoRelay.
The default sorter function puts public addresses first as a first criterium and certified addresses as a second criterium. This comparator function should probably be moved into
libp2p-utils.Needs: