Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Core proof generation Cargo.toml, crates/walletkit-core/src/authenticator/mod.rs |
The workspace pins world-id-core and world-id-proof to version 0.17.1. The core adds timestamp-based proof generation, and the existing method delegates to it. |
Web ownership proof API crates/walletkit-web/src/authenticator.rs, crates/walletkit-web/src/lib.rs, web/walletkit/README.md, web/walletkit/tests/types/consumer.ts |
The web authenticator exposes proveCredentialSub and returns encoded proof bytes. The documentation and consumer typing check describe and exercise the API. |
Cross-target proof validation web/walletkit/tests/staging-ownership.spec.ts, crates/walletkit-core/tests/ownership_cross_target.rs, .github/workflows/ci.yml |
An opt-in browser test writes a generated proof and its inputs to JSON. An opt-in native test verifies the proof and checks altered inputs. The CI job installs Nargo and prefetches Noir dependencies. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant JsAuthenticator
participant Authenticator
Caller->>JsAuthenticator: proveCredentialSub inputs and optional timestamp
JsAuthenticator->>Authenticator: prove_credential_sub_at with resolved timestamp
Authenticator-->>JsAuthenticator: OwnershipProof
JsAuthenticator-->>Caller: CBOR-encoded Uint8Array
Suggested reviewers: dzejkop, paolodamico
Merge Risk: 
fa10c
-
The browser proof API returns proof bytes as documented, and no actionable issue remains before merge. Firefox, Safari, and phone performance have not been validated.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 6 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: browser-side generation of WIP-103 ownership proofs. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 6 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
Comment @coderabbitai help to get the list of available commands.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baa772ba95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[wasm_bindgen(js_name = proveCredentialSub, unchecked_return_type = "Promise<Uint8Array>")] | ||
| pub fn prove_credential_sub( |
There was a problem hiding this comment.
When developers inspect the Rust facade documentation, this new export contradicts crates/walletkit-web/src/lib.rs, whose module docs still state that credential ownership proofs are native-only. This makes the newly supported browser API appear unavailable, so update that module-level description alongside the export.
AGENTS.md reference: AGENTS.md:L53-L55
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Bump world-id-core and world-id-proof to 0.17.1, which enables ownership proving on wasm32 and requires provekit-spongefish 0.2.1 (target-independent transcripts). Remove the wasm32 stub: prove_credential_sub delegates to prove_credential_sub_at(now), and walletkit-web exposes authenticator.proveCredentialSub(nonce, context, blindingFactor, sub, now?) returning the CBOR proof. Add an opt-in cross-target check: the staging browser spec writes a proof that tests/ownership_cross_target.rs verifies natively and rejects with a changed context, nonce or sub.
world-id-proof 0.17.1 builds the ownership proof artifacts for wasm32, so its build script now needs nargo there too, as the native jobs already provide.
004f37b to
10328a1
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive proving and transcript changes warrant human sign-off, with cross-browser and phone validation still outstanding.
No open issues 🎉
What changed in this PR
Enables browser-side WIP-103 credential ownership proofs through WalletKit’s existing worker API, using protocol dependencies that support matching browser and native transcripts.
Changes:
- Adds
proveCredentialSub, returning CBOR proof bytes with an optional browser-clock timestamp. - Updates protocol and transcript dependencies and adds the required WASM CI tooling.
- Adds consumer type coverage, opt-in browser-to-native verification, and usage documentation.
No critical issues were identified. Residual validation gaps include cross-browser behavior and phone performance; cross-target verification remains opt-in.
| File | Description |
|---|---|
web/walletkit/tests/types/consumer.ts |
Checks the ownership-proof API’s return type. |
web/walletkit/tests/staging-ownership.spec.ts |
Generates a browser proof for opt-in native verification. |
web/walletkit/README.md |
Documents ownership proving, timing defaults, and cancellation. |
crates/walletkit-web/src/lib.rs |
Removes the outdated native-only ownership-proof limitation. |
crates/walletkit-web/src/authenticator.rs |
Exposes ownership proving as CBOR bytes to JavaScript. |
crates/walletkit-core/tests/ownership_cross_target.rs |
Verifies browser proofs natively and rejects altered public inputs. |
crates/walletkit-core/src/authenticator/mod.rs |
Replaces the WASM stub with a shared, caller-timed proving path. |
Cargo.toml |
Requires protocol dependencies at version 0.17.1. |
Cargo.lock |
Updates protocol and transcript packages and adds WASM prover dependencies. |
.github/workflows/ci.yml |
Installs nargo and prefetches Noir dependencies for WASM checks. |
Review effort: Balanced
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Auto-fix stopped before satisfying the configured readiness conditions. Blocking condition: Auto-fix requires data retention to be enabled for the organization. Triggered by rule Auto-fix PRs. This is the Auto-fix result for commit fa10c16; it does not confirm that the pull request was merged. |
Why
sub(WIP-103) in the browser.proveCredentialSubwas stubbed out on wasm32 because ProveKit was native-only.provekit-spongefish0.2.1, which hashes its squeeze counters asu64. Browser (32-bit) and native transcripts now match.What
world-id-coreandworld-id-proof0.16 → 0.17.1. The lockfile moves everyworld-id-*crate to 0.17.1 andprovekit-spongefish/-powto 0.2.1, and addslzma-rs. No git patches.prove_credential_subdelegates to a Rust-onlyprove_credential_sub_at(…, now), which isn't exported over uniffi.authenticator.proveCredentialSub(nonce, context, blindingFactor, sub, now?)returns the CBOR-encoded proof as aUint8Array. The README documents the user-action requirement, the defaultnow, and cancelling withterminate().Test
cargo fmt, Prettierwalletkit-core --all-targetsand wasm32walletkit-web,-D warningswalletkit-coreunit testscontext,nonceorsubis rejectedcargo deny check bans licenses sourcescargo deny check advisoriesctutilsandtokiobumped as in #630 (RUSTSEC-2026-0385, -0386)The cross-target check is opt-in and uses
WALLETKIT_STAGING_PROOF:tests/staging-ownership.spec.tsgenerates a proof in Chrome for a registered staging account and writes it to that path.crates/walletkit-core/tests/ownership_cross_target.rsverifies it natively with the embedded ownership verifier. Without the variable it does nothing.Measured on desktop Chrome against staging: about 1.0 s to prove, and a 570 KB CBOR proof.
Not run yet: the wasm-bindgen worker tests on Firefox; the package tests on Safari or Firefox; proving time on a phone.
Risk
walletkit.wasmgrows from 42.14 MB to 44.20 MB (+2.06 MB raw, +1.1 MB gzip, +1.0 MB brotli). About 1.1 MB is the ownership.pkp/.pkvkeys; the rest is the WASM witness solver and decoder. For context, 38.1 MB of the module is embedded proving keys (36 MB of it OPRF zkeys). Loading keys on demand instead of embedding them would be a separate change.AI assistance
Note
Medium Risk
Adds browser-side ZK ownership proving and bumps cryptographic/protocol dependencies (~2 MB WASM growth); proving is security-sensitive and must stay user-initiated, but verification and cross-target tests mitigate transcript mismatch risk.
Overview
Enables WIP-103 credential ownership proving in the browser by bumping
world-id-core/world-id-proofto 0.17.1 (withprovekit-spongefish0.2.1 in the lockfile) so wasm32 can run the same ownership prover as native.Core drops the wasm32 error stub:
prove_credential_subnow forwards to a newprove_credential_sub_at(..., now)path so wasm callers supply Unix time (native still resolvesnowitself). Web exposesauthenticator.proveCredentialSub, returning CBOR asUint8Array, with optionalnowdefaulting likegenerateProof. Docs remove ownership proving from the browser limitations table and describe user-action and worker-blocking behavior.CI installs nargo and prefetches Noir deps in the wasm-check job so
world-id-proofcan build wasm32 artifacts. Tests: opt-in Playwright staging spec writes a proof;ownership_cross_targetverifies it natively and rejects tampered public inputs; consumer typings include the new API.Reviewed by Cursor Bugbot for commit fa10c16. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit