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

Simplify abstract and move credential types to introduction. #82

Merged
merged 9 commits into from
Feb 27, 2024

Conversation

msporny
Copy link
Contributor

@msporny msporny commented Feb 17, 2024

This PR performs a few editorial updates to the Abstract and Introduction in an attempt to simplify the former and make the latter convey more details around the types of credentials that are contemplated for use with the API.


Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
<li>
a driver's license issued by a testing facility
</li>
<li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm hesitant to include these at the moment because we haven't really looked at supporting these and I don't think any implementer supports these yet... though these could be supported, I'm worried about set expectations.

Copy link
Contributor Author

@msporny msporny Feb 22, 2024

Choose a reason for hiding this comment

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

I moved many of these examples down from the text that already existed in the abstract; I was attempting to preserve text that already existed. If you want to remove it, that's fine, though the utility of the API becomes less interesting for a variety of stakeholders that are watching this work closely.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to marcos. I understand you just moved the text, but mentioning passports in this file, either here or in the intro could be sensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the list down to the only credential type we have consensus on right now: 06271a7. Preserved the full list in #85 (comment).

index.html Outdated
<li>
a traveler's boarding pass issued by an airline
</li>
<li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably falls more on FedCM...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the list down to the only credential type we have consensus on right now: 06271a7. Preserved the full list in #85 (comment).

index.html Outdated
<li>
an employee identification card issued by a company
</li>
<li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This falls outside the scope, I think... we want just credentials that related to people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I was just preserving text that already existed:

https://github.com/WICG/digital-identities/pull/82/files#diff-0eb547304658805aad788d320f10bf1f292797b5e6d745a3bf617584da017051L49

If we want to put this out of scope, we can do that, but I know that this is a use case that is of interest for a number of stakeholders that are interested in using this API.

Copy link
Contributor

Choose a reason for hiding this comment

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

how is what in scope being decided/enforced? meaning, will the browser look into the request from the verifier and make a decision whether a requested credential type should be requested from the wallet or not? that sounds like a lot of discretion on the browser api...

Copy link
Collaborator

@marcoscaceres marcoscaceres Feb 23, 2024

Choose a reason for hiding this comment

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

Right now, I think it will be determined by the supported request protocol/format/query. That's not to say that it won't eventually support a larger set of things, but the browser might get a peek at what's being requested (at least for some formats).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the list down to the only credential type we have consensus on right now: 06271a7. Preserved the full list in #85 (comment).

index.html Outdated
Comment on lines 91 to 75
<li>
a healthcare record issued by a hospital, and
</li>
<li>
a professional license issued by a trade association.
</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with these two... these might end up being supported... but they are a bit murky right now.

Copy link
Contributor Author

@msporny msporny Feb 22, 2024

Choose a reason for hiding this comment

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

First one is same as comment in: https://github.com/WICG/digital-identities/pull/82/files#r1499358644

2nd item is a use case that is discussed often in the Verifiable Credentials for Education community. /cc @dmitrizagidulin @longpd @kayaelle

Generally speaking, I'm not suggesting we keep/remove any to any of these examples... wherever consensus lands is fine with me. I'm just noting that this text already existed and I didn't want to remove it entirely -- was meaning this PR to be a purely editorial PR.

If you want to make some concrete change suggestions for addition/removal, I could process them. We'd need to hear from others... perhaps the way to go about this is to see if there would be any objections to removing credential types? ... or, the other way to go about it is to see which credential types we have consensus to support.

Copy link
Collaborator

@marcoscaceres marcoscaceres Feb 23, 2024

Choose a reason for hiding this comment

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

Right, I wouldn't read anything into "text was already there", as it's mostly just my initial ramblings.... which are now reflected back at me 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the list down to the only credential type we have consensus on right now: 06271a7. Preserved the full list in #85 (comment).

index.html Outdated Show resolved Hide resolved
@@ -64,6 +62,39 @@ <h2>
<p>
TBW
</p>
<p id="credential-type-examples">
Copy link
Contributor

Choose a reason for hiding this comment

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

in general, share a lot of the same sentiment as expressed by @marcoscaceres. One suggestion would be to remove any specific references to the credential types in this PR, and than re-add them once we have more agreement in issue #85

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the list down to the only credential type we have consensus on right now: 06271a7. Preserved the full list in #85 (comment).

Copy link
Contributor

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

probably better to have a discussion in issue #85 first to understand what kind of credential types implementers are interested in and discuss if there will be a list of supported/unsupported credential types and if so, how supported credential types will be enforced...

index.html Outdated Show resolved Hide resolved
@msporny msporny force-pushed the update-abstract branch 2 times, most recently from 06271a7 to 1244a0c Compare February 25, 2024 22:32
@msporny
Copy link
Contributor Author

msporny commented Feb 25, 2024

I've applied suggested changes from @marcoscaceres @Sakurann and @TallTed. Please re-review.

index.html Outdated
Comment on lines 46 to 47
access to, and present, a digital credential such as a driver's license,
a government-issued identification card, or [=credential type
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it reads better with all plural (or with all singular, but that rephrasing would be harder to write). Also, I think this API is meant to allow presenting multiple credentials (or selective-disclosure portions thereof), so and/or.

Suggested change
access to, and present, a digital credential such as a driver's license,
a government-issued identification card, or [=credential type
access to, and present, digital credentials such as driver's licenses,
government-issued identification cards, and/or [=credential type

Copy link
Collaborator

Choose a reason for hiding this comment

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

My only reservation was that generally people only have one driver's license, passport etc., but it's certainly possible that they could have multiple of other things.

I don't mind though... @msporny, I'll leave the choice up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attempted to address @marcoscaceres and @TallTed's concerns in 100094d.

Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

Looks generally good to me, modulo minor comments!

Thanks for putting this together!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Approving with @samuelgoto suggestions.

@msporny
Copy link
Contributor Author

msporny commented Feb 27, 2024

@samuelgoto wrote:

Looks generally good to me, modulo minor comments!

@marcoscaceres wrote:

Approving with @samuelgoto suggestions.

I've applied all of @samuelgoto, @TallTed, and @marcoscaceres change suggestions.

We're good to merge here if the Editors agree.

@marcoscaceres marcoscaceres merged commit 0a84e70 into WICG:main Feb 27, 2024
1 check passed
@marcoscaceres
Copy link
Collaborator

Amazing! thanks so much @msporny for your time with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants