Improve popup switching UX and unify RPC service ownership - #1606
Conversation
|
/review |
KillariDev
changed the title
KillariDev
changed the title
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
The three security/correctness-focused reviews (Defender, Security, Bug Hunter) found no security vulnerabilities or high-confidence runtime defects; the changes are well-encapsulated and extensively tested. Two structural/architectural concerns were raised and are detailed in the line comments below.
| sendPopupMessageToOpenWindows({ method: 'popup_accounts_update' }) | ||
| const rpcChainChanged = previousSettings.activeRpcNetwork.chainId !== updatedSettings.activeRpcNetwork.chainId | ||
| const rpcEndpointChanged = rpcChainChanged || previousSettings.activeRpcNetwork.httpsRpc !== updatedSettings.activeRpcNetwork.httpsRpc | ||
| const activeServices = rpcEndpointChanged && change.rpcNetwork?.httpsRpc !== undefined |
There was a problem hiding this comment.
Architectural concern: the reset abstraction changed from (rpcNetwork) => void to (rpcNetwork) => SimulationServices | void (and background-startup.ts now returns the freshly-installed services), so changeActiveAddressAndChain computes activeServices as resetSimulationServices(change.rpcNetwork) ?? { ethereum, tokenPriceService }. The function still takes ethereum/tokenPriceService as injected parameters, but after this change it may silently discard them and substitute the side-effecting global reset's result, with the ?? fallback keeping the contract ambiguous. Callers can no longer tell whether the instance they passed will actually be used — that becomes a runtime-dependent distinction enforced only by the ?? fallback. This leaks the internal service-registry replacement semantics into the caller and into the previously-void reset callback. Consider returning the reset services as the canonical result of the reset-and-activate operations and threading them through callers explicitly, rather than embedding an optional return-value convention on the reset callback.
| }), | ||
| ) | ||
|
|
||
| export type ModifyMakeMeRich = funtypes.Static<typeof ModifyMakeMeRich> |
There was a problem hiding this comment.
Architectural concern: the popup→background request message types (ModifyMakeMeRich, EnableSimulationMode, ChangeActiveChain, ChangeActiveAddress) were moved out of interceptor-messages.ts into interceptor-reply-messages.ts, and interceptor-messages.ts now just re-exports them. This was done as a cycle workaround so the reply-protocol tables can reference the request types without importing from interceptor-messages.ts. The result inverts the naming/cohesion contract: a file named 'reply-messages' is now the canonical home for several request definitions, which degrades navigability and may normalize this misplaced pattern for future request types. Consider keeping request definitions in interceptor-messages.ts and resolving the import cycle another way, or introducing a shared request/reply protocol module.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The popup settings-change and RPC/chain-switch refactor is broadly sound and well-tested, but introduces two maintainability concerns (dual tracking of the installed simulation services, and a hand-rolled cross-popup coordinator whose hardcoded method list and revision counter must be kept in sync across several files) plus one moderate-confidence user-facing risk in the new wallet chain-switch deadline handling, where an expired-switch marker can block unrelated later switches to the same chain.
| const pending: PendingSignerChainChange = { | ||
| timeout: undefined, | ||
| future: new Future< | ||
| | { readonly type: 'reply', readonly confirmation: SignerChainChangeConfirmation } |
There was a problem hiding this comment.
Expired wallet-switch markers can poison unrelated later switches to the same chain. expiredSignerChainChanges is keyed only by (port, signerProviderGeneration, chainId) and requestSignerChainChange rejects any new switch to that chain while the signer generation is unchanged — including a legitimate dapp-initiated wallet_switchEthereumChain after the user already approved it. The marker is only cleared by a late matching reply (which a silent wallet may never send) or a signer reconnect/generation advance. A user whose popup switch to chain X timed out is then locked out of switching to chain X (e.g. the dapp gets an error or its wallet approval is consumed as a stale reply) until they dismiss the stale wallet prompt or reconnect. The blocking of retries and consumption of late replies is intended, but the global per-chain scope leaks into unrelated switches with no test covering the popup-timeout vs. dapp-initiated cross-case.
| export type NewBlockAttemptCallback = (blockHeader: EthereumBlockHeader, ethereumClientService: EthereumClientService, isNewBlock: boolean) => Promise<void> | ||
| export type OnErrorBlockCallback = (ethereumClientService: EthereumClientService, error: unknown) => Promise<void> | ||
| export type ResetSimulationServices = (rpcNetwork: RpcEntry) => void | ||
| // Installs the selected RPC services and returns the pair that subsequent work must use. |
There was a problem hiding this comment.
The identity of the currently installed simulation services is now tracked by two parallel mechanisms that must be kept consistent by hand. ResetSimulationServices both mutates the module-global simulationServices (in background-startup.ts) and returns the installed pair, while runActiveSettingsChange threads a separate local activeServices through the transaction; callers at the message boundaries re-read the global via getSimulationServices(). These agree only because every reset funnels through the same closure that does both. Any future path that resets services or touches the global must remember to update both representations, and the return value is discarded at most call sites, so the new signature adds coupling without guaranteeing the result is consumed. Recommend retiring the getSimulationServices() reads at the message boundaries in favor of the threaded pair so there is a single source of truth.
| const publishSettingsChangeStatus = async () => await sendPopupMessageToOpenWindows({ method: 'popup_settingsChangeStatus', data: settingsChangeStatus }) | ||
| function setSettingsChangeOperation(operation: PopupSettingsChangeStatus['data']['operation']) { | ||
| settingsChangeStatus = { revision: Math.max(Date.now(), settingsChangeStatus.revision + 1), operation } | ||
| } |
There was a problem hiding this comment.
The cross-popup settings coordinator is a hand-rolled singleton (settingsChangeStatus) with a manually enforced monotonic revision counter and a hardcoded inline list of which methods count as settings changes (isSettingsChange). This works for a single background worker, but the revision invariant and the method list are duplicated knowledge that the popup mirrors in App.tsx (backgroundSettingsChange, pendingSettingsChange, isActiveAddressChangePending) and useLiveSimulationHomeData (the revision >= comparison). Every newly added settings-changing message type must be added in isSettingsChange, in the operation status codes, and in the UI's local tracking in lockstep; a miss in any one file leaves popups showing stale 'changing' state or allowing concurrent changes the background intends to reject. This is a fragile 'modify in lockstep across multiple files' pattern in one of the most actively changing modules.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Two structural concerns in the popup settings-change and chain-switch refactor. (1) runActiveSettingsChange now broadcasts the same popup_settingsUpdated payload twice with two independent generation counters, and committedAddressChange is attached only to the first emission, making the message contract ambiguous for consumers. (2) App.tsx is becoming the coordination hub for the settings-change state machine (pending settings/address-change state plus changePopupSettings/setSimulationMode/setRichState orchestration) and that coordination is additionally threaded through the shared useLiveSimulationHomeData hook, which is also consumed by non-popup callers. No functional or security defects were reported by the other reviewers.
| if (rpcEndpointChanged && change.rpcNetwork?.httpsRpc !== undefined) activeServices = resetSimulationServices(change.rpcNetwork) | ||
| if (updatedSettings.simulationMode && rpcChainChanged) await clearSimulationStateFromConfig() | ||
| // Publish the committed selection once its provider and stack are ready, before slow access work. | ||
| await sendPopupMessageToOpenWindows({ |
There was a problem hiding this comment.
Structural concern: every transition through runActiveSettingsChange now emits popup_settingsUpdated twice with two independent, non-synchronized generation sources (bumpPopupRefreshGeneration() inside the try block, then accessUpdate.popupRefreshGeneration after reconcileWebsiteApprovalAccesses) carrying the identical updatedSettings payload. The consumer must Math.max-combine generations and re-run the stack-context invalidation/requestHomeDataForLiveUpdate path twice for one logical settings change. Worse, committedAddressChange is attached only to the first of the two identical payloads, so which of the two broadcasts a consumer receives determines whether the message carries the commit field — the same message type is no longer self-consistent and consumers must assemble state by merging two broadcasts. Consider splitting this into a dedicated early-commit message or a single authoritative broadcast carrying the commit field.
| export function App() { | ||
| const appPage = useSignal<PopupPage>({ page: 'Unknown' }) | ||
| const pendingAddressChangeRequestId = useSignal<string | undefined>(undefined) | ||
| const isActiveAddressChanging = useSignal(false) |
There was a problem hiding this comment.
Structural concern: the root App component is becoming the coordination hub for the entire settings-change state machine — pendingAddressChangeRequestId, isActiveAddressChanging, pendingSettingsChange, backgroundSettingsChange, plus the changePopupSettings, setActiveRpcAndInformAboutIt, setSimulationMode and setRichState orchestration functions — inside the component that already owns page routing, error display, and top-level rendering. This coordination is also threaded through the shared useLiveSimulationHomeData hook (onSettingsChangeStatus / onAddressSelectionCommitted), whose answerMainPopupOpen consumers include non-popup callers such as the full-stack tab page, coupling settings-admission/committed-address transport into components that never render it. This is a god-component accretion risk: every new settings-change kind will touch both App's coordination state and the shared hook's option surface. Consider extracting a dedicated settings-change controller/hook.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Two concrete findings were raised across the review agents.
-
The new
popupSwitchingChrome.tsbenchmark will always fail on its "stacked rich off with RPC failure" sample. In that scenariofailSimulationmakes theeth_simulateV1fixture return an RPC error, sorefreshPopupSimulationreturnsfalseand thepopup_modifyMakeMeRichhandler replies withok: false. Themeasure(...)helper defaultsexpectedSuccess = true, so it throws "Unexpected switch outcome" and aborts the iteration before the recovery assertions run. -
The "did the network change?" decision is re-derived in five places with three divergent predicates (full-serialized
RpcNetworkequality inchangeActiveRpc, chainId-only in thepopupChangeActiveRpcupstream gate, and chainId-or-httpsRpc inrunActiveSettingsChange,changeSignerChain, anduseLiveSimulationHomeData). These predicates disagree: a metadata-only edit (rename/currency/primary flag) is treated as "no change" upstream but "changed" by the full-serialization equality, so a wallet switch can still be dispatched for the same endpoint.
| const failuresBefore = injectedFailures | ||
| failSimulation = true | ||
| try { | ||
| samples.push(await measure(popup, 'stacked rich off with RPC failure', 'popup_modifyMakeMeRich', `document.querySelector('input[type="checkbox"]').click()`, `document.body.textContent.includes('Updating balances...')`, `document.querySelector('input[type="checkbox"]')?.checked === false`)) |
There was a problem hiding this comment.
This sample passes no expected-success argument, so measure(...) uses the default expectedSuccess = true. But the scenario runs with failSimulation = true: the eth_simulateV1 fixture returns an RPC error, refreshPopupSimulation returns false (the visualization is stamped invalid/failed), and the popup_modifyMakeMeRich handler replies { type: 'PopupSettingsChangeReply', ok: false }. Thus sample.ok is false while expectedSuccess is true, and the helper throws "Unexpected switch outcome", aborting runIteration before the RPC-recovery assertions. Supplying false for the expected success (as the 'wallet RPC rejection' sample does) will make this scenario pass.
| export async function changeActiveRpc(ethereum: EthereumClientService, tokenPriceService: TokenPriceService, resetSimulationServices: ResetSimulationServices, websiteTabConnections: WebsiteTabConnections, rpcNetwork: RpcNetwork, simulationMode: boolean, signerTabId: number | undefined, walletSwitchRequestId: string = crypto.randomUUID()) { | ||
| const currentRpc = (await getSettings()).activeRpcNetwork | ||
| // Metadata edits at the same endpoint still need to update the active selection. | ||
| if (JSON.stringify(RpcNetwork.serialize(currentRpc)) === JSON.stringify(RpcNetwork.serialize(rpcNetwork))) { |
There was a problem hiding this comment.
"Did the network change?" is hand-rolled in five separate places with three different predicates that already contradict each other: (a) this full-serialized JSON.stringify(RpcNetwork.serialize(...)) equality in changeActiveRpc, (b) the chainId-only gate in popupChangeActiveRpc, and (c) the chainId-or-httpsRpc checks in runActiveSettingsChange, changeSignerChain, and useLiveSimulationHomeData. Because the full-serialization equality compares the entire RpcEntry, a metadata-only edit (rename, currency label, primary/minimized flag) is treated as "same network" by the chainId-only upstream gate but "changed" here, so a wallet_switchEthereumChain request can still be dispatched for an unchanged endpoint. This lock-step duplication will quietly diverge as the network model grows. Introduce a single change-detection abstraction (same endpoint / chain changed / endpoint changed) defined beside the RpcNetwork type and route all five call sites through it.
|
/review |
There was a problem hiding this comment.
CI Agent Review
No security or correctness issues were reported by the security, defensive, or bug-hunting agents; the remaining concerns are architectural and maintainability-related. Two findings stand out: (1) the wallet chain-switch reply handshake is now orchestrated from providerMessageHandlers.ts using six narrow accessors into windows/changeChain.ts's private pending-switch state machine, breaking that module's encapsulation and coupling the two files to evolve in lockstep; and (2) runActiveSettingsChange maintains its own returned activeServices snapshot alongside the new SimulationServicesOwner, producing a second, potentially divergent source of truth for the installed simulation services whose correctness depends on caller discipline.
| if (!isPendingWalletSwitchRequest(params.walletSwitchRequestId)) return returnValue | ||
| const pendingSignerStateToken = getPendingSignerChainChangeTokenForCallback(port, params.signerProviderGeneration, params.chainId) | ||
| const callbackSignerStateToken = pendingSignerStateToken | ||
| ?? (currentSignerStateToken.signerProviderGeneration === params.signerProviderGeneration ? currentSignerStateToken : undefined) |
There was a problem hiding this comment.
Architectural concern: the wallet chain-switch state machine that was previously owned entirely inside windows/changeChain.ts is now orchestrated from this file. The consumer imports six narrow accessors/mutators (isPendingWalletSwitchRequest, markSignerChainReplyReceived, getPendingSignerChainChangeRpc, getPendingSignerChainChangeTokenForCallback, isPendingSignerChainChangeReply, resolveSignerChainChange) and must re-implement the correct call ordering, including the release-the-waiting-popup-on-failure sequencing (calling resolveSignerChainChange twice in the catch path). This is a leaky abstraction: the caller depends on internal details of the private pendingSignerChainChange global (exact walletSwitchRequestId matching, the replyReceived flag), so any future change to the state machine (adding transitions, changing reply matching, or moving when the timeout is cleared) now forces edits here in lockstep. Consider encapsulating the whole 'apply a wallet switch reply' sequence as a single responsibility inside changeChain.ts that this handler simply invokes.
| ) { | ||
| ): Promise<SimulationServices> { | ||
| const { change } = transition | ||
| let accessUpdate: WebsiteAccessUpdate | undefined |
There was a problem hiding this comment.
Maintainability concern: runActiveSettingsChange initializes and returns its own mutable activeServices snapshot while background-startup.ts now reads the installed services live via SimulationServicesOwner.getCurrent(). The finally block feeds activeServices into finishWebsiteAccessUpdate, which itself takes a resetSimulationServices and can re-install services, so the returned snapshot can diverge from owner.getCurrent(). This yields two ways to obtain the 'current' service pair, and correctness depends on the caller knowing which is authoritative at any point. The 'transition snapshot, not a second registry' comment acknowledges the hazard rather than removing it; tying this function's return value to the owner's getCurrent() (or not exposing a snapshot at all) would reduce the risk of divergence.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Architectural concern: business/change-classification logic was added to a wire-codec module. The new getRpcNetworkChange helper encodes system policy (chain changes require wallet approval, endpoint changes replace services, metadata changes only update the selection) inside app/ts/types/rpc.ts, which otherwise only defines funtypes wire-format codecs. It couples the types layer to behavioral decisions and is consumed by handlers in at least three architectural layers (popupMessageHandlers.ts, windows/changeChain.ts / activeSettings.ts / providerMessageHandlers.ts, and useLiveSimulationHomeData.ts). No security or correctness findings were reported by the other agents.
| minimized: funtypes.Literal(true), | ||
| }) | ||
| ) | ||
|
|
There was a problem hiding this comment.
This function adds consequential domain/business logic (chain-change vs endpoint-change vs metadata-change classification and its behavioral consequences) to a module whose sole purpose is defining RPC wire-format codecs. It is now consumed across several layers (background handlers, chain-window state machine, and the UI hook), so any behavioral change to 'what counts as a change' must be made inside a types/ codec module, which becomes a mix of data definitions and system policy. Recommend moving getRpcNetworkChange (with its descriptive type) to a pure helper under app/ts/utils/ (alongside helpers like activeStackContext.ts / simulationStackTargets.ts) and re-importing it from the consumers, leaving types/rpc.ts as a pure codec module.
There was a problem hiding this comment.
CI Agent Review
Architectural concerns from the review. (1) The wallet-vs-local RPC-switch routing policy is computed twice: popupChangeActiveRpc gates on getRpcChangeRoute(...) === 'wallet', and changeActiveRpc re-derives the same route internally. The Safe-selection gate exists only in the former, while reply matching/deadlines live in walletSwitch.requestSignerChainChange and changeActiveRpc still carries its own wallet-send branch, so the popup path and the dapp path (windows/changeChain.ts) can silently drift unless policy changes are mirrored across modules. (2) The new SimulationServicesOwner is only partially adopted: settings transitions still receive services by value and maintain a local reassigned copy, so a transition queued behind an endpoint reset can run against a stale pre-reset pair. (3) updatePopupVisualisationIfNeeded now takes six positional arguments beside a new parallel popupSimulationRefreshQueue abstraction, an expanding scheduling/execution surface worth consolidating. No security or correctness defects were reported by the other reviewers.
| export async function popupChangeActiveRpc(ethereum: EthereumClientService, tokenPriceService: TokenPriceService, resetSimulationServices: ResetSimulationServices, websiteTabConnections: WebsiteTabConnections, params: ChangeActiveChain, settings: Settings) { | ||
| await changeActiveRpc(ethereum, tokenPriceService, resetSimulationServices, websiteTabConnections, params.data, settings.simulationMode, await getLastKnownCurrentTabId()) | ||
| if (getRpcChangeRoute(settings.activeRpcNetwork, params.data, settings.simulationMode) === 'wallet') { | ||
| const tabId = await getLastKnownCurrentTabId() |
There was a problem hiding this comment.
Architecture: the wallet-vs-local RPC routing decision is now computed in two independent enforcement points. This gate calls getRpcChangeRoute(...) === 'wallet', and changeActiveRpc (activeSettings.ts) re-derives the same route internally, while the Safe-selection gate sits only here. Reply matching/deadlines live in walletSwitch.requestSignerChainChange and the raw wallet-send branch remains reachable in changeActiveRpc, so the invariants ('don't promote primary until accepted', 'a signer-only chain has no provider', 'a Safe-tied network may not be switched by the popup') are enforced by convention across three modules. Any future route variant or gating constraint must be mirrored in both sites or the popup and dapp paths will diverge. Consider giving one module ownership of the routing policy.
| // The preference belongs to the committed selection, even if later provider preparation fails. | ||
| if (transition.signingPreference !== undefined) await rememberSigningAddressSelection(transition.signingPreference) | ||
| // A signer-only chain has no provider to install; simulation is disabled until a configured endpoint is selected. | ||
| if (rpcEndpointChanged && change.rpcNetwork?.httpsRpc !== undefined) activeServices = resetSimulationServices(change.rpcNetwork) |
There was a problem hiding this comment.
Architecture: the SimulationServicesOwner abstraction is only half-wired. This transition reassigns a local activeServices on endpoint reset, but the transition is seeded from whatever snapshot the caller passed rather than from owner.getCurrent(). Because the semaphore is acquired inside runActiveSettingsChange after the caller has already captured its snapshot, a second transition queued behind one that resets the endpoint runs against its original pre-reset pair unless it also happens to reset. The comment 'This pair is the sole service reference for the transition' describes the guarantee but it is not enforced by the abstraction; threading owner.getCurrent() through the transition entry points would make the service-generation invariant structural rather than caller-discipline dependent.
| @@ -54,7 +54,8 @@ const hasSimulationInputOperations = (simulationState: SimulationState) => ( | |||
| simulationState.simulationStateInput.some((block) => block.transactions.length > 0 || block.signedMessages.length > 0) | |||
| ) | |||
There was a problem hiding this comment.
Minor: updatePopupVisualisationIfNeeded now takes six positional arguments (ethereum, tokenPriceService, invalidateOldState, onlyIfNotAlreadyUpdating, skipIfUnchanged, snapshot), and the diff adds a parallel popupSimulationRefreshQueue abstraction with four documented entry points whose cancellation/visibility/error/return contracts differ (documented in docs/popup-simulation-refresh.md to disambiguate). This scheduling/execution surface will keep growing as more triggers are added; consider converting to options/context objects and consolidating the two scheduling paths before the fork becomes permanent.
|
/review |
There was a problem hiding this comment.
CI Agent Review
One regression was identified in the wallet-network-switch refactor: a dapp-triggered wallet_switchEthereumChain in Gnosis Safe signing mode now fails with an internal error after the wallet approves, even though the same flow completed successfully before this change.
| const requestedRpc = getPendingSignerChainChangeRpc(callbackSignerStateToken, params.chainId) | ||
| await applyChain(currentSignerStateToken, params.chainId, requestedRpc) | ||
| const activeRpc = (await getSettings()).activeRpcNetwork | ||
| if (requestedRpc !== undefined && getRpcNetworkChange(activeRpc, requestedRpc).endpointChanged) { |
There was a problem hiding this comment.
Dapp-triggered wallet_switchEthereumChain in Safe signing mode now resolves to an internal error. changeActiveRpc only short-circuits the Safe restriction when request.source === 'popup', so a dapp dispatch proceeds to the wallet. When the user approves, applyChain calls changeSignerChain, which returns early (selectedSafe !== undefined) without changing the active RPC because Safe signing is pinned to the Safe's chain. The new accepted-endpoint re-check then sees the active RPC did not become the requested RPC, concludes the network "could not be activated" (INTERNAL_ERROR), and rejects the reply — even though the wallet has actually switched chains. Regression vs. the removed path: the old walletSwitchEthereumChainReply/resolve called the same changeSignerChain and then unconditionally settled the pending request with the accepted confirmation, returning { result: null } on success. Result: the dapp receives an internal error and Interceptor's active RPC diverges from the wallet's actual chain. The endpoint re-check should tolerate the Safe-pinned case (no RPC change expected) or the dapp path should also be gated before dispatching to the wallet.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The Architect agent identified one architectural cohesion concern: the refactor introduces two competing sources of truth for the currently-installed simulation services. The main request path (background-startup.ts → background.ts handleInterceptedRequest → handleContentScriptMessage → handleRPCRequest) captures a service snapshot at queue start and threads both that captured pair AND the mutable SimulationServicesOwner down the stack, while the same changeset's runActiveSettingsChange (activeSettings.ts) was converted to read only simulationServicesOwner.getCurrent(). If a re-entrant background task resets the owner while a request is in flight, RPC execution within one request uses the stale captured pair while the same request's access-update completion uses the freshly-reset owner. This is an API-design ambiguity introduced by the diff (not a runtime bug): the handler signatures do not make it clear which services will actually execute, and the abstraction intended as the single source of current services is bypassed on the main request path. Security, Defender, and Bug Hunter agents reported no findings.
| @@ -388,12 +388,12 @@ function replyWithSignerAccountError(websiteTabConnections: WebsiteTabConnection | |||
| }) | |||
There was a problem hiding this comment.
Cohesion concern: handleInterceptedRequest (and handleContentScriptMessage/handleRPCRequest below it) thread BOTH a captured service pair (ethereum, tokenPriceService) captured at queue start AND the mutable simulationServicesOwner. If another background task calls simulationServicesOwner.reset(...) re-entrantly while this request is in flight, RPC execution inside this request uses the stale captured pair while the same request's access-update completion (finishWebsiteAccessUpdate → getCurrent()) uses the freshly-reset owner. This contradicts the pattern established elsewhere in this change (e.g. runActiveSettingsChange reads only getCurrent()), so a single operation has two sources of truth for installed services and the handler signatures no longer reveal which will be used. Prefer carrying only the owner and deriving the snapshot from getCurrent() where work begins, mirroring runActiveSettingsChange.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The SimulationServicesOwner migration is applied unevenly across the background layers, leaving two coexisting (and mutually ambiguous) service-delivery patterns and the exact stale-pair hazard the owner was introduced to remove. In the access-admission/dialog path, the background captures { ethereum, tokenPriceService } from simulationServicesOwner.getCurrent() and threads that captured pair through the entire interaction layer (gateKeepRequestBehindAccessDialog → requestAccessFromUser → resolve → changeAccess → persistWebsiteAccessChange → finishWebsiteAccessUpdate). These access dialogs are among the longest-lived flows in the extension and routinely cross RPC resets, so they keep operating on a pair that may have been superseded — contradicting the documented rule that "admission obtains services when its work starts." The request-execution path and activeSettings.ts correctly read getCurrent() at stage start, so callers must now reason per-call-site about which idiom is authoritative, with no structural signal. PopupMessageDispatcherContext exposes ethereum, tokenPriceService, and simulationServicesOwner together, codifying the ambiguity, and several handlers retain _-prefixed dead parameters purely to satisfy the uniform signature. Fix direction: make the admission/resolution layers read owner.getCurrent() when each prompt is opened/resolved and drop the redundant service-pair parameters (mirroring handleContentScriptMessage), or else document an explicit per-boundary capture contract and reconcile the half-migrated parameter lists.
| switch (access) { | ||
| case 'askAccess': return await gateKeepRequestBehindAccessDialog(ethereum, tokenPriceService, resetSimulationServices, websiteTabConnections, socket, request, await websitePromise, activeAddress, await getSettings(), publishRpcConnectionStatus) | ||
| case 'askAccess': { | ||
| const website = await websitePromise |
There was a problem hiding this comment.
Architectural issue: the 'askAccess' branch captures { ethereum, tokenPriceService } = simulationServicesOwner.getCurrent() and binds this snapshot for the lifetime of the admission/dialog flow. The pair is threaded through gateKeepRequestBehindAccessDialog → requestAccessFromUser → resolve → changeAccess → persistWebsiteAccessChange → finishWebsiteAccessUpdate, all of which span long-lived user prompts that can outlive an RPC/endpoint reset. Because the services are captured here rather than re-read from simulationServicesOwner.getCurrent() at each prompt/resolution (as handleContentScriptMessage does), this path retains exactly the stale-pair hazard SimulationServicesOwner was introduced to eliminate, contradicting the doc claim that access admission "obtains services when its work starts." Recommended fix: have the admission/resolution layers read getCurrent() when each prompt opens/resolves and drop the redundant ethereum/tokenPriceService parameters (as done in handleContentScriptMessage) — or explicitly document a per-boundary capture contract. Relatedly, resolveChainChange, signerChainChanged, walletSwitchEthereumChainReply, connectedToSigner, and signerReply in providerMessageHandlers.ts / windows/changeChain.ts retain _-prefixed dead parameters and the dispatcher context exposes ethereum, tokenPriceService, and simulationServicesOwner together, leaving two delivery idioms and an ambiguous API surface.
|
/review |
KillariDev
changed the title
|
/review |
There was a problem hiding this comment.
CI Agent Review
The refactor removed the website-authorization gate from the signerChainChanged provider callback, a security regression not present in the sibling callback handlers. simpler chain events (signer_chainChanged) can now mutate the active network and drive per-address consent changes from an origin that has no hasAccess` and no approved tab-level connection, as long as its port still matches the tab's confirmed signer state token.
| @@ -187,43 +191,21 @@ export async function signerChainChanged(ethereum: EthereumClientService, tokenP | |||
| return await runSignerStateOperation(websiteTabConnections, socket.tabId, async () => { | |||
There was a problem hiding this comment.
Security regression: the signerChainChanged handler lost its hasSignerCallbackAccess(websiteTabConnections, socket.tabId, approval) authorization gate during the SimulationServicesOwner refactor. It now unconditionally calls changeSignerChain(simulationServicesOwner, websiteTabConnections, signerStateToken, signerChain, 'hasAccess'), and the literal 'hasAccess' makes the if (approval !== 'hasAccess') return check inside changeSignerChain always pass. The approval parameter is now unused in this handler. Unlike the sibling callbacks (ethAccountsReply and connectedToSigner), which retain hasSignerCallbackAccess, this path can mutate the active chain/address from an origin that was never granted access or whose approval was revoked (provider-callback messages bypass the normal verifyAccess gate and only compute providerCallbackApproval from the origin). Restore the hasSignerCallbackAccess check before runSignerStateOperation so a revoked or unapproved page cannot drive the chain switch.
|
/review |
There was a problem hiding this comment.
CI Agent Review
No security, supply-chain, or runtime correctness findings were identified. The single substantive is a structural/architectural concern: the dispatch layer now ships two coexisting conventions for accessing simulation services — the new SimulationServicesOwner (getCurrent()/reset()) used by the coordinator path, and direct (ethereum, tokenPriceService) snapshot unpacking used by the remaining snapshot-taking handlers — with nothing at the type level enforcing which a handler must use. A handler that unwraps a snapshot and then awaits (e.g. one that opens or resolves prompts or touches storage before RPC use) can silently keep operating on a provider another in-flight transition has already reset()-and-cleanup()-ed, since reset() destroys the replaced ethereum the snapshot still references. This yields a subtle stale/corrupted-provider defect rather than a compile error, and the extensive new timing tests exist precisely to defend this coupling.
| popup_confirmDialog: popupMessageHandler('popup_confirmDialog', async (context, request) => await confirmDialog(context.ethereum, context.tokenPriceService, context.websiteTabConnections, request)), | ||
| popup_changeActiveAddress: popupMessageHandler('popup_changeActiveAddress', async (context, request) => await changeActiveAddress(context.ethereum, context.tokenPriceService, context.resetSimulationServices, context.websiteTabConnections, request)), | ||
| popup_modifyMakeMeRich: popupMessageHandler('popup_modifyMakeMeRich', async (_context, request) => await modifyMakeMeRich(request)), | ||
| popup_confirmDialog: popupMessageHandler('popup_confirmDialog', async (context, request) => { |
There was a problem hiding this comment.
Mixed service-access conventions now coexist in the dispatch layer. Some handlers receive SimulationServicesOwner and call getCurrent() when work starts (coordinator path: changeActiveAddress, confirmRequestAccess, changeSettings, popupChangeActiveRpc, setSafeSimulationSigner, etc.), while ~15 handlers still unpack a snapshot via context.simulationServicesOwner.getCurrent() and thread (ethereum, tokenPriceService) (e.g. popup_confirmDialog, popup_removeTransactionOrSignedMessage, popup_refreshHomeData, popup_requestCompleteVisualizedSimulation). Nothing at the type level forces the correct choice. A leaf handler that unwraps a snapshot and then awaits before using it (opening/resolving prompts, touching storage) will keep operating on a provider that an in-flight transition has already reset()-and-cleanup()-ed, producing a silent stale/corrupted-provider defect rather than a compile error. This is a genuine improvement over the old all-snapshot baseline, but the partial conversion leaves two conventions without enforcement and adds temporal-coupling reasoning to every future handler. Consider sweeping remaining snapshot-taking handlers that can cross an await onto the owner shape, or explicitly documenting the resolved boundary.
|
/review |
There was a problem hiding this comment.
CI Agent Review
One finding from the review: the settings-transition error handler in activeSettings.ts routes every failure through publishFailedPopupVisualisation, conflating a settings/orchestration failure with a simulation failure and invalidating previously-good visualization state.
| if (rpcEndpointChanged && change.rpcNetwork?.httpsRpc !== undefined) simulationServicesOwner.reset(change.rpcNetwork) | ||
| if (updatedSettings.simulationMode && rpcChainChanged) await clearSimulationStateFromConfig() | ||
| } finally { | ||
| // Publish committed settings even if installing their services fails. |
There was a problem hiding this comment.
The catch block in runActiveSettingsChange calls publishFailedPopupVisualisation() for any error raised inside the inner settings-transition block, but that block is not limited to provider/simulation work: it also awaits rememberSigningAddressSelection() (signing-preference storage), clearSimulationStateFromConfig() (storage update), and the finally broadcasts popup_settingsUpdated. As written, a failed signing-preference persistence or failed settings broadcast would write simulationUpdatingState:'failed' and broadcast popup_simulation_state_changed, surfacing to the user as a broken simulation that never ran and invalidating a previously-good visualization (simulationResultState:'invalid'). This couples the settings layer's error handling to the simulation executor's output channel, so 'a settings transition failed' and 'the simulation failed' are no longer distinguishable. Only the provider-install step (simulationServicesOwner.reset() / the queued refresh) should own publishFailedPopupVisualisation(); unrelated failures should propagate to the caller/reporting layer without mutating the visualization state.
|
/review |
2 participants
What changes
Changing a wallet in the popup previously left the old selection visible while background work continued. This PR gives wallet, simulation/signing mode, RPC, and rich-mode changes immediate progress feedback and coordinates their completion across open and reopened popups.
Visual demonstration
Wallet-backed RPC switch: pending feedback followed by the selected network.
Captured from
0a4f4f1din Chromium with the existing isolated benchmark fixture: synthetic addresses, a controlled wallet, 1-second RPC delay, and 2-second wallet delay. The animation pauses at completion. These demonstrate UI behavior, not real-wallet or remote-RPC latency. Media is hosted separately and is not included in the extension code diff.Validation
Latest validation:
bun run test— 1,460 passed, 0 failed.bun run setup-chrome,bun run typecheck, andbun run lint— passed.bun run benchmark:popup-switching— passed, 30 samples across 3 isolated profiles.Regression coverage includes signer callbacks rejected without website authorization, overlapping commands, late and rejected wallet replies, provider resets during queued work and access prompts, confirmation after wallet-account refresh, snapshot capture timing and RPC usability after replacement, RPC failure/recovery, and multiple or reopened popups. Generated build output is excluded.