feat: add walletkit-testkit crate for reusable e2e test helpers#430
feat: add walletkit-testkit crate for reusable e2e test helpers#430kilianglas wants to merge 29 commits into
Conversation
…t-crate # Conflicts: # walletkit-cli/src/commands/proof.rs
![semgrep-code-worldcoin[bot]](https://avatars.githubusercontent.com/u/60940476?s=60&v=4){kind=small}
The CLI migration to consume walletkit-testkit will land in a separate PR. This keeps this PR focused on the testkit crate and supporting core/db changes. Co-authored-by: Cursor <cursoragent@cursor.com>
…t-crate Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # walletkit-cli/src/commands/proof.rs
…alletkit into feat/walletkit-testkit-crate
|
/otto review with opus |
|
I've kicked off an Opus subagent to do a full review of PR #430. It will:
I'll let you know when it's done. |
![agentotto[bot]](https://avatars.githubusercontent.com/in/2964408?s=60&v=4){kind=small}
| edition.workspace = true | ||
| rust-version.workspace = true | ||
| authors.workspace = true | ||
| license.workspace = true |
There was a problem hiding this comment.
The PR description states walletkit-cli is migrated to consume this crate, but the CLI-migration commits were reverted later in the branch (revert: remove walletkit-cli changes from testkit PR, chore: undo remaining changes to walletkit-cli and walletkit-core). The CLI still contains its own copies of FsStorageProvider, NoopDeviceKeystore, FsAtomicBlobStore, and create_fs_credential_store in walletkit-cli/src/provider.rs, plus its own IWorldIDVerifier sol! interface, verify_proof_onchain, and build_test_request in walletkit-cli/src/commands/proof.rs. That means the primary in-repo consumer that would validate the API of this crate is not exercising it, and the duplication the PR advertises as eliminated still exists. Either land the CLI migration in this PR or update the description to be clear this is testkit-only (and land the CLI migration as an immediate follow-up).
| pub const LOCAL_ISSUER_SCHEMA_ID: u64 = 47; | ||
|
|
||
| /// `EdDSA` private key (32 bytes) for the registered local issuer. | ||
| pub const LOCAL_ISSUER_EDDSA_KEY: [u8; 32] = |
There was a problem hiding this comment.
These STAGING_* / LOCAL_ISSUER_EDDSA_KEY bytes are the well-known low-entropy value 0x1111…1111 and match the pattern walletkit-cli currently uses. That's safe only because they are pre-registered as staging keys for a staging RP and a staging issuer, and possession confers no cross-environment privilege. Please make that invariant much more explicit here — a doc comment along the lines of "WARNING: this key is intentionally public and only ever authorised for the staging RP/issuer registration. Never re-use this constant, or the TestEnv::default_staging fixture, against non-staging environments. Non-staging deployments MUST supply their own rp_signing_key / local_issuer_eddsa_key via the public struct fields." — plus a compile-time or runtime guard (e.g. a debug-assert in a helper that constructs a mainnet TestEnv variant) would materially reduce foot-gun risk once this crate starts being consumed by external services like orb-tools.
| Self { | ||
| rp_id: STAGING_RP_ID, | ||
| rp_signing_key: STAGING_RP_SIGNING_KEY, | ||
| world_id_verifier: verifier, |
There was a problem hiding this comment.
rpc_url is hard-coded to World Chain mainnet in both default_staging() and default_with_config_and_verifier(), but world_id_verifier is derived from Environment::Staging and every constant in this file is scoped to staging. That works today only because the staging WorldIDVerifier happens to be deployed on World Chain mainnet — but the config surface hides that assumption. Two concrete concerns: (1) A caller who does TestEnv::default_with_config_and_verifier(config_for_sepolia, sepolia_verifier) will silently keep rpc_url = worldchain-mainnet and ProviderBuilder::new().connect_http(env.rpc_url.parse()?) will call the wrong chain, producing confusing call-level errors far from the root cause. (2) The public Alchemy /public endpoint is heavily rate-limited and is unsuitable as a default for anything running in CI or from orb-tools — consider requiring the caller to supply it, or deriving it from the environment. At minimum, default_with_config_and_verifier should take an rpc_url too, and the doc on rpc_url should call out the chain/environment coupling.
|
|
||
| impl Default for TestEnv { | ||
| fn default() -> Self { | ||
| Self::default_staging() |
There was a problem hiding this comment.
impl Default for TestEnv unconditionally delegates to default_staging(), which unwraps default_config(...) and is documented as # Panics. That makes TestEnv::default() a panicking Default impl, which is surprising and inconsistent with the usual Rust convention that Default::default() is infallible. Either add a mirroring # Panics note on the Default impl and (ideally) prove-out that the staging config really can't fail to construct so the unwrap becomes an expect("static staging config") with justification, or drop the Default impl and force callers through default_staging() where the panic is at least discoverable at the call site.
| "0x{:040x}", | ||
| env.rp_id | ||
| ))) | ||
| .wrap_err("failed to construct oprf_key_id")?, |
There was a problem hiding this comment.
Constructing an OprfKeyId by JSON round-tripping (serde_json::from_value(json!(format!("0x{:040x}", env.rp_id)))) is a code smell — it hides the type behind two Results, obscures intent, and duplicates the exact same trick from walletkit-cli/src/commands/proof.rs:489. walletkit-core already constructs these as OprfKeyId::new(U160::from(rp_id)) in its own tests (walletkit-core/tests/proof_generation_integration.rs:230), and the whole point of this crate is to consolidate exactly this kind of duplication. Please either (a) re-export taceo_oprf::types::OprfKeyId (or a walletkit-core alias) from a public location and use it directly here, or (b) add a small fn oprf_key_id_from_rp_id(rp_id: u64) -> OprfKeyId helper in walletkit-core and depend on that. As written this is also a footgun: any future change to how OprfKeyId is serialised silently breaks this construction with only a runtime error.
| })?; | ||
| std::fs::rename(&tmp, &full).map_err(|e| { | ||
| StorageError::BlobStore(format!("rename {}: {e}", full.display())) | ||
| })?; |
There was a problem hiding this comment.
Path::with_extension("tmp") replaces any existing extension, it doesn't append. So two blobs that live in the same directory and share a stem — e.g. foo.sqlite and foo.sqlite-wal, which cleanup_storage explicitly assumes exist for the vault DB — both map to the same tmp file foo.tmp. If two write_atomic calls for those paths ever land concurrently, one writer's tmp will be renamed out from under the other, and (worse) a partial write on the loser's tmp could be renamed into the wrong final path. Even if the current CredentialStore never actually triggers this today, it's a sharp edge that shouldn't ship in a reusable testkit. Two easy fixes: use a random suffix (format!("{}.{}.tmp", full.file_name()?.to_string_lossy(), Uuid::new_v4())) in the same parent dir, or append rather than replace (full.with_added_extension("tmp") on nightly, or full.as_os_str() + ".tmp" manually). Same issue exists verbatim in walletkit-cli/src/provider.rs.\n\nAlso note there's no cleanup of the leftover tmp file if the rename step fails — minor but worth fixing.
| use walletkit_testkit::utils::now_secs; | ||
| use walletkit_testkit::{generate_and_verify_test_proof, CredentialType, TestEnv}; | ||
|
|
||
| const TEST_SEED: [u8; 32] = [7u8; 32]; |
There was a problem hiding this comment.
Both e2e tests share TEST_SEED = [7u8; 32] and are marked #[tokio::test(flavor = "multi_thread")]. cargo test runs test functions in parallel by default, and since the seed deterministically derives the on-chain World ID account, both tests will simultaneously init_or_register the same account against staging on the very first run, and afterwards will both drive OPRF / faux-issuer / on-chain calls for that same identity concurrently. That's a needless source of flakiness (and unnecessary state coupling between the two tests). Give each test its own distinct seed — e.g. const FAUX_TEST_SEED: [u8; 32] = [7u8; 32]; const LOCAL_TEST_SEED: [u8; 32] = [8u8; 32]; — so they exercise independent identities. Bonus: it also makes it easier to reset one test's account without disturbing the other.
| .await | ||
| .expect("faux-issuer flow should succeed"); | ||
|
|
||
| cleanup_storage(&root); |
There was a problem hiding this comment.
Cleanup is done positionally before the assert! so the happy path works, but if generate_and_verify_test_proof(...).await.expect(...) panics (which is the usual way an e2e test against live staging fails — flaky OPRF, gateway 5xx, chain RPC blip) or if a panic happens between temp_root() and the cleanup_storage call, the temp directory is leaked under std::env::temp_dir() forever. Over many CI runs this accumulates. Consider either using the tempfile crate (TempDir cleans up on drop, even on unwind) or wrapping the root in a small RAII guard type that calls cleanup_storage in Drop. That also removes the need to remember to call cleanup_storage at all in downstream consumers of the testkit.
| /// Returns an error if account setup, credential issuance, proof generation, | ||
| /// or on-chain verification setup fails. | ||
| pub async fn generate_and_verify_test_proof( | ||
| credential_type: CredentialType, |
There was a problem hiding this comment.
generate_and_verify_test_proof hard-codes REQUEST_TTL_SECS = 300 and ProofType::Uniqueness, and build_test_request's session_id is passed as None. That's fine as a convenience wrapper, but it means the flagship high-level flow can only be used to exercise the uniqueness path — the session-proof branch of build_test_request / verify_proof_onchain (which is quite a bit of code and the only place we'd catch a session-nullifier regression) has no e2e coverage at all in this PR. Consider either adding a generate_and_verify_test_session_proof variant that takes a session_id and returns the session outcome, or making proof_type + session_id + ttl explicit parameters, and adding a third #[ignore]d e2e test for the session flow. Otherwise "end-to-end helpers" is a slightly generous label — half of the on-chain surface is untested by the very tests that ship with the crate.
| /// A closure that turns a derived `sub` into an issued credential. | ||
| issue_fn: IssueCredentialFn, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Two smaller things on the closure surface:\n\n1. CredentialType::Custom.issue_fn is a boxed FnOnce(FieldElement) -> CredentialFuture, but issue_custom_credential in issuer.rs derives sub = authenticator.compute_credential_sub(&bf) and passes only sub to the closure. It never surfaces the underlying blinding_factor to the caller. Some real-world issuers need the blinding factor (or the leaf index) to sign or contact the OPRF — with the current signature they can't get either, so Custom is really only useful for issuers that only need sub. Consider passing a small CustomIssuanceContext { sub, blinding_factor, leaf_index } struct to the closure, which is a strictly additive API change.\n\n2. The doc on IssueCredentialFn says "turns a derived sub into an issued credential" but the CredentialType::Custom variant doc says "A closure that turns a derived sub into an issued credential" — the type-alias is the wrong level of abstraction to document that; move that doc onto the enum variant field (which already has it) and keep the alias doc purely structural.
| walletkit-core = { workspace = true, features = ["issuers", "embed-zkeys"] } | ||
| world-id-core = { workspace = true, features = ["authenticator", "embed-zkeys"] } | ||
| alloy-core = { workspace = true } | ||
| alloy = { version = "2", default-features = false, features = ["contract", "json", "getrandom", "signer-local"] } |
There was a problem hiding this comment.
walletkit-core = { workspace = true, features = ["issuers", "embed-zkeys"] } and world-id-core = { workspace = true, features = ["authenticator", "embed-zkeys"] } both enable embed-zkeys, which bakes the Groth16 proving/verifying keys into the compiled artifact. This is fine for the testkit's own e2e tests, but any git-rev consumer (e.g. orb-tools) that pulls walletkit-testkit as a normal dependency will also get those embedded zkeys — dramatically inflating binary size for consumers who may only want the storage-provider helpers, not the proving pipeline. Consider exposing this via a walletkit-testkit feature (default-on if you like) so downstream can turn it off:\n\ntoml\n[features]\ndefault = ["embed-zkeys"]\nembed-zkeys = ["walletkit-core/embed-zkeys", "world-id-core/embed-zkeys"]\n\n\nAlso worth double-checking that world-id-core's embed-zkeys isn't already implied by walletkit-core/embed-zkeys (it may be redundant to enable in both places).\n\nSeparately: alloy at version = "2" is pinned very loosely — cargo will pick the latest 2.x compatible version. Given this crate is consumed by git rev, downstream builds will silently see alloy minor bumps on cargo update. That's usually what people want, but it's worth being deliberate about whether you'd rather pin to 2.0 or workspace = true and manage upgrades centrally like the rest of the workspace does (e.g. alloy-core, walletkit-core).
Review summaryOverall direction is good — consolidating the storage-provider + on-chain-verify + issuer-shim triplication into a single not-published crate consumed by git rev is the right shape for this problem, and the module layout ( A few higher-level things that didn't fit inline:
Nothing here is a blocker; the FS/atomic-write and |
|
The review is complete. The Opus subagent posted 9 inline comments + a summary comment on PR #430. Here's what was flagged: Security / Correctness
Bugs
Design / Ergonomics
Missing
|
3 participants
Summary
Adds a new
walletkit-testkitworkspace crate that consolidates the end-to-end / test-support logic previously triplicated acrosswalletkit-core's integration tests, thewalletkit-clibinary, and external consumers (e.g. orb-tools).walletkit-cliis migrated to consume it.The motivation: other services that interact with the World ID protocol via WalletKit (e.g. orb-tools) currently re-implement WalletKit's storage providers and on-chain proof logic by copy-pasting from the CLI. This crate exposes that surface as a reusable library so consumers can depend on it directly (by git rev) instead of duplicating it.
walletkit-testkitis not published (publish = false): it ships staging URLs and pre-registered test-RP / test-issuer private keys, and is intended to be consumed by git revision.