feat: OWS wallet signing + dual-backend execution auth#50
Conversation
Route standard EVM submit commands off persisted execution_backend metadata instead of direct signer selection. - add explicit local and OWS EVM submit backends - keep tempo submit on the persisted tempo lane - reject legacy signer flags for wallet-backed submits while preserving legacy_local compatibility Reference: Task 6
Reject --signer tempo for legacy_local and empty-backend actions so submit routing stays exclusive to persisted execution_backend. - keep legacy actions on the deprecated local signer lane - add a regression test covering tempo override rejection Reference: Task 6 follow-up
Tighten OWS submit validation and re-resolve wallet sender identity at submit time. - reject malformed non-empty tx hashes before receipt polling - resolve the persisted wallet sender from OWS metadata for the action chain - fail when persisted action sender disagrees with the resolved wallet sender - keep submit auth metadata OWS-first with deprecated local compatibility second Reference: Task 6 follow-up
Remove the obsolete ResolveStepExecutor path now that backend selection lives in ResolveExecutionBackend. Validate ExecuteAction against the resolved effective sender before persisting state so mismatched or empty senders fail closed while blank planned senders are still backfilled. Add focused regression tests for ExecuteAction sender handling and resolvePersistedOWSSender mismatch coverage.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if err := client.SendTransaction(ctx, signed); err != nil { | ||
| return common.Hash{}, wrapEVMExecutionError(clierr.CodeUnavailable, "broadcast transaction", err) | ||
| } | ||
| return signed.Hash(), nil |
There was a problem hiding this comment.
Local submit backend opens redundant RPC connections per step
Medium Severity
localSubmitBackend.SubmitDynamicFeeTx creates a brand-new ethclient connection (dial, TLS handshake) on every call to broadcast a transaction. Meanwhile, EVMStepExecutor already maintains a cached rpcClients map connected to the same RPC URLs for simulation, gas estimation, and nonce queries. This means every legacy-local step execution opens two separate TCP connections to the same endpoint. For multi-step actions (e.g., approve + swap), this compounds. The backend could accept or reuse the existing client instead of dialing a new one each time.
Additional Locations (1)
There was a problem hiding this comment.
Acknowledged — this is a valid optimization opportunity but not a bug introduced by this PR. The localSubmitBackend was extracted from the existing single-signer code path which already opened per-call connections. Reusing the executor's cached rpcClients map would reduce connection overhead for multi-step actions. Filed as a follow-up optimization, not blocking for this PR.
![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: 927f8a009d
ℹ️ 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".
| case "", string(execution.ExecutionBackendLegacyLocal): | ||
| signerBackend := strings.ToLower(strings.TrimSpace(input.Signer)) | ||
| if signerBackend == "" { | ||
| signerBackend = "local" | ||
| } | ||
| if signerBackend != "local" { | ||
| return resolvedSubmitExecution{}, clierr.New(clierr.CodeUsage, "legacy actions only support --signer local; tempo submit requires execution_backend=tempo") |
There was a problem hiding this comment.
This branch treats an empty execution_backend as legacy_local, which breaks backward compatibility for actions planned before this migration (those records do not have execution_backend set). A previously planned Tempo action now falls into the legacy-local lane and rejects --signer tempo (or tries local signing), so users cannot submit existing Tempo plans after upgrading. Please infer Tempo for empty-backend actions when the persisted chain is a Tempo chain (eip155:4217, eip155:42431, eip155:31318) to keep old actions executable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not a real issue for this codebase. Tempo execution was added in the same release cycle as the OWS migration — there are no pre-existing Tempo actions without execution_backend=tempo set. The Tempo planning path in runner.go always persists execution_backend=tempo explicitly (see the plan spec: 'Tempo plans should persist execution_backend = tempo explicitly so submit routing never depends on heuristics'). The empty-backend-to-legacy_local fallback only affects pre-OWS standard EVM actions, which correctly used local signing.
…release notes Add Wallet Setup (OWS) section to README between Install and Quick Start so users have signing prerequisites before hitting execution examples. Add explicit Deprecated section to CHANGELOG for --from-address, local signing flags, and local key env vars. Switch GoReleaser changelog to github-native for PR-linked release notes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace all deprecation language for --from-address/local signer with recommendation language so both signing backends are presented as first-class options, with OWS as the recommended path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
OWS remains recommended but local signer is now presented as a first-class option alongside it rather than as deprecated compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ggonzalez94
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate Tempo chain detection across two packages
Low Severity
The new isTempoChain function in execution_identity.go duplicates the existing IsTempoChain in step_executor.go. Both hardcode the same three Tempo chain IDs (4217, 42431, 31318) but operate on different types (CAIP-2 string vs int64). If a Tempo chain ID is added or changed, both must be updated independently, creating a risk of silent divergence.
Additional Locations (1)
There was a problem hiding this comment.
Valid observation about code duplication. Both isTempoChain functions exist because they operate on different types (CAIP-2 string vs int64 chain ID) in different packages. Consolidating them would require a shared package dependency — worth doing as a follow-up cleanup but low risk since the Tempo chain ID set is stable and both are tested.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27d26b97ac
ℹ️ 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".
| case "", ExecutionBackendLegacyLocal: | ||
| return ExecutionBackendLegacyLocal |
There was a problem hiding this comment.
Actions planned before this migration do not have execution_backend, but normalizeExecutionBackend now coerces an empty value to legacy_local. For persisted Tempo actions (e.g. eip155:4217, eip155:42431, eip155:31318), this routes execution through the standard EVM path instead of the Tempo executor, so previously valid plans can no longer be submitted correctly after upgrade. The empty-backend fallback should detect Tempo chains and preserve the Tempo execution path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same response as the parallel comment on execution_helpers.go — Tempo planning always persists execution_backend=tempo explicitly. There are no pre-migration Tempo actions without this field because Tempo execution was introduced in the same release.
| Kind: "wallet", | ||
| EnvVars: []string{ows.EnvOWSToken}, | ||
| Description: "Primary auth for wallet-backed execution (execution_backend=ows): set DEFI_OWS_TOKEN in the environment. " + |
There was a problem hiding this comment.
executionSubmitAuthRequirements marks DEFI_OWS_TOKEN as a required auth input for every submit command, but submit still supports non-OWS actions (legacy_local and tempo) that do not require this env var. Schema-driven clients that enforce required auth fields will reject valid local/tempo submissions before calling the CLI, so this metadata is now inconsistent with runtime behavior and should be optional/conditional.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The wallet auth entry intentionally omits Optional: true to signal that it is the primary/default auth path for new OWS-backed actions. The signer entry has Optional: true because it only applies to legacy_local actions. Schema-driven clients should use the execution_backend field from the persisted action (or the input_constraints metadata) to determine which auth path applies — not treat all non-optional auth entries as universally required. The schema description text makes this clear: 'Primary auth for wallet-backed execution (execution_backend=ows)'.
1 participant
Summary
--walletas the primary identity input--from-address+ private key) as a fully supported alternative — no deprecationEVMSubmitBackendinterface, converging atEVMStepExecutorfor shared simulation, gas estimation, nonce management, and receipt polling--from-address+--signer tempoNew
internal/ows/package~/.ows/wallets/ows sign send-txfor signing/broadcast withDEFI_OWS_TOKENExecution model changes
Actionmodel extended withwallet_id,wallet_name,execution_backendfieldsexecution_backend(ows,legacy_local,tempo)--walletand--from-addressare mutually exclusive at plan timeinput_constraintsforexactly_one_of(wallet, from_address)Docs consolidation
docs/concepts/execution-auth.mdxpage — single source of truth for signing backendsTest plan
go test ./...passesgo test -race ./internal/app/... ./internal/execution/...passesgo vet ./...passesfrom_addressdescriptionsnpx mint validate,broken-links,a11y)wallet_id+execution_backend=owsexecution_backend=legacy_local🤖 Generated with Claude Code