Conversation
tplooker
left a comment
tplooker
left a comment
There was a problem hiding this comment.
Minor editorial review, generally very supportive of this proposal I think its a critical feature. Few other thoughts
- We should consider making this feature required as leaving it optional will make communicating credential updates/refreshes difficult.
- I believe the specification would benefit from a seperate additional endpoint that enables a wallet to ask if there are any updates for a specific credential. Otherwise without this a wallet is forced to ask for a new credential in order to determine whether anything has changed.
|
Only other thing that came to mind on this topic that perhaps we need to discuss is how we support different datasets versus different versions of the same dataset as I suspect in the event an issuer is issuing two different datasets for the same credential (e.g two credentials about different people), to the same wallet this identifier would become ambiguous. |
|
temporarily close to prevent confusion - will reopen once 1.0 goes out |
|
reopening now that 1.0 has been published. Please push the changes to 1.1.md, and not 1.0.md |
1af74c9 to
5846457
Compare
@tplooker If the same credential configuration is used for two different initial data sets, then you would need some additional mechanism. Wouldn't this be rather two distinct credential configurations, e.g., child, parent configuration? We could also introduce another layer between credential configuration and credential dataset identifier (version)? Is there a third option and do you have a proposal, e.g., through some new endpoint? |
Sakurann
left a comment
Sakurann
left a comment
There was a problem hiding this comment.
I think it would be good to add a bit more description of the feature this parameter enables outside the definition of a term?
That issue hasn't been raised yet I think? |
Co-authored-by: Joseph Heenan <joseph@heenan.me.uk>
|
Discussed in WG today: @brentzundel will raise the issue mentioned. |
|
FYI #802 created. |
Co-authored-by: Christian Bormann <chris.bormann@gmx.de>
| * `notification_id`: OPTIONAL. String identifying one or more Credentials issued in one Credential Response. It MUST be included in the Notification Request as defined in (#notification). It MUST not be used if the `credentials` parameter is not present. | ||
| * `credential_metadata`: OPTIONAL. Object that contains additional metadata specific to the issued Credential(s). The definitions and contained parameters for this Object are identical to the `credential_metadata` parameter as defined in Credential Issuer Metadata (see (#credential-issuer-metadata)) See (#display-metadata-considerations) for implementation considerations on credential metadata. | ||
| * `credential_dataset_id`: RECOMMENDED. A string containing the Credential Dataset Identifier of the Credential Dataset from which the returned Credential(s) were issued. Together with the `credential_dataset_version` parameter, it allows Wallets to determine if previously received Credentials may be superseded. See (#credential-dataset-identifier-implementation) for implementation considerations. | ||
| * `credential_dataset_version`: RECOMMENDED. A string containing the Credential Dataset Version associated with the returned Credential(s). This allows Wallets to detect changes to the underlying Credential Dataset across different Credential Responses. It MUST be present if `credential_dataset_id` is present and not be present otherwise. See (#credential-dataset-identifier-implementation) for implementation considerations. |
There was a problem hiding this comment.
@c2bo I added your suggested text, but removed your suggestion to change to OPTIONAL, as it did not match the RECOMMENDED in credential_dataset_id. Please re-discuss if you think OPTIONAL makes sense
There was a problem hiding this comment.
@paulbastian It seems @c2bo approved after changing it to RECOMMENDED, so this should be fine.
c2bo
left a comment
c2bo
left a comment
There was a problem hiding this comment.
We probably want to add similar text that we already have with notification_id for both values that makes clear these values should only co-exist with a credential response - something like this
It MUST not be used if the
credentialsparameter is not present.
otherwise looks good to me now
fkj
left a comment
fkj
left a comment
There was a problem hiding this comment.
Generally looks fine, but some editorial nits.
Co-authored-by: Frederik Krogsdal Jacobsen <fkj@users.noreply.github.com>
Co-authored-by: Frederik Krogsdal Jacobsen <fkj@users.noreply.github.com>
Co-authored-by: Frederik Krogsdal Jacobsen <fkj@users.noreply.github.com>
Co-authored-by: Frederik Krogsdal Jacobsen <fkj@users.noreply.github.com>
Co-authored-by: Frederik Krogsdal Jacobsen <fkj@users.noreply.github.com>
Co-authored-by: Frederik Krogsdal Jacobsen <fkj@users.noreply.github.com>
Addresses c2bo's review comment: mirror the `notification_id` presence rule for the Credential Dataset parameters, and apply fkj's remaining "may be superseded" -> "are superseded" suggestion. - `credential_dataset_id` is RECOMMENDED only when `credentials` is present, and MUST NOT be used otherwise. `credential_dataset_tag` is already tied to `credential_dataset_id`, so it inherits the rule. - The Deferred Credential Response said unconditionally that it SHOULD use both parameters, which also covered the 202 "more time needed" branch that carries no Credentials. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pre-existing casing slips on the two neighbouring Credential Response parameters. Separate commit so it can be dropped if reviewers would rather keep this PR's diff minimal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses Joseph's review comment asking whether these fields are intended to be user visible. Explanatory only, no new normative requirement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Use RECOMMENDED rather than SHOULD so the requirement level matches `credential_dataset_id`/`credential_dataset_tag` in (#credential-response) verbatim, and drop the restated `credentials` condition, which the MUST NOT in the parameter definition already covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MUST NOT already scopes the parameter to responses carrying `credentials`, matching how `notification_id` is specified, and the Deferred Credential Response now reads RECOMMENDED unconditionally too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@c2bo Applied your suggestion on "if credentials param is present". |
Done |
|
discussed today, additional text in privacy considerations as in #472 (comment) is needed From @awoie Credential Issuers that want to avoid this correlation factor SHOULD assign a different Credential Dataset Identifier per Wallet, and avoid deriving the Credential Dataset Tag solely from the content of the Credential Dataset, for example by computing a hash over it, since such a derivation yields identical values across Wallets." |
|
Discussed today. Change the text to:
Oliver will also check if there is already a general restriction that applies to his first point here: #472 (comment) in general Wallet -> Wallet Provider |
This branch has not been deployed
10 participants
Fixes #278