Skip to content

feat(validator-readiness): keystore security hardening + Filecoin (Lighthouse) provider adapter#16

Open
Santiagocetran wants to merge 12 commits into
InfiniteZeroFoundation:developfrom
Santiagocetran:feat/validator-readiness
Open

feat(validator-readiness): keystore security hardening + Filecoin (Lighthouse) provider adapter#16
Santiagocetran wants to merge 12 commits into
InfiniteZeroFoundation:developfrom
Santiagocetran:feat/validator-readiness

Conversation

@Santiagocetran

Copy link
Copy Markdown

PR draft — Validator Readiness: Keystore Security + Filecoin (Lighthouse) Adapter

Not opened yet. Draft body below, ready for gh pr create --base develop --title "..." --body-file Plans/pr-description-draft.md once reviewed.

Suggested title: feat(validator-readiness): keystore security hardening + Filecoin (Lighthouse) provider adapter


needs-security-review

The wallet/keystore-handling commits must be reviewed by @umeradl before merge, per task_300626_3: the two feature commits (5cfd390 in-memory password cache / --keystore import / named store, 4c7881a --wallet wiring) plus the follow-up fix (8436e50 clean invalid---name handling in connect-wallet) and the .gitignore guard (3d1bf04 stray root-level key files). Everything else (docs, IPFS/Lighthouse adapter) does not need the same scrutiny.

Summary

Implements both parts of task_300626_3:

  • Part 1 (P3-T0.2b): keystore security hardening — encrypted named multi-account keystores, --keystore import, in-memory password cache (removes on-disk plaintext session cache), burner-wallet + migration guides, production-key callouts across docs.
  • Part 2: Filecoin-backed IPFS storage via a first-class Lighthouse provider adapter, alongside the existing env/filebase/custom providers.

Part 1 — Keystore Security

  • connect-wallet --keystore <path>: imports a standard eth-account JSON keystore, decrypts to verify + derive the checksummed address, preserves the original keystore verbatim (source: "imported").
  • connect-wallet --name <name>: named multi-account keystores at wallets/wallet_<name>.json (shared wrapper schema); legacy wallet.json still loads as default (named default wins when both exist).
  • --wallet <name> / DIN_WALLET_NAME env / config wallet_name: runtime account selection, resolved through DinContext; new dincli system set-wallet <name> persists a default. Resolution: --wallet → env → config → "default".
  • Name validation (^[A-Za-z0-9_-]{1,64}$) enforced at the filesystem-boundary function (wallet_path_for_name) itself, not just at CLI entry points — so it holds regardless of call path.
  • In-memory TTL password cache replaces the on-disk CONFIG_DIR/.session file (no plaintext password ever written to disk); stale .session files are cleaned up automatically. This intentionally removes cross-invocation password caching — the only no-reprompt option across separate commands is DIN_WALLET_PASSWORD in .env.
  • dincli system list-accounts: enumerates named wallets + legacy fallback without decrypting.
  • New guides: Documentation/guides/wallet-setup.md (burner wallet rationale, generation via eth-account/OWS, funding — 10 DIN stake + ETH gas), Documentation/guides/keystore-migration.md (old .env pattern → encrypted keystore migration path).
  • Production-keystore callouts added to every doc that instructs raw-key .env setup (setup.md, GettingStarted.md, common.md, Model-workflow.md, DINCLI_Containerizaton_Guide.md, DEVELOPMENT_SETUP.md, docker node README/.env.example).

OWS (Open Wallet Standard) feasibility

Assessed per the task's ask. OWS (openwallet.sh) has a real CLI (ows wallet create, sign message, sign tx, mnemonic generate/derive), Node/Python SDKs, and an MCP server — but its documented command reference has no export subcommand, so wallet-setup.md uses it only for keystore creation, not export, and points operators to ows --help/upstream docs for the current export surface rather than asserting an unverified command. Recommendation: document-only for now — full signing delegation (dincli never holding the raw key) is plausible given OWS's MCP/REST/SDK interfaces, but implementing that integration is out of scope for this task.

Design deviations (flagged for confirmation, not blocking)

# Deviation Why
D1 Cross-invocation password caching removed entirely (in-memory only, per-process) The old on-disk .session cache is a real plaintext-on-disk risk; a module-level cache can't replicate cross-command convenience since each dincli invocation is a new process. DIN_WALLET_PASSWORD env is the only remaining no-reprompt path.
D2 --account stays an int (dev key index); --name labels saved keystores; --wallet selects at runtime Avoids one flag meaning two different things (dev index vs. production keystore name).
D3 Added dincli system set-wallet <name> to persist a default wallet name in config The --wallet/env/config three-tier resolution needs a way to actually set the config tier; nothing did before.
D4 Name validation moved into wallet_path_for_name() itself Makes path safety a filesystem-boundary invariant rather than something every caller must remember.

Part 2 — Filecoin Provider Adapter (Lighthouse)

  • New dincli/services/ipfs_lighthouse.py: upload_via_lighthouse/retrieve_via_lighthouse, verified against Lighthouse's own GitHub docs (not assumed) — POST https://upload.lighthouse.storage/api/v0/add, Bearer auth, multipart file field, nested {"data": {"Hash": ...}} response (different from Filebase's flat shape — this one place would silently break on a copy-paste from the Filebase code, so it's explicitly tested).
  • Wired in as a genuine first-class provider ("lighthouse" in SUPPORTED_IPFS_PROVIDERS), not the generic dynamic-module custom path — matches the task's concrete deliverables (dedicated module, dedicated test file, dedicated env var) better than the discussion doc's more general "use the custom path" framing.
  • IPFS_PROVIDER env var for provider selection (config wins if set — this is a real, generic addition to resolve_ipfs_config(), not Lighthouse-specific) and LIGHTHOUSE_API_KEY as an API-key fallback, satisfying the task's literal env-var ask on top of the config-first mechanism every other provider already uses.
  • API keys are now stored per-provider (ipfs_api_key_<provider>) instead of one shared flat field — fixes a real bug found during review: switching configure-ipfs from filebase to lighthouse without a fresh --api-key used to silently reuse the Filebase token instead of failing with a clear error. Legacy flat key still resolves for filebase (back-compat).
  • Docs: ipfs.md (new lighthouse section), plus CLAUDE.md/ReadMe.md/setup.md updated so they don't keep describing a stale "three provider" picture.
  • Tests: 15 across test_ipfs_lighthouse.py (new) and test_ipfs_config.py (extended) — nested-hash extraction, malformed-response handling, missing-key error, HTTP errors, exact retrieve URL/method, env-var provider selection, config-over-env precedence, cross-provider key isolation.

⚠️ Confirmed finding: Lighthouse retrieval is payment-gated, even for the file's own owner

Live-tested with a real account (not just mocks): upload works exactly as documented and returns a valid CID. Retrieval does notGET https://gateway.lighthouse.storage/ipfs/<cid> returns 402 Payment Required, with and without the uploader's own API key attached (identical response either way — this isn't an auth-header issue). A generic public IPFS gateway (ipfs.io) also fails for the same CID: "no providers found for the CID" — the content isn't discoverable on the public IPFS network at all.

This contradicts two explicit assumptions this task was built on:

  • The discussion doc's framing of Lighthouse as providing "a fast retrieval gateway" as a solved problem.
  • The task's own stated fallback: "Retrieval can fall back to the standard IPFS HTTP gateway; the CID is a CID regardless of which provider stored it" — not true here; no gateway currently serves this content without payment.

This is not a bug in ipfs_lighthouse.py — the adapter matches Lighthouse's documented API contract exactly, and the upload path is fully correct. It's an account/platform-level gate (Lighthouse appears to use HTTP 402 + likely the x402 micropayment protocol for gateway retrieval) that neither the discussion doc nor the task anticipated. Recommendation: before relying on Lighthouse for production retrieval, confirm whether this resolves with a funded/paid account or is a structural characteristic of the service — this may affect whether Lighthouse remains the right "first adapter" per the discussion doc, versus Storacha or another option. Raising this with Umer/Abraham directly; flagging here so it's not lost.

Test plan

tests/test_connect_wallet.py + test_ipfs_config.py + test_ipfs_lighthouse.py: 61 passed
Full suite: 68 passed, 1 pre-existing unrelated failure (test_dintoken.py::test_stake_uses_requested_amount),
            1 skipped, 134 environment-specific errors (require /home/azureuser, pre-existing)

All new code paths, all previously-existing tests (regression-checked), no new failures introduced.

Not in scope / deliberately deferred

  • SIGTERM handling (T0.2a) — deferred to P4 per the task.
  • Sponsored-upload / on-chain storage budget accounting for Lighthouse — P3 fee design work, per the discussion doc's own sequencing.
  • Resolving the Lighthouse payment-gate finding above — needs an architectural call, not a code fix.

Santiagocetran and others added 9 commits July 3, 2026 14:49
…i-account store, list-accounts

Security-sensitive keystore work for task_300626_3 Part 1 (P3-T0.2b).
Flag for Umer's needs-security-review before merge.

- In-memory TTL password cache (_PASSWORD_CACHE) replaces the on-disk
  CONFIG_DIR/.session cache; stale .session files are cleaned up on
  load. No cross-invocation persistence by design (D1) — only
  DIN_WALLET_PASSWORD env avoids re-prompting across commands.
- connect-wallet --keystore <path>: imports a standard eth-account
  JSON keystore, validates by decrypting to derive the checksum
  address, preserves the original keystore verbatim.
- Named multi-account keystore: CONFIG_DIR/wallets/wallet_<name>.json
  with a shared wrapper schema ({version, address, keystore, source,
  name}); _extract_keystore() normalizes wrapper/bare/demo forms.
  connect-wallet --name <name> saves under that name; legacy
  wallet.json still loads as "default" (named default wins over
  legacy when both exist).
- validate_account_name() enforces ^[A-Za-z0-9_-]{1,64}$ and is
  called from wallet_path_for_name() itself, the one function every
  read/write path funnels through — so path safety is a boundary
  invariant, not something every caller has to remember.
- Atomic writes (temp file at 0o600 + os.replace + defensive chmod)
  so wallet files end up 0o600 even when overwriting a pre-existing
  loose-permission file; wallets/ dir created at 0o700.
- New `dincli system list-accounts` command enumerates named wallets
  + legacy wallet.json without decrypting, dedups so a named default
  hides the legacy row.
- read_wallet/todo updated to resolve the active named wallet instead
  of hardcoding legacy wallet.json.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Top-level --wallet option in main.py (mirrors --network), --wallet
added to GlobalOptionsGroup so it can appear anywhere on the command
line, and DinContext.select_wallet()/resolved_wallet_name resolving
--wallet -> DIN_WALLET_NAME env -> config wallet_name -> "default".
select_wallet() validates the name and fails fast with a clean exit,
and main.py resolves the wallet before printing network status so an
invalid --wallet exits before any other output is printed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lates, and production-key callouts

- New Documentation/guides/wallet-setup.md: why a dedicated burner
  wallet, eth-account/OWS keystore generation, funding (10 DIN stake +
  ETH gas), and the key-management tiers table.
- New Documentation/guides/keystore-migration.md: old .env pattern vs
  encrypted keystores, migration via --keystore import, docker/operator
  setup, named-wallet precedence and runtime selection.
- New /.env.example (root, authoritative) and hardhat/.env.example
  (signposted, non-authoritative since Hardhat reads ../.env) with
  dev-only ETH_PRIVATE_KEY_<n> labeling and a keystore pointer.
- dincli/docker/node/.env.example + README.md: key-management section,
  wallets/ dir in the file tree, .session marked legacy-cleanup-only.
- Production-keystore callouts added wherever GettingStarted.md,
  DEVELOPMENT_SETUP.md, Model-workflow.md, common.md,
  DINCLI_Containerizaton_Guide.md, setup.md, and ipfs.md instruct
  raw-key/--account setup or link wallet-setup.md as a prerequisite.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers: existing key-loading paths (positional/--key-file/--account/
getpass/demo), --keystore import (valid/wrong-password/missing/
malformed), named accounts + --wallet/DIN_WALLET_NAME runtime
selection, legacy wallet.json fallback and named-default precedence,
read-wallet/todo wallet-awareness, wrapper-schema normalization,
name validation, atomic-write permissions, in-memory password cache,
mutual exclusivity, and CliRunner-based routing coverage for the
account-resolution skip-list. 46 tests, all passing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…R env var + provider-scoped API keys (D6/D7)

Part 2 of task_300626_3 (Filecoin Provider Adapter), per
Plans/part2-filecoin-lighthouse.md.

- New dincli/services/ipfs_lighthouse.py: upload_via_lighthouse posts
  to upload.lighthouse.storage/api/v0/add with Bearer auth and
  extracts the nested data.Hash response shape (different from
  Filebase's flat shape); retrieve_via_lighthouse gets from
  gateway.lighthouse.storage/ipfs/{cid} (no auth). Malformed
  responses raise a clear RuntimeError instead of leaking
  KeyError/JSONDecodeError.
- Wired into ipfs.py's upload_to_ipfs/retrieve_from_ipfs dispatch as
  a genuine first-class provider ("lighthouse" in
  SUPPORTED_IPFS_PROVIDERS), not the generic dynamic-module "custom"
  path (D5) — matches the task's concrete deliverables (dedicated
  module, dedicated env var, dedicated test file).
- D6: resolve_ipfs_config() now also checks IPFS_PROVIDER env var as
  a fallback when no ipfs_provider is set in config (config wins if
  set); applies to all providers, not just Lighthouse.
- D7: API keys are now stored per-provider
  (ipfs_api_key_<provider>) instead of one flat ipfs_api_key field,
  fixing a silent-misconfiguration bug — switching from Filebase to
  Lighthouse without a fresh --api-key no longer silently reuses the
  Filebase token. Legacy flat ipfs_api_key still resolves for
  filebase (back-compat); LIGHTHOUSE_API_KEY env var is a fallback
  for lighthouse.
- configure_ipfs validates each provider's key independently.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r references

- ipfs.md: four modes instead of three, full lighthouse provider
  section mirroring the filebase section, migration notes updated for
  the provider-scoped API key change.
- CLAUDE.md: "three interchangeable backends" -> four, Lighthouse
  added to the one-line list — this file is read as authoritative
  project context every session, so it needs to stay accurate.
- ReadMe.md: "three supported IPFS modes" -> four.
- setup.md: new "Option B - Lighthouse (Filecoin-backed IPFS)"
  alongside the existing Filebase option; env provider renumbered to
  Option C, custom to Option D.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rovider selection and API-key scoping

New tests/test_ipfs_lighthouse.py:
- Upload: extracts nested Hash from Lighthouse's data.Hash response
  shape; malformed response raises RuntimeError, not KeyError;
  missing API key raises ValueError before any HTTP call; HTTP error
  wraps as RuntimeError matching Filebase's format.
- Retrieve: uses the exact gateway URL, GET method, no auth header;
  HTTP error wraps as RuntimeError.

Extended tests/test_ipfs_config.py (regression + new-behavior):
- resolve_ipfs_config returns lighthouse provider when config-sourced
  and when IPFS_PROVIDER env-sourced; config wins over env when both
  are set.
- Filebase regression: legacy flat ipfs_api_key still resolves.
- Cross-provider isolation: a Filebase key is never reused as a
  Lighthouse key.
- Existing env-provider tests still pass unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Guard against a wallet.json / keystore.json containing a raw private key
accidentally landing in a commit during manual testing. dincli's real
wallet state lives under CONFIG_DIR (outside the repo), so these patterns
are purely a safety net for the keystore-security work in this branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
connect-wallet passed --name straight into validate_account_name at the
top of the command, so an invalid name raised a raw ValueError traceback
instead of the console.print + Exit(1) pattern used everywhere else
(including set-wallet). Wrap it to match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Santiagocetran

Copy link
Copy Markdown
Author

@umeradl flagging for security review before merge, per task_300626_3 — the keystore/wallet commits (5cfd390, 4c7881a, 8436e50) and the .gitignore key-file guard (3d1bf04) are the ones needing your eyes. Also see the ⚠️ Lighthouse retrieval finding in the PR description. Please don't merge until you've reviewed.

@umeradl

umeradl commented Jul 5, 2026

Copy link
Copy Markdown
Member

Merge status: partially merged into develop

We've manually cherry-picked the non-Lighthouse portions of this PR (commit 8436e50) into develop. The keystore/multi-account hardening (Part 1) is independent of the Filecoin/Lighthouse adapter (Part 2), so we split the merge rather than block Part 1 on Part 2's open issue (see the review comment below).

Fully merged:

  • .env.example
  • Developer/DEVELOPMENT_SETUP.md
  • Documentation/GettingStarted.md
  • Documentation/Model-workflow.md
  • Documentation/common.md
  • Documentation/guides/wallet-setup.md
  • Documentation/guides/keystore-migration.md
  • Documentation/technical/DINCLI_Containerizaton_Guide.md
  • dincli/cli/context.py
  • dincli/cli/core.py
  • dincli/docker/node/.env.example
  • dincli/docker/node/README.md
  • dincli/main.py
  • hardhat/.env.example
  • tests/test_connect_wallet.py

Partially merged (Lighthouse-specific parts excluded):

  • dincli/cli/system.py — kept keystore/multi-account logic (connect-wallet, list-accounts, set-wallet, read-wallet, etc.); dropped Lighthouse-specific branches (configure-ipfs lighthouse key handling/help text, todo diagnostics for the lighthouse provider).
  • dincli/cli/utils.py — kept multi-account/keystore/password-cache logic; dropped lighthouse from SUPPORTED_IPFS_PROVIDERS and the LIGHTHOUSE_API_KEY fallback in resolve_ipfs_config().
  • Documentation/guides/ipfs.md — kept the wallet-setup prerequisite link and the per-provider API-key migration note; dropped the Lighthouse provider section (doc still describes three IPFS modes: env/filebase/custom).
  • Documentation/setup.md — kept the production-keystore callout; dropped "Option B — Lighthouse" (.env/custom remain Options B/C).

Not merged yet:

  • .gitignore
  • CLAUDE.md
  • Documentation/ReadMe.md
  • dincli/services/ipfs.py
  • dincli/services/ipfs_lighthouse.py
  • tests/test_ipfs_config.py
  • tests/test_ipfs_lighthouse.py

Why Lighthouse is held back: this PR's own live test found the Lighthouse gateway returns 402 Payment Required on retrieval — even for the uploader's own file — and public ipfs.io also can't resolve the CID. Shipping the adapter now would add an IPFS provider that can't retrieve what it uploads, so it stays out of develop until that's resolved.

This PR remains open. Once the Lighthouse retrieval issue is resolved (or a workaround is confirmed), we'll merge the remaining Lighthouse-adapter files in a follow-up. See the review comment below for the full code review and a list of bugs to fix in the merged portion.

@umeradl

umeradl commented Jul 5, 2026

Copy link
Copy Markdown
Member

Code review — Part 1 (keystore hardening) + Part 2 (Lighthouse adapter)

Reviewed at commit 8436e50 (feat/validator-readiness). Scope: Developer/tasks/task_300626_3.md Parts 1 & 2.

Status: Part 1 (keystore/multi-account hardening) is merged into develop (see merge-status comment above). Part 2 (Lighthouse) is not merged — the adapter code is reviewed below for when it's ready, but it's blocked on the retrieval issue described in Bug #4.

Exceptional work

  • _extract_keystore() correctly normalizes three keystore shapes (wrapper/bare/demo) and is regression-tested for the double-wrap edge case (re-importing a file that's itself a previously-exported dincli wallet doesn't nest a keystore-inside-a-keystore).
  • Path-safety is enforced at the filesystem boundary (validate_account_name() runs inside wallet_path_for_name() itself), so every caller — CLI flag, env var, config value — goes through the same traversal check, proven with actual ../, a/b, /etc/passwd payloads in tests.
  • Found and fixed an independent bug while building the Lighthouse adapter: all providers previously shared one flat ipfs_api_key field, so switching configure-ipfs from filebase to lighthouse without a fresh --api-key would have silently reused the Filebase token. Fixed generically (ipfs_api_key_<provider>) rather than special-cased.
  • Live-tested the Lighthouse gateway rather than trusting the discussion doc or mocks, and reported a real 402 Payment Required on retrieval (see Bug Bump the npm_and_yarn group across 1 directory with 1 update #4).
  • 46 tests in test_connect_wallet.py covering mutual exclusivity, wrong-password/malformed-keystore/missing-file paths, atomic-write permission edges, and CLI skip-list routing.
  • main.py resolves --wallet before printing any other output — verified live: an invalid wallet name fails cleanly before Active Network: ... is printed.

Bugs to fix

  1. list-accounts source tag was swallowed by Rich markup (dincli/cli/system.py, was line 506): f" [{e['source']}]" — Rich interprets [imported]/[created]/[legacy] as markup tags and silently drops them, so the one piece of UX added to distinguish imported/created/legacy wallets never rendered.
    Status: already fixed in the merged develop copy (f" ({e['source']})" instead of brackets) — please carry this fix back into the PR branch so it isn't lost/reintroduced on the next rebase.

  2. No confirmation guard on connect-wallet --name <existing>. Running connect-wallet <new-key> --name aggregator a second time silently overwrites wallet_aggregator.json with a new key, no "already exists, overwrite?" prompt. Matches pre-PR single-wallet behavior, but the new multi-account model raises the cost of a typo'd --name on a real validator identity. Suggest an _confirm_or_exit-style guard, same pattern already used for send-eth. Not yet fixed in develop — still open.

  3. Latent risk: overwriting a named wallet can silently reuse a stale in-memory-cached password. In connect_wallet, _get_password(name, prompt=False) returns a cached password for name without going through the "Create wallet password"/"Confirm password" flow. Low practical exposure in a normal one-shot CLI invocation, but real for any future daemon/programmatic use that calls load_account() and connect_wallet() for the same name in one process. Suggest an explicit is_new_wallet flag to force the create/confirm path regardless of cache state.

  4. Lighthouse gateway returns 402 Payment Required on retrieval — this is what's blocking Part 2's merge. Reported in the PR: GET https://gateway.lighthouse.storage/ipfs/<cid> fails even for the uploader's own file, and public ipfs.io also can't resolve it. This contradicts the "a CID is a CID regardless of provider" assumption the task was scoped under. Needs to be resolved (paid tier? different gateway? account config?) before Lighthouse can ship as a supported IPFS provider — until then dincli/services/ipfs_lighthouse.py, the lighthouse provider wiring, and the associated docs/tests stay out of develop.

  5. Minor redundant I/O: load_account()_get_password() calls get_env_key("DIN_WALLET_PASSWORD"), then _cache_password_in_memory() calls it again — get_env_key re-parses .env from disk on every call (no memoization), so a single wallet unlock does two full .env parses. Not a functional bug, just avoidable overhead on a path that runs on nearly every dincli invocation touching an account.

  6. Doc nit: dincli/docker/node/README.md listed (legacy) .session/ with a trailing slash, implying a directory, but .session is a single file. Status: already fixed in the merged develop copy — please carry back into the PR branch.

Missed deliverables

  • OWS feasibility assessment is desk research, not a hands-on spike. The task asked whether dincli can enumerate accounts and request a signed transaction without handling the raw key, and what the failure modes look like (OWS not running / account not found / wrong chain). There's no evidence ows was actually installed and exercised — the write-up is honest about what it didn't verify, but the "recommendation" is effectively "we don't know yet," which the task treated as a load-bearing decision point.
  • Plans/part2-filecoin-lighthouse.md and Plans/pr-description-draft.md, referenced by commit messages/PR body, aren't part of the diff. Minor process note — the actual deliverables landed — but worth adding if a reviewer follows the commit trail into Plans/.

Test results (reproduced independently, not taken on faith)

  • tests/test_connect_wallet.py tests/test_ipfs_lighthouse.py tests/test_ipfs_config.py: 61 passed, matching the PR description.
  • Full suite (excluding tests/dincli/, which needs a Hardhat npm install not present in a fresh worktree): 68 passed, 1 failed, 1 skipped — the one failure (test_stake_uses_requested_amount) is pre-existing and unrelated to this PR.
  • File permissions verified live: 0700 on wallets/, 0600 on every wallet_*.json.
  • Both wrapped (dincli-exported) and bare (Account.encrypt()) keystore formats round-trip correctly through --keystore import.
  • Doc cross-references (wallet-setup.md/keystore-migration.md links from the 6 docs that reference them) all resolve.
  • No SIGTERM/signal handling added anywhere in dincli/ — correctly honors the task's note that T0.2a is deferred to P4.

Net take

Part 1 (keystore hardening) is solid — no security blockers, good test coverage, path-traversal handling is done at the right layer. It's merged. Part 2 (Lighthouse) is well-built adapter code but currently ships a provider that can't reliably retrieve what it stores; that's the one item actually blocking this PR from closing. Bugs #2, #3, #5 are open items worth fixing on this branch regardless of the Lighthouse timeline; #1 and #6 are already fixed on develop and should be ported back here so they don't regress.

umeradl added a commit that referenced this pull request Jul 5, 2026
Cherry-picks the keystore/multi-account portion of PR #16 into develop:
encrypted named keystores, --keystore import, in-memory password cache,
--wallet/set-wallet/list-accounts, and the wallet-setup + keystore-migration
guides. The Lighthouse/Filecoin IPFS adapter (Part 2) is held back — its own
live test found the retrieval gateway returns 402 even for the uploader's
own file, so it stays out of develop until that's resolved. PR #16 remains
open for that follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@umeradl

umeradl commented Jul 5, 2026

Copy link
Copy Markdown
Member

Merge commit pushed to develop: dc6ff23

dc6ff23

This is the Part 1 (keystore/wallet hardening) cherry-pick described above — Lighthouse/Filecoin adapter (Part 2) remains out of develop pending resolution of the 402 retrieval issue. PR stays open for that follow-up.

…om develop

Carry back the two review fixes already applied on develop so they don't
regress on the next rebase (PR InfiniteZeroFoundation#16 review, bugs InfiniteZeroFoundation#1 and InfiniteZeroFoundation#6):

- list-accounts: source label used [..] which Rich parsed as a markup tag
  and dropped; use (..) so imported/created/legacy renders.
- docker node README: .session is a file, not a directory — drop the
  trailing slash.
…teZeroFoundation#2, InfiniteZeroFoundation#3, InfiniteZeroFoundation#5 in connect-wallet

Bugs InfiniteZeroFoundation#2/InfiniteZeroFoundation#3 both touch connect_wallet and InfiniteZeroFoundation#3/InfiniteZeroFoundation#5 share the _get_password
signature + a new _UNSET sentinel, so they land together as one coherent,
cherry-pickable unit rather than incoherent split commits.

InfiniteZeroFoundation#2 — overwrite guard: connect-wallet now confirms before overwriting an
  existing named keystore, with --yes/-y to bypass (mirrors send-eth).
  `yes` is normalized to a real bool so direct callers that omit it (which
  receive a truthy OptionInfo) don't silently skip the guard. Keystore
  non-secret validation (exists/parse/_extract_keystore) is hoisted above
  the guard; the passphrase getpass + decrypt stay after it, so no secret is
  requested before the overwrite decision and a doomed --keystore fails fast
  without prompting. Guard keys on the exact target file; a legacy wallet.json
  for 'default' is a non-destructive migration and intentionally not guarded.
  Docstring corrected: demo mode is config-driven, not tied to --yes.

InfiniteZeroFoundation#3 — new-wallet password: _get_password gains is_new_wallet; when creating
  or overwriting a wallet the in-memory cache is skipped so a fresh keystore
  is never silently encrypted with a stale cached password. DIN_WALLET_PASSWORD
  env is still honored (deliberate automation path); only the cache is ignored.

InfiniteZeroFoundation#5 — single .env parse per unlock: load_account fetches DIN_WALLET_PASSWORD
  once and threads it into _get_password/_cache_password_in_memory via an
  _UNSET sentinel, instead of each self-parsing .env (get_env_key is not
  memoized). Callers that omit env_pass self-fetch as before.

Adds 7 tests: overwrite guard (omitted-yes/decline/accept/--yes/doomed-input),
stale-cache-not-reused on create, and single-env-parse on unlock.
@Santiagocetran

Copy link
Copy Markdown
Author

Update: review items addressed (Part 1)

@umeradl — pushed two commits to feat/validator-readiness addressing the review. All of this is Part 1 (keystore/wallet) code that already lives on develop, so each fix is written to be cherry-picked into develop independently — no need to wait on the Lighthouse item.

634ce1e — port #1 and #6 back from develop

The two fixes you'd already applied on develop were still in their broken form on this branch; carried them back so they don't regress on the next rebase:

5cd1a9b — review items #2, #3, #5

  • Bump cryptography from 46.0.4 to 46.0.7 in /dincli in the pip group across 1 directory #2: connect-wallet now confirms before overwriting an existing named wallet, with a --yes/-y bypass (mirrors send-eth). The direct-call Typer OptionInfo default is normalized so an omitted yes does not silently bypass the guard. Keystore non-secret validation (exists/parse/_extract_keystore) runs before the guard; the passphrase prompt + decrypt stay after it, so no secret is requested before the overwrite decision and a doomed --keystore fails fast without prompting. The guard keys on the exact target file; a legacy wallet.json for the default name is a non-destructive migration and is intentionally not guarded. (Also fixed the docstring — demo mode is config-driven, not tied to --yes.)
  • Bump web3 from 7.10.0 to 7.15.0 in /dincli in the pip group across 1 directory #3: new wallet creation/overwrite skips any stale in-memory cached password via is_new_wallet=True, so a fresh keystore is never silently encrypted with a previously-cached password. DIN_WALLET_PASSWORD is still honored for automation — only the cache is ignored.
  • Bump lodash from 4.17.21 to 4.18.1 in /hardhat in the npm_and_yarn group across 1 directory #5: wallet unlock fetches DIN_WALLET_PASSWORD once and threads it through the password helpers (_UNSET sentinel), avoiding the double .env parse per unlock.

The three were committed as one unit because they're coupled — _get_password gains both is_new_wallet (#3) and env_pass (#5) on the same signature, sharing the new _UNSET sentinel — so splitting them wouldn't cherry-pick cleanly anyway. The commit body enumerates each item.

Validation

  • uv run pytest tests/test_connect_wallet.py -q53 passed (46 existing + 7 new: overwrite guard incl. the omitted-yes case, stale-cache-not-reused on create, single-.env-parse on unlock).
  • Broader suite unchanged except the known, unrelated test_dintoken.py::test_stake_uses_requested_amount failure.

Still open on this branch

… OWS section

Closes the PR InfiniteZeroFoundation#16 review's 'OWS feasibility is desk research, not a hands-on
spike' gap with an actual spike (ows v1.4.2 + Python SDK installed, wallet
created, real Optimism Sepolia tx signed and cryptographically verified):
Developer/discussion/ows-delegation-feasibility.md.

Verdict is scoped: CONFIRMED that OWS can sign an EVM tx without exposing the
private key (verified via CLI and in-process Python SDK, byte-identical, signer
recovers to the OWS address); NOT proven for the stronger production claim of
mandatory scoped-policy delegation — the verified path is unscoped local vault
signing that isn't even passphrase-gated. Policy-enforced signing, passphrase
behavior, and an eth-account-compatible adapter are follow-up work.

Also corrects wallet-setup.md, which shipped a broken flow: OWS does NOT export
an Ethereum JSON keystore (ows wallet export prints the raw secret, interactive
only), so the 'export keystore -> connect-wallet --keystore' loop can't work.
Fixed the duplicate import, made the imported wallet active (set-wallet), fixed
the delegation status (planned, not shipped) and the policy guidance, and showed
the actual export command.
@Santiagocetran

Copy link
Copy Markdown
Author

OWS delegation — hands-on feasibility spike (closes the review's "desk-research only" gap)

@umeradl — redid the OWS assessment as an actual spike: installed ows v1.4.2 + the Python SDK, created a wallet, inspected the on-disk vault, and signed a real Optimism Sepolia (chainId 11155420) transaction, verified cryptographically (no funds, no broadcast). Pushed in 6905808.

Full write-up (on my fork branch, part of this PR's diff):
https://github.com/Santiagocetran/DevNet/blob/6905808081e895f9c22a18a3647b4fbf726b19d6/Developer/discussion/ows-delegation-feasibility.md

Verdict — scoped precisely

  • CONFIRMED: OWS can sign an EVM tx without exposing the private key — verified via both the CLI and the in-process Python SDK (byte-identical signatures; the signer recovers to the OWS account two independent ways). No daemon; fits dincli's one-shot model.
  • NOT yet proven (don't ship on this basis): the stronger claim that dincli can safely support OWS delegation with a mandatory scoped policy. The verified path is unscoped local vault signing by wallet name that bypasses the policy/API-key layer — and in v1.4.2 there's no CLI/env way to set a passphrase, so that path isn't even passphrase-gated (protection = filesystem perms only, i.e. weaker than our own passphrase-encrypted keystore). Policy-enforced signing (and any REST/MCP transport) was not exercised.

Caveats found (not in OWS docs)

  • Blind signingows sign tx signed 0xdeadbeef (exit 0) without validating it decodes as a tx.
  • Unscoped keysows key create mints a token with no --policy; whether a policy actually blocks a disallowed tx is untested.

Recommendation

Keep named multi-keystore as the shipped default (self-contained, passphrase-gated). Treat OWS delegation as a follow-up, not production-ready as described: a future task must verify (a) policy-enforced signing end-to-end, (b) passphrase-protected wallets + non-interactive credential passing, (c) an eth-account-compatible OwsAccount adapter (accept a web3 tx dict → serialize EIP-1559 → sign → reassemble raw → return .raw_transaction). OWS stays an operator tool, not a bundled dependency.

Also in this commit — wallet-setup.md corrections

The guide shipped a broken flow: OWS does not export an Ethereum JSON keystore (ows wallet export prints the raw secret, interactive-only), so the "export keystore → connect-wallet --keystore" loop can't work. Fixed that, plus a duplicate-import step, made the imported wallet active (set-wallet), corrected the delegation status (planned, not shipped) and the policy guidance, and showed the real export command.

These are Part-1/task deliverables and are cherry-pickable into develop independently of the Lighthouse item — the wallet-setup.md fix in particular corrects a currently-live misleading doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants