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

Change verifiable_credential to type Vec<CRED> in Presentation #1231

Merged
merged 10 commits into from
Sep 11, 2023

Conversation

nanderstabel
Copy link
Collaborator

@nanderstabel nanderstabel commented Sep 5, 2023

Description of change

This change ensures that verifiable_credential in Presentation will serialize into a [T] instead of a T, which was previously the case when verifiable_credential would only hold one single credential.

This previous serialization is in line with how credentialSubject can be serialized: https://www.w3.org/TR/vc-data-model/#credential-subject, however this is not explicitly stated for verifiableCredential: https://www.w3.org/TR/vc-data-model/#presentations-0.
The following examples that are used in the specification seem to imply that 'single credentials' should be serialized as [<credential>]:

The W3C VC Data Model 2.0 specification fortunately is more specific in this regard: https://www.w3.org/TR/vc-data-model-2.0/#presentations-0:

verifiableCredential
The verifiableCredential property MAY be present. The value MUST be an array of one or more verifiable credentials, or of data derived from verifiable credentials in a cryptographically verifiable format.

Therefore the introduction of the change in this PR.

Important detail:
Both W3C VC Data Model versions state that: MAY be present and **If** present..., which imply that verifiableCredential is an optional property. I fail to think of any use case where one would have a presentation without any credentials included but this optional status of verifiableCredential is included in this PR as well.

Links to any relevant issues

n/a

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

  • Added unit test: test_presentation_deserialization which tests deserializing from an example verifiable presentation (with some minor adjustments): https://www.w3.org/TR/vc-data-model/#example-a-simple-example-of-a-verifiable-presentation
  • Added unit test: test_presentation_deserialization_without_credentials which tests deserializing a verifiable presentation as well, but one without any verifiableCredential.
  • Added unit test: test_presentation_builder_valid_without_credentials which tests building a Presentation without any credentials set.

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@eike-hass eike-hass added Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Sep 5, 2023
@nanderstabel nanderstabel changed the title WIP: replace OneOrMany with Vec Change verifiable_credential to type Vec<CRED> in Presentation Sep 5, 2023
@nanderstabel nanderstabel marked this pull request as ready for review September 5, 2023 15:52
@nanderstabel nanderstabel force-pushed the fix/verifiable-credential-as-vec branch from daa0cd8 to 2b02872 Compare September 6, 2023 17:50
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks again 🙏.

I'll approve this with additional comments to mean that I leave it to your judgement whether to implement them or not.

identity_credential/src/error.rs Outdated Show resolved Hide resolved
identity_credential/src/presentation/presentation.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@abdulmth abdulmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for putting in the effort.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Wasm examples CI workflow ran for 2:50h but didn't finish until then so I cancelled it. Had this before at some point but a rerun did the trick back then. I haven't restarted in case you want to fix the comment, otherwise just rerun the workflow.

identity_credential/src/presentation/presentation.rs Outdated Show resolved Hide resolved
@nanderstabel
Copy link
Collaborator Author

@PhilippGackstatter @abdulmth thank you both for reviewing! I made the last suggested change to the unit test. Hopefully the CI workflow won't have any issues :)

@PhilippGackstatter
Copy link
Contributor

@nanderstabel Our convention is to let the PR creator hit the merge button, so feel free to go ahead whenever you want. Thanks again 🚀!

@nanderstabel
Copy link
Collaborator Author

Great, thanks again!

@nanderstabel nanderstabel merged commit 54a86cf into main Sep 11, 2023
10 checks passed
@nanderstabel nanderstabel deleted the fix/verifiable-credential-as-vec branch September 11, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants