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

Additional date/times #48

Closed
wants to merge 1 commit into from
Closed

Additional date/times #48

wants to merge 1 commit into from

Conversation

David-Chadwick
Copy link
Contributor

@David-Chadwick David-Chadwick commented Aug 22, 2019

fixes #44


Preview | Diff

Additional Times
@TallTed
Copy link
Member

TallTed commented Aug 22, 2019

This doesn't seem like the right way to do this. Properties of the physical credential are not properties of the (electronic) VC. Likewise and moreso, properties of the fact (Joe is licensed to drive) which is represented by the physical credential (Joe's physical driver's license) are not properties of the VC.

Rather than issuedAt and expiresAt, I would suggest originalIssuedAt and originalExpiresAt, or perhaps paperIssuedAt and paperExpiresAt, or even physicalIssuedAt and physicalExpiresAt.

These values would seem to me to be along the lines of "evidence based on which the VC was issued" and so belong in the <code>evidence</code> <a>property</a>.

@TallTed TallTed mentioned this pull request Aug 22, 2019
@David-Chadwick
Copy link
Contributor Author

This doesn't seem like the right way to do this. Properties of the physical credential are not properties of the (electronic) VC.

Well I guess we will just have to disagree on this, since all the properties of the VC have been taken from the physical one :-). Without the physical one there would not be a VC.

Likewise and moreso, properties of the fact (Joe is licensed to drive) which is represented by the physical credential (Joe's physical driver's license) are not properties of the VC.

Again I disagree with you

Rather than issuedAt and expiresAt, I would suggest originalIssuedAt and originalExpiresAt, or perhaps paperIssuedAt and paperExpiresAt, or even physicalIssuedAt and physicalExpiresAt.

We can bikeshed the exact terms. We just need ones that clearly indicate the semantics

These values would seem to me to be along the lines of "evidence based on which the VC was issued" and so belong in the <code>evidence</code> <a>property</a>.

The whole physical credential can be regarded as evidence. But what we are doing is copying the physical properties into the electronic one

@TallTed
Copy link
Member

TallTed commented Aug 23, 2019

The electronic VC is derived from the physical certificate; fine.

Some (perhaps even most) of the properties of the physical certificate are being copied to the electronic VC -- and that's fine.

But as you've explicitly stated, each of these has its own issuance/validFrom and expiration/validUntil date. The electronic VC is not inheriting/copying these from the physical.

In this case, it's clear that the physical certificate is all the evidence (and perhaps it's the only acceptable evidence) required or used for issuance of the electronic VC -- and it's perfectly fine to include any desired attributes of the physical certificate (including but not limited to the physical certificate's dates of validity and/or lifespan) in the evidence property of the electronic VC.

But it makes no sense to treat properties of the physical certificate as if they were properties of the electronic VC.

@deiu
Copy link
Contributor

deiu commented Aug 27, 2019

Wouldn't adding those properties directly impact the VC data model, as they introduce additional semantics?

@David-Chadwick
Copy link
Contributor Author

Wouldn't adding those properties directly impact the VC data model, as they introduce additional semantics?

Not really, since each claim about the credentialSubject has its own semantics, and the vast majority of these have not been defined yet. So they do not and cannot alter the VC data model.

@David-Chadwick
Copy link
Contributor Author

Some (perhaps even most) of the properties of the physical certificate are being copied to the electronic VC -- and that's fine.

I would argue that all of the properties of the physical certificate MAY be copied to the electronic VC, as long as it is clear that these are all properties of the physical certificate. So for example, the physical certificate number may be copied over, but this is not the VC certificate number. Similarly the physical certificate expiry date may be copied over, but this is not the VC certificate expiry date.

@deiu
Copy link
Contributor

deiu commented Aug 27, 2019

Sure, as long they are used within the credentialSubject.

@deiu
Copy link
Contributor

deiu commented Aug 27, 2019

We're going to defer to the CCG wrt. adding new terms. Let's leave this PR and issue open for now.

@deiu deiu added the Unresolved Pending conflict resolution / consensus label Aug 27, 2019
@dlongley
Copy link
Contributor

We might want to model this quite differently -- where the credentialSubject has a driversLicense and then put those properties on the graph node that driversLicense points at. There's a lot to discuss here, so I think premature to accept at this time.

Copy link

@mprorock mprorock left a comment

Choose a reason for hiding this comment

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

concur with @dlongley on this

We might want to model this quite differently -- where the credentialSubject has a driversLicense and then put those properties on the graph node that driversLicense points at. There's a lot to discuss here, so I think premature to accept at this time.

</ul>

</p>
Note. We will need to add issuedAt and expiresAt to the new implementors guide context
Copy link

Choose a reason for hiding this comment

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

This should probably be called out as an issue, e.g.
<p class="issue"></p>

Copy link

Choose a reason for hiding this comment

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

Suggested change
Note. We will need to add issuedAt and expiresAt to the new implementors guide context
<p class="issue">Note. We will need to add issuedAt and expiresAt to the new implementors guide context

</p>
Note. We will need to add issuedAt and expiresAt to the new implementors guide context
to be published at "https://www.w3.org/2018/credImpGuide/v1" then this note
can be removed.
Copy link

Choose a reason for hiding this comment

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

Suggested change
can be removed.
can be removed.</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR can be dropped, because the issuer of a VC can publish its own schema for the contents of its VCs and the properties of the VC's subject. If the issuer wants the subject's properties to contain half a dozen different dates and times it can do this without affecting the VC data model or any other VC issuer.

@TallTed
Copy link
Member

TallTed commented Sep 9, 2021

[@David-Chadwick] This PR can be dropped, because the issuer of a VC can publish its own schema for the contents of its VCs and the properties of the VC's subject. If the issuer wants the subject's properties to contain half a dozen different dates and times it can do this without affecting the VC data model or any other VC issuer.

As this is your PR, and all comments have been aimed at changing it in ways you disagreed with, it seems to me you should be the one to close it. Anyone who feels this PR or the comments to date have exposed an issue that still needs addressing, whether in VCDM or VCIG, can open such an issue referring back to here.

@David-Chadwick
Copy link
Contributor Author

As this is your PR, and all comments have been aimed at changing it in ways you disagreed with, it seems to me you should be the one to close it. Anyone who feels this PR or the comments to date have exposed an issue that still needs addressing, whether in VCDM or VCIG, can open such an issue referring back to here.

Please advise how to do this.
Kind regards
David

@TallTed
Copy link
Member

TallTed commented Sep 9, 2021

Near the bottom of the page for PR #48, there's a big green "Comment" button.

Next to it is a "Close pull request" (if you've not written a comment) or "Close with comment" (if you've written one) button.

Click it.


Every past comment has a "..." on the upper-right of the comment. Click it, and you'll get a menu that lets you "Reference in new issue". I think the rest should be self-explanatory.


That said, it seems follow-up discussion about this PR really belongs on the associated issue, #44, which is currently mostly empty, because its discussion landed here. I probably should have only put comments here that were about specific text changes, and raised my other concerns (that this advisory was mis-aimed) on the issue. We can revert to that going forward.

@David-Chadwick
Copy link
Contributor Author

Closed because issuers can insert any time properties into their issued VCs without any need to change the VC data model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unresolved Pending conflict resolution / consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple times
5 participants