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

Add refresh status purpose. #185

Merged
merged 3 commits into from
Nov 30, 2024
Merged

Add refresh status purpose. #185

merged 3 commits into from
Nov 30, 2024

Conversation

msporny
Copy link
Member

@msporny msporny commented Nov 24, 2024

This PR is an attempt to address issue #183 by adding a refresh status purpose.


Preview | Diff

@msporny msporny added normative The item is normative in nature. CR1 This item was processed during the first Candidate Recommendation phase. labels Nov 24, 2024
Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

The new property should be added, I presume, to the bitstring vocabulary document, and to the diagram include therein.

It is o.k. if this is handled in a separate Issue+PR, but it ought to be done. Please raise an issue if you prefer to handle it separately.

Similarly, shouldn't the relevant context file be changed?

(I am happy to do the changes on the vocabulary... eventually. I am not at my computer right now, it would be difficult to do it on my tablet...)

@msporny
Copy link
Member Author

msporny commented Nov 25, 2024

The new property should be added, I presume, to the bitstring vocabulary document, and to the diagram include therein.

Nope, it's not a term, it's an arbitrary string. :)

@PatStLouis
Copy link
Contributor

I think we can have the purpose added and explore discussions around if it needs an additional defined property separately. I think if a new term needs to be defined it would be more towards an associated refreshService, which can live in a separate context.

@msporny Would it be reasonable to add to this text that this refresh status doesn't have an effect on the validity of the current credential, meaning it doesn't become revoked because there's an update available, simply signals that there's an update. Revocation should still be managed by a separate status entry. If there's a "breaking update", the credential would get updated + revoked. If its a "soft" (meaning a minor change) or "additive"(adding a data element) update, the credential gets updated, but the consumer of the credential can still use the non updated version without concern (they don't need to get the updated version). If its also revoked then they would need to get the updated version if they want to continue using the data for their services.

I think just a sentence that the refresh status doesn't invalidate the current credential.

@TallTed
Copy link
Member

TallTed commented Nov 25, 2024

simply signals that there's an update

Except that it doesn't signal that there's an update. It signals that there's a place to ask for any update that might be available.

@PatStLouis
Copy link
Contributor

PatStLouis commented Nov 25, 2024

@TallTed the purpose is that you would resolve the index of the refresh status, 0 means no update available and 1 means an update is available you MAY hit the refresh endpoint, so it does signal that there's an update available.

The original purpose label was "supersession", meaning 0 means the credential isn't superseded, and 1 the credential has been superseded. The suggestion to make the purpose label refresh is to explicitly signal that this supersession/amendment/update or however we call this be tied to a refreshService instead of being an out of band process (which could still be allowed, but better to tie it to a resfreshService)

A refreshService on its own would do what you describe, the goal of this status purpose is to "notify" any party consuming this credential that an update is available.

@iherman
Copy link
Member

iherman commented Nov 26, 2024

The new property should be added, I presume, to the bitstring vocabulary document, and to the diagram include therein.

Nope, it's not a term, it's an arbitrary string. :)

Oops, sorry.

(For some reasons preview does not work, and I have only my ipad with me where I am this week, so it was difficult to see the exact context of the changes. My approval holds :-)

Copy link
Contributor

@brianorwhatever brianorwhatever left a comment

Choose a reason for hiding this comment

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

Something like this to address the comments?

Copy link

@longpd longpd left a comment

Choose a reason for hiding this comment

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

I approve this PR with the amended language it contains.

@msporny
Copy link
Member Author

msporny commented Nov 30, 2024

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

@msporny msporny merged commit 886b5e8 into main Nov 30, 2024
2 checks passed
@msporny msporny deleted the msporny-status-refresh branch November 30, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 This item was processed during the first Candidate Recommendation phase. normative The item is normative in nature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants