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

Pex select creds #159

Merged
merged 23 commits into from
Dec 20, 2023
Merged

Pex select creds #159

merged 23 commits into from
Dec 20, 2023

Conversation

nitro-neal
Copy link
Contributor

@nitro-neal nitro-neal commented Dec 7, 2023

Overview

Selects credentials that satisfy a given presentation definition.

Description

This PR Implements a new features selectCredentials

This would be used by the user to select which credentials they want to use to satisfy a presentation definition

  /**
   * Selects credentials that satisfy a given presentation definition.
   *
   * @param vcJwts Iterable of VCs in JWT format to select from.
   * @param presentationDefinition The Presentation Definition to match against.
   * @return A list of Verifiable Credentials that satisfy the Presentation Definition.
   */
  public fun selectCredentials(
    vcJwts: Iterable<String>,
    presentationDefinition: PresentationDefinitionV2
  ): List<String> {
    val inputDescriptorToVcMap = mapInputDescriptorsToVCs(vcJwts, presentationDefinition)
    return inputDescriptorToVcMap.flatMap { it.value }.toSet().toList()
  }
  • Added unit tests
  • Added test vector test

@nitro-neal
Copy link
Contributor Author

Note this is built upon this pr - #151

this pr - #151 - should be merged in first and this would become much smaller

@nitro-neal nitro-neal marked this pull request as ready for review December 7, 2023 22:53
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #159 (8d45703) into main (466e8d8) will decrease coverage by 0.30%.
Report is 5 commits behind head on main.
The diff coverage is 54.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   77.43%   77.13%   -0.30%     
==========================================
  Files          33       33              
  Lines        1928     1929       +1     
  Branches      265      265              
==========================================
- Hits         1493     1488       -5     
+ Misses        301      297       -4     
- Partials      134      144      +10     
Components Coverage Δ
credentials 72.15% <100.00%> (-8.19%) ⬇️
crypto 38.07% <ø> (ø)
dids 89.59% <ø> (ø)
common 80.44% <ø> (+15.64%) ⬆️

@nitro-neal nitro-neal marked this pull request as draft December 8, 2023 14:14
@nitro-neal nitro-neal marked this pull request as ready for review December 11, 2023 16:27
@Test
fun select_credentials() {
val typeRef = object : TypeReference<TestVectors<SelectCredTestInput, SelectCredTestOutput>>() {}
val testVectors = mapper.readValue(File("../test-vectors/presentation_exchange/select_credentials.json"), typeRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be tests for the error cases ? Some examples I'm thinking about:

  1. UnsupportedOperationException is thrown
  2. When inputDescriptorToVcMap.size != presentationDefinition.inputDescriptors.size
  3. When JsonPathParseException is thrown.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I think some of the tests above (multiple input descriptors, filtering array vs filtering on value) could be included in the vectors as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes many more test vectors coming soon (tm),
I was doing this one simple success as a steel thread for the overall web5-kt + web5-js check marks in web5-sdk-development

@@ -427,4 +430,152 @@ class PresentationExchangeTest {
assertEquals("$.verifiableCredential[0]", presentationSubmission.descriptorMap[0].path)
}
}

@Nested
inner class SelectCredentials {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these tests unrelated to the vector tests?

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 these are just normal unit tests, testing some of the same things

}


class Web5TestVectorsPresentationExchangeTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt about extracting all the vector tests into a separate file so you can tell based on the file name whether it's a vector test?

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 thought did cross my mind.
You mean having a new test file called TestVectorTests.kt
and then having credentials test and pex and everything in the credentials package in there?

I could go either way, I do like having the test vector in the same file as the normal unit tests. Less overhead for the devs I think.

Also there would be a lot of imports in the TestVectorTests.kt file, would have to import all stuff from all packages basically

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah did mean something like that, but agree it does feel more natural to have the tests oriented around feature.

@nitro-neal nitro-neal merged commit fca2c14 into main Dec 20, 2023
4 of 8 checks passed
@nitro-neal nitro-neal deleted the pex-select-creds branch December 20, 2023 17:22
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

Successfully merging this pull request may close these issues.

3 participants