-
Notifications
You must be signed in to change notification settings - Fork 57
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
fix: consider WS extMultiAddrs before publishing host address #2122
Conversation
You can find the image built from this PR at
Built from cf4485e |
Notice that currently the same case happens with external addresses that aren't WS/WSS: if an external address is set with the We can add the same logic for that case. However I do think that it's better to require users to add The way I see it is: Let me know what you think about this approach |
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.
Thanks for this!
I just added a comment mentioning how we could do the same (not a big deal though.)
However, shall we have in the PR description or somewhere an example on how to validate that the issue is sorted out with that? Maybe a snippet log will help as well.
Cheers!
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, seems to correctly produce the list of listen addresses
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! Thank you!!!
Description
When the user runs a Waku node with the flag
--ext-multiaddr
using a WS/WSS address, our current logic does not notice we specified a WS/WSS address and incorrectly adds the defaultwsHostAddress
to theannouncedAddresses
Changes
extMultiAddrs
before announcing the defaultwsHostAddress
How to test
When WS/WSS is enabled and no WS/WSS ext-multiaddr provided, default WS/WSS address is published:
When WS/WSS is enabled and a WS/WSS ext-multiaddr is provided, the default WS/WSS is not published:
Issue
closes #1797