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

Add support for ed25519 in IonKey (1/2) #20

Merged

Conversation

gjgd
Copy link
Member

@gjgd gjgd commented Nov 28, 2021

Part 1 of adding support for Ed25519 in IonSDK:

  • Add JwkEd25519 model
  • Add Ed25519 test vectors
  • Add methods for generating Ed25519 keys in IonKey

Part 2 will be updating IonRequest and IonDid

tests/IonKey.spec.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/IonKey.ts Outdated Show resolved Hide resolved
lib/IonKey.ts Outdated Show resolved Hide resolved
lib/IonKey.ts Outdated Show resolved Hide resolved
gjgd and others added 5 commits November 28, 2021 16:45
Co-authored-by: Kyle Den Hartog <kdenhartog@users.noreply.github.com>
Co-authored-by: Kyle Den Hartog <kdenhartog@users.noreply.github.com>
package.json Outdated Show resolved Hide resolved
@OR13 OR13 self-requested a review November 29, 2021 14:55
@csuwildcat
Copy link
Member

So if this addition makes it possible to use Ed25519 for operation keys, will there be a PR in Sidetree Core to allow them to be used in the actual nodes?

@gjgd
Copy link
Member Author

gjgd commented Dec 2, 2021

My plan was to use this change to generate Ed25519 sidetree fixtures, but it can also be used to allow Ed25519 in Sidetree core node.

@thehenrytsai
Copy link
Contributor

Will take a look in the next couple of days, adding @xinaxu in case he has bandwidth first.

lib/LocalSigner.ts Outdated Show resolved Hide resolved
@thehenrytsai
Copy link
Contributor

@gjgd, thanks for taking the time to start to adding ED25519 support. The PR looks great overall, given that it really isn't hooked up with any thing really, so I see this more as a preparation to more PRs to come, so just a few comments, please do keep the code coverage at 100%.

@OR13
Copy link
Contributor

OR13 commented Dec 10, 2021

We are blocked by this, and need a timeline for merge. @thehenrytsai are you able to approve and merge? or should we fork the implementation and push support for this here, removing the ION SDK dep?:

https://github.com/transmute-industries/sidetree.js/blob/main/packages/wallet/src/operations/update.ts#L4

We are trying to wrap this SDK to maintain interop and support ION at the same time, but we are required to support EdDSA.

@thehenrytsai
Copy link
Contributor

@OR13, I reviewed the PR couple of days ago with a few comments, expecting them to be addressed or responded to before I can approve the merge.

@OR13
Copy link
Contributor

OR13 commented Dec 10, 2021

@thehenrytsai thanks, I was surprised not to see a "change request" from you on the PR... you left comments, but did not request changes.

cc @gjgd do you understand the "comments that are requesting changes"?

@gjgd
Copy link
Member Author

gjgd commented Dec 10, 2021

I didn't get a chance to review the comments yet, will try to do that today and make the appropriate changes

@gjgd
Copy link
Member Author

gjgd commented Dec 20, 2021

@thehenrytsai Thanks for the review, I believe I addressed all of the comments. Part 2 coming next

@csuwildcat
Copy link
Member

@thehenrytsai what's the status on this? Let's sync as a WG after the holidays to ensure this PR doesn't fall to the wayside.

Copy link
Contributor

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

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

Read your comment on file naming and it's a very good point. Probably changing the name to something like ed25519jwk1private across the board is the way to go, but won't hold the PR on this change.

@thehenrytsai thehenrytsai merged commit 0af90f7 into decentralized-identity:main Jan 3, 2022
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.

5 participants