-
Notifications
You must be signed in to change notification settings - Fork 20
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/CWALL-174 based on Sander's work #108
Conversation
…sadjad # Conflicts: # pnpm-lock.yaml
…s that are failing
… that are failing
… added credential_definition to credential metadata of v13
…djad' into feature/CWALL-174_impl-draft1-sadjad
…djad' into feature/CWALL-174_impl-draft1-sadjad # Conflicts: # packages/issuer-rest/lib/oid4vci-api-functions.ts
…djad' into feature/CWALL-174_impl-draft1-sadjad # Conflicts: # packages/common/lib/types/CredentialIssuance.types.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick review. Some comments.
I think it is best to simply merge and fix issues from there
@@ -29,7 +34,7 @@ export class AccessTokenClient { | |||
const { asOpts, pin, codeVerifier, code, redirectUri, metadata } = opts; | |||
|
|||
const credentialOffer = opts.credentialOffer ? await assertedUniformCredentialOffer(opts.credentialOffer) : undefined; | |||
const isPinRequired = credentialOffer && this.isPinRequiredValue(credentialOffer.credential_offer); | |||
const pinMetadata: TxCodeAndPinRequired | undefined = credentialOffer && this.getPinMetadata(credentialOffer.credential_offer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep the isPinRequired as well?
} | ||
|
||
// Default regex for alphanumeric with no specific length limit if no input_mode is specified. | ||
regex = regex || /^[a-zA-Z0-9]+$|^[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes more sense to move this before the if statements and let these overwrite it instead of the || yo are doing now
credentialsSupported?: Record<string, CredentialConfigurationSupportedV1_0_13>, | ||
): CredentialConfigurationSupportedV1_0_13[] { | ||
if (!credentialOffer.credential_configuration_ids || !credentialsSupported) { | ||
return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is an or. Looks more like it should be an and. If not, then this code is now tied to v13 by the looks of it. Probably wise to make a comment about that.
edit: I see that it gets called when v13 is detected, so please add the comment
@@ -190,25 +240,25 @@ export interface CredentialRequestSdJwtVc extends CommonCredentialRequest { | |||
} | |||
|
|||
export interface CommonCredentialResponse { | |||
format: string; | |||
// format: string; TODO do we still need this for previous version support? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was used for matching credential types previously. Not sure how tests can still pass if this is gone. Or has it been introduced elsewhere? Same for the other format
properties that have been commented in this file
This is right now a work in progress. I've fixed the build issues and refactored code quite a bit. also added a few todos that needs discussing.
Right now fixing the test failures