Skip to content

refactor(node-ui): consolidate approve signer planning into the owner-access model + close the snapshot-load lock (#1470, #1469)#1472

Open
Jurij89 wants to merge 1 commit into
mainfrom
refactor/pca-owner-access-signer-plan
Open

refactor(node-ui): consolidate approve signer planning into the owner-access model + close the snapshot-load lock (#1470, #1469)#1472
Jurij89 wants to merge 1 commit into
mainfrom
refactor/pca-owner-access-signer-plan

Conversation

@Jurij89

@Jurij89 Jurij89 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What & why

Consolidates the last of the PCA owner/signer planning into the centralized owner-access model (#1470) and closes the snapshot-load-window sibling of the #1468 R4 wallet-signing lock (#1469).

Fixes #1470
Fixes #1469

Changes

Model (pca/ownerAccess.ts)

  • Replace the overloaded walletsUnknown boolean with a typed primaryWalletState: 'loading' | 'unreadable' | 'ready' (+ a wallets-loading unknown cause). The two consumers had diverged on the loading signal, so a daemon-owned PCA could flash read-only in the detail view during the balances load; the model now unifies them (loading → unknown, never a definitive non-owner verdict).
  • Add a derived mayPromptWallet — the single target-write signing plan the approve modal reads. It subsumes the R4 branch and closes PCA node-UI: arm the approve wallet-signing lock during the SNAPSHOT-load window (pre-existing sibling of #1468 R4) #1469: the lock now arms in the snapshot-load window too (owner not-yet-known + a connected wallet), so a wallet-signed approve can never run under the daemon (no-prompt, freely-dismissable) contract. inv-17 classification is byte-unchanged.

Consumers

  • ApproveWalletsModal: targetWalletManaged = access.mayPromptWallet; the async cross-account signerKindForAccount is folded behind a shared resolveSignerKindForAccount.
  • ConvictionDetailView: switches to primaryWalletState; splits the owner-unknown gate — pure loading shows a neutral "Confirming ownership…" (no false "Couldn't load", no Retry); only a genuine read failure keeps the error + Retry.

Strategy-named submitter factories (#1470 item 2) — behaviour-preserving rename across all 5 call sites: useResolvedOwnerActionSubmitteruseOwnerActionSubmitter (same-account, access-pinned), useResolvingOwnerActionSubmitteruseOwnerActionSubmitterForAccount (per-call resolving).

Invariants

  • inv-16: web3/walletOwnerActionSubmitter.ts byte-untouched (per-prompt liveness guards stay call-time).
  • inv-17: owner precedence + address-only classification unchanged.

Verification

  • 556 node-UI PCA unit tests green; tsc zero-delta (0 errors in touched files).
  • 4-lens adversarial gate: GO. It caught one real MEDIUM — the folded resolveSignerKindForAccount initially did an internal fetchWalletsBalances, adding an uncorrelated failure path that could disarm the R4 lock on a transient blip during a cross-signer renew. Fixed: the resolver now takes the caller's already-loaded primaryWallet, restoring the old inline predicate's correlation; pinned by new resolver unit tests (incl. "wallet branch does not depend on the node wallet list").
  • Spend-free testnet browser smoke to follow.

🤖 Generated with Claude Code

…-access model + close the snapshot-load lock (#1470, #1469)

Centralizes the last of the PCA owner/signer planning into the owner-access
model and closes the snapshot-load-window sibling of the #1468 R4 lock.

Model (pca/ownerAccess.ts):
- Replace the overloaded `walletsUnknown` boolean with a typed
  `primaryWalletState: 'loading' | 'unreadable' | 'ready'` (+ `wallets-loading`
  unknown cause). This unifies the two consumers, which had diverged, and fixes
  a daemon-owned PCA flashing read-only in the detail view during the balances
  load (it now shows loading, never a definitive non-owner verdict).
- Add a derived `mayPromptWallet` — the single target-write signing plan the
  approve modal reads. It subsumes the R4 branch and closes #1469: the lock now
  arms in the snapshot-load window too (owner not-yet-known + a connected
  wallet), so a wallet-signed approve can never run under the daemon (no-prompt,
  freely-dismissable) contract. inv-17 classification is byte-unchanged.

Consumers:
- ApproveWalletsModal: `targetWalletManaged = access.mayPromptWallet`; the async
  cross-account `signerKindForAccount` is folded behind a shared
  `resolveSignerKindForAccount` in ownerActions.ts.
- ConvictionDetailView: switches to `primaryWalletState`; splits the owner-unknown
  gate — pure loading shows a neutral "Confirming ownership…" (no false
  "Couldn't load", no Retry); only a genuine read failure keeps the error + Retry.

Strategy-named submitter factories (#1470 item 2): rename
`useResolvedOwnerActionSubmitter` -> `useOwnerActionSubmitter` (same-account,
access-pinned) and `useResolvingOwnerActionSubmitter` ->
`useOwnerActionSubmitterForAccount` (per-call resolving) across all 5 call sites.
Behavior-preserving rename.

Adversarial-gate hardening: `resolveSignerKindForAccount` takes the caller's
already-loaded `primaryWallet` instead of an internal `fetchWalletsBalances`,
keeping the renew device-prompt planning correlated with the old inline
predicate — an uncorrelated wallets-fetch blip could otherwise disarm the R4
lock on a cross-signer renew. Pinned by new resolver unit tests.

inv-16 (walletOwnerActionSubmitter.ts) byte-untouched; inv-17 preserved.
556 PCA unit tests green; tsc zero-delta; 4-lens gate GO. Closes #1470, #1469.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3swmeQGT8WGttaZsp1NQJ
* refuses read-only / daemon-confirms once the re-fetched owner is known. See
* `computeMayPromptWallet`.
*/
mayPromptWallet: boolean;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Keep approve-batch signing-plan state out of the generic owner-access model

What's wrong
The owner-access abstraction was supposed to classify ownership and write eligibility, but this PR adds a workflow-specific device-prompt lock flag to the same object. The comments already call out ApproveWalletsModal.targetWalletManaged, which is a sign the model is absorbing a consumer-specific concern instead of removing complexity.

Example
A detail-page caller that only needs owner classification now receives an approve-batch signing-plan flag whose semantics depend on device-prompt locking, not ownership access. That makes the central model harder to keep small and forces every future access consumer to understand a field that exists for one workflow.

Suggested direction
Delete mayPromptWallet from PcaOwnerAccess and compute the approve target prompt plan through an explicit helper such as mayTargetWritePromptWallet(access, owner, connectedWallet) or a small approve-batch signing plan object. That keeps the shared model cohesive while still centralizing the tricky predicate.

For Agents
Look at ownerAccess.ts and ApproveWalletsModal.tsx. Preserve the current R4/#1469 locking behavior, but move the approve-batch prompt planning behind a dedicated helper or signing-plan function near the approve-batch/owner-action boundary. Keep PcaOwnerAccess focused on owner classification and write gating; prove parity with the existing mayPromptWallet truth-table tests.

if (!accountId) return undefined;
const snapshot = await fetchPca(accountId).catch(() => null);
if (!snapshot?.owner) return undefined;
const walletState = useWalletStore.getState();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Make signer-kind resolver dependencies explicit instead of half ambient

What's wrong
The extracted helper improves reuse on the surface, but it hides part of its real contract behind useWalletStore.getState(). That makes the function harder to reason about than the old inline predicate because callers can see only one of the three values that affect the result.

Example
Two inputs that define the same signer classification are split across different sources: primaryWallet is injected by the caller, while connectedWallet and walletWrongNetwork are ambient global state. A future caller cannot tell from the signature that wallet connection state affects the result, and tests have to mutate the store to exercise what looks like a simple resolver.

Suggested direction
Change the helper signature to accept a typed options object with accountId, primaryWallet, connectedWallet, and walletWrongNetwork or keep the logic component-local as a closure. The important simplification is one clear boundary for all inputs used by the classifier.

For Agents
In ownerActions.ts, make resolveSignerKindForAccount receive all classification inputs explicitly, or create a resolver factory in ApproveWalletsModal that closes over primaryWallet, connectedWallet, and walletWrongNetwork. Preserve the existing fetch-PCA behavior and the no-wallet-list-refetch contract; update the signer-kind tests to pass explicit wallet state rather than relying on hidden store reads.

// "loading" (mode 'unknown'), NOT a read-only/external flash, while the balances fetch is in
// flight. L3: a balances READ FAILURE (wb null + error) still classifies unknown ('unreadable') —
// the owner controls read "can't confirm ownership" + Retry, never a definitive non-owner.
const primaryWalletState: 'loading' | 'unreadable' | 'ready' = wb ? 'ready' : wbError ? 'unreadable' : 'loading';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Centralize the wallet-list load-state mapping

What's wrong
The PR introduces a useful three-state model, but leaves construction of that model at each component. That is exactly where small state-machine policies tend to drift, and the duplicated literal union makes the boundary feel ad hoc rather than canonical.

Example
Both changed components now carry const primaryWalletState: 'loading' | 'unreadable' | 'ready' = wb ? 'ready' : wbError ? 'unreadable' : 'loading';. Any future caller has to rediscover the same loading/error precedence and duplicate the string-literal union.

Suggested direction
Move the repeated ternary and union literals into the owner-access module or hook boundary. That gives callers a named concept instead of a hand-built protocol and keeps the loading-vs-unreadable split from becoming copy-pasted policy.

Confidence note
This is based on the two changed call sites in the PR; if more callers exist outside the scanned diff, the duplication pressure is stronger.

For Agents
In ownerAccess.ts, export a PcaPrimaryWalletState type and a small helper like primaryWalletStateFor({ data, error }) or resolvePrimaryWalletState(wb, wbError). Use it from ApproveWalletsModal, ConvictionDetailView, and usePcaOwnerAccess. Preserve the current precedence: data means ready, error without data means unreadable, otherwise loading.

// (mode 'unknown' with a connected wallet whose address is — or may turn out to be — the owner).
// runApproveBatch sets `walletBatchSigning`/deviceTotal BEFORE the in-flight wallet action, so a
// wallet-signed approve never runs under the daemon (no-prompt, freely-dismissable) contract.
const targetWalletManaged = access.mayPromptWallet;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: The new no-snapshot wallet-lock path is only tested as a pure flag

What's wrong
This line is the behavior boundary that protects browser-wallet approvals from running without the device-progress and dismiss lock. The added truth-table case proves mayPromptWallet can be true when the owner snapshot is missing, but it does not prove ApproveWalletsModal uses that flag when the user clicks before the snapshot finishes loading.

Example
A regression could leave resolvePcaOwnerAccess(...).mayPromptWallet === true for owner: undefined, but change ApproveWalletsModal back to computing targetWalletManaged from only access.mode === 'wallet' && access.writesEnabled. The pure owner-access test would still pass, while clicking Approve before fetchPca(accountId) resolves could open a wallet prompt without the device row or disabled close button.

Suggested direction
Cover the actual modal behavior for the owner-not-yet-loaded case, similar to the existing loading-window R4 tests, so the test fails if the modal stops passing access.mayPromptWallet into runApproveBatch.

For Agents
Add an ApproveWalletsModal test in packages/node-ui/test/pca-approve-modal.test.ts where a connected wallet is present, seedBulk enables immediate submit, the initial account snapshot is pending at click time, and the resolving submitter later fetches a snapshot owned by the connected wallet. Assert the device progress appears and modal dismissal is locked before the wallet register promise resolves.

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

Labels

None yet

Projects

None yet

2 participants