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

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

Open
gmulhearn-anonyome opened this issue Dec 19, 2023 · 8 comments

Comments

@gmulhearn-anonyome
Copy link

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

@gmulhearn-anonyome gmulhearn-anonyome changed the title seeking clarity on nullability of credential_preview in issue-credential-v2 offers (Aries RFC 0453) Clarity on nullability of credential_preview in issue-credential-v2 offers (Aries RFC 0453) Dec 20, 2023
@swcurran
Copy link
Member

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

@TimoGlastra
Copy link
Member

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
Copy link
Member

swcurran commented Dec 21, 2023

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
Copy link

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
Copy link
Member

💯% 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
Copy link
Member

swcurran commented Jan 2, 2024

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
Copy link
Member

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
Copy link
Member

swcurran commented Jan 8, 2024

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.

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

No branches or pull requests

4 participants