-
Notifications
You must be signed in to change notification settings - Fork 52
Key exchange with jsrsasign #115
Conversation
@diasdavid Could you help me fix the circle CI build, see https://circleci.com/gh/libp2p/js-libp2p-crypto/296?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link |
mBdkD5r+ixWF174naw53L8U9wF8kiK7pIE1N9TR4USEeovLwX6Ni/2MMDZedOfof | ||
2f77eUdLsK19/5/lcgAAYaXauXWhy2d2r3SayFrC9woy0lh2VLKRMBjcx1oWb7dp | ||
0uxzo5Y= | ||
-----END ENCRYPTED 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.
Can you put these in files as text data?
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.
Sure can, but why? They are only used in one place.
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.
Do you want me to use fs
to read the file. Or wrap them in module.export
with back tick and then require
them.
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.
You can have a file with these pem keys and do fs.readFileSync once.
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 can do. But why. Tests should be easy to read. Why the indirection and requiring file support.
Will fs.readFileSync
be supported in browser tests?
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.
@richardschneider fs.readFileSync is supported (thanks to the bundler).
Tests should be easy to read.
Agreed and that is why having 40 lines of a string that has no other information that is human readable than the fact that it is a private key is better kept in a file that has that name so that in the test code you just have one line to refer to the 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.
If you feel really strongly about leaving them here, we can deal with it. It might just be my personal preference. No need for bikeshedding :)
@victorbjelkholm Circle is having issues again? It would be great if you could create a doc on explaining why these issues happen and what you do to fix them. |
@diasdavid sure, I can add a document about it, where you want me to add it? TLDR is that if you see |
thanks @victorbjelkholm ! just updated it :) Is there any repo to track CI stuff? I feel that info belongs there, even if just in a issue for now. |
@diasdavid perfect! I created a issue in ipfs/ci-sync (which holds bunch of scripts for CI-related things) for now, here: ipfs-inactive/ci-sync#3 Eventually, that repository will be folded into ipfs/jenkins to create a new ipfs/ci repository where we can keep everything. |
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. One last request. @richardschneider mind checking the new bundle size vs the previous one? Run npm run build
to get it.
@diasdavid
|
@diasdavid RTM. Provides all the primitives required by keychain to implement |
@richardschneider Did you manage to get npm run build to run? Can I get that bundle size comparison? |
@diasdavid No,, did not get |
@diasdavid https://unpkg.com/[email protected]/lib/ reports that the package is 259 kB |
This uses the jsrsasign package to create password protected private keys.
Currently, it only works with RSA key types.
See issue #114
PR libp2p/js-libp2p-keychain#18 is dependent upon this PR