-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add verification event log mechanism to verify() operation (for fine-grained UI) #92
base: main
Are you sure you want to change the base?
Conversation
Context: What this PR is trying to do is add additional finer-grained results to the The idea is, verify() would return an additional, optional const {valid, log} = await vc.verify({...}); Where the [
{id: 'issuer_check', valid: true},
{id: 'expiration_check', valid: false, error: new Error('Credential has expired')},
] So, currently, verify() returns a boolean pass/fail, plus an array of errors. But for the purposes of a typical verification UI, it's useful to display a set of checks / errors, regarding which steps of verification passed or failed. This is a non-breaking (additive) change to the verify() API that I think might be really useful. |
lib/vc.js
Outdated
@@ -221,10 +221,22 @@ async function verifyCredential(options = {}) { | |||
} | |||
return _verifyCredential(options); | |||
} catch(error) { | |||
const result_array = []; |
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.
Couple of comments here.
One, please rename result_array
to log
.
Two, move log
into the results
array's object, on the same level as credential
, verified
, etc.
lib/vc.js
Outdated
@@ -261,6 +273,8 @@ async function _verifyCredential(options = {}) { | |||
'"credentialStatus".'); | |||
} | |||
|
|||
const result_array = []; |
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.
Rename to log
} | ||
} | ||
|
||
log.push({id: 'check_status', valid: true}); |
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.
Wouldn't this line overwrite line 302, since we are not returning after we push that log entry?
} catch(error) { | ||
const log = []; | ||
log.push({ | ||
id: 'check_credential_required_field', |
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.
Should we include user-friendly event descriptions in these log entries as well or do you think the ID-description mapping responsibility should be left to the client code?
Left a couple comments above, but generally, I support the utility this PR provides 👍 |
I propose we close this PR (it'll be replaced by the downstream PR digitalcredentials#4 -- once we test it and get initial deployment feedback, stabilize the API, etc, I'll make an upstream PR to this lib). |
No description provided.