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

Credential Exchange RFCs --> ACCEPTED - deadline 2019-08-19 #158

Merged
merged 3 commits into from
Aug 30, 2019

Conversation

KitHat
Copy link
Contributor

@KitHat KitHat commented Aug 6, 2019

Signed-off-by: Nikita Khateev nikita.khateev@dsr-corporation.com

Nikita Khateev added 2 commits August 6, 2019 11:41
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
Signed-off-by: Nikita Khateev <nikita.khateev@dsr-corporation.com>
@llorllale
Copy link
Contributor

llorllale commented Aug 6, 2019

I don't think this RFC is quite there yet:

  • Many aspects are still fuzzy with only an Indy example for illustration
  • A recent question has emerged: how does Alice ask Bob for a credential with claims about Eve?
  • I find it a bit strange how there isn't a flow where request-credential initiates the protocol

I'd first like to see some implementations added to the RFC before we proceed.

@swcurran
Copy link
Member

swcurran commented Aug 6, 2019

@llorllale - Other than the comment about implementations, I disagree with points in your assessment.

  • Agree that we need non-Indy implementations, but I don't think we should wait for that to accept the PR. I'm not aware of any activity in that area. I think there will be things that need to change when that happens, but that's why we have versioning. Until we have some real experience we don't know what is fuzzy.
  • I don't think the specific Alice/Bob/Eve (or Carol?) use case needs to be covered in the 1.0 use case. I suspect that if our team came across this use case, we would use the proposal message type to tell the issuer the specifics about the credential we wanted. And with real experience, might suggest a future change to the protocol.
  • I think it is pretty clear in the text that both the proposal and offer messages are optional. In the Indy case, an offer message is required, in other credential exchange models it might not be required. That's in the RFC now.

There are a number of implementations of the 0.1 protocol, and this one is not a big step up from that protocol. So while there aren't implementations, there are many that are very similar.

I think we should go ahead with this and then look at some possible other changes in a 1.1 version. This is what was discussed on a recent Aries Working Group call. This is core to what we are all trying to achieve and we have several years experience in doing credential exchange in Aries-like contexts.

@dhh1128 dhh1128 changed the title Change status of Credential Exchange RFCs to Accepted Change status of Credential Exchange RFCs to Accepted - deadline 2019-08-19 Aug 7, 2019
@dhh1128 dhh1128 changed the title Change status of Credential Exchange RFCs to Accepted - deadline 2019-08-19 Credential Exchange RFCs --> ACCEPTED - deadline 2019-08-19 Aug 7, 2019
@llorllale
Copy link
Contributor

@swcurran You're right. It just so happens the choreography diagrams - which occupy a lot of prime screen estate - do not depict that.

The other reason why this RFC feels so "crude" is due to the prevalence of references to a specific implementation in 4 out of 5 structures defined in this protocol. So instead of the RFC itself defending its design choices, it's effectively deferring to the specific implementation. Maybe that can be fixed with a change in language.

All this may be a misunderstanding on my part about how RFCs are "released". ACCEPTED just means:

An accepted RFC is incubating on a standards track; the community has decided to polish it and is exploring or pursuing implementation.

"Incubating" and "polish" are keywords that to me imply this RFC is not at 1.0 yet. It is unusual (though not unheard of) to have a 1.0 release still in incubating and polishing stage. And judging from your words, sounds like you expect moving this to ACCEPTED is equivalent to a 1.0 release?

@swcurran
Copy link
Member

swcurran commented Aug 7, 2019

Reasonable comments.

The pictures in the RFC get a mixed reaction - pretty on first glance, but really not that useful. I find them overly complex, and now that we need a different one for each optional message, they are over the top.

The use of the term "1.0" was just taken as the default when work started on the protocols (a year ago this week, interestingly) and it wasn't until much later someone said, should we be starting with 1.0? So don't put any weight in that number.

My thought is that the RFCs have not substantively changed in months, despite repeated discussions on calls and work on the code. Each call (after the first) has resulted in agreement with what we have and has meant just refinement of the wording/pictures. As such, I don't see NOT moving the RFCs forward as likely to make any difference. I could be convinced otherwise, but since this is the core of the VC model, it would be good to have teams building compatible code and further evolving from a position of experience. I think moving the status could lead to that.

@kdenhartog kdenhartog added the Status Blocked This issue or PR has something block it from upgrading the status of the RFC. label Aug 12, 2019
@TelegramSam TelegramSam removed the Status Blocked This issue or PR has something block it from upgrading the status of the RFC. label Aug 14, 2019
@kdenhartog
Copy link
Contributor

@TelegramSam I see you removed the status blocked tag, but we still have 3 issues open on this RFC ( #190 #162 and #145 ) Are these issues completed and just haven't been closed out yet?

@dhh1128
Copy link
Contributor

dhh1128 commented Aug 24, 2019

@kdenhartog Issues #190 and #162 are closed. I think the remaining issue should be deferred--not closed, but tackled in an update to the protocol. Let's get this change merged. What do you think?

@kdenhartog
Copy link
Contributor

@kdenhartog Issues #190 and #162 are closed. I think the remaining issue should be deferred--not closed, but tackled in an update to the protocol. Let's get this change merged. What do you think?

I'm good for that approach.

@swcurran
Copy link
Member

Can we merge this change? Objections from the community?

@dhh1128 @TelegramSam @llorllale @KitHat @esplinr @kdenhartog @tplooker @tmarkovski @andrewwhitehead

@dhh1128
Copy link
Contributor

dhh1128 commented Aug 29, 2019

I am in favor of merging. There is at least one open issue, but my vote is to defer it temporarily.

@llorllale
Copy link
Contributor

I am in favor of merging. But like Daniel said, expect a 1.1 soon-ish

@dhh1128
Copy link
Contributor

dhh1128 commented Aug 30, 2019

Given the fact that the deadline has passed with no naysaying other than the one ticket we're temporarily deferring, I say we merge. Do any maintainers feel otherwise?

@swcurran
Copy link
Member

Go for it. Please :-). I notice there are now conflicts, so I'll pretend I'm waiting for others to agree vs. just merging it myself...

@dhh1128
Copy link
Contributor

dhh1128 commented Aug 30, 2019

Okay, I'll resolve the conflict and merge. Give me a few minutes.

Signed-off-by: Daniel Hardman <daniel.hardman@gmail.com>
@dhh1128 dhh1128 merged commit 899d70c into hyperledger:master Aug 30, 2019
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.

6 participants