-
Notifications
You must be signed in to change notification settings - Fork 50
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
Waku v2 Websocket support #732
Conversation
Jenkins BuildsClick to see older builds (73)
|
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.
Good to see an increment! I know this is just a Draft
so you may have been planning to change things in any case, but I have made some comments FWIW.
Please let me know when the PR is ready for the review :) @rshiv |
How is this going @rshiv? Would you mind updating PR description to explain current state and blocker? Feel free to include logs in here too with what is the current problem you are trying to figure out. |
The integration is successful I will update the PR to reflect the changes for web socket support, and later add wss transport support as those would need tls certificate to work. |
sure. |
41cb12d
to
a92491d
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.
I see this PR is in draft mode. It'd be good to communicate clearly what is working and what is still a WIP. For example, with excerpt of logs showing how it works and/or how it doesn't.
Also seems to miss tests. Useful tests would be to ensure ws switch works in isolation, and that it works when a node supports tcp and ws (for example).
We can do this in a separate PR once we basic capabilities done here |
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.
As discussed at the pm call I'd suggest to also have tests present to confirm that the ws connection is viable.
Not sure what is the best way to test this in nim. Happy to provide a mini-js script if needed.
Do you have some work in progress tests somewhere? Can't see any test commits. |
@rshiv, as you address individual comments, please mark them "resolved" and re-request a review from contributors that have reviewed before. While this is WIP/Draft it will be good to see progress on adding tests, etc. Also a good idea to keep your feature branch updated with |
|
Signed-off-by: rshiv <reeshav96@gmail.com>
8b0112f
to
262146d
Compare
Signed-off-by: rshiv <reeshav96@gmail.com>
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.
Looks Nice!
I had a quick look and left some comments, will leave a more thorough review tomorrow
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, @rshiv! Things seem to be taking shape. :) Some comments below. LMK what you think.
Signed-off-by: rshiv <reeshav96@gmail.com>
Signed-off-by: rshiv <reeshav96@gmail.com>
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.
Nice one! Definitely taking shaping :) Minor comments
Forgot to add: please update CHANGELOG.md as well. Needed for release documentation. |
Co-authored-by: oskarth <ot@oskarthoren.com>
Co-authored-by: oskarth <ot@oskarthoren.com>
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.
I've added a couple more comments, and would like to see the CHANGELOG updated too, but think we're v close to merging. 👍
Signed-off-by: rshiv <reeshav96@gmail.com>
Signed-off-by: rshiv <reeshav96@gmail.com>
Signed-off-by: rshiv <reeshav96@gmail.com>
Signed-off-by: rshiv <reeshav96@gmail.com>
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
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 looks good! Let's get it merged. Make sure you squash all commits into a single commit based on master
(Github's default "Squash and Merge" should work).
This PR introduces web socket transport to wakunode and subsequent unit tests.
Next iteration will introduce secure websocket capablities to the same.
Closes issue #756 and increment towards issue #434
The PR is ready to be merged