Skip to content
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

Support creating from Secp256k1 / harmonize algorithm with Go #95

Merged
merged 1 commit into from
Jul 11, 2019

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Jul 11, 2019

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 libp2p/interop#21

@aknuds1
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019

@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.

package.json Outdated
"libp2p-crypto": "~0.16.1",
"multihashes": "~0.4.14"
"async": "^3.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the opportunity of upgrading this dependency. Hope it's OK.

@aknuds1 aknuds1 force-pushed the feature/secp256k1 branch 2 times, most recently from 6080f4a to a03f123 Compare July 11, 2019 15:11
@vasco-santos
Copy link
Member

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
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019

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

@jacobheun
Copy link
Contributor

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
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019

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

@aknuds1
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019

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

@jacobheun
Copy link
Contributor

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.

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

This looks good.

A note for releasing:

  • This should be applied as a patch to the 0.12 line.
  • Once feat: async await #87 is merged and released (as 0.13.0), this should be rebased and released on top of that as well.

This will enable work to continue on secp256k1 keys without worrying about the async iterators work.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019 via email

@aknuds1
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019

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

@jacobheun
Copy link
Contributor

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

@jacobheun
Copy link
Contributor

0.12.3 contains this fix.

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

@jacobheun jacobheun force-pushed the feature/secp256k1 branch from ac53c8d to e72a1d8 Compare July 11, 2019 18:04
@jacobheun
Copy link
Contributor

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 aknuds1 force-pushed the feature/secp256k1 branch from e72a1d8 to 3a0bdf0 Compare July 11, 2019 18:22
@aknuds1
Copy link
Contributor Author

aknuds1 commented Jul 11, 2019

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

@jacobheun jacobheun merged commit 17440a3 into libp2p:master Jul 11, 2019
@jacobheun
Copy link
Contributor

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

@vasco-santos
Copy link
Member

awesome 🚀

@aknuds1 aknuds1 deleted the feature/secp256k1 branch July 12, 2019 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants