Skip to content

Use URI id for credentialStatus and refreshService #819

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

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Conversation

clehner
Copy link
Member

@clehner clehner commented Sep 14, 2021

Fix #748. This changes the MUST requirement for credentialStatus id and
refreshService id to be URI instead of URL. The purpose of this change is to
improve consistency with the rest of the data model, where ids are expected
to be URIs (but not necessarily URLs). This should help implementers who want to ensure conformance with the VC Data Model without feeling compelled to check whether or not certain values are URLs, which may be a difficult determination to make without further specification by extensions. For example, there the VC Test Suite doesn't check URL-ness of refresh service ids: https://github.com/w3c/vc-test-suite/blob/593d5a5fb83f89ba60f32c0443d01fced5b4a3db/test/vc-data-model-1.0/21-refresh.js#L59 . Also, this specification doesn't seem to define URL or directly reference RFC 1738 - Uniform Resource Locators (URL), although it does have a definition for URI and identifier, and cites RFC 3986 - Uniform Resource Identifier (URI): Generic Syntax.

The other use of URL, in evidence ids, I have not changed in this PR, as that is a SHOULD, so I understand it would not be necessary to check for strict conformance. Maybe it is more useful to leave that part in to indicate the intended use of it?

As discussed on the 2021-09-08 VCWG call, these changes may be substantive, since it changes MUSTs, but also might not be, if it is considered that checking whether it is a URL might not by programatically possible as currently specified.


Preview | Diff

@dlongley
Copy link
Contributor

Maybe we may want to use URI and add an additional statement(s) that there's an expectation that machine readable information needs to be retrievable either from the URI or via some other field value in the credentialStatus/refreshService sections. That may resolve the concern around us wanting to be clear that there's some "locator" requirements/expectations here.

@iherman
Copy link
Member

iherman commented Sep 15, 2021

The issue was discussed in a meeting on 2021-09-15

  • no resolutions were taken
View the transcript

3.1. Use URI id for credentialStatus and refreshService (pr vc-data-model#819)

See github pull request #819.

David Chadwick: This is a PR that uses a URI id for credentialStatus and refresh service. It has a couple approvals already.
… This was in response to an issue that said... some say URI, some say URL. This takes the two instances that say must be URL, and change to say must be a URI.
… Last time, we discussed, this might not actually be a breaking change?

Manu Sporny: This is a normative breaking change. It doesn't matter if it's a subset: we are changing the normative... what's allowed.
… Even if nobody cares, it's technically a 1.2, thing, and it's labeled as such.

Ted Thibodeau Jr.: normative, but should not be breaking, because it relaxes requirements. I think it does class as a typo bug.

Brent Zundel: Question for group: do we agree that this is a bug in the specification?
… If it's not, it needs to be deferred to v2.

Manu Sporny: I didn't get a change to review... I recall we limited it on purpose - but don't remember if it was these two things specifically.
… I want to be careful about this change because I believe we said you want to be actually able to go out and discover this information.
… I'd like a little more time to review it, to make sure we are not undoing something we wanted to do.

David Chadwick: @manu +1

David Chadwick: I also remember that it was a URL so that it could be fetched

Brent Zundel: That is the primary question. Is this addressing errata. Is it substantive... because we can only make substantive changes in response to errata.
… If we can't answer that, it will be deferred.

Kyle Den Hartog: The only time I'm aware of this being a concern is with credentialStatus and Revocation Status 2020. There is a normative statement that references an id and says it needs to be linkable.
… They are utilizing the VC Data Model... that could create a conflict... suggest taking a look at it.

Manu Sporny: Now... my memory is being jogged... we absolutely meant URL. So I believe probably the fix is wrong and we shouldn't merge this.
… But I would like more time to dive in and consider it.

Ted Thibodeau Jr.: I'm fine with having the discussion in the GitHub pages... but having a URL does not guarantee it is dereferencable any more than URI...
… URL abstracts by location... just a construct that makes it easier for humans to look at it... It's a difference without a difference.
… Like I said, we can have the argument in the GitHub pages... but it will just go on forever if we need to.

Dave Longley: there would be no problem with revocation status type imposing additional constraints on top of the vc data model spec

Dave Longley: (in response to kyle)

Brent Zundel: Reminder, the conversation minuted here will automatically go into the PR. I agree that conversation should continue there.

Manu Sporny: I agree with TallTed -- we probably need to say "you have to be able to get information from the URI" or something to that effect

Brent Zundel: We'll move to the next Pull Request.

@kdenhartog kdenhartog added errata Erratum for a W3C Recommendation maintenance issues that may be considered part of the work of the maintenance group and removed PossibleErratum WG should determine if this is Errata labels Sep 29, 2021
@wyc
Copy link
Contributor

wyc commented Sep 29, 2021

Needs to be targeting the v1.2 branch instead of main.

@iherman
Copy link
Member

iherman commented Sep 29, 2021

The issue was discussed in a meeting on 2021-09-29

  • no resolutions were taken
View the transcript

1.5. Use URI id for credentialStatus and refreshService (pr vc-data-model#819)

See github pull request #819.

Wayne Chang: One more PR review... This one by cel.
… this looks like it's good to go, needs to be retargeted to v1.2?

Manu Sporny: yes

Charles Lehner: yes

Wayne Chang: Looks like there is no v1.2 branch, we'll resolve out of band.

@msporny msporny changed the base branch from main to v1.2 September 29, 2021 23:45
@msporny
Copy link
Member

msporny commented Sep 29, 2021

Merge conflicts, please fix @clehner. We're trying to get this merged down by the end of the week.

@kdenhartog
Copy link
Member

cleaned up the merge conflicts on this.

Normative and substantive, multiple reviews, no objections, merging.

@kdenhartog kdenhartog merged commit 74fbbc1 into v1.2 Sep 30, 2021
@kdenhartog kdenhartog deleted the cel/issue-748 branch September 30, 2021 11:56
@clehner
Copy link
Member Author

clehner commented Sep 30, 2021

Thanks @kdenhartog!

@kdenhartog kdenhartog removed the errata Erratum for a W3C Recommendation label Nov 18, 2021
@kdenhartog
Copy link
Member

kdenhartog commented Nov 18, 2021

There's a linked issue with the Errata label, removing the Errata label from the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance issues that may be considered part of the work of the maintenance group merge after 14 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URIs or URLs for ids
8 participants