-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
3beef19
to
d5c7aa5
Compare
opts.log(`generating ${opts.bits}-bit RSA keypair...`, false) | ||
self.log('generating peer id: %s bits', opts.bits) | ||
peerId.create({ bits: opts.bits }, cb) | ||
} |
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 calling it "pregen ID", just say that it takes a Private Key.
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.
And make it an option and not a env variable.
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.
+1 for an option. i.e. a top level privateKey
option to the constructor that you can also pass as a CLI option --private-key=...
.
Also needs to be documented in the README.
opts.log(`generating ${opts.bits}-bit RSA keypair...`, false) | ||
self.log('generating peer id: %s bits', opts.bits) | ||
peerId.create({ bits: opts.bits }, cb) | ||
} |
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.
+1 for an option. i.e. a top level privateKey
option to the constructor that you can also pass as a CLI option --private-key=...
.
Also needs to be documented in the README.
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.
@mkg20001 could you add a test here to ensure the passed private key is used? Apologies I should have raised this in the previous review.
@alanshaw Added test |
Also the |
I know I’m way late to this, but reading it, I worry that it won’t be clear that the private key is being used to generate a peer ID here (as opposed to, say, encrypting the repo contents on disk or something). At the very least, I think it would be really helpful to state that this is the key for generating a peer ID in the README & CLI help. I’m kind of wondering if a different name for the option might be better, though, like |
4b3d360
to
1a31d3d
Compare
@mkg20001 I have rebased and tweaked the README a little - hope that's ok :D Lets get CI green and then get this merged! |
@Mr0grog IMHO just |
1a31d3d
to
3065c40
Compare
3065c40
to
a476cbe
Compare
Needed for ipfs/js-ipfsd-ctl#284