Add extensibility to Credential Response

[c2bo]

Option 2 as introduced by @paulbastian and discussed in #386:

• Credential Response always returns an array
• This array holds an object with the credential and additional (optional) metadata
• notification_id is also moved into this new object
• deferred Endpoint re-uses this array for its response

Closes #386
Closes #58

[paulbastian]

Shouldn't we have a single notification_id for the whole response?

[jogu]

I've been struggling with the same question for a few days:

A notification_id for each credential in the batch is cleaner/simpler, as we don't need to worry about partial states (i.e. the question of what notification to make if you receive 10 credentials and only manage to store 5 of them).

However it's kindof sucky to make 10 notification http calls for a batch of 10 credentials.

I'm probably veering towards cleaner/simpler and ignoring the 'kind of sucky' part.

[paulbastian]

We may change the notification request to an array as well?

[jogu]

I think that could make a lot of sense.

[babisRoutis]

FWIW, I think that notification_id should point to a single credential not in a batch.

For instance, an Issuer can react to a notification event carrying a credential_deleted or credential_failure to invalidate the credential from a status list.

We can avoid multiple posts for notifications by using an array of notification_id per possible event.

{
  "credential_accepted" : [ "notId#1"],
  "credential_failure" : ["notid#4"],
  "credential_deteled": [ ]
}

Something like a summary notification

[paulbastian]

I'm also okay with moving notification_id in

[awoie]

I think it might be useful to move it in because of credential_deleted which is specific to one issued credential although when I think about it, notification_id overlaps a bit with the concept of credential version IDs or data set IDs but this is out of scope of this PR.

[paulbastian]

Why would a wallet delete a single instance rather than the whole batch or the same Credential Dataset?

[Sakurann]

maybe because the user has used that one instance? but not sure that information needs to be communicated to the issuer real-time, since that would allow the issuer to get more information about when credential is used

[paulbastian]

Communicating this to the issuer seems like a privacy violation, in most ecosystems issuers should not get knowledge about the usage of their credentials, so I don't think this is a valid use case for notification_id in the array. As was discussed in the last DCP call, I would favor leaving it out

[paulbastian]

I'm not entirely sure about notification_id, but apart from that, looks good to me! We also figured out that there may be cases around revocation that may benefit from this in the future.

[c2bo]

How should we proceed with the notification endpoint? If we want to change that one to expect an array, should this happen in this PR or should we create another issue/PR?

[paulbastian]

I would say it makes sense in this PR, as this one also changes that the behavior of notification_id in the credential response

[awoie]

I'm ok with the direction of this PR. I believe it is useful that notification_id is moved in too. Although I believe that notification_id should be potentially renamed in the future to credential version ID or credential data set ID.

[awoie]

When I think about this another time, I also that a single notification id would be more logical When I approved, I thought it would also allow for different data sets. If the use case is really just different keys, not data sets such as math-exam vs science-exam, I think it should be a top-level notification id instead of individual notification ids.

[c2bo]

discussed on the DCP WG call (17/09):
People seem to prefer notification_id as a top level parameter and convey only the overall issuance process (batch or not)

• notification endpoint can remain unchanged
• we need a bit more language on how to deal with partial failures
  I'll adjust the PR accordingly and mark it as ready for review

[babisRoutis]

discussed on the DCP WG call (17/09): People seem to prefer notification_id as a top level parameter and convey only the overall issuance process (batch or not)

* notification endpoint can remain unchanged

* we need a bit more language on how to deal with partial failures
  I'll adjust the PR accordingly and mark it as ready for review

The notification_id represents a notification (event) that can be send from the wallet.

Currently (d13,d14), such a notification event is related to a single - issued - credential, hence the event types names: credential_accepted, credential_failed, credential_deleted.

Should we change notification_id to be applicable for the credentials array, I think we should also change the names of the events and their description, to be consistent.

For instance , credentials_accepted, credentials_failed, credentials_deleted.

BTW, I don't understand very well what's the difference between failed & deleted credentials.

[babisRoutis]

Please consider keeping consistent the granularity of notification (event) names with the notification_id scope.

Keeping notification_id as a top-level item of credential response, means that a notification is related to the whole array of credentials.

Perhaps the names credential_accepted, credential_failed and credential_deleted should be in plural.

[babisRoutis]

discussed on the DCP WG call (17/09): People seem to prefer notification_id as a top level parameter and convey only the overall issuance process (batch or not)

It is not very clear (to me) what the rules for this top-level attributes would be.
I guess there are two options:
a) notification_id can be provided only if there is the credentials array
b) notification_id can be provided in any case (either when credentials or transaction_id is present.)

I think (a) is more clear because you get the notification_id together with your credentials both in the simple and deferred cases.

(Sorry for the multiple comments)

[jogu]

@babisRoutis

For instance , credentials_accepted, credentials_failed, credentials_deleted.

BTW, I don't understand very well what's the difference between failed & deleted credentials.

From memory, "failed" is "a technical failure somewhere, e.g. wallet thinks the credential is invalid" and "deleted" is more along the lines of an operation explicitly performed by the user (so e.g. the wallet shows the user the issued credential, and the user decides they don't want to actually store it). If this needs to be clarified in the spec I'd suggest a new issue is opened for that.

[c2bo]

I've tried to add all of the discussed changes. Two points that I am uncertain about:

• I agree that the naming of the events is not a 100% fit now, but am not sure if we should change them at this state. I decided to keep them as is and explain the meaning in the description
• Partial Issuance Errors seem weird and I am not sure if the current language is sufficient

[Sakurann]

not sure why this PR modifies deferred endpoint to accept notification_id? was it meant to say transaction_id?

also would like to see some descriptions to be more precise

[c2bo]

not sure why this PR modifies deferred endpoint to accept notification_id? was it meant to say transaction_id?

It is modifying the Deferred Credential Response not the Request. The idea would be that notification_id is always returned when the wallet receives credentials.

In the case of deferred issuance that means that the credential endpoint returns a transaction_id and the deferred credential endpoint then returns credentials and notification_id

[Sakurann]

good to merge once @awoie approves

[nemqe]

if the response is always a list containing a credentials key, does it make sense to rename these event to use the plural form like credentials_accepted...

Maybe you had this discussion already somewhere so please feel free just to disregard this comment