Skip to content
This repository was archived by the owner on Feb 26, 2021. It is now read-only.

Async Crypto Endeavour#22

Merged
daviddias merged 11 commits intomasterfrom
fast-keys
Nov 3, 2016
Merged

Async Crypto Endeavour#22
daviddias merged 11 commits intomasterfrom
fast-keys

Conversation

@dignifiedquire
Copy link
Copy Markdown
Member

Ref libp2p/js-libp2p-crypto#10

BREAKING CHANGE

New method PeerInfo.create(cb) replaces the previous new PeerInfo() to create a fresh id


it('add multiaddr', () => {
const pi = new PeerInfo()
expect(pi).to.exist
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking something to exist every time doesn't seem useful

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed


it('add multiaddr that are buffers', () => {
const pi = new PeerInfo()
expect(pi).to.exist
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same


it('add repeated multiaddr', () => {
const pi = new PeerInfo()
expect(pi).to.exist
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it goes on

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@daviddias
Copy link
Copy Markdown
Member

How did you solve the PCKS#1 vs PKCS#6. Can we confirm that we are manipulating and storing the keys in the same way as in go? This will be crucial for config file share, identify and secio crypto handshake.

@dignifiedquire
Copy link
Copy Markdown
Member Author

How did you solve the PCKS#1 vs PKCS#6. Can we confirm that we are manipulating and storing the keys in the same way as in go? This will be crucial for config file share, identify and secio crypto handshake.

We already had tests for this (the go interop part) here and in libp2p-crypto. You can see the details of the conversion here: libp2p/js-libp2p-crypto@eb5de10

src/index.js Outdated
// look at https://github.com/whyrusleeping/js-mafmt/blob/master/src/index.js
}

Peer.create = (cb) => {
Copy link
Copy Markdown
Member

@daviddias daviddias Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, to simplify, let's default to use PeerInfo.create

  • s/Peer/PeerInfo
  • signature becomes (peerId, callback) =>
  • Use always PeerInfo.create everywhere

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean with peerId being optional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/index.js Outdated
return cb(err)
}

cb(null, new Peer(key))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key? or Id?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@daviddias
Copy link
Copy Markdown
Member

daviddias commented Oct 5, 2016

  • This PR is missing the documentation update

Copy link
Copy Markdown
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daviddias daviddias changed the title refactor: PeerId.create is now async Async Crypto Endeavour Oct 30, 2016
.travis.yml Outdated
env: CXX=g++-4.8
- node_js: stable
env:
- SAUCE=true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do Sauce on LTS

@dignifiedquire
Copy link
Copy Markdown
Member Author

@diasdavid should be ready now

@daviddias daviddias merged commit 5dc1162 into master Nov 3, 2016
@daviddias daviddias deleted the fast-keys branch November 3, 2016 07:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants