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.

Sign up for GitHub

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

Jump to bottom

Add support for credentialId #361

Merged
merged 6 commits into from
Feb 19, 2024
Merged

Add support for credentialId #361

merged 6 commits into from
Feb 19, 2024

Conversation

wes-smith
Copy link
Collaborator

credentialId allows identification of credentials without credential.id set.

@wes-smith
Add support for credentialId
dd0add5 
-credentialId identifies credentials without credential.id set
@wes-smith wes-smith linked an issue Jan 30, 2024 that may be closed by this pull request
credentialId in updateCredentialStatus may not fit all use cases #126
Closed
@mkhraisha
Copy link
Collaborator

If credentialId is set but credential.id is empty is the intention to tell the issuer to assign credential.id the value of crdentialId?

msporny
msporny reviewed Jan 30, 2024
View reviewed changes
components/Credential.yml Outdated Show resolved Hide resolved
@dlongley
Copy link
Contributor

dlongley commented Jan 30, 2024 •
edited
Loading

@mkhraisha,

If credentialId is set but credential.id is empty is the intention to tell the issuer to assign credential.id the value of crdentialId?

No, credential.id should remain empty (unset) as the client requests -- since clients need to be able to generate VCs that do not include credential.id (which are valid VCs -- and useful for things like unlinkable selective disclosure). The credentialId option allows clients to ensure that they have a reference to the credential they intended to issue even when credential.id is not set.

@mkhraisha
Copy link
Collaborator

@mkhraisha,

If credentialId is set but credential.id is empty is the intention to tell the issuer to assign credential.id the value of crdentialId?

No, credential.id should remain empty (unset) as the client requests -- since clients need to be able to generate VCs that do not include credential.id (which are valid VCs -- and useful for things like unlinkable selective disclosure). The credentialId option allows clients to ensure that they have a reference to the credential they intended to issue even when credential.id is not set.

With the initial wording it was a bit confusing but after reading Manu's suggested changes, this explanation makes sense.

TallTed
TallTed suggested changes Feb 12, 2024
View reviewed changes
components/Credential.yml Outdated
@@ -23,7 +23,7 @@ components:
type: string
"id":
type: string
description: The ID of the credential. This property MAY be empty. The issuer SHOULD NOT auto-generate the id property, as this can lead to unrecoverable partitioning errors.
description: The ID of the credential. This property MAY be empty. The issuer SHOULD NOT auto-generate the id property since the client wanted to issue a VC without the id.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: The ID of the credential. This property MAY be empty. The issuer SHOULD NOT auto-generate the id property since the client wanted to issue a VC without the id.
description: The ID of the credential. This property MAY be empty. The issuer SHOULD NOT auto-generate the id property since the client wanted to issue a VC without an ID.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Outdated - see newest version of Credential.yml.

components/IssueCredentialOptions.yml Outdated Show resolved Hide resolved
@PatStLouis
Copy link
Collaborator

Should we include some text about duplicate credentialId and maybe add a 409 error on the issuance endpoint?

@TallTed
Copy link
Collaborator

TallTed commented Feb 13, 2024

Updated change request, as discussed.

@msporny
Copy link
Contributor

msporny commented Feb 19, 2024

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 4fc156e into main Feb 19, 2024
1 check passed
@msporny msporny deleted the 126-credentialid-in-updatecredentialstatus-may-not-fit-all-use-cases branch February 19, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TallTed TallTed TallTed requested changes

@dlongley dlongley dlongley left review comments

@msporny msporny msporny approved these changes

@mavarley mavarley Awaiting requested review from mavarley mavarley is a code owner

@mkhraisha mkhraisha Awaiting requested review from mkhraisha mkhraisha is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

credentialId in updateCredentialStatus may not fit all use cases
6 participants
@wes-smith @mkhraisha @dlongley @PatStLouis @TallTed @msporny