-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature/ipfs storage engine #9
Conversation
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.
It is a best practice to put IO operations into the try-catch statements in order to be saved from race conditions.
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.
Try catch expression doesn't solve the race condition, in fact we need mutex and sync. Let's put this issue to backlog since it isn't urgent for now.
const nodeP2p = await createLibp2p(config); | ||
// Manual patch for node bootstrap | ||
const addresses = [ | ||
'/dnsaddr/bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN', |
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.
Would be cool to have hard-coded values as static constants to make them more meaningful.
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.
It's bootstrap nodes of IPFS, no need to make it configurable for now, and other people shouldn't care about this. Will add list off bootstrap nodes.
const config: Libp2pOptions = { | ||
start: true, | ||
addresses: { | ||
listen: ['/ip4/127.0.0.1/tcp/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.
For example, here we can have the LOCAL_HOST constant
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.
Just like above reason, this value won't change. Passing a constant isn't free so i pass the value directly.
'/dnsaddr/bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb', | ||
'/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt', | ||
].map((e) => multiaddr(e)); | ||
// Dial to bootstrap nodes |
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.
'Golden rule' to have a separate function instead of a comment
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.
It just call .dial() method, I don't think, we need another function here. Function cost a call frame so I don't prefer it.
Feature type
Feature description
Provide a module that allowed you to work with IPFS like, store, retrieve and BSON document to IPFS.
Pull request checklist
Please check if your PR fulfills the following requirements: