hyperledger / aries-rfcs

Hyperledger Aries is infrastructure for blockchain-rooted, peer-to-peer interactions
https://hyperledger.github.io/aries-rfcs/
Apache License 2.0
323 stars 217 forks source link

Clarity on nullability of `credential_preview` in issue-credential-v2 offers (Aries RFC 0453) #806

Open gmulhearn-anonyome opened 7 months ago

gmulhearn-anonyome commented 7 months ago

RFC: https://github.com/hyperledger/aries-rfcs/blob/main/features/0453-issue-credential-v2/README.md

For credential_preview in the issue-credential-v2 proposals, the following statement is made:

an optional JSON-LD object that represents the credential data that Prover wants to receive. It matches the schema of Credential Preview.

Emphasis on optional.

For credential_preview in offers, the following statement is made:

a JSON-LD object that represents the credential data that Issuer is willing to issue. It matches the schema of Credential Preview;

Emphasis on the lack of optional. Generally, every optional field is stated so in the Aries RFCs, and logically every other field is mandatory. Which leads me to believe that credential_preview in the issue-credential-v2 offer is indeed mandatory.

However this does not match some of the practical implementations out there. The implementations state it as optional:

Note; I'm not really familiar with AFJ's approach to this, but i think AFJ will handle null/undefined credential_previews, but it prefers when it is defined. e.g. AFJ has this code when creating an offer:

    if (!credentialPreview) {
      // If no preview attributes were provided, use a blank preview. Not all formats use this object
      // but it is required by the protocol
      credentialPreview = new V2CredentialPreview({
        attributes: [],
      })
    }

In Aries-VCX we have currently implemented it as a non-nullable field (i was following the spec rather than implementation when making these Aries-VCX message structures), so we get deserialization issues when deserializing ACApy cred offers WITHOUT a cred preview.

My gut tells me that maybe the RFC should have the credential_preview as nullable. Since the credential_preview structure is not really applicable to some formats (e.g. for aries LDP / w3c it doesn't really make sense to have a credential_preview structure).

Also, coming from a perspective that credential_previews only really apply to hlindy/anoncreds credentials, I wonder in general if the credential_preview is better suited to be embedded in hlindy/anoncreds attachments rather than being a field on the base level of offers/proposals.

Anyway, i guess I'm seeking clarity on what the true type of credential_preview is in theory, and maybe some developer recommendations for what we should implement in practice. Cheers

swcurran commented 7 months ago

@dbluhm — could you comment from the ACA-Py side? Should ACA-Py be changed to make it required/nullable?

TimoGlastra commented 7 months ago

We've made it nullable as it doesn't make sense when issuing W3C credentials, and there's no disadvantage in making it optional AFAIK.

swcurran commented 7 months ago

Dumb PHB question time. Making it nullable mean that it is required but can be “[]”, correct? So we need to update ACA-Py to be the same — not optional, but default to “[]”? That gets us into alignment. And we can/should clarify the RFC accordingly.

quinndevos-anonyome commented 6 months ago

I believe the most correct answer is found in the second last paragraph of the OP's question.

Also, coming from a perspective that credential_previews only really apply to hlindy/anoncreds credentials, I wonder in general if the credential_preview is better suited to be embedded in hlindy/anoncreds attachments rather than being a field on the base level of offers/proposals.

I believe credential_preview should be moved inside the anoncreds, indy and dif attachment formats. Doing this would give parity between those attachment formats and the w3c attachment format, as all formats would then be responsible for defining their own attribute key:value pairs.

For reference, the attachment formats mentioned above are listed here.

My reasoning: Currently, the w3c attachment format defines its own attribute key:value pairs which means credential_preview could potentially contain redundant or [worse] contradictory information. Additionally, it doesn't make sense to me why attribute key:value pairs are part of the "credential preview" meanwhile the definition of those attribute keys (in the schema and credDef) are not part of the "credential preview".

TimoGlastra commented 6 months ago

💯% agree with you @quinndevos-anonyome.

The reason this wasn't done when e.g. the w3c attachment format was defined is that we didn't want to make a breaking change.

Then v3 of the protocol was defined as part of WACI-PEX outside the Aries RFC ecosystem and so there wasn't really a chance to incorporate lessons learned in v3 of the protocol

swcurran commented 6 months ago

Credential Preview is not needed in AnonCreds and that is not why it is in the Issue Credentials protocol messages. It was added as a “general purpose” mechansim to allow an issuer and holder to negotiate about what data would be issued. As such, moving it to the AnonCreds (‘hlindy’) attachments does not make any sense.

Its current place is where I think it makes sense — independent of any credential format, and optional, available for use if needed for negotiating an issuance.

TimoGlastra commented 6 months ago

It was added as a “general purpose” mechansim to allow an issuer and holder to negotiate about what data would be issued. As such, moving it to the AnonCreds (‘hlindy’) attachments does not make any sense.

But as it has turned out, the general purpose structure caters very well to AnonCreds/Indy type of credentials (only one level, no complex value types) and doesn't transfer very well to any other credential format that I'm aware of.

So saying it's a general purpose way to do things is misleading in my opinion, and I think it only adds confusion to label it as a general purpose way to do this. IMO previewing credential data is highly dependant on the actual credential format

swcurran commented 6 months ago

But as it has turned out, the general purpose structure caters very well to AnonCreds/Indy type of credentials (only one level, no complex value types) and doesn't transfer very well to any other credential format that I'm aware of.

Got it — agreed.