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.

Sign up for GitHub

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

Jump to bottom

Implement createPresentationFromCredentials #151

Merged
merged 13 commits into from
Dec 11, 2023
Merged

Implement createPresentationFromCredentials #151

merged 13 commits into from
Dec 11, 2023

Conversation

nitro-neal
Copy link
Contributor

@nitro-neal nitro-neal commented Nov 30, 2023 •
edited
Loading

Overview

Implement createPresentationFromCredentials for full presentation exchange

Description

Creates a Presentation Submission against a list of Verifiable Credentials (VCs) against a specified Presentation Definition.

Address this #134 and this #135

How Has This Been Tested?

New unit tests, test vector in the works

  /**
   * Creates a Presentation Submission against a list of Verifiable Credentials (VCs) against a specified
   * Presentation Definition.
   *
   *
   * @param vcJwts Iterable of VCs in JWT format to validate.
   * @param presentationDefinition The Presentation Definition V2 object against which VCs are validated.
   * @return A PresentationSubmission object.
   * @throws UnsupportedOperationException if the presentation definition contains submission requirements.
   * @throws IllegalStateException if no VC corresponds to an input descriptor or if a VC's index is not found.
   * @throws PresentationExchangeError If the number of input descriptors matched is less than required.
   */

@nitro-neal nitro-neal marked this pull request as ready for review November 30, 2023 18:24
@codecov Codecov
Copy link

codecov bot commented Nov 30, 2023 •
edited
Loading

Codecov Report

Merging #151 (e095bc9) into main (4c1f29f) will increase coverage by 0.16%.
The diff coverage is 90.32%.

❗ Current head e095bc9 differs from pull request most recent head 5dc2ca7. Consider uploading reports for the commit 5dc2ca7 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   77.46%   77.62%   +0.16%     
==========================================
  Files          31       30       -1     
  Lines        1890     1908      +18     
  Branches      262      264       +2     
==========================================
+ Hits         1464     1481      +17     
+ Misses        294      291       -3     
- Partials      132      136       +4
Components Coverage Δ
credentials 79.60% <90.32%> (+0.54%) ⬆️
crypto 38.07% <ø> (ø)
dids 89.67% <ø> (+0.07%) ⬆️
common 64.80% <ø> (ø)

@nitro-neal nitro-neal changed the title Validate def Implement createPresentationFromCredentials Nov 30, 2023
andresuribe87
andresuribe87 approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Some minor comments below

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt Outdated Show resolved Hide resolved
credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt
DescriptorMap(
id = inputDescriptor.id,
path = "$.verifiableCredential[$vcIndex]",
format = "jwt_vc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use Format as the type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since the input to the function is vcJwts: Iterable, we know that he descriptor map will be jwt_vc mapping

credentials/src/main/kotlin/web5/sdk/credentials/model/PresentationSubmission.kt Outdated
@JsonInclude(JsonInclude.Include.NON_NULL)
public data class DescriptorMap(
val id: String,
val format: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use Format as the type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the format is a string int he DescriptorMap where it is an object in the inputDescriptor - https://identity.foundation/presentation-exchange/#presentation-submission

credentials/src/main/kotlin/web5/sdk/credentials/model/PresentationSubmission.kt Outdated Show resolved Hide resolved
credentials/src/main/kotlin/web5/sdk/credentials/model/PresentationSubmission.kt Outdated Show resolved Hide resolved
credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt
Comment on lines +316 to +322
assertNotNull(presentationSubmission.id)
assertEquals(pd.id, presentationSubmission.definitionId)

assertEquals(1, presentationSubmission.descriptorMap.size)
assertNotNull(presentationSubmission.descriptorMap[0].id)
assertEquals("jwt_vc", presentationSubmission.descriptorMap[0].format)
assertEquals("$.verifiableCredential[0]", presentationSubmission.descriptorMap[0].path)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about asserting against a presentation submission loaded from a json file? Would be nice for test-vectors, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a test vector PR tee'd up after this, so we can hav ea clean PR that shows how to add test vector tests

This was referenced Dec 7, 2023
Merged
Closed
Closed
phoebe-lew
phoebe-lew reviewed Dec 11, 2023
View reviewed changes
credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt

@Test
fun `throws when VC does not satisfy sanctions requirements`() {
val pd = jsonMapper.readValue(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to read this from the hosted vectors in a follow up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup we will be doing the same stuff we do here in this PR decentralized-identity/web5-js#344

@nitro-neal nitro-neal merged commit 6273780 into main Dec 11, 2023
2 of 4 checks passed
@nitro-neal nitro-neal deleted the validate-def branch December 11, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phoebe-lew phoebe-lew phoebe-lew approved these changes

@andresuribe87 andresuribe87 andresuribe87 approved these changes

@mistermoe mistermoe Awaiting requested review from mistermoe mistermoe is a code owner

@tomdaffurn tomdaffurn Awaiting requested review from tomdaffurn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nitro-neal @andresuribe87 @phoebe-lew