Private Credentials - Presentation Request #231
Conversation
…to feat/privCred-credentials
…nd modify minajs version
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Great work!
What worries me a bit is how complex this integration with Pallad is. The only thing I can think of to reduce that complexity for other wallets is moving reusable logic into mina-credentials. I guess that mainly includes the logic for pretty-printing credentials and presentations.
We should move those parts to mina-credentials in a follow-up PR!
apps/extension/src/sandbox/index.ts
Outdated
| // compile | ||
| const compiled = await Presentation.compile(deserialized) |
There was a problem hiding this comment.
note for the future: in an ideal world, we could cache compiled presentation requests so if the same one is used several times we don't have to compile it every time
is the sandbox JS process long-lived? or is a new one spun up every time? at least caching within one browser session seems easy to do if it's long-lived
There was a problem hiding this comment.
Each Web Connector's prompt is a single sandbox instance, we can definitely use chrome.storage.local to persist vk objects.
packages/features/src/web-connector/components/credential-selection-form.tsx
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| window.addEventListener("message", messageHandler) |
There was a problem hiding this comment.
who can send messages to this listener? is this isolated from the web page? I'm just worried the web page could trick the wallet into signing something
maybe that's a question for @mrcnk
There was a problem hiding this comment.
Yeah, especially that we have signFieldsWithPassphrase, I would check if we can change window message handler. If it's to communicate with iframe, we can check iframe.contentWindow.body.addEventListener. If we need to rely on window, we should check if there are suitable methods in webext-bridge that we use for messaging in Pallad.
|
@martonmoro can you switch everything to o1js v2.2.0 before merging this? nicer to be on an official version |
…llad into feat/privCred-presentation
…llad into feat/privCred-presentation
…llad into feat/privCred-presentation
There was a problem hiding this comment.
Thanks for the updates @martonmoro! As for me, this PR works well and is good to go.
We should keep in mind the window messaging question though, but could possibly address that in a separate PR.
We also still need proper e2e testing of the zkApp verifier case, but again I would address any modifications needed for that in future PRs, because this PR already delivers a significant amount of features that are well-tested.
Describe changes
Ticket or discussion link
Review checklist
Screenshots