Add crypto utility function and use in BearerDid #430
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Frank Hinek <[email protected]>
frankhinek
added
package: crypto
|
TBDocs Report 🛑 Errors: 0 @web5/api
@web5/crypto
@web5/crypto-aws-kms
@web5/dids
@web5/credentials
TBDocs Report Updated at 2024-02-27T01:11:17Z |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
=======================================
Coverage 93.34% 93.35%
=======================================
Files 80 80
Lines 23804 23827 +23
Branches 1890 1891 +1
=======================================
+ Hits 22220 22243 +23
Misses 1544 1544
Partials 40 40
|
jiyoonie9
reviewed
Feb 23, 2024
jiyoonie9
reviewed
Feb 23, 2024
jiyoonie9
approved these changes
Feb 23, 2024
packages/crypto/src/utils.ts
Outdated
| return curveToJoseAlgorithm[publicKey.crv]; | ||
| } | ||
|
|
||
| throw new Error(`Unable to determine algorithm based on provided input: alg=${publicKey.alg}, crv=${publicKey.crv}`); |
There was a problem hiding this comment.
Maybe we should include the curves we support here and/or in the docs?
There was a problem hiding this comment.
Thanks @KendallWeihe. Added the list of supported algorithm identifiers and curve names to the error in 0aa36d1.
KendallWeihe
approved these changes
Feb 26, 2024
|
Only nitpicky feedback item, so take it or leave it, would've been to do this one #418 is a separate PR. Reasoning being, it's a disparate matter to the other two items in this PR so caused a slight disruption when reviewing, but also it may just be because of my naivete to this code base. |
diehuxx
reviewed
Feb 26, 2024
|
|
||
| // If the key contains an `alg` property that matches a JOSE registered algorithm identifier, | ||
| // return its value. | ||
| if (publicKey.alg && Object.values(curveToJoseAlgorithm).includes(publicKey.alg)) { |
There was a problem hiding this comment.
Nit clarification: Previously, we didn't check that the alg is in curveToJoseAlgorithm values. This seems correct to me, but also took me a second to notice while reviewing. Did we previously want to return "unsupported" algorithms or was that just an oversight that we're not correcting?
There was a problem hiding this comment.
I may be missing the question. Unsupported algorithms won't be returned and an error will be thrown.
diehuxx
approved these changes
Feb 26, 2024
Signed-off-by: Frank Hinek <[email protected]>
finn-block
pushed a commit
that referenced
this pull request
Mar 19, 2024
* Add crypto utility function and use in BearerDid * Improve error message --------- Signed-off-by: Frank Hinek <[email protected]>
finn-block
pushed a commit
that referenced
this pull request
Mar 19, 2024
* Add crypto utility function and use in BearerDid * Improve error message --------- Signed-off-by: Frank Hinek <[email protected]>
finn-block
pushed a commit
that referenced
this pull request
Mar 19, 2024
* Add crypto utility function and use in BearerDid * Improve error message --------- Signed-off-by: Frank Hinek <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will:
BearerDidto the@web5/cryptopackage.BearerDidto use the utility function.BearerDidto close CorrectBearerDiddocumentation #418.Note
The package versions for
@web5/cryptoand@web5/didsare not incremented in this PR as this change is rather minimal, has no immediate impact on external API consumers, and consequently, isn't worth publishing a new release.A future PR will bump the versions and publish new releases as the utility function added to
@web5/cryptois consumed in other packages in this monorepo (which is already the case for a branch that is still a WIP.