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

feature/funke-mdoc #166

Merged
merged 23 commits into from
Sep 18, 2024
Merged

feature/funke-mdoc #166

merged 23 commits into from
Sep 18, 2024

Conversation

sanderPostma
Copy link
Contributor

mdoc support in types

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.

Project coverage is 93.59%. Comparing base (6023f0e) to head (66db088).
Report is 24 commits behind head on develop.

Files with missing lines Patch % Lines
...uation/handlers/didRestrictionEvaluationHandler.ts 0.00% 3 Missing and 1 partial ⚠️
lib/PEX.ts 88.88% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #166      +/-   ##
===========================================
- Coverage    93.76%   93.59%   -0.17%     
===========================================
  Files           66       66              
  Lines         2629     2638       +9     
  Branches       662      665       +3     
===========================================
+ Hits          2465     2469       +4     
- Misses         161      165       +4     
- Partials         3        4       +1     
Flag Coverage Δ
unittest 93.59% <61.53%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/PEX.ts Outdated
@@ -525,7 +530,8 @@ export class PEX {

let presentation = presentationResult.presentation;

if (CredentialMapper.isSdJwtDecodedCredential(presentationResult.presentation)) {
if (CredentialMapper.isSdJwtDecodedCredential(presentationResult.presentation as unknown as SdJwtDecodedVerifiableCredential)) {
// FIXME? SdJwtDecodedVerifiableCredentialWithKbJwtInput is local type and is not supported in ssi-sdk
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has a FIXME in there, but this is not a new situation. We still pass the same data into isSdJwtDecodedCredential, typescript just got in the way suddenly.

Copy link
Contributor

Choose a reason for hiding this comment

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

That comment makes no sense IMO. Obviously the types changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SdJwtDecodedVerifiableCredentialWithKbJwtInput does not overlap with SdJwtDecodedVerifiableCredential
Was is not matching is local type

export interface SdJwtKbJwtInput {
  header: {
    typ: 'kb+jwt';
  };
  payload: {
    iat: number;
    sd_hash: string;
    nonce?: string;
  };
}

vs in SSI-SDK

  kbJwt?: {
    compact: CompactJWT
    payload: SdJwtVcKbJwtPayload
  }

Copy link
Contributor Author

@sanderPostma sanderPostma Sep 17, 2024

Choose a reason for hiding this comment

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

@nklomp
We have the latest SSI types available now in PEX, so perhaps PresentationResult should be like this now

export interface PresentationResult {
  /**
   * The resulting presentation, can have an embedded submission data depending on the location parameter
   */
  presentation: OriginalVerifiablePresentation;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I synched it up with ssi-types, but since some of the fields in header & payload have to be loaded by the signer I had to change the types a bit

export type PartialSdJwtKbJwt = {
  header: Partial<SdJwtVcKbJwtHeader>;
  payload: Partial<SdJwtVcKbJwtPayload>;
};

export type PartialSdJwtDecodedVerifiableCredential = Omit<SdJwtDecodedVerifiableCredential, 'kbJwt'> & { kbJwt: PartialSdJwtKbJwt };

lib/PEX.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sksadjad sksadjad left a comment

Choose a reason for hiding this comment

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

small changes requested

lib/PEX.ts Outdated Show resolved Hide resolved
lib/PEX.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sksadjad sksadjad left a comment

Choose a reason for hiding this comment

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

lgtm

@sanderPostma sanderPostma merged commit ce95ed3 into develop Sep 18, 2024
2 checks passed
@@ -538,19 +546,20 @@ export class PEX {
// alg MUST be set by the signer
header: {
typ: 'kb+jwt',
alg: hashAlg,
Copy link
Contributor

Choose a reason for hiding this comment

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

@sanderPostma I think this is a mistake, and shouldn't have been added here. The hashAlg is different from the signing alg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, I already removed it from verifiablePresentationFrom but in constructPresentation it's still there

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