-
Notifications
You must be signed in to change notification settings - Fork 459
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
refactor: examples/pnet #523
Conversation
;(async () => { | ||
const node1 = await privateLibp2pNode(swarmKey) | ||
const node2 = await privateLibp2pNode(swarmKey) | ||
// const node2 = await privateLibp2pNode(otherSwarmKey) |
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.
@jacobheun I am having an issue using a invalid key.
On the transport listener, for example libp2p-tcp
https://github.com/libp2p/js-libp2p-tcp/blob/master/src/listener.js#L23 the upgradeInbound
throws and the promise rejection is not handled. What do you recommend to do here? A try...catch
in the encrypt and just log the error?
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.
btw, the error comes from:
https://github.com/libp2p/js-libp2p/blob/refactor/async-await/src/upgrader.js#L328
https://github.com/multiformats/js-multistream-select/blob/master/src/handle.js#L14
https://github.com/multiformats/js-multistream-select/blob/master/src/multistream.js#L37
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.
@vasco-santos the upgradeInbound should probably still throw. The Transport needs to know that there has been a failure so that it doesn't do any emitting. I think the best thing to do is to make sure that the transport listeners are catching that and logging errors. We should add a test for this to the transport interface and roll out patches to each of the transports.
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, I agree that should be the transports to handle this. Will PR everything
const swarmKey1Path = path.resolve(repo1, 'swarm.key') | ||
const swarmKey2Path = path.resolve(repo2, 'swarm.key') | ||
fs.writeFileSync(swarmKey1Path, swarmKey) | ||
// TASK: switch the commented out line below so we're using a different key, to see the nodes fail to connect |
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 README mentions the TASK
comments in the code for trying out the different swarm keys. How this is being done has changed, can we add a TASK
comment for replacing const node2 = await privateLibp2pNode(swarmKey)
with const node2 = await privateLibp2pNode(otherSwarmKey)
. The code is there, there's just no comment saying to do this.
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 catch
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! 🚀
* refactor: examples/pnet * chore: rename pnet-ipfs to pnet * chore: address review
This PR refactors the
pnet
example to the refactored asyncjs-libp2p
. Moreover, it now does not use ipfs and relies only onlibp2p