Add extensibility to Credential Response

c2bo commented 2 months ago

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

c2bo commented 2 months ago

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 commented 2 months ago

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

c2bo commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

I must say credentials.credential looks weird....

{
  "credentials": [
    {  "credential": "omppc3N1ZXJBdXRohEOhASahG...ArQwggKwMIICVqADAgEC"   }
  ]
}

c2bo commented 2 months ago

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 commented 1 month ago

good to merge once @awoie approves

openid / OpenID4VCI

Add extensibility to Credential Response #391