feat: KEEP-177 add Safe wallet integration

[joelorzet]

Summary

Per-org, per-chain Gnosis Safe v1.4.1 smart accounts, owned by the org's Turnkey EOA at threshold 1, with Zodiac Roles v2.0.0 enforcing per-call policies on every workflow write. Workflow msg.sender becomes the Safe; the EOA still pays gas and signs but holds no funds.

Three-mode signer routing

lib/safe/signer-resolver.ts chooses per (org, chain):

• eoa — direct Turnkey EOA write (default; signing toggle off or no Safe)
• safe — safe.execTransaction signed by the EOA owner. msg.sender = Safe. No policy gating.
• safe-role — rolesModifier.execTransactionWithRole from the EOA delegate. Modifier validates target + selector + parameter scope + per-period allowance bucket. The policy-enforcing path.

Hot-path is DB-first; recovers from cache drift via a one-shot chain probe + background reconcile when a Safe has Roles enabled on chain but no safe_roles row.

Zodiac Roles Modifier integration

Per-Safe Roles proxy deployed via the canonical ModuleProxyFactory + singleton (CREATE2 deterministic). Two policy mechanisms:

Per-protocol presets — wizard offers a catalog with per-parameter or contract-allowlist enforcement:

• Per-parameter (recipient pinned, amount bound to allowance): Aave V3, Compound V3, CoW Protocol, Uniswap V3, Spark, Lido, Rocket Pool, Balancer V2, Wrapped Native (WETH)
• Contract-allowlist: Morpho Blue, Pendle, Aerodrome (Base), Ajna, Chainlink CCIP, Sky/MakerDAO, Ethena, Yearn V3

Direct rules — per-recipient ERC-20 transfers/approves and native sends, scoped via c.calldataMatches([c.eq(counterparty), c.withinAllowance(allowanceKey)]) for ERC-20 (selector + spender/recipient + amount all bound) and target-only for native transfers.

Per-rule allowance buckets: the on-chain bucket key is keccak256(roleKey, "direct:<kind>:<counterparty>", token), so two direct rules on the same token (e.g. an approve and a transfer) get independent caps that match what the wizard's per-rule cap implies. Two transfer rules to different recipients are also independent.

Wallet overlay UX

• Safe card per chain with copy / explorer / "View on Safe" deep-link
• Active-signer badge when the per-chain signing toggle is on
• Status chip + button-and-dialog deploy flow for new chains
• Role permissions card: Active protocols section (with per-protocol Manage policies), Direct rules section (per-rule pencil edit, kind badge, token logo, remaining/cap display, counterparty with explorer link), Technical details disclosure
• Per-rule edit dialog for direct rules: edit cap, period, counterparty without leaving the panel
• Manage policies wizard: protocol catalog with chevron-expand preview, per-token cap inputs, defi-kit-backed per-parameter conditions, full diff against current on-chain state on Apply

Recovery + sync

• reconcileSafeRoleFromChain rebuilds the DB cache from on-chain state via getModulesPaginated + the Zodiac subgraph. Probes per-(token, kind, counterparty) for direct rules. Marks the role revoked if no modifier is enabled on chain.
• "Refresh from chain" button on the role panel triggers reconcile.
• Auto-backfill on the GET path when a Safe is deployed and signing is active but the DB shows no role row.

Chain support

Verified by direct eth_getCode probe on 2026-05-06 (see lib/safe/zodiac-contracts.ts):

• Mainnets (7): Ethereum, Optimism, Base, Arbitrum One, BNB, Polygon, Avalanche
• Testnets (6): Sepolia, Optimism Sepolia, Base Sepolia, Arbitrum Sepolia, BSC Testnet, Polygon Amoy

Avalanche Fuji (43113) is excluded from ROLES_SUPPORTED_CHAIN_IDS: the ModuleProxyFactory is deployed there but the singleton at the canonical address returns 0x (no bytecode). installRolesWithInitialConfig rejects unsupported chains up front with a structured error. Plain Safe execution still works on every chain in SUPPORTED_SAFE_CHAIN_IDS (14 chains total).

Plugin coverage

Four web3 step files route through the signer-resolver: transfer-token, transfer-funds, approve-token, write-contract. The pre-flight ERC-20 balance check in transfer-token-core reads balanceOf(safeAddress) when in safe-role/safe mode (was hitting the EOA, which never holds tokens). Read-only / transformation steps (read-contract, check-balance, query-events, etc.) bypass the resolver.

Out of scope (follow-ups)

• Native-transfer per-period value cap — needs a transaction guard or custom condition; deferred
• Avalanche Fuji support — needs Roles singleton deployed at canonical address
• Owner management UI (add / remove / threshold change)
• Wizard surface for skippedDirectRules (the API now returns dropped rules, the wizard doesn't render them yet)
• Per-rule invariant test (directRuleAllowanceKey separation across kind/counterparty)
• Subgraph indexer fallback when Zodiac's official subgraph isn't available on a target chain — reconcile falls back to "unknown, re-run wizard"

Migrations

One migration: 0069_safe_wallet_integration. Creates 5 tables: safe_wallets, safe_roles, safe_role_protocols, safe_role_allowances, safe_role_direct_rules with FKs and indexes. Generated against a synthetic 0068 baseline so the diff is safe-only and lands cleanly on top of staging's 0065-0068 without re-applying their DDL.

[suisuss]

Review: Zodiac Roles Modifier integration

Focused pass on the role install / scope / allowance surface. Verified against the merged code; one HIGH finding worth resolving before this lands on staging, plus a few medium/low items and follow-up suggestions.

Verified

Claim	Where
Canonical ModuleProxyFactory (CREATE2)	lib/safe/zodiac-contracts.ts:27
Singleton-via-deployModule	roles-orchestrator.ts:684-714
Per-parameter presets (Aave V3, Compound V3, CoW, Uniswap V3, Spark, Lido, Rocket Pool, Balancer V2, Wrapped Native)	condition-templates.ts:255-313 via defi-kit
Contract-allowlist presets	protocol-registry.ts entries with templateSlug: \"target-only\"
ERC-20 direct rule condition	roles-orchestrator.ts:246-255 — calldataMatches([eq(counterparty), withinAllowance(allowanceKey)], [\"address\", \"uint256\"])
Per-rule allowance key	zodiac-contracts.ts:136-148 — keccak256(roleKey ‖ keccak256(\"direct:<kind>:<lowercase-counterparty>\") ‖ getAddress(token))

---

HIGH — native-transfer caps are silently disabled

buildDirectRulePermission (roles-orchestrator.ts:218-234) has a conditional etherWithinAllowance: allowanceKey branch for native rules, but the install site at line 739-747 hard-codes allowanceKey = null for native:

const allowanceKey =
  rule.kind === \"native-transfer\" || !rule.tokenAddress
    ? null
    : directRuleAllowanceKey(...);

So the etherWithinAllowance branch is unreachable in production. Meanwhile:

• flattenInstallInput still emits an allowance row for native rules (using NATIVE_TOKEN_SENTINEL as the token)
• The per-allowance loop at line 819-858 calls setAllowance on chain for that bucket
• But no condition reads from it

Net effect: a native-transfer direct rule with "10 ETH / week" cap will store the cap on chain as a (key, refill, period) bucket and enforce only target-only at the role level — i.e., the role can send any amount as long as the recipient matches. The wizard's per-rule cap for native rules is collected and persisted but not enforced.

This is either a code bug (line 739 should compute the native key and pass it through) or a UI bug (wizard shouldn't collect a cap until the EtherWithinAllowance path is fully wired). Pick one and fix the dead branch accordingly.

MEDIUM — defi-kit is caret-pinned

\"defi-kit\": \"^2.26.9\",
\"zodiac-roles-deployments\": \"3.3.0\",
\"zodiac-roles-sdk\": \"3.4.1\"

zodiac-roles-* are exact (good). defi-kit is ^2.26.9 — a minor release can change the audited presets (e.g., aave_v3.deposit(...)) silently. The entire security boundary of per-parameter mode IS what those presets emit. Suggest exact pinning so upgrades become explicit code review with diff against new preset outputs.

- \"defi-kit\": \"^2.26.9\",
+ \"defi-kit\": \"2.26.9\",

MEDIUM — "contract-allowlist" mode is coarser than the name suggests

buildTargetOnlyTemplate (condition-templates.ts:221) lets the role call any function on the protocol's target addresses. Concretely under Morpho Blue's target-only mode the role can call:

• supply, borrow, withdraw — expected
• createMarket, setFeeRecipient, enableLltv — likely not expected by the admin who toggled "Morpho Blue" on

The code is honest about this ("Target-level allowlist for protocols without per-parameter templates"), but the wizard's UX should make the asymmetry explicit. Suggest a different badge / explainer for target-only protocols, e.g. "allows all functions on listed contracts; per-function scoping ships in a future release."

LOW — roleKey derivation packs orgId as raw string

ethers.solidityPacked(
  [\"string\", \"string\", \"string\", \"uint256\", \"string\"],
  [\"kh-role:\", organizationId, \":\", BigInt(chainId), \":automation\"]
)

solidityPacked does not length-prefix strings. UUID v4 orgIds are safe (no :), but if a future serializer change introduces orgIds with : in them, two different (orgId, chainId) pairs could pack to the same bytes. Cheap defensive fix:

ethers.keccak256(
  ethers.solidityPacked(
    [\"string\", \"bytes32\", \"uint256\"],
    [\"kh-role:\", ethers.id(organizationId), BigInt(chainId)]
  )
)

LOW — counterparty case-folding inconsistency

directRuleAllowanceKey lowercases the counterparty before hashing into the string digest; the token in the same function is checksum-normalized via getAddress. Internally consistent today, but if another caller reconstructs the key using ethers.getAddress(counterparty) they'd get a different digest. Worth a docstring on directRuleAllowanceKey noting that counterparty is lowercased before hashing.

---

Worth surfacing (intentional, not a bug)

The role-holding EOA can bypass the modifier. ownerAddress (the Turnkey EOA) is assigned the role AND is the Safe's threshold-1 owner. It can:

1. Call rolesModifier.execTransactionWithRole(...) — gated by the role's scope + allowances
2. Call safe.execTransaction(...) directly — bypasses the modifier entirely

This is the intended design (the EOA needs to be able to recover the Safe), but it means the role is policy, not boundary. A compromised Turnkey EOA can drain the Safe via path (2). Worth being explicit about this in user-facing language so readers don't assume the modifier is enforcing absolute limits.

The hardening path (if/when desired): multi-owner Safe with threshold>1. The schema (safe_wallets.owners jsonb, threshold integer) already supports this; the install wizard sets threshold=1.

---

What's well-designed

• Modifier ownership = avatar = target = Safe address. Puts the Safe in control of its own modifier configuration. The EOA can use the role but cannot widen it. Clean threat model.
• Single MultiSend for enableModule + assignRoles + setDefaultRole + scope + setAllowance. Atomic install — either everything lands or nothing does.
• Per-rule allowance keys. (roleKey, kind, counterparty, token) quadruple isolates buckets correctly. "approve to A for USDC" and "transfer to B for USDC" do not share a cap. Verified by reading both directRuleAllowanceKey and the per-allowance setAllowance loop.
• Reliance on defi-kit's audited presets for per-parameter mode. These are reviewed code shared across Karpatkey, Gnosis Guild, and others.
• parseModuleProxyCreationEvent filters by factoryAddress before parsing — cannot be fooled by a spoofed event from a malicious contract in the same tx.

---

Suggested follow-up tickets

1. Decide what to do with native-transfer caps (HIGH finding) — wire the EtherWithinAllowance path through end-to-end, or remove the dead branch + DB bucket + wizard control.
2. Pin defi-kit exactly (one-line package.json change).
3. Wizard UX clarity for target-only protocols (different badge / language).
4. Document the EOA-bypass property in user-facing Safe wizard help text.

Items 1 and 2 are small. 3 and 4 are UX/comms work.

[suisuss]

Review: Recovery + sync

Focused pass on reconcileSafeRoleFromChain, the GET-path auto-backfill, and the "Refresh from chain" UI affordance.

Verified

Claim	Where
reconcileSafeRoleFromChain rebuilds DB cache from on-chain state	roles-orchestrator.ts:2564+
getModulesPaginated enumerates Safe modules	zodiac-roles.ts:377-405
Zodiac subgraph for direct-rule details	roles-orchestrator.ts:2832 via zodiac-roles-sdk fetchRole
Probes per-(token, kind, counterparty) for direct rules	buildReconcileProbeSet + extractDirectRulesFromChain (roles-orchestrator.ts:2455+ and :2332+)
Marks role revoked when no modifier enabled	roles-orchestrator.ts:2587-2594 — update(safeRoles).set({status: \"revoked\"}) when findRolesModifierForSafe returns null
"Refresh from chain" button	components/safe/role-permissions-card.tsx:195 (syncFromChain) wires to POST /api/user/safe/[safeId]/role/reconcile
Auto-backfill on GET when Safe deployed + signing active but no DB role	getSafeRoleWithBackfill (roles-orchestrator.ts:2173+) and the signer-resolver probe (signer-resolver.ts:60-100, the workflow-write twin path)

Findings

HIGH — subgraph outage during update can silently delete direct rules

The reconcile path's direct-rule discovery has two distinct sources of truth depending on what's being rebuilt:

• allowance buckets (caps, period, refill): probed directly on chain via readRoleAllowance(modifier, key) for each candidate key. Chain is source of truth ✓
• direct rule details (kind, counterparty): only the Zodiac subgraph via fetchRole(...). On subgraph outage the comment at roles-orchestrator.ts:2829-2832 says it's swallowed and the step is skipped

The skip path returns [] from extractDirectRulesFromChain. That on its own would just mean "no direct rules surfaced in the UI this reconcile" — degraded but not destructive.

The compound risk is updateRolesConfig: that function calls reconcileSafeRoleFromChain internally to read "current" state before computing the diff to apply on chain. If the subgraph is unreachable during an update:

1. Reconcile completes, but safeRoleDirectRules is empty (subgraph skipped)
2. updateRolesConfig reads currentDirectRuleRows = listRoleDirectRules(role.id) → empty
3. The desired-role build does not include those direct rules
4. The diff against the on-chain scope removes their target/function permissions
5. MultiSend lands, modifier loses the direct-rule scope

Net: a subgraph outage during update silently removes direct rules from the modifier's on-chain scope. The cap buckets stay (set on chain by the next setAllowance pass), but the conditions that gate them are gone.

Fix shape options:

• (a) updateRolesConfig refuses to proceed if subgraph is unreachable / returns empty when the modifier on chain has functions matching the ERC-20 selectors. Force user to retry when subgraph is back.
• (b) Decode the modifier's scope tree from chain (via the modifier's view methods, not the subgraph) and reconstruct direct rules from there. Heavier but removes the subgraph as a write-path dependency.
• (c) Persist direct-rule rows in a way that survives reconcile-from-empty-subgraph — i.e., only delete direct rules from DB when chain explicitly confirms they're gone, not when the subgraph couldn't be reached.

I'd lean (a) for KEEP-177's timeline (smallest change) with a follow-up ticket for (b) or (c) as a hardening pass.

MEDIUM — reconcile's chain reads bypass executeWithFailover

reconcileSafeRoleFromChain constructs a single-armed RpcProviderManager at roles-orchestrator.ts:2568-2572 then reads chain state via the raw provider:

const rpcManager = await getRpcProviderFromUrls(rpcUrl, undefined, safe.chainId);
const provider = rpcManager.getProvider();
const enabledModules = await readEnabledSafeModules(provider, safe.safeAddress);
const rolesModifierAddress = await findRolesModifierForSafe(provider, ...);

This is inconsistent with the rest of the post-KEEP-551 read paths and inconsistent with the signer-resolver's probeRolesModifierFromChain (which is routed through executeWithFailover — see the local commit aa040b67 "route Safe role-modifier chain probe through RPC failover").

Practical effect: a transient primary-RPC outage during "Refresh from chain" or during workflow-write GET auto-backfill returns a failure / null where a failover-aware probe would have succeeded.

Fix: route the two probe reads (readEnabledSafeModules, findRolesModifierForSafe) and the per-allowance readRoleAllowance calls through rpcManager.executeWithFailover(p => p.X), and pass a fallbackRpcUrl to getRpcProviderFromUrls (the chain row has it; signer-resolver does this already). Same pattern, two functions.

MEDIUM — delegateAddress is rebuilt from the current org wallet each reconcile

const ownerWallet = await getOrganizationWallet(safe.organizationId);
const ownerAddress = normalizeAddressForStorage(ownerWallet.walletAddress);
// ...
{
  delegateAddress: ownerAddress,
  // ...
}

If the org rotates its EOA between install and reconcile, the DB row writes the new wallet address as the delegate. On chain, the role's assignRoles was bound to the old delegate, so:

• DB says: "delegate = X"
• Chain enforces: "delegate = Y"
• Workflow writes use initializeWalletSigner (resolves to current EOA = X) → modifier rejects because X doesn't hold the role

Wallet rotation is rare but not impossible (Turnkey wallet recovery, org migration). Worth fixing by reading the delegate from chain — either from a RolesModifier.AssignRoles event log or from the modifier's members(roleKey, address) mapping — rather than reconstructing it from the current DB org wallet.

LOW — probe set is bounded by KeeperHub's catalog + existing DB rows

buildReconcileProbeSet builds candidates from:

1. Catalog protocols × per-protocol default tokens
2. Synthetic \"direct\" slug × well-known tokens on this chain
3. Existing DB rows (preserves custom tokens)

A modifier configured outside the wizard (someone enabled a Roles modifier via safe.global or a script and gave it scope for a custom protocol or a non-well-known token) would have on-chain allowance buckets the probe never tries to read. Those buckets stay on chain, the DB sees nothing.

This is probably acceptable — KeeperHub only manages modifiers it installed itself. But the in-code comment says reconcile "closes the silent-bypass hole that previously downgraded these workflows to unscoped safe.execTransaction", which is true for the routing decision in signer-resolver but NOT for the DB cache itself. Worth being explicit that the cache is rebuilt from our catalog's view of the world, not from on-chain ground truth. Users will only ever see what we thought to probe for.

LOW — MAX_PAGES = 10 × PAGE_SIZE = 100 caps modules at 1000

readEnabledSafeModules (zodiac-roles.ts:377-405) hard-caps at 1000 enabled modules per Safe. That's plenty for normal Safes; only weird power-user configurations would hit it. Worth raising the cap or removing it (the iteration is bounded by the next == 0x0 sentinel anyway) so we don't silently miss modules on a Safe that has many enabled. Cheap defense.

NOTE — "Refresh from chain" / install race window is small but non-zero

If a user clicks "Refresh from chain" while an install or update's MultiSend has just confirmed on chain but the orchestrator's DB write is still in flight, the reconcile observes chain-installed + DB-not-installed, runs upsert via (safe_wallet_id, role_type) unique index. The install's subsequent transaction then upserts again. Order-of-writes determines which row "wins." No duplicate rows (good), but field values from the slower-to-commit transaction overwrite the faster one. Likely benign in practice; worth knowing exists.

What's well-designed

• Revocation on no-modifier-enabled correctly distinguishes "never had a role" from "role was revoked." Only flips status to revoked if there's an existing non-revoked row — no spurious DB writes on first-time probes of a vanilla Safe.
• API route authorization is layered correctly: validateSafeAdmin checks the caller's role, then getSafeForOrg(safeId, organizationId) confirms the Safe belongs to the caller's org. No IDOR.
• Subgraph integration is best-effort with logged failures — outage doesn't break the rest of reconcile. The cap-vs-detail separation (chain for caps, subgraph for kind/counterparty) is the right line — finding #1 is about what happens downstream of the failure, not the failure handling itself.
• Pagination is correct for ordinary scale. Cursor sentinel + zero-check is the documented Zodiac pattern.
• GET-path backfill is idempotent via (safe_wallet_id, role_type) unique index — concurrent backfills cannot produce duplicate role rows.

Suggested follow-up tickets

1. Subgraph-outage destructive-update (HIGH) — refuse to update if direct rules can't be re-fetched, or decode from chain.
2. Route reconcile reads through executeWithFailover (MEDIUM) — consistency with the rest of post-KEEP-551 read paths.
3. Source delegateAddress from chain (MEDIUM) — read from members() mapping or AssignRoles event, not from current org wallet.
4. Raise / remove MAX_PAGES cap (LOW) — sentinel-based termination is already correct.

[suisuss]

Review: Plugin coverage

Focused pass on the four write-path step files and verification that read-only steps don't accidentally route through the resolver.

Verified

Claim	Where
Four write steps route through resolveSignerMode	transfer-token-core.ts:308, write-contract-core.ts:245, transfer-funds-core.ts:168, approve-token-core.ts:209
Three-mode dispatch (eoa / safe / safe-role)	Each of the four files has the same if (safe-role) … else if (safe) … else (eoa) block around lines 274/376/394 of the respective files
ERC-20 balance preflight reads balanceOf(safeAddress) in safe/safe-role	transfer-token-core.ts:436-439: tokenHolderAddress = signerMode.kind === \"safe-role\" || signerMode.kind === \"safe\" ? signerMode.safeAddress : signerAddress
Sponsorship (ERC-4337) gated to EOA mode	All four files have signerMode.kind === \"eoa\" in the sponsorship eligibility check (the comment explicitly cites the 4337 bundler's msg.sender swap as the reason)
Read-only / transformation steps bypass the resolver	grep of read-contract*, check-balance*, check-allowance*, check-token-balance*, query-events*, query-transactions*, get-transaction*, decode-calldata*, batch-read-contract*, assess-risk* returns no resolveSignerMode or Safe-executor imports ✓

The balance-check fix in transfer-token-core is real and load-bearing — the previous code checked the EOA's balance, which is always near-zero in Safe mode and would have produced false-positive "insufficient balance" failures before the tx ever simulated against the Safe.

Findings

MEDIUM — preflight balance check exists only for transfer-token, not transfer-funds

transfer-token-core.ts:436-449 reads balanceOf(tokenHolder) and short-circuits with a clean error if insufficient. transfer-funds-core.ts (native ETH) has no analogous provider.getBalance(safeAddress) check. The orchestrator's estimateGas will still fail downstream, but the error message a user sees is much further from the cause ("transaction reverted" with cryptic revert data rather than "Safe has 0.4 ETH, you tried to send 10").

This is a pre-existing gap, not a regression from this PR — flagging because it's the natural completion of the same fix the description highlights. Suggest a follow-up to add a provider.getBalance(holderAddress) preflight in transfer-funds-core mirroring the transfer-token-core pattern (holder = signerMode.safeAddress in safe/safe-role, else EOA).

MEDIUM — Safe-routing block is duplicated across four files

Each of the four cores hand-codes the same three-arm dispatch:

if (signerMode.kind === \"safe-role\") {
  receipt = await executeContractCallAsRole(signer, {
    safeAddress, delegateAddress, rolesModifierAddress, roleKey,
    contractAddress, abi, functionKey, args,
  }, session, { chainId, workflowId, rpcManager });
} else if (signerMode.kind === \"safe\") {
  receipt = await executeContractCallAsSafe(signer, {
    safeAddress, ownerAddress, contractAddress, abi, functionKey, args,
  }, session, { chainId, workflowId, rpcManager });
} else {
  receipt = await adapter.executeContractCall(...);
}

The boilerplate is ~25-30 lines per file × 4 files. Concerns:

1. Drift risk: a change to one site (e.g. plumbing a new option through ExecuteAsSafeOptions) needs to be replicated in three more, with no compile-time check that it was.
2. New-plugin onboarding: a fifth write plugin (e.g. a swap step) needs to copy this same block. Easy to forget a field, easy to forget workflowId, easy to forget to thread rpcManager.

Suggest extracting a thin helper, e.g.:

// lib/safe/route-write.ts (or similar)
export async function executeWriteRoutedBySigner(
  signer: ethers.Signer,
  signerMode: SignerMode,
  call: { contractAddress; abi; functionKey; args; value? },
  session: NonceSession,
  options: { chainId; workflowId?; rpcManager: RpcProviderManager;
             adapter?: ChainAdapter; gasOverrides?; ... },
): Promise<TransactionReceipt>

Each core file's call shrinks to ~5 lines. Not blocking — file as a refactor follow-up.

LOW — read-only steps have their own RPC failover story; worth a one-line note in plugins/CLAUDE.md

The read-only steps correctly bypass resolveSignerMode, but they do their own RPC reads — whether each routes through rpcManager.executeWithFailover vs raw provider was outside my review scope and is potentially uneven. Not flagging individual files here; just noting that the convention "writes go through the resolver, reads stay raw" is enforced by code-pattern rather than by lint or type. A one-liner in plugins/CLAUDE.md ("writes that may run from a Safe must route through resolveSignerMode; reads don't, but they should still go through rpcManager.executeWithFailover not the raw provider") would make the convention durable for future plugins.

LOW — NonceConflictError from the helper surfaces as a generic step error at the plugin layer

After KEEP-551, the Safe executors throw NonceConflictError when broadcast failed AND on-chain has no trace of the signed hash. The four core files catch only via withStepLogging's generic handler. So:

• The workflow executor sees "step failed"
• It retries the step
• Retry resolves: new nonce acquired, fresh sign + broadcast, usually succeeds
• The user-facing log says nothing about why the first attempt failed

The system recovers, but the cause is hidden. Worth catching NonceConflictError specifically at the plugin layer and surfacing a more informative message ("transaction was lost due to nonce conflict — retrying"). Tiny UX win, no correctness gain. File as a follow-up.

NOTE — signing toggle off → EOA fallback shows EOA balance in error messages

When isSigningActive = false on a Safe row, resolveSignerMode returns kind: \"eoa\". The balance preflight then correctly uses signerAddress (EOA) at line 436-439. If the user toggles signing off while their Safe is funded but their EOA is empty, they'll see "Insufficient X balance: have 0, need …" referring to the EOA, not the Safe. That's technically correct (the EOA is what's being charged when signing is off) but could surprise users who expected "signing off" to mean "sign locally, don't use Safe" rather than "use EOA's balance." Not a code issue; a UX clarification worth making in the wizard's signing-toggle copy.

What's well-designed

• Three-arm dispatch is explicit and consistent. Each branch builds the right request shape for its executor; no silent fallthrough between modes. The compiler enforces that all three signerMode.kind values are handled because the type union is \"eoa\" | \"safe\" | \"safe-role\".
• ERC-4337 sponsorship correctly skipped in Safe modes. The comment cites the right reason (bundler swaps msg.sender to its own smart account); the explicit kind === \"eoa\" gate prevents this from regressing silently if someone re-orders code.
• Token holder for balance check correctly follows mode. The fix from EOA to Safe address makes the preflight error actually match the on-chain reality.
• Read-only / transformation steps don't import resolveSignerMode. Verified across 10 read-side files; clean separation between read and write surfaces, and crucially no DB read from safe_wallets on hot read paths.

Suggested follow-ups

1. Add provider.getBalance(holderAddress) preflight to transfer-funds-core (mirrors the transfer-token-core pattern, ~15 lines).
2. Extract executeWriteRoutedBySigner(signer, signerMode, call, session, options) to dissolve the duplicated three-arm dispatch.
3. Add NonceConflictError-specific catch at the plugin layer for better error messages on retry.
4. One-line convention note in plugins/CLAUDE.md: writes must route through resolveSignerMode; reads stay raw but should use rpcManager.executeWithFailover.

None blocking — these are refactor / UX polish items, not correctness gaps.

[suisuss]

Review: DB schema + migration (0073_safe_wallet_integration)

Fresh pass on the schema/migration surface, complementing the prior reviews of roles/recovery/plugin coverage. Verified against the snapshot at drizzle/meta/0073_snapshot.json and the upsert call sites in roles-orchestrator.ts.

Note: the PR description still refers to migration 0069 and a "0065-0068 baseline" — the rebase against staging shifted this to 0073 (staging now sits at 0072). Cosmetic, but worth correcting for downstream reviewers.

---

HIGH — safe_role_direct_rules unique index doesn't enforce uniqueness for native-transfer rules

schema-extensions.ts:317 declares tokenAddress: text("token_address") (nullable; "null for native-transfer" per the inline comment). The unique index at schema-extensions.ts:330-335:

uniqueIndex("safe_role_direct_rules_role_kind_token_cp_unique").on(
  table.roleId, table.kind, table.tokenAddress, table.counterparty,
)

does not pass .nullsNotDistinct(). Confirmed in the snapshot — nullsNotDistinct is absent on this index. Postgres' default is NULLs-are-distinct, so two rows of (r1, "native-transfer", NULL, 0xRecip) do not conflict.

This bites the upsert in three places in roles-orchestrator.ts (lines 1093, 1726, 2965), all of which target [roleId, kind, tokenAddress, counterparty]. For native-transfer rules:

1. First Apply: row inserted with tokenAddress = null (line 1078: rule.tokenAddress ? normalizeAddressForStorage(...) : null).
2. Second Apply / edit of same rule: ON CONFLICT target compares (r1, native-transfer, NULL, 0xRecip) against existing — NULL ≠ NULL → no conflict → new row inserted instead of update.
3. Orphan-cleanup at roles-orchestrator.ts:1748-1755 coalesces tokenAddress ?? "" when building both desiredRuleKeys (line 1710) and the existence check (line 1749). Two duplicate DB rows both match the same desired key → neither is deleted.

Net: each edit of a native-transfer rule's cap/period leaves an orphaned duplicate row. On-chain state remains correct (the modifier doesn't read this table); the wizard and role-permissions card will start rendering N rows for one logical rule, and the "current state" diff input becomes wrong.

Worth noting this is independent from the EtherWithinAllowance HIGH the recent ec07954c commit fixed — that addressed the on-chain enforcement path (allowance-key derivation); this is a DB-layer integrity issue on the cache rows.

Fix options (one-line each):

1. .nullsNotDistinct() on the unique index (PG 15+ only — confirm the deployed major version supports it; no other migration in this repo currently uses it).
2. Use NATIVE_TOKEN_SENTINEL for native rules and make token_address NOT NULL. Mirrors what safe_role_allowances already does and removes the NULL semantics from the index entirely. This is the more defensive fix and works on any PG version.

Option 2 also resolves the next item.

---

MEDIUM — token_address nullability inconsistent between sibling tables

Same column name, two conventions:

Table	token_address	Native-rule representation
safe_role_allowances	text NOT NULL	NATIVE_TOKEN_SENTINEL (snapshot confirms notNull=true)
safe_role_direct_rules	text (nullable)	NULL (snapshot confirms notNull=false)

The tokenAddress ?? "" coalescing on roles-orchestrator.ts:1710,1749 is a tell — readers of one table need to remember which convention applies. Picking the sentinel for both is consistent with the HIGH fix above and removes the asymmetry.

---

MEDIUM — status text columns have no CHECK / enum constraint

Four columns with text defaults but unconstrained values:

Column	Default	Code-side valid values
safe_wallets.status	'pending'	pending, deploying, deployed, failed
safe_roles.status	'active'	active, revoked
safe_role_protocols.status	'allowed'	allowed, revoked
safe_role_direct_rules.status	'allowed'	allowed, revoked

A typo at any write site ("REVOKED", "revoked ") inserts silently and breaks the WHERE status = 'revoked' predicate reconcileSafeRoleFromChain uses to flip the row. Either a PG enum type per column or a CHECK (status IN (...)) per table catches this at insert time. Hygiene, not a current bug — schedule for a follow-up migration.

---

MEDIUM — wei columns are text with no numeric validation

max_refill_wei, refill_wei, last_chain_balance_wei, amount_human are all text. Text is the right choice over int8 (uint256 overflows int64), but text permits any string. A serialization regression elsewhere could insert "undefined" or "1e18" (which BigInt() rejects) and you'd discover it at read.

Two cheap defenses, in increasing strictness:

1. CHECK (max_refill_wei ~ '^[0-9]+$') per column.
2. Drizzle numeric → PG numeric(78,0). Arbitrary precision, validated on insert. Mature ETH-adjacent ORMs (Ponder, Rindexer) default to this.

Hygiene, follow-up.

---

LOW — owner_wallet_id ... ON DELETE set null silently loses delegate context

safe_wallets.owner_wallet_id references organization_wallets (legacy table name para_wallets) with ON DELETE set null. If the org's wallet row is replaced by delete-and-recreate rather than update (e.g., a provider migration), the Safe's owner_wallet_id is NULLed silently. On-chain ownership is unchanged, so the chain still knows the original owner — but DB and chain now disagree, with no read-time reconciliation in safe_wallets (unlike safe_roles.delegate_address, which the prior recovery review already flagged).

Either tighten to ON DELETE restrict (rare event; better visible) or resolve from chain when NULL. Not blocking.

---

What's well-designed

• Cascade chain bottoms out cleanly — safe_wallets delete → safe_roles → {protocols, allowances, direct_rules}. No orphaned children possible from below.
• (safe_wallet_id, role_type) unique correctly bounds the schema to one-role-per-Safe-per-type while leaving multi-role-type as a purely additive future change.
• Indexes match access patterns — every WHERE roleId = ? / WHERE safeWalletId = ? / WHERE organizationId = ? has a covering single-column index alongside the composite unique.
• ALTER TABLE organization_tokens ADD COLUMN safe_wallet_id is metadata-only on PG 11+ (nullable, no default). Safe on a large existing table.

---

Suggested follow-up tickets

1. nullsNotDistinct() or native-sentinel on safe_role_direct_rules (HIGH — small fix, in this PR). Add a unit test that re-Applies the same native-transfer rule and asserts row count stays 1.
2. Unify tokenAddress nullability across safe_role_* (MEDIUM — implied by Bump better-auth from 1.3.34 to 1.4.2 #1 if you take the sentinel route).
3. CHECK / enum on status (MEDIUM — follow-up).
4. numeric(78,0) for wei columns (MEDIUM — follow-up).
5. owner_wallet_id cascade behavior (LOW — follow-up).

Items 1 and 2 are the only ones I'd block on; the rest are scheduling-before-the-tables-grow-large hygiene.

[suisuss]

Review: API route / authorization surface (/api/user/safe/**)

Focused pass on AuthZ, IDOR, input validation, and error handling for the nine new route handlers. Auth pattern is solid overall — validateSafeAdmin + getSafeForOrg correctly enforces org-scoping with 404 (not 403) on cross-org access, no IDOR leakage. A few inconsistencies and a destructive-default worth flagging.

Verified

Claim	Where
getSafeForOrg filters (safeId, organizationId) and returns null → 404, not "exists but wrong org" → 403	lib/safe/auth.ts:67-83
Membership + admin/owner gate before any DB read on mutational routes	7 of 9 routes ([safeId]/route.ts, [safeId]/role/*)
Body type assertions only — no Zod / Valibot	All routes ((await request.json()) as XxxBody)

---

MEDIUM — three different admin-auth implementations across the Safe surface

1. lib/safe/auth.ts::validateSafeAdmin — admin/owner gate, used by 7 routes.
2. app/api/user/safe/route.ts:23-58::validateAdmin (local) — admin/owner gate, duplicates the shared logic. Maintenance drift risk; the only delta is the rejection-message string.
3. app/api/user/safe/simulate-deploy/route.ts:142-161 — accepts any member, hand-rolled inline. No admin gate.

Item 3 is the substantive one: simulate-deploy is a read-only preview (no DB writes, no on-chain tx), so allowing non-admin members to see "what would happen if we deployed" is defensible. But:

• The decision is unstated. A reviewer can't tell whether non-admin access is intentional or oversight.
• It hits RPC (getMaxFeePerGas) and a price oracle (getNativeUsdPrice, weiToUsd) per request. With no rate-limit primitive anywhere in the codebase, this is the most-abusable endpoint in the Safe surface.

Suggested: route 1 and 2 should both call the shared helper (DRY); route 3 should either also call the shared helper (closing the preview to non-admins, simplest), or get an explicit comment justifying the looser gate.

---

MEDIUM — destructive /role/update accepts empty body and wipes everything

app/api/user/safe/[safeId]/role/update/route.ts:106-110 documents the contract: "FULL desired state (protocols + direct rules), not a patch". Verified at roles-orchestrator.ts:1418 — flattenInstallInput(protocolInputs, directRules) with empty arrays produces an empty desired state, and the SDK diff at line 1511 (callsPlannedForApplyRole(currentRole, desiredRole)) emits revoke calls for every current protocol scope, plus allowance-zero calls for every current bucket.

The only existing guard is the subgraph-outage check at line 1378 (shouldBailOnSubgraphOutage requires existingDirectRulesCount > 0 || desiredDirectRulesCount > 0). When subgraph is healthy, there is no guard at all — an empty body wipes the modifier's entire scope.

By-design (per the doc comment), but the failure modes are:

• UI bug serializes {} instead of { protocols: [...current], directRules: [...current] }
• Mid-request truncation
• Misinterpreted client contract during partner integration

Suggested defense-in-depth: refuse an empty desired state when the existing role is non-empty, unless the request body sets an explicit confirm: "remove-all-policies" flag. The legitimate "wipe everything" path becomes opt-in; an accidental empty body returns 400 instead of executing a destructive MultiSend.

---

MEDIUM — /role/allowances POST accepts arbitrary protocolSlug without catalog validation

app/api/user/safe/[safeId]/role/allowances/route.ts:96-101 checks protocolSlug is a non-empty string but doesn't validate it against PROTOCOL_CATALOG. Sibling routes (/role install, /role/update, /role/simulate, /role/simulate-deploy) all gate slugs through if (!(slug in PROTOCOL_CATALOG)). The [tokenAddress] DELETE handler also accepts arbitrary slugs.

Probably benign — setRoleTokenAllowance looks up by (roleId, allowanceKey) where allowanceKey is computed from (roleKey, protocolSlug, token), so an unknown slug just produces a non-matching key. But:

• It's a footgun: a caller passing protocolSlug="aaveV3" (camelCase typo) silently writes a phantom allowance keyed to a slug nothing else can reach.
• The asymmetry across sibling routes is a maintainability hazard — anyone copying the pattern from this file gets less validation than from the others.

Add the same slug in PROTOCOL_CATALOG check both routes are missing.

---

MEDIUM — silent malformed-input dropping in normalisers

normaliseDirectRules, normaliseProtocols, and the inline flatMap token normalisers all continue / return [] on malformed entries. The route response includes skipped: [] and skippedDirectRules: [], but those arrays only carry unknown protocol slugs. Token-level entries dropped due to missing tokenDecimals/periodSeconds/amountHuman, and rules dropped due to missing counterparty/kind, are filtered with no trace.

User submits 5 rules in the wizard, 3 install (because 2 had a typo somewhere upstream), response says success: true, skippedDirectRules: []. User has no signal that anything was dropped, and on the next Apply the "missing" rules silently re-disappear.

Suggested: extend skipped (or add a droppedInputs) array that surfaces each filtered entry with a reason. Failing that, reject the whole request on any malformed entry (400) so the client sees a clear validation failure rather than a partial success.

---

LOW — input validation precedes auth in DELETE /role/allowances/[tokenAddress]

[tokenAddress]/route.ts:17-32 validates the address and presence of protocolSlug before validateSafeAdmin at line 34. An unauthenticated probe gets a 400 with a specific message ("Invalid token address" / "protocolSlug query parameter is required"), confirming the route shape. Auth-first would return 401 and give no shape information.

Minor — doesn't grant access, only reveals API surface a determined attacker could discover anyway from JS bundles. But the pattern is inconsistent: every other route does auth first.

---

LOW — 404 message inconsistency

Three of the routes return "Safe not found for this organization" (good — explicit about org-scoping); three return just "Safe not found". Both are 404, neither leaks existence, but the inconsistency is cosmetic noise. Pick one.

---

LOW — duplicated body normalisers across install / update / simulate / simulate-deploy

TokenLimitBody, DirectRuleBody, normaliseProtocols, normaliseDirectRules, and the shape-switching logic appear in 3-4 places. The shapes are subtly different per route (install accepts both legacy and new shapes, update only new). Drift risk: a future field addition will have to be hand-applied in N places.

Extracting to lib/safe/route-input.ts would consolidate and let the normalisers carry their own validation reports for the MEDIUM "silent dropping" issue above.

---

What's well-designed

• getSafeForOrg returns null → 404 — no 403 cross-org leak. Pattern documented in the helper itself.
• validateSafeAdmin gates by (session.user, activeOrgId, member.role) in that order — short-circuits cleanly on each missing piece with distinct status codes (401, 400, 403, 403).
• Mutation routes log to logSystemError with structured fields (endpoint, component, chain_id) — consistent observability surface for Sentry triage.
• PATCH on /api/user/safe/[safeId] re-asserts organizationId in the UPDATE WHERE clause (line 62-66) — defense-in-depth against the getSafeForOrg check accidentally letting through stale state. Good belt-and-braces pattern.
• GET /role returns { installed: false, role: null, ... } rather than 404 when the role doesn't exist yet — correct, the Safe exists, just no role on it. UI can render the "Install" CTA without ambiguity.

---

Suggested follow-up tickets

1. Unify admin auth — either route both deploy and simulate-deploy through validateSafeAdmin, or document the intentional asymmetry (MEDIUM).
2. Empty-body guard on /role/update — require confirm: "remove-all-policies" to wipe (MEDIUM).
3. protocolSlug catalog validation on /role/allowances POST + DELETE (MEDIUM).
4. Surface dropped-input reasons in normaliser responses (MEDIUM).
5. Consolidate normalisers to a shared module (LOW — follow-up).

Items 1 and 3 are the smallest. Item 2 is the highest-value defense-in-depth.

[github-actions]

PR Environment Deployed

Your PR environment has been deployed!

Environment Details:

• URL: https://app-pr-923.keeperhub.com
• Namespace: pr-923
• Cluster: techops-staging
• Image Tag: da5c35e
• Commit: da5c35e64cc60327f27a8125f9262948987bbd88

Components:

• Keeperhub Application
• PostgreSQL Database (isolated instance)
• LocalStack (SQS emulation)
• Redis (isolated instance)
• Schedule Dispatcher (staging image)
• Block Dispatcher (staging image)
• Event Tracker (staging image)

The environment will be automatically cleaned up when this PR is closed or merged.

[suisuss]

Works really well @joelorzet I added a comment on the Linear ticket advising some UX changes:

• Having EOA and Safe wallets automatically added to the Address Book on creation
• Web3 Connection property for node configurations should be the deciding factor as what wallet is used - right now it has EOA selected even thought Safe is enabled. Safe being enabled should just be considered a global default and should reflect in the Web3 Connection Property
• We need to decode Safe contract revert errors so they are human readable