-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: evaluate multi-presentation submissions #153
feat: evaluate multi-presentation submissions #153
Conversation
Signed-off-by: Timo Glastra <timo@animo.id>
I would prefer when we include a breaking change, we release a new major version. Implementing old approaches will just blow up the code base and I would prefer to have higher major counts than having a lot of redundant code we have to maintain. A bump to a nest major version would solve the problem we have in #152 |
Agreed, but we could keep backwards compatibility and not introduce a breaking change, only adding new functionality. If that helps get this PR merged and release sooner, I'm happy to make those changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick first pass. Looks okay. Will do a more proper review soon. Some small initial remarks. We will have to go for a new major release given the external API changes (which is okay)
*/ | ||
public evaluatePresentation( | ||
presentationDefinition: IPresentationDefinition, | ||
presentation: OriginalVerifiablePresentation | IPresentation, | ||
presentations: OrArray<OriginalVerifiablePresentation | IPresentation>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means a new Major release. Which I am okay with btw
Hey @nklomp, any chance you can take a look at this PR? We would like to support multi VP presentations in Paradym, and are blocked by this PR (and a follow up PR I will make in SIOP-OID4VP after this is merged and released). |
Small nudge on this PR :) Anything i can help with to make it the reviewing easier? I can remove the breaking changes? |
Hi @TimoGlastra We will have a look at this either today or tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Going to merge and create a release later today, as I think multiple parties are waiting for this
This PR is a first step towards supporting multi-presentation submissions in PEX, and partially fixes #149 and also partially fixes Sphereon-Opensource/SIOP-OID4VP#62
This PR only implements support for multiple presentations in
evaluatePresentation
. If a submission is provided it will make sure presehtation(s) are validated against the submission. If no submission is provided, a submission (that supports multiple VPs) is generated.In a follow up PR i want to add the
(verifiable)PresentationFrom
support, but now that we support constructing a valid submission and validating against it, this should be quite straightforward to do. (create VPs, generate submission based on the VPs). But as the PR is already quite large, I didn't want to also add it here.For our use case, this is actually already enough, as we call
verifiablePresentationFrom
multiple times for each VP and then 'hack' the submission together from multiple submissions. But it would be great if we can remove that logic and support it natively in PEX.I added several tests to PEX.spec.ts to cover the new cases and flows
There is a breaking change in that
evaluatePresentation
now returns an object withpresentation
property instead ofverifiableCredential
. We could also return bothpresentation
andverifiableCredential
to not make a breaking change. In this case,verifiableCredential
would include ALL credentials (from all VPs) and presentation would be the separate presentation (it's the same as the input value). We could also make it a more complex structure (so presentaiton is decoded has verifiableCredential for each VP. Input welcome)cc @nklomp @cre8