-
Notifications
You must be signed in to change notification settings - Fork 445
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
chore: auto relay example #795
Conversation
65a7aaf
to
dfceafb
Compare
0f68e5d
to
6850cd2
Compare
6850cd2
to
a09ac4a
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.
Once all the code is updated we should make sure to update the code shown in the readme so it matches properly.
examples/auto-relay/README.md
Outdated
node.multiaddrs.forEach((ma) => console.log(`${ma.toString()}/p2p/${node.peerId.toB58String()}`)) | ||
``` | ||
|
||
The Relay HOP advertise functionality is **NOT** required to be enabled. However, if you are interested in advertising on the network that this node is available to be used as a HOP Relay you can enable it. |
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 problem with this example is that the node has no way of actually advertising. A content router has to be included for this to work. I would disable it for this example and mention here that a router is required. Ideally this should link to available routers so we can just update that section with Rendezvous is ready.
examples/auto-relay/README.md
Outdated
/ip4/192.168.1.120/tcp/61592/ws/p2p/QmWDn2LY8nannvSWJzruUYoLZ4vV83vfCBwd8DipvdgQc3 | ||
``` | ||
|
||
TODO: Docker Image with a repo |
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.
What's this for?
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 think we should create a simple Docker Image with relay and HOP enabled to enable people to easily deploy Relays. I will remove this from now, while I think in the best solution to also include the announce addresses logic
examples/auto-relay/README.md
Outdated
Node started: QmerrWofKF358JE6gv3z74cEAyL7z1KqhuUoVfGEynqjRm | ||
connected to the HOP relay | ||
Listening on: | ||
/ip4/192.168.1.120/tcp/61592/ws/p2p/QmWDn2LY8nannvSWJzruUYoLZ4vV83vfCBwd8DipvdgQc3/p2p-circuit/p2p/QmerrWofKF358JE6gv3z74cEAyL7z1KqhuUoVfGEynqjRm |
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.
Consider cleaning the logging up a bit to be a bit clearer, something like:
Node started: QmerrWofKF358JE6gv3z74cEAyL7z1KqhuUoVfGEynqjRm | |
connected to the HOP relay | |
Listening on: | |
/ip4/192.168.1.120/tcp/61592/ws/p2p/QmWDn2LY8nannvSWJzruUYoLZ4vV83vfCBwd8DipvdgQc3/p2p-circuit/p2p/QmerrWofKF358JE6gv3z74cEAyL7z1KqhuUoVfGEynqjRm | |
Node started with id QmerrWofKF358JE6gv3z74cEAyL7z1KqhuUoVfGEynqjRm | |
Connected to the HOP relay QmWDn2LY8nannvSWJzruUYoLZ4vV83vfCBwd8DipvdgQc3 | |
Advertising with a relay address of /ip4/192.168.1.120/tcp/61592/ws/p2p/QmWDn2LY8nannvSWJzruUYoLZ4vV83vfCBwd8DipvdgQc3/p2p-circuit/p2p/QmerrWofKF358JE6gv3z74cEAyL7z1KqhuUoVfGEynqjRm |
examples/auto-relay/auto-relay.js
Outdated
await node.dial(relayAddr) | ||
|
||
// Wait for connection and relay to be bind for the example purpose | ||
await pWaitFor(() => node.multiaddrs.length > 0) |
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.
Instead of pWaitFor
perhaps it would be best to listen on the Peer Store for our own address change. This also ensures we're only using libp2p deps in this example.
examples/auto-relay/auto-relay.js
Outdated
console.log('connected to the HOP relay') | ||
console.log('Listening on:') | ||
node.multiaddrs.forEach((ma) => console.log(`${ma.toString()}/p2p/${node.peerId.toB58String()}`)) | ||
})() |
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.
nit: instead of a self executing anonymous function it might be helpful to put these in a function like main
and then execute that. Same for the other files.
examples/auto-relay/other-node.js
Outdated
@@ -0,0 +1,27 @@ | |||
'use strict' |
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.
Instead of other-node
perhaps we should use the same format as other examples: dialer
, listener
?
examples/auto-relay/relay.js
Outdated
}, | ||
addresses: { | ||
listen: ['/ip4/0.0.0.0/tcp/0/ws'] | ||
// announceFilter: TODO check production section |
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.
What production section?
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.
Changed, it was the What's next section
where I mention that the announceAddrs should be changed for production
a0f305c
to
5623449
Compare
examples/auto-relay/listener.js
Outdated
node.peerStore.on('change:multiaddrs', ({ peerId }) => { | ||
// Updated self multiaddrs? | ||
if (peerId.equals(node.peerId)) { | ||
resolve() |
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.
Maybe get rid of the promise logic and just put the console logs here? This event listener never gets cleaned up and can throw an error if our addresses change again and call resolve
on the already resolved promise.
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.
yes, that makes a lot of sense! thanks
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
* chore: auto relay example * chore: update examples to use process arguments * chore: add test setup for node tests and test for auto-relay * chore: apply suggestions from code review * chore: do not use promise for multiaddrs event on example
* chore: auto relay example * chore: update examples to use process arguments * chore: add test setup for node tests and test for auto-relay * chore: apply suggestions from code review * chore: do not use promise for multiaddrs event on example
This PR adds an example to use libp2p's auto relay.
With it, I added a setup to run the examples in CI, inspired by
js-ipfs
older test setup pre lerna migration ipfs/js-ipfs#2528Needs: