refactor(evm-wallet-experiment): drop SES-lockdown workarounds#943
Merged
refactor(evm-wallet-experiment): drop SES-lockdown workarounds#943
Conversation
sirtimid
added
the
no-changelog
Closes #938. Now that vats can request `crypto`, `SubtleCrypto`, and `Math` via their `globals` allowlist (via #937), drop the workarounds that existed because `crypto.getRandomValues` and `Math.random` were unreachable inside vat compartments: - Drop `entropy?: Hex` from the throwaway `KeyringInitOptions` and its plumbing through both coordinators, setup scripts, docs, and the docker e2e helper. The keyring vat now generates the throwaway key itself via `crypto.getRandomValues`. - Collapse `makeSaltGenerator` in `lib/delegation.ts` to a crypto-only implementation; drop the counter fallback and its `entropy` param. - Endow `crypto` + `SubtleCrypto` in the keyring and delegator vat globals (delegator imports `delegation.ts`, which evaluates `generateSalt = makeSaltGenerator()` at load). - Drop stale "Math.random is blocked under SES lockdown" JSDoc from `bundler-client.ts` and `provider.ts`; the raw-fetch implementations are left in place per the issue's speculative/lower-priority note. - Simplify the `initializeKeyring` option unwrapping in both coordinators now that the throwaway branch carries no payload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per principle of least authority. Neither the keyring nor delegator vat references `crypto.subtle` (mnemonic encryption uses `@noble/*` pure-JS implementations of AES-GCM and PBKDF2). Endowing only `crypto` keeps `crypto.getRandomValues` available to both vats while shrinking the capability surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sirtimid
force-pushed
the
sirtimid/remove-ses-lockdown-workarounds
branch
from
ec1cc4f to
d00e27e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Coverage Report
File Coverage |
Document that throwaway keyrings regenerate their private key on each
`makeKeyring` call (including vat restarts), since baggage only persists
`{ type: 'throwaway' }`. Callers needing stability across restarts must
use `type: 'srp'`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cripts `setup-home.sh` and `setup-away.sh` listed only `TextEncoder`/`TextDecoder` in the delegation vat's globals, but `delegator-vat.ts` evaluates `makeSaltGenerator()` at module load, which now unconditionally calls `crypto.getRandomValues`. Matches the canonical config in `cluster-config.ts`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cripts and docs The delegator vat was renamed from `delegation` to `delegator` (and its bundle from `delegation-vat.bundle` to `delegator-vat.bundle`) in PR #939 when `coordinator-vat` was split. The setup scripts and the setup guide were not updated, so they configure a vat key (`delegation`) the home coordinator doesn't read at bootstrap (`vats.delegator` is undefined) and reference a stale bundle artifact that `yarn build` no longer produces. Also brings the README's endowment description in line with the current globals list and adds a note to Recent Improvements summarizing this PR's direction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…crypto guards - `generateSalt` is now a single guarded function; `SaltGenerator` type, `makeSaltGenerator`, and the redundant `saltGenerator` option on `makeDelegation` are all dropped (callers can pass `salt?: Hex` for deterministic tests). - Restore the explicit `crypto.getRandomValues` guard in both the throwaway-keyring path (`keyring.ts`) and the delegation-salt path (`delegation.ts`). The previous PR removed them in favor of letting viem throw from the inside; the explicit guards give the clear "add 'crypto' to this vat's globals" diagnostic that would have caught the now-fixed missing endowment in the setup scripts. - Simplify the coordinator `initializeKeyring` wrappers: replace the union-typed ternary-with-spread with an early-return for throwaway + a plain `if` for `addressIndex`. - Dedupe the parallel `generateSalt` / `makeSaltGenerator` test blocks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ording After endowments became per-vat-explicit via `cluster-config.ts`, the failure mode for missing `setTimeout` / `Date` / `crypto` is not "SES lockdown blocks it" — it's "this vat is not endowed with the global". Update the diagnostic strings to point maintainers at the correct fix. Also reword the delegation timestamp-caveat error in the same direction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…itation
Throwaway keyrings intentionally do not survive a vat restart: baggage
only persists `{ type: 'throwaway' }`, so rebuild produces a fresh random
key at a new address. Locks in that contract with an executable test
alongside the existing SRP resuscitation case, so a future refactor that
accidentally persists throwaway key material (or reuses the prior
address) fails loudly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Scripts and setup-guide referenced the stale `coordinator-vat.bundle`; canonical build emits `home-coordinator.bundle` / `away-coordinator.bundle` (since PR #939). setup-home.sh now points at the home bundle, setup-away.sh at the away bundle, and the setup-guide example (under the home-device section) at the home bundle. - README endowment paragraph was only accurate for the home role; clarify that the fourth vat is `delegator` (home) or `redeemer` (away) and that `redeemer` does not receive `crypto`. - Add negative tests for the new crypto-endowment guards in `makeKeyring` (throwaway branch) and `generateSalt` that assert on the actionable error substring, so a future rewording cannot silently regress the diagnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bdd43f6. Configure here.
… and docs Round-3 Bugbot catch: in commit 4f1b259 I renamed the away setup script's fourth vat from `delegation` to `delegator`, but the away coordinator reads `vats.redeemer` (not `vats.delegator`), and cluster-config.ts pairs the away role with a `redeemer` vat backed by `redeemer-vat.bundle`. The incorrect rename would silently leave `redeemerVat` undefined at bootstrap, breaking delegation redemption — the away wallet's primary purpose. Restore the correct away-role vat (`redeemer` + `redeemer-vat.bundle`, no `crypto` global, no parameters) and drop the now-unused `DELEGATION_MANAGER` / `DM` plumbing from `setup-away.sh`. Also replace the setup-guide section 3d "same as the home device" pointer with an explicit away-role config block so away-device users are not directed at the home-specific config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…config
The `setup-home.sh` / `setup-away.sh` config blobs duplicate the vat
topology from `cluster-config.ts`. When the canonical config changed
role-specific vat names (`delegator` for home, `redeemer` for away),
the scripts silently drifted — caught only by Cursor Bugbot after
merge was requested.
Add a test that regex-extracts each script's `vats.<key>: { bundleSpec }`
pairs and asserts them against `makeWalletClusterConfig({ role })`, plus
targeted assertions for `crypto` endowments and the removed
`delegation` / `delegation-vat.bundle` names. Catches the exact class
of bug we hit without requiring the scripts to be executed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n setup scripts
Both setup scripts configured the provider vat with `platformConfig: { fetch:
{ allowedHosts } }` and no fetch-family globals. The kernel's
`platformConfigStruct` (`kernel-platforms/src/capabilities/index.ts`) only
accepts `fs?: FsConfig`, so the `fetch` field was either rejected by
`isVatConfig` or silently dropped — leaving the provider vat unable to reach
any RPC. The correct form (already used in `cluster-config.ts`, the Docker e2e
helper, and the setup-guide config blocks) is:
globals: [..., 'fetch', 'Request', 'Headers', 'Response'],
network: { allowedHosts: hosts },
Pre-existing since the package's original introduction in PR #877. Also
extends `test/setup-scripts.test.ts` with per-role provider-vat assertions so
this exact class of drift (missing fetch globals, invalid `platformConfig`)
cannot silently regress.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #938.
Now that #937 endows
crypto,SubtleCrypto, andMathvia the Snaps attenuated endowment factories, severalevm-wallet-experimentworkarounds are obsolete. This drops them.Summary
entropy?: Hexoption fromKeyringInitOptionsand its plumbing through both coordinators'initializeKeyring, the docker e2e helper, the setup scripts, three integration test harnesses, and one e2e test harness. The keyring vat now self-generates the throwaway private key viacrypto.getRandomValues(called insideviem'sgeneratePrivateKey).makeSaltGenerator: collapse to a single crypto-only implementation. Drop theentropy?: Hexparameter, the counter fallback, and the corresponding tests; keep a stripped-down "two generators produce distinct salts" assertion.cryptoon thekeyringanddelegatorvats incluster-config.ts. The keyring needs it for throwaway-key generation; the delegator needs it becausedelegation.tsevaluatesgenerateSalt = makeSaltGenerator()at module load.bundler-client.tsandprovider.ts. The raw-fetch implementations themselves are kept (per the issue's lower-priority note).initializeKeyringif/else + nested ternary into a single ternary + conditional spread (addressIndexonly included when defined).Followup commit drops
SubtleCryptofrom the endowment list — neither vat referencescrypto.subtle(mnemonic encryption uses@noble/*pure-JS), so least-authority says skip it.Test plan
yarn workspace @ocap/evm-wallet-experiment lintclean.yarn workspace @ocap/evm-wallet-experiment test:dev:quiet— 373 unit tests pass, including the updatedcluster-config.test.tsthat asserts the new vat globals.yarn workspace @ocap/evm-wallet-experiment test:node— 34/34 real-SES integration assertions pass (covers both SRP and throwaway keyring init, signing, full delegation lifecycle).yarn workspace @ocap/evm-wallet-experiment test:node:peer— 27/27 peer wallet assertions pass over QUIC, including throwaway init on the away kernel and delegation transfer + redemption.yarn workspace @ocap/evm-wallet-experiment build— all six bundles emit cleanly.Notes for reviewers
cluster-config.tsandwallet-setup.ts. The two PRs do not conflict semantically — one will rebase trivially on top of the other.bundler-client.ts/provider.tsfrom raw-fetch to viem clients (issue item Publish packages to npm #3) — the issue marks that as speculative and the raw-fetch wrappers are small and focused.SubtleCrypto, addressed in commit 2.🤖 Generated with Claude Code
Note
Medium Risk
Medium risk because it changes throwaway key and delegation salt generation to require
cryptoendowments (removing entropy/counter fallbacks), which can break wallet initialization in misconfigured environments and affects key material generation behavior.Overview
Removes SES-lockdown fallbacks by making throwaway key creation and delegation salt generation depend exclusively on
crypto.getRandomValues, dropping the caller-suppliedentropypath and the counter-based salt generator.Updates cluster configuration and all setup/test harnesses to explicitly endow
cryptoon thekeyringanddelegatorvats (and switches provider config tonetwork.allowedHosts), while simplifyinginitializeKeyringhandling in both coordinators and tightening runtime errors to point to missing vat globals.Cleans up public API/docs/tests accordingly: removes
makeSaltGeneratorexports/tests, updates throwaway keyring persistence expectations (new key after resuscitation), and refreshes README/setup guide/script examples to use the renamed coordinator bundles and new init signatures.Reviewed by Cursor Bugbot for commit 4f48e10. Bugbot is set up for automated code reviews on this repo. Configure here.