-
Notifications
You must be signed in to change notification settings - Fork 18
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: Created an issuer verification of an EBSI issued credential #236
Conversation
|
||
const issuer = wrappedVc.decoded?.iss ?? (typeof wrappedVc.decoded?.vc?.issuer === 'string' ? wrappedVc.decoded?.vc?.issuer : wrappedVc.decoded?.vc?.issuer?.existingInstanceId) | ||
|
||
if (!issuer) { |
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.
Why are you using an approach that we have never used?
Return a promise reject with an error so you do not have to list undefined as a result type. Or resolve undefined if that is an okay outcome. Not a reject with undefined. That makes no sense.
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.
Refactored error handling
const payload = await response.json() | ||
|
||
if (!payload.attributes.some((a: Attribute) => ['RootTAO', 'TAO', 'TI'].includes(a.issuerType))) { | ||
return Promise.reject(undefined) |
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.
This makes no sense. Provide a proper error.
Also I believe the type should be an optional argument as well. If I want to only verify a party to be a trusted issuer I need to be able to provide TI as a single array value. If I want more types I should be able to provide these as well. The default should be TI and a TI only
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.
Refactored error handling and moved issuer type to a parameter
@@ -139,6 +139,15 @@ export const verifyCredentialToAccept = async (args: VerifyCredentialToAcceptArg | |||
} | |||
} | |||
|
|||
if (onVerifyEBSICredentialIssuer) { |
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.
And where is the code that only applies this if we encounter actual EBSI issuers? You cannot just assume EBSI and only EBSI or not of course. A wallet or RP can be member of many ecosystems
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.
Moved it to the if clause to make sure the code will only execute for an EBSI issuer
No description provided.