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

feat: Merge anoncreds proof request credentials #118

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

Tommylans
Copy link
Member

Made it so it favors the way of combining the credentials in Paradym, after that it will combine them based on the credentialId. And if that isn't available it will still use the groupname.

So the groupname will be used when the credential is not available in the wallet and the proof request didn't came from Paradym.

image

@Tommylans Tommylans requested a review from TimoGlastra July 4, 2024 16:35
Copy link
Member

@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.

How does this work with credential selection? If there's multiple
credential available, changing one should change all

So we migut want to do something smarter, also looking at requirements. So it works generally

@Tommylans
Copy link
Member Author

How does this work with credential selection? If there's multiple credential available, changing one should change all

So we migut want to do something smarter, also looking at requirements. So it works generally

But what would make sense? When you click on it that you get the selector of #113 and that they explode again so they get splitted again?

Or should we always look for the credentialId and ignore the __CREDENTIAL__ because that implementation doesn't look at the selected credential.

@Tommylans Tommylans requested a review from TimoGlastra July 11, 2024 09:14
@@ -94,8 +111,11 @@ export function useDidCommPresentationActions(proofExchangeId: string) {
// This should probably be fixed in AFJ.
const firstMatch = predicateArray[0]

// When the credentialId isn't available, we use the groupName as the key but it will result in multiple entries in the view. But I think it's not an easy task to merge them
const credentialKey = firstMatch?.credentialId ?? groupName
Copy link
Member

Choose a reason for hiding this comment

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

Maybe if no credentialId is available we match based on the CREDENTIAL_ prefix like you had before as fallback?

Otherwise there's not an easy way to group them

Copy link
Member Author

@Tommylans Tommylans Jul 11, 2024

Choose a reason for hiding this comment

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

Sure brought it back but in the different order 😄

@TimoGlastra
Copy link
Member

I was a bit confused, but see now that #113 hasn't been merged yet 🤦 in my head it was.

But your approach probably will also work with that pr, so i think we're good 👍

@TimoGlastra TimoGlastra merged commit e62a97a into main Jul 11, 2024
1 check passed
@TimoGlastra TimoGlastra deleted the feature/PR-251-merge-multiple-credentials-in-proof branch July 11, 2024 14:46
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