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

fix: AnonCreds proof requests with unqualified dids #1891

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Jun 6, 2024

Following issues found during integration with BCGov Wallet, where AnonCreds with unqualified identifiers are being used, here we try to adjust the identifiers used in a Present Proof flow depending on the proof request contents.

Basically, we analyze whether there is any restriction that contains an unqualified did and, if true, assume that we should use unqualified identifiers. Otherwise, we use the default ones (qualified).

I'm testing with BCGov wallet's main branch in both verifier and holder side and seems to work fine (at least with a basic presentation request using Person credential from BC Wallet Showcase.

genaris added 7 commits June 1, 2024 22:31
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris genaris marked this pull request as ready for review June 6, 2024 16:42
@genaris
Copy link
Contributor Author

genaris commented Jun 6, 2024

cc @bryce-mcmath @wadeking98 hopefully this can solve the remaining issues with that awful thing popularly known as 'unqualified hell'.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Another great fix, thanks @genaris!

Do we need a simple test to cover this? (I'm fine with merging it as is to get it fixed ASAP)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris
Copy link
Contributor Author

genaris commented Jun 6, 2024

Another great fix, thanks @genaris!

Do we need a simple test to cover this? (I'm fine with merging it as is to get it fixed ASAP)

I've added some based on v2-indy-proofs.e2e.test.ts (basically a copy-paste, maybe we can improve it in a future when reorganizing core modules). They fail when running with previous code and OK with these fixes, so it seems we are OK!

I'd like to wait for @bryce-mcmath feedback before merging, to make sure we are handling all relevant cases.

@genaris genaris merged commit 2de96bb into openwallet-foundation:main Jun 7, 2024
12 checks passed
@genaris genaris deleted the fix/proof-request-with-unqualified-dids branch June 7, 2024 17:18
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.

2 participants