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

feat: fetch verification method types by proof type #913

Conversation

karimStekelenburg
Copy link
Contributor

Signed-off-by: Karim [email protected]

@karimStekelenburg karimStekelenburg requested a review from a team as a code owner June 29, 2022 13:32
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.

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

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

@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

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
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants