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

#206 - Introduced support for custom signer #502

Merged
merged 11 commits into from
Sep 18, 2023
Merged

Conversation

thehenrytsai
Copy link
Contributor

  • Introduced new Signer interface for custom signer
  • Replaced PrivateJwk with Signer in SignatureInput
  • Renamed existing Signer to SignatureAlgorithm for disambiguation
  • Renamed GeneralJwsSigner to GeneralJwsBuilder

Copy link

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

A couple thoughts.

First, less pressing. In a follow up PR, we should migrate onto @web5/crypto. Thanks to @adam4leos that package has 100% test coverage. I can start on that after we get this PR merged.

Second, people are gonna have a hard time figuring out how to use a custom signer if we don't make it super clear in documentation. It's nested pretty deep in the options for <Message>#create where to pass a custom Signer. Do you mind adding a test that creates a DWN message with a custom signer just so we have it written down somewhere how to do it? We don't need the test coverage but it'll be useful to have something that we can link to. I anticipate people in Discord asking us how to use custom signers.

I'm wondering if there's a way to restructure the input to #create() methods that will make this more ergonomic or more self-documenting. Maybe flattening authorizationSignatureInput: SignatureInput into protectedHeader?: JwsHeaderParameters; customSigner?: Signer;?

Let's sync tomorrow. I've got some more thoughts based on how I see messages being created in web5-js that I can't express concisely right now.

README.md Outdated Show resolved Hide resolved
LiranCohen
LiranCohen previously approved these changes Sep 18, 2023
Copy link
Member

@LiranCohen LiranCohen 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 great! The updated naming of Builder/Signer/SignatureAlgorithm definitely reads really nicely.

🎸 🚀

src/types/jws-types.ts Outdated Show resolved Hide resolved
@thehenrytsai thehenrytsai force-pushed the henrytsai/custom-signer branch from 97a1383 to faf1057 Compare September 18, 2023 18:14
@thehenrytsai
Copy link
Contributor Author

@diehuxx and @LiranCohen pushed an overhaul replacing SignatureInput with Signer directly. Tons of files are impacted, but the refactoring streamlines the code quite a lot IMO.

diehuxx
diehuxx previously approved these changes Sep 18, 2023
@thehenrytsai thehenrytsai merged commit 725b610 into main Sep 18, 2023
3 checks passed
@thehenrytsai thehenrytsai deleted the henrytsai/custom-signer branch September 18, 2023 23:13
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