-
Notifications
You must be signed in to change notification settings - Fork 15
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: dcql and a lot of UI stuff #227
Conversation
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
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.
nice
@@ -132,6 +139,12 @@ export function FunkeCredentialNotificationScreen() { | |||
[agent] | |||
) | |||
|
|||
// TODO: Should we add this to the activitiy? We also don't do it for issuance |
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.
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.
Okay. Reason for the comment is that i see this more as a failed issuance. But yeah if that's also on the list.
We don't support failed issuance though? And just adding this as failed proof request is weird i think as it's during issuance
// We only have the attribute paths, no way to know how to render | ||
// TODO: we could look at the vct? | ||
// TODO: we should maybe support partial matches (i.e. vct matches), as then we can | ||
// show a much better UI (you have the cred, but age is not valid, or this param is missing) |
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.
Do you mean then rendering the credential card but with 'date_of_birth' in red when it doesn't match the required age?
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.
Yes exactly!
@@ -67,7 +76,7 @@ export class ReceivePidUseCaseCFlow extends ReceivePidUseCaseFlow { | |||
resolvedCredentialOffer: this.resolvedCredentialOffer, | |||
credentialConfigurationIdsToRequest, | |||
clientId: ReceivePidUseCaseCFlow.CLIENT_ID, | |||
requestBatch: getShouldUseCloudHsm() ? 10 : false, | |||
requestBatch: getShouldUseCloudHsm() ? 2 : false, |
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.
Was this for testing?
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.
Need to update WSP. It doesn't batch soo is way too slow at 10
@@ -93,7 +94,7 @@ export class RefreshPidUseCase { | |||
resolvedCredentialOffer: this.resolvedCredentialOffer, | |||
credentialConfigurationIdsToRequest, | |||
clientId: RefreshPidUseCase.CLIENT_ID, | |||
requestBatch: 10, | |||
requestBatch: 2, |
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.
same here
Signed-off-by: Timo Glastra <[email protected]>
This PR grew a lot bigger than anticipated but it hasA LOT of great improvements: