-
Notifications
You must be signed in to change notification settings - Fork 460
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
feat: custom announce filter #783
Conversation
b4cf337
to
86a42a8
Compare
86a42a8
to
6faf350
Compare
|
||
await keychainWithPassword.createKey(id, 'ed25519') | ||
const { id } = await keychainWithPassword.createKey(name, 'ed25519') |
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.
This was not failing in the past, but the promise below would return undefined. I think that the reason behind it was not using chai-as-promised
. Leveraging the aegir chai that uses if every time was causing this to fail.
6faf350
to
7c401e9
Compare
7c401e9
to
614c2ea
Compare
src/index.js
Outdated
// Filter noAnnounce multiaddrs | ||
const filterMa = this.addressManager.getNoAnnounceAddrs() | ||
|
||
// Create advertising list | ||
return this.transportManager.getAddrs() | ||
return announceFilter(this.transportManager.getAddrs() |
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.
Looking at this code I realized the announce behavior is not the same as go. Go doesn't concat the listen and announce addresses. If you provide announce addresses, they are the only addresses that get advertised.
So the behavior should likely be:
- If
announce
addresses are provided, only they will be announced (no filters are applied) - If
announce
is not provided, the transport addresses will be filtered- If
noAnnounce
is provided it will be applied as a filter - if the
announceFilter
is provide it will be run
- If
I feel like with announceFilter
we should just drop support for noAnnounce
. Downstream projects like js-ipfs can use the filter to do achieve the same end, but with more control.
Additionally, something to think about for js-ipfs that we can use the announceFilter for, is that go-ipfs takes noAnnounce
from the config and applies those as net masks. It leverages https://github.com/whyrusleeping/multiaddr-filter, so you can more easily filter out ip ranges.
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.
yap, I will change this accordingly. I agree on deprecating noAnnounce
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.
The multiaddr-filter with the net masks is Interesting. I will create an issue in libp2p-utils for us to discuss it better later on.
c69f7d2
to
8cbac36
Compare
652aaa9
to
dc6a53e
Compare
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.
LGTM, looks like there is an empty file added though test/dialing/utils.js
f897dc2
to
4e1fc90
Compare
This PR adds a custom announce multiaddrs filter. The default is kept, but users can easily change it to not announce private addresses via config.
This PR also changes the
chai
requires on all the codebase to leverageaegir
Closes #769