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.

Sign up for GitHub

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

Jump to bottom

feat: fetch verification method types by proof type #913

Merged
merged 4 commits into from
Aug 11, 2022
Merged

feat: fetch verification method types by proof type #913

merged 4 commits into from
Aug 11, 2022

Conversation

karimStekelenburg
Copy link
Contributor

Signed-off-by: Karim karim@animo.id

@karimStekelenburg karimStekelenburg requested a review from a team as a code owner June 29, 2022 13:32
TimoGlastra
TimoGlastra requested changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Can you add tests for the added methods?

packages/core/src/modules/vc/W3cCredentialService.ts Outdated Show resolved Hide resolved
@karimStekelenburg
Copy link
Contributor Author

Can you add tests for the added methods?

@TimoGlastra good catch, thanks. Time for a PR checklist.. I'll open a discussion about it.

TimoGlastra
TimoGlastra reviewed Jun 30, 2022
View reviewed changes
packages/core/src/modules/vc/SignatureSuiteRegistry.ts Outdated
@@ -11,7 +11,7 @@ export interface SuiteInfo {
suiteClass: typeof LinkedDataSignature
proofType: string
requiredKeyType: string
keyType: string
keyTypes: [KeyType]
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be KeyType[]. This actually means an array with one item of type KeyType (you can use this to create tuple types)

Suggested change
keyTypes: [KeyType]
keyTypes: KeyType[]

Copy link
Contributor

@TimoGlastra TimoGlastra Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm after thinking about it a bit more. This is the key type as we use it in the framework, but not the key as is used in did documents. Those key types are e.g. Ed25519Key2018 (or something like it). So we still have to make the transformation, but only the signature suites know which actual types is supports.

Should this be updated to include the supported key types as used by did documents? E.g. if the suites only supports 2018 and 2020 ed25519 keys and there's a new 2022 one that isn't supported we should only extract the 2018 and 2020 keys from the did document, as that's the only ones we can sign with.

@TimoGlastra TimoGlastra force-pushed the 0.3.0-pre branch 2 times, most recently from 65f8856 to 938a889 Compare July 3, 2022 10:35
@TimoGlastra TimoGlastra added this to the v0.3.0 milestone Jul 3, 2022
TimoGlastra
TimoGlastra approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

LGTM 👍. If you can resolve the conflicts we can merge

karimStekelenburg and others added 4 commits August 11, 2022 10:48
@karimStekelenburg @TimoGlastra
feat: add method to fetch KeyType by proof type
426fea8 
Signed-off-by: Karim <karim@animo.id>
@karimStekelenburg @TimoGlastra
test: add tests for getKeyTypesByProofType
371adde 
Signed-off-by: Karim <karim@animo.id>
@karimStekelenburg @TimoGlastra
fix: replace internal keytype with keytype strings
f31ffce 
Signed-off-by: Karim <karim@animo.id>
@TimoGlastra
fix(w3c): key type and verification method type
f2b2082 
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra force-pushed the feat/fetch-keytype-by-prooftype branch from edf8cd8 to f2b2082 Compare August 11, 2022 11:30
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022 •
edited
Loading

Codecov Report

Merging #913 (f2b2082) into 0.3.0-pre (d2fe29e) will decrease coverage by 0.03%.
The diff coverage is 70.37%.

@@              Coverage Diff              @@
##           0.3.0-pre     #913      +/-   ##
=============================================
- Coverage      87.67%   87.64%   -0.04%     
=============================================
  Files            574      574              
  Lines          13521    13539      +18     
  Branches        2190     2194       +4     
=============================================
+ Hits           11855    11866      +11     
- Misses          1662     1669       +7     
  Partials           4        4
Impacted Files Coverage Δ
...ages/core/src/modules/vc/SignatureSuiteRegistry.ts 50.00% <0.00%> (-20.59%) ⬇️
...ore/src/modules/dids/domain/key-type/bls12381g2.ts 100.00% <100.00%> (ø)
...s/core/src/modules/dids/domain/key-type/ed25519.ts 100.00% <100.00%> (ø)
...ckages/core/src/modules/vc/W3cCredentialService.ts 81.56% <100.00%> (+0.53%) ⬆️
packages/core/src/modules/vc/W3cVcModule.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@TimoGlastra
Copy link
Contributor

BBS tests failing in Node.JS 17/18 But otherwise tests are passing. Merging

@TimoGlastra TimoGlastra changed the title feat: add method to fetch KeyType by proof type feat: fetch verification method types by proof type Aug 11, 2022
@TimoGlastra TimoGlastra merged commit a457773 into openwallet-foundation:0.3.0-pre Aug 11, 2022
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
@karimStekelenburg @TimoGlastra
feat: fetch verification method types by proof type (#913)
92240a6 
Signed-off-by: Karim <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
@karimStekelenburg @TimoGlastra
feat: fetch verification method types by proof type (#913)
a79bdb6 
Signed-off-by: Karim <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
@karimStekelenburg @TimoGlastra
feat: fetch verification method types by proof type (#913)
ed69dac 
Signed-off-by: Karim <karim@animo.id>
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Sep 16, 2022
@karimStekelenburg @genaris
feat: fetch verification method types by proof type (openwallet-found…
d02330b 
…ation#913)

Signed-off-by: Karim <karim@animo.id>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@berendsliedrecht berendsliedrecht mentioned this pull request Jan 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimoGlastra TimoGlastra TimoGlastra approved these changes

Assignees
No one assigned
Labels
Waiting on Author
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@karimStekelenburg @codecov-commenter @TimoGlastra