Conversation
…tkit into kilianglas/walletkit-cli
|
@cursor review |
Bugbot couldn't runBugbot is not enabled for your user on this team. Ask your team administrator to increase your team's hard limit for Bugbot seats or add you to the allowlist in the Cursor dashboard. |
|
/otto review |
|
:orb-salute: On it — spinning up a review job for this PR now. Will post inline comments shortly. |
There was a problem hiding this comment.
Overall Review Summary
This is a solid developer CLI with good ergonomics overall. The command structure is clean, the latency layer is a nice touch, and the error handling is generally thoughtful. Below are several issues worth addressing before merge — ranging from a security concern (seed material in stdout) to correctness bugs and a few UX rough edges.
|
|
||
| if !all_passed { | ||
| std::process::exit(1); | ||
| } |
There was a problem hiding this comment.
Bug: process::exit(1) bypasses the latency reporter
When verification fails, std::process::exit(1) is called directly. This skips the if show_latency { latency::print_report(...) } block at the end of main, so the user never sees the latency summary that was the whole point of passing --latency.
It also bypasses any future cleanup hooks (destructors, async task shutdown, etc.) that might be added.
Prefer returning an Err so the normal error path in main handles the exit:
if !all_passed {
eyre::bail!("one or more proofs failed on-chain verification");
}There was a problem hiding this comment.
:orb-salute: On it — fixing this now.
There was a problem hiding this comment.
Fixed in 4f9de20 — replaced process::exit(1) with eyre::bail! so the latency reporter and cleanup hooks run normally.
| .wrap_err("remote fetch failed")?; | ||
| let local = authenticator.packed_account_data(); | ||
| let matches = remote.to_string() == local.to_string(); | ||
|
|
There was a problem hiding this comment.
Correctness: comparing Uint256 via string formatting
ruint::Uint implements PartialEq, so comparing through to_string() is both unnecessary and fragile — it would silently break if the Display impl ever changes format (e.g., adds a 0x prefix).
Replace with a direct comparison:
let matches = remote == local;There was a problem hiding this comment.
:orb-salute: On it — fixing this too.
There was a problem hiding this comment.
Fixed in 4f9de20 — replaced .to_string() comparison with direct PartialEq.
walletkit-cli/src/main.rs
Outdated
|
|
||
| if let Err(err) = commands::run(cli).await { | ||
| let json_flag = std::env::args().any(|a| a == "--json"); | ||
| if json_flag { |
There was a problem hiding this comment.
Redundant arg re-parse after clap has already parsed the CLI
At this point cli (which was parsed by clap at the top of main) is no longer accessible because it was moved into commands::run(cli). So the code re-scans std::env::args() for "--json" to decide how to format the error.
The simplest fix is to capture the flag before the move:
let json_mode = cli.json; // already captured above
// ... later in the error handler:
if json_mode {
...
}json_mode is already saved on line 24; just reuse it in the error branch instead of re-parsing argv.
There was a problem hiding this comment.
:orb-salute: Adding this to the active fix job now.
There was a problem hiding this comment.
Fixed in 4f9de20 — replaced the redundant std::env::args() re-parse with the already-captured json_mode variable.
|
|
||
| let result = if let Some(ref config) = config_json { | ||
| InitializingAuthenticator::register( | ||
| &seed, |
There was a problem hiding this comment.
Code duplication: registration logic repeated verbatim in run_register and run_register_wait
Both functions perform the exact same registration call (including the config_json/register_with_defaults split and the AccountAlreadyExists early-return). The only difference is what happens after init_auth is obtained.
Extract a shared helper:
async fn do_register(cli: &Cli, recovery_address: Option<&str>)
-> eyre::Result<Option<InitializingAuthenticator>>that returns None on AccountAlreadyExists. Both callers then decide what to do with the InitializingAuthenticator (single poll vs. loop). This avoids the two copies diverging silently in the future.
There was a problem hiding this comment.
:orb-salute: On it — adding to the active job.
There was a problem hiding this comment.
Refactored in 4f9de20 — extracted do_register helper shared by both run_register and run_register_wait.
Code Review — WalletKit CLI (PR #294)Overall: This is a well-structured developer CLI. The command taxonomy ( I've posted inline comments on each issue below; here's a quick summary of severity: 🔴 Security
🟠 Correctness / Reliability
🟡 UX / Quality
The first three items (seed exposure + file permissions + staging key guard) should be resolved before merge; the rest are improvements that would meaningfully raise code quality. |
|
/otto resolve ci issues |
|
:orb-salute: On it — investigating CI failures now. |
…r-name race sqlite3mc_cipher_name() uses a process-global static char buffer that is overwritten on every call. Concurrent sqlite3_open_v2 calls race on this buffer, causing intermittent "unknown cipher 'chacha20'" errors when tests run in parallel (e.g. cargo test --workspace). Add a process-wide OnceLock<Mutex<()>> in ffi.rs that serializes all sqlite3_open_v2 calls on the Rust side. Only the open call is serialized; operations on established connections are unaffected. This fixes test_null_handling which intermittently panicked with: "unknown cipher 'chacha20'" at walletkit-db/src/tests.rs:96 Upstream bug: utelle/SQLite3MultipleCiphers#228 Can be removed once a fixed sqlite3mc release is pinned.
CI Fix —
|
- proof.rs: replace std::process::exit(1) with eyre::bail! so the latency reporter and cleanup hooks run on verification failure - auth.rs: replace .to_string() equality comparison with direct PartialEq on Uint256 values - main.rs: drop redundant std::env::args() re-parse; reuse the already-captured json_mode variable in the error handler - auth.rs: extract do_register() helper shared by run_register and run_register_wait to eliminate duplicated registration logic
Summary
This PR adds a CLI for WalletKit. The idea here is to get a CLI that can be used for dev and testing. This is not meant to be used as an authenticator for an actual World ID — the environment defaults to staging.
Features
Wallet management (
walletkit wallet)init— Bootstrap wallet data directory (~/.walletkitby default) with required subdirectories and databasespaths— Print resolved storage paths (useful for debugging and scripting)doctor— Health check: verifies root exists, Groth16 material is cached, and databases are openableexport— Export the vault to a plaintext backup fileimport— Restore credentials from a vault backup filedanger-clear— Permanently delete all credentials (requires--confirm)Authenticator lifecycle (
walletkit auth)register— Submit a new World ID registration (returns immediately)register-wait— Register and poll until finalized, with configurable intervalinit— Initialize an authenticator for an already-registered World IDinfo— Print authenticator details (leaf index, onchain address, packed account data)remote-account-data— Fetch on-chain packed account data and compare with local stateCredential operations (
walletkit credential)import— Import a raw credential with a pre-computed blinding factorissue— End-to-end credential issuance: generate blinding factor via OPRF, then storeissue-test— Issue a test credential from the staging faux issuer (issuer schema 128) in a single step (OPRF + sub + faux issuer + store)list— List stored credentials, optionally filtered by issuer schema IDshow— Show details of the latest credential for an issuer schemadelete— Delete a credential by IDblinding-factor— Generate a credential blinding factor via OPRF nodescompute-sub— Derive a credential sub from a blinding factorProof generation (
walletkit proof)generate— Generate a ZK proof from a proof-request JSON (file or stdin)generate-test-request— Generate a signed test proof request using hardcoded staging RP keysinspect-request— Parse and display a proof request without generating a proofverify— Verify a previously generated proof on-chain via the WorldIDVerifier contractGlobal options
--root/WALLETKIT_ROOT— Custom wallet data directory--seed/WALLETKIT_SEED— 32-byte hex authenticator seed--random-seed— Generate a fresh random seed for quick testing--environment— Target environment (stagingorproduction, defaults to staging)--region— OPRF/indexer region selection (eu,us,ap)--rpc-url/WORLDCHAIN_RPC_URL— World Chain RPC endpoint--json— Machine-readable JSON output for all commands--verbose— Enable debug logging--config/WALLETKIT_CONFIG— Path to a custom config JSON file (overrides--environmentand--region)--latency— Print per-network-call latency summary after the commandproof verifyoptions--verifier-address— Override the WorldID verifier contract address (default: mainnet); useful for testnet