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

Clarify the domain and range of the verifiablePresentation.verifiableCredential property #1366

Closed
andresuribe87 opened this issue Nov 29, 2023 · 7 comments
Labels
before-CR pending close Close if no objection within 7 days

Comments

@andresuribe87
Copy link
Contributor

From #1358 (comment), it became evident that the name for the property verifiablePresentation.verifiableCredential is misleading, and the definition needs further clarification.

Relevant excerpt from the conversation below:

In an ideal world, I would agree with you. Yes, the term verifiableCredential is misleading insofar as the graph naming aspect is hidden (and this misconception actually may have led to a bug in v1.0). The problem I see is that renaming a term that is already widely used in implementations out there might be the source of problems. I leave the judgement to others, but, pragmatically, we may have to live with the current term.

I would argue that if renaming fixes these sort of bugs, that's exactly what we should do in 2.0 :)

As I said: in theory yes, pragmatically probably not. But that is a really separate issue, too, I believe.

@msporny
Copy link
Member

msporny commented Dec 3, 2023

-1 for at least these reasons:

We don't embed the data structure name in the property names for most (if not all?) other properties. We don't use "mediaTypeString", we use "mediaType", we don't use "typeArray", we use "type". So, listing the data structure in the name feels like the wrong thing to do here.

Everything we do in the spec is a part of a graph, so injecting the word "graph" here and not elsewhere doesn't seem to make sense either... and injecting it everywhere feels unnecessary.

I'm not convinced that adding this word is going to "fix" any sort of bug, as the case for why it would prevent bugs hasn't been made.

Finally, implementations have been using verifiableCredential for years now w/o major issues. We're past what would be theoretically nice to do what's the practical ROI on this change? Presently, it feels like the ROI would be negative on this change: it wouldn't fix any bugs, it would look weird because it's not how we (generally) name properties, and it would be incompatible with all current implementations out there.

It falls into the same category as renaming "credentialSubject" to "subject" -- it would've been nice to do that in v1.0, but given that this is the 3rd version of the specification, changing it now doesn't feel like it would add much value while further complicating processors.

@andresuribe87
Copy link
Contributor Author

The definition was changed from what VDCM 1.1 stated to say that only verifiable credential graphs are acceptable as the value.

Previously, implementations would use verifiableCredential to put in externally secured verifiable credentials. Now, the WG is stating that it isn't an acceptable use. This means that implementations are already being asked to update the semantics of the field. The decisions of the WG require implementations to update the handling of the logic of the verifiableCredential property.

Without renaming, the property is likely to be used in the same way as it was used in VCDM 1.1. Renaming that property prevents implementations from misusing the term.

@dlongley
Copy link
Contributor

dlongley commented Dec 5, 2023

@andresuribe87,

The definition was changed from what VDCM 1.1 stated to say that only verifiable credential graphs are acceptable as the value.

How this is not a breaking change was discussed here: #1259 (comment)

@OR13
Copy link
Contributor

OR13 commented Dec 6, 2023

@andresuribe87 andresuribe87 changed the title Rename verifiablePresentation.verifiableCredential to verifiablePresentation.verifiableCredentialGraph Clarify the domain and range of the verifiablePresentation.verifiableCredential property Dec 6, 2023
@iherman
Copy link
Member

iherman commented Dec 7, 2023

The issue was discussed in a meeting on 2023-12-06

  • no resolutions were taken
View the transcript

2.6. Clarify the domain and range of the verifiablePresentation.verifiableCredential property (issue vc-data-model#1366)

See github issue vc-data-model#1366.

Andres Uribe: I think that if the clarifications are made in #1365, then I think the point made in this issue is moot. It will also be addressed by the solution that was suggested yesterday of adding a type property.

Manu Sporny: +1 to what andres just said, agree with that direction.

Brent Zundel: Do we need both issues to track the concern?

Andres Uribe: I think one builds upon the other. One would be automatically closed.

Manu Sporny: Maybe changing the title would help. You're not asking for a rename of the property now, only asking for clarification. So we can change the title of the issue.

Orie Steele: People have been putting Verifiable Credentials in the verifiableCredential property of Verifiable Presentations already.
… Changing this now will be unacceptable to Transmute.
… I suggest allowing either using IRIs or the VCs directly. That's also consistent with the RDF here.

Orie Steele: I read the minutes.

Brent Zundel: I'll make this before CR. Who's willing to be assigned?

Dave Longley: I think Orie is referring to a specific JSON-LD property that has a bug, and once this has been fixed there won't be an issue.

Orie Steele: dlongley, confirming you are disclosing that https://json-ld.org/playground/ produces invalid N-Quads?

Sebastian Crane: Personally, is this a clarification on whether you can embed the definition of a VC within a VP? Or is this something different?

Brent Zundel: It is related to that. There are RDF concerns about whether ... you put the VC thing instead VP.verifiableCredential and that's ok, a VC that's been externally secured using a mechanism that uses an envelope, it creates a new data structure so it no longer fits within that property.

Sebastian Crane: So this is an uncontroversial change in the mechanics to our RDF data model.

Brent Zundel: I don't think I'm comfortable with saying uncontroversial here.

Sebastian Crane: Thank you for the clarification.

@msporny
Copy link
Member

msporny commented Dec 10, 2023

@andresuribe87, I read back over the current specification text and think it does state the domain and range pretty clearly:

image

What concrete changes are you requesting to this text?

IOW, I agree with @OR13, this seems to be a dupe of #1365 (based on the way the discussions are going in the WG). Marking this as pending close. @andresuribe87 feel free to object if you disagree w/ the above and we're not seeing the nuance between the two issues.

@msporny msporny added the pending close Close if no objection within 7 days label Dec 10, 2023
@brentzundel
Copy link
Member

Closing this as a duplicate of #1365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before-CR pending close Close if no objection within 7 days
Projects
None yet
Development

No branches or pull requests

6 participants