-
Notifications
You must be signed in to change notification settings - Fork 249
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: testing discovery and dialing with nwaku integration #5940
Conversation
Hey @gabrielmer, and thank you so much for making your first pull request in status-go! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
Jenkins BuildsClick to see older builds (30)
|
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! Thanks for it! 💯
In all the commented code, shall we add the tag "TODO-nwaku", or sth like that, so that we can revisit all of them in the future?
Ooh great idea! Just updated it and added "TODO-nwaku" to all the newly commented blocks :) Thank you! |
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.
Overall LGTM. Just left a comment regarding the enrtree usage!
NodeKey: "11d0dcea28e86f81937a3bd1163473c7fbc0a0db54fd72914849bc47bdf78710", | ||
EnableRelay: true, | ||
LogLevel: "DEBUG", | ||
DnsDiscoveryUrl: "enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im", |
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 while it will work, it has the inconvenient of polluting the bootnodes with nodes that will only live when we run the tests (hence why the fake enr tree was being created)
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.
Makes sense 100%! As we discussed, I created the following issue to track this improvement waku-org/nwaku#3119
Thanks so much!
- some minor progress to add nwaku in status-go - nwaku.go: GetNumConnectedPeers controls when passed pubsub is empty - waku_test.go: adapt TestWakuV2Store - add missing shard.go - feat_: build nwaku with nix and use build tags to choose between go-waku and nwaku (#5896) - chore_: update nwaku - nwaku bump (#5911) - bump: nwaku - chore: add USE_NWAKU env flag - fix: build libwaku only if needed - feat: testing discovery and dialing with nwaku integration (#5940)
- some minor progress to add nwaku in status-go - nwaku.go: GetNumConnectedPeers controls when passed pubsub is empty - waku_test.go: adapt TestWakuV2Store - add missing shard.go - feat_: build nwaku with nix and use build tags to choose between go-waku and nwaku (#5896) - chore_: update nwaku - nwaku bump (#5911) - bump: nwaku - chore: add USE_NWAKU env flag - fix: build libwaku only if needed - feat: testing discovery and dialing with nwaku integration (#5940)
- some minor progress to add nwaku in status-go - nwaku.go: GetNumConnectedPeers controls when passed pubsub is empty - waku_test.go: adapt TestWakuV2Store - add missing shard.go - feat_: build nwaku with nix and use build tags to choose between go-waku and nwaku (#5896) - chore_: update nwaku - nwaku bump (#5911) - bump: nwaku - chore: add USE_NWAKU env flag - fix: build libwaku only if needed - feat: testing discovery and dialing with nwaku integration (#5940)
Description
Testing DNS discovery and peer connections and disconnections when integrating nwaku.
I commented everything in
api.go
,api_test.go
,message_publishing.go
and a big part ofnwaku.go
. I propose to remove the comments incrementally while we implement and test new features with the nwaku integration.Important changes:
api.go
,api_test.go
,message_publishing.go
and part ofnwaku.go
cGoWakuDialPeerById
andcGoWakuDisconnectPeerById
using newly added bindingsWakuConfig
with more nwaku node configurationscfg
field inWaku
to be of type*WakuConfig
WakuDialPeerById
andDisconnectPeerById
functionsTestBasicWakuV2
to use nwkaku's DNS discovery, dialing and disconnecting from peers and checking that it indeed worksHow to test it
chore-adding-libwaku-dial-and-disconnect-by-id
branch to have the new libwaku's procs that are usedTestBasicWakuV2
by runningCloses waku-org/nwaku#3039