libp2p / js-peer-id

peer-id implementation in JavaScript. Deprecated; use https://github.com/libp2p/js-libp2p-peer-id instead.
https://github.com/libp2p/js-libp2p-peer-id
MIT License
80 stars 44 forks source link

Support creating from Secp256k1 / harmonize algorithm with Go #95

Closed aknuds1 closed 5 years ago

aknuds1 commented 5 years ago

Support creating peer IDs from Secp256k1 type keys and harmonize ID algorithm with Go so that for public keys of <= 42 bytes length, we inline the public key.

Required by https://github.com/libp2p/interop/pull/21

aknuds1 commented 5 years ago

@jacobheun @vasco-santos This is my solution for fixing interop between Go and JS peers when Secp256k1 keys are used. Basically, it makes JS peers behave the same as Go peers when Secp256k1 keys are used, i.e. their peer ID is generated by inlining the PK. From my testing (with the interop suite) it works well.

vasco-santos commented 5 years ago

hey @aknuds1

We are in the process of merging libp2p/js-peer-id#87, which will create a lof of conflicts in here. Maybe it is worth getting that in first

aknuds1 commented 5 years ago

@vasco-santos Sure, I can see that it would be the natural order. Do that one first if you want.

jacobheun commented 5 years ago

Once this is ready, we will likely need to release this as a patch on the current version (because it is backwards compatible), and then rebase this onto the async changes and release a patch on top of that version.

If we only do the latter, this will probably create some annoying blockers getting the key updates done while the async changes are propagated up the chain.

aknuds1 commented 5 years ago

@jacobheun Aha - you guys know best in which order to do this :)

aknuds1 commented 5 years ago

I guess I need to add a test or two to reach the coverage target.

jacobheun commented 5 years ago

I guess I need to add a test or two to reach the coverage target.

Looking at the lines that aren't covered I think it's fine. It's a nominal drop and the error states that would need to be covered aren't very likely imo.

aknuds1 commented 5 years ago

Will do @jacobheun! Just have to get back on the computer.

On Thu, Jul 11, 2019, 17:53 Jacob Heun notifications@github.com wrote:

@jacobheun commented on this pull request.

In src/index.js https://github.com/libp2p/js-peer-id/pull/95#discussion_r302620279:

@@ -145,18 +166,14 @@ exports.create = function (opts, callback) {

}

opts = opts || {}

opts.bits = opts.bits || 2048

  • opts.keyType = opts.keyType || 'RSA'

Forgot one thing! Can you update the options in the readme for create? 🙏

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libp2p/js-peer-id/pull/95?email_source=notifications&email_token=AACEVV3MHEGFVQDEL56PAF3P65JQTA5CNFSM4IBJXJL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6FXH5A#pullrequestreview-260797428, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEVV3PJ4SEE3SEDCX3QCTP65JQTANCNFSM4IBJXJLQ .

aknuds1 commented 5 years ago

@jacobheun I pushed an update to the README, please have a look.

jacobheun commented 5 years ago

That works, thanks! Going to start the release hydra for this and async await.

jacobheun commented 5 years ago

0.12.3 contains this fix.

Will rebase this once I've finished the 0.13 async release

jacobheun commented 5 years ago

Rebased this if you want to take a look @aknuds1, and make sure I didnt miss anything. I'll let ci do it's thing and then do a patch of this for the 0.13 async line.

aknuds1 commented 5 years ago

I looked it over @jacobheun, it looks fine to me. Thanks!

jacobheun commented 5 years ago

This is now in both the 0.12 and 0.13 version lines. 🎉

vasco-santos commented 5 years ago

awesome 🚀