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: ldp_vp submission with 1 VC should not index credential property #2624

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

reinkrul
Copy link
Member

@reinkrul reinkrul commented Nov 28, 2023

go-did marshals verifiableCredential to a JSON object instead of an array in that case.

@reinkrul reinkrul changed the title PEX: ldp_vp with 1 VC should not index credential property PEX: ldp_vp submission with 1 VC should not index credential property Nov 28, 2023
@gerardsn
Copy link
Member

The VC v1.1 spec is not clear if this can be an object, but all examples are an array. The v2 spec states that it MUST be an array if the field is present. VerifiablePresentation.VerifiableCredential is also always an array in go-did. Am I misinterpreting what you want to fix here?

@gerardsn
Copy link
Member

gerardsn commented Nov 28, 2023

ahh... I think the Unplural in go-did marshaling for VCs in a VP is the bug. Have we used VPs in production yet? If not we can probably just fix that

@reinkrul
Copy link
Member Author

reinkrul commented Nov 28, 2023

It's not a bug per se, since it's allowed. But it's unconvenient for sure. What makes it complex is that we make the submission before we make the verifiable presentation. So it would be more practical to always marshal to an array, and then we can always use an indexer.

Agree @woutslakhorst ?

@woutslakhorst
Copy link
Member

Making it always an array (as specified by v2.0) wouldn't hurt v1.1 so sounds great

Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

is this still relevant given nuts-foundation/go-did#98?

@woutslakhorst
Copy link
Member

woutslakhorst commented Dec 1, 2023

is this still relevant given nuts-foundation/go-did#98?

PR should update to latest go-did and make changes if needed.
There could be a problem for the validation since other implementations could make a VP without an array and we compare one with an array?

@reinkrul
Copy link
Member Author

reinkrul commented Dec 5, 2023

PR relevant again. Can be reviewed.

@reinkrul reinkrul requested a review from gerardsn December 5, 2023 12:49
@reinkrul reinkrul merged commit 47df193 into master Dec 6, 2023
9 checks passed
@reinkrul reinkrul deleted the bugfix/pex-ldp_vp-1-vc branch December 6, 2023 05:21
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