-
Notifications
You must be signed in to change notification settings - Fork 44
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
Async Crypto Endeavour #33
Conversation
|
||
### `createFromJSON(obj)` | ||
### `createFromJSON(obj, 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.
why does this have to be async?
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.
Because it uses createFromPubKey
and createFromPrivKey
, but I just realised they don't have to be async either, not even when using webcrypto in the future, so I can revert these :) Thank you for questioning me
@@ -2,7 +2,7 @@ | |||
"name": "peer-id", | |||
"version": "0.7.0", | |||
"description": "IPFS Peer Id implementation in Node.js", | |||
"main": "lib/index.js", | |||
"main": "src/index.js", |
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.
TODO change before merge
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.
done
0874fe1
to
d8fb6f4
Compare
@@ -93,11 +93,14 @@ const PeerId = require('peer-id') | |||
|
|||
The key format is detailed in [libp2p-crypto](https://github.com/ipfs/js-libp2p-crypto). | |||
|
|||
### `create([opts])` | |||
### `create([opts], 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.
As mentioned before, let's keep the top level callback named 'callback', it reads better and we always used until the 'refactor' tornado started pushing cb
everywhere
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.
done
/* eslint-env mocha */ | ||
'use strict' | ||
|
||
require('loud-rejection')() |
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.
"facepalm" that a module had to be created to enable this.
Let's put a comment on top saying what it is 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.
It actually shouldn't be here, it was just for debugging. I solved the issue why I need 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.
done
console.log('id ', id.toB58String()) | ||
console.log('priv key ', bs58.encode(id.privKey.bytes)) | ||
console.log('pub key ', bs58.encode(id.pubKey.bytes)) | ||
PeerId.create({ bits: 512 }, (err, id) => { |
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.
any reason for the increase?
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 webcrypto has a minimum of 1024 I actually need to increase it even more
(cb) => crypto.unmarshalPrivateKey(rawPrivKey, cb), | ||
(priv, cb) => priv.public.hash((err, digest) => { | ||
cb(err, digest, priv) | ||
}), |
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 feels weird to me to use waterfall and then capture the callbacks to call the waterfall callback with arguments in a certain order. It is way more simpler to read to just have the shared state between async operations at the top and then run series through 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.
hmm not sure, right now all versions I can come up with are complicated and not particular nice :/
} | ||
|
||
if (pub && !privDigest.equals(pubDigest)) { | ||
callback(new Error('Public and private key do not match')) |
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.
missing a return
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.
done
} | ||
|
||
if (id && !privDigest.equals(id)) { | ||
callback(new Error('Id and private key do not match')) |
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.
missing a return
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.
done
expect(shortId.privKey.bytes.length).is.below(longId.privKey.bytes.length) | ||
PeerId.createFromPubKey(id1.marshalPubKey(), (err, id2) => { | ||
expect(err).to.not.exist | ||
expect(id1.id).to.be.eql(id2.id) |
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.
did you know that .be
is actually not needed :D?
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.
to .be
or not to .be
parallel([ | ||
(cb) => crypto.generateKeyPair('RSA', 1024, cb), | ||
(cb) => crypto.generateKeyPair('RSA', 1024, cb), | ||
(cb) => crypto.generateKeyPair('RSA', 1024, 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.
Do we support ECDH keys now too? :D
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.
no :(
CR'ed |
This allows the use of native key generation in the browser BREAKING CHANGE: This changes the interface of .create, .createFromPrivKey, .createFromPubKey, .createFromJSON
7d2e546
to
42b4890
Compare
V7kF9RKUOHG0Tr0oZ3CGzqRNDNUeznJPEUP3amb1YCXMJLPkR+AONQQJBAO5huuYxxEDEjNn4zCiJFGwdbZSA097Gyyi8PS0po+X/0KRx8WgQun | ||
Za+dh+LohEmxEk85gQ+GJrvbaPG94G4aECQQDOmEK1zmuJvI2PRZ/QPHs9CgRL1bIZEg8rJfhnv0YDD7NUeTpyT8+3/A4nbJjw8/zsr0A2UdSW6 | ||
EfEaaaplsr7AkEAvJQ5q4MxMt+KYaEtuN+AdWruVi137mOrMgWAC+tGClxOLNkq1V1udNTRk892djx3w59MyT6bkBiVkwcxT3p4IQJAV914GdzF | ||
7dmsly+0bZsbivVUqHAlg/YjT2WhxXYbL7ggvB+nFPEO1iA0YN4WGfybKIrMk42wDdKSm12XzW7duwJAS9oUNnPDNEWnmNYXQsrkSMSloKJPsyC |
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 trim these
@diasdavid this should be ready now |
This allows the use of native key generation in the browser
Depends on libp2p/js-libp2p-crypto#10
BREAKING CHANGE:
This changes the interface of
.create