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: credential selection in proof request #113

Merged
merged 8 commits into from
Jul 13, 2024

Conversation

TimoGlastra
Copy link
Member

This supersedes #71 and adds credential selection. It's a bit meh as you can't really see the credential you're selection.

So I had two potential ideas:

  • merge as is (with the known UX flaws)
  • disable credential selection still in the UI for now, but then we at least have the logic in place to add it in later

Works with both DIDComm and OpenID4VC.

@berendsliedrecht @janrtvld WDYT?

RPReplay_Final1719332123.MP4

@TimoGlastra
Copy link
Member Author

As you can see in the last commit, I disabled it for now in the UI. We can enable it again soon when we reworked the UX / design for selecting a credential

packages/agent/src/hooks/useDidCommPresentationActions.ts Outdated Show resolved Hide resolved
packages/agent/src/hooks/useDidCommPresentationActions.ts Outdated Show resolved Hide resolved
}}
// The index is stable enough here
// biome-ignore lint/suspicious/noArrayIndexKey: <explanation>
key={credentialIndex}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the credential not have an id we can use? I learned my lesson a few weeks back when debugging an index used as a key for way too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can add it. But we don't have it right now in this model. But yeah no reason to not use the id

Copy link
Contributor

Choose a reason for hiding this comment

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

What was wrong with the tamagui sheet?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a year ago that I implemented this so ... TBH I don't know anymore. I can vaguely remember that It was just buggy and I've had good experience with this sheet (as it's rendered as native sheet on iOS)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it had to do also with scroll. If you have a lot of credentials, it will become a scroll the sheet, which also didn't work very well with the tamagui sheet (if i remember correctly)

@berendsliedrecht
Copy link
Member

@TimoGlastra do you need an additional review on this?

@TimoGlastra
Copy link
Member Author

No it's ok 👍

@TimoGlastra
Copy link
Member Author

I've updated this with the grouping, but have improved upon it a bit. We prefer grouping by CREDENTIAL, as it's what we use in Paradym and otherwise if all the matches for a requested entry match. In most cases I think this should suffice. And we will filter the matches so it correctly shows the options if selection is enabled that can be used for all credentials in the group.

There is one case that is not catched by this, and this is when you request a predicate and values from the same credential basically, but without the CREDENTIAL prefix in group name. This can gives multiple separate optoins ,but I feel this is more a limitation of AnonCreds proof requests, which we have now sovled using the CREDENTIAL prefix

There has been some pushback on adding this feature to AnonCreds v1, so I think our 'workaround' is fine here. hyperledger/anoncreds-rs#292

@TimoGlastra
Copy link
Member Author

Adding a "received on XXX" will probably already help a lot with being able to distinguish credentials when selecting

Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra enabled auto-merge (squash) July 13, 2024 16:29
@TimoGlastra TimoGlastra merged commit 09e6459 into main Jul 13, 2024
1 check passed
@TimoGlastra TimoGlastra deleted the feat/select-credentials-now branch July 13, 2024 16:30
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