feat(connect): add in-page wallet selection and remember the session wallet - #63
feat(connect): add in-page wallet selection and remember the session wallet#63fernandomg wants to merge 9 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
fernandomg
changed the title
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
This PR closes the second half of #51 in canton-connect. It adds an opt-in in-page wallet selection mode (walletSelection: 'in-page') that publishes the SDK's offered wallet entries and a select/cancel pair through a new useWalletPicker() hook, letting a dApp render its own chooser instead of the SDK popup. It also remembers which wallet a session belongs to across reloads (useParty().wallet, backed by a canton-connect:connected-wallet localStorage record), and hardens the connect/disconnect lifecycle so a killed attempt can no longer write state or deadlock disconnect().
Changes:
- New
walletSelection: 'in-page'mode +useWalletPicker()hook; explicitconfig.walletPickerstill takes precedence, popup remains the default (non-breaking). - Attempt-scoped cancellation model (
cancelledBy+condemn) sodisconnect()/unmount settle a never-answering wallet, and a late/abandoned answer writes nothing; a replacement SDK instance that restores nothing resets state. useParty()gainswallet(persisted viaconnectedWallet.ts); comment sweep plus README/architecture coverage of the picker seam and its documented limits.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
canton-connect/src/CantonConnectProvider.tsx |
Core: in-page picker bridge, attempt condemnation/cancellation, remember/forget wallet, replacement-instance reset. |
canton-connect/src/connectedWallet.ts |
New localStorage read/write/clear for the session's wallet label, with a defensive type guard. |
canton-connect/src/hooks/useWalletPicker.ts |
New hook exposing the pending in-page choice (isOpen, wallets, select, cancel). |
canton-connect/src/hooks/useParty.ts |
Adds wallet to the result; wagmi mapping note updated. |
canton-connect/src/types.ts |
New ConnectedWallet type and walletSelection config field. |
canton-connect/src/index.ts |
Barrel exports useWalletPicker and ConnectedWallet. |
canton-connect/src/hooks/useParties.ts, useConnect.ts, mock/mockAdapter.ts, walletAccount.ts |
Comment/JSDoc sweep; no behavioral change. |
canton-connect/src/connectedWallet.test.ts |
New unit tests for the record round-trip and corrupt/partial reads. |
canton-connect/src/CantonConnectProvider.test.tsx |
Large suite of concurrency/lifecycle tests for the new modes and cancellation races. |
canton-connect/README.md, architecture.md |
Document wallet selection, precedence, and the documented limits. |
I found no concrete, blocking defects: the cancellation/condemnation logic correctly attaches handlers to avoid unhandled rejections, the remember/forget ordering tracks the SDK session as the authority, and the new behavior is backed by extensive tests. However, this is a large, intricate concurrency change to the core connection lifecycle whose runtime behavior was verified by hand in a harness (the SDK graph is browser-only and not installed here), which makes it a poor candidate for automated sign-off.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5b3b93c to
f05a67b
Compare
f05a67b to
c3f7c30
Compare
- walletSelection: 'in-page' hands the wallet choice to the dApp's own UI - the provider keeps the picker's resolve/reject pair and publishes the offered entries - useWalletPicker() exposes isOpen/wallets/select/cancel; select answers the SDK - an explicit walletPicker still wins over the mode; the default popup path is untouched - select with an unoffered id rejects the attempt with a plain Error naming the id - ConnectedWallet type defined for the wallet-identity task that follows
…d connect attempt - record why a pending choice was cancelled: user, disconnect, or unmount - connect()'s failure path recovers only after a user cancel; disconnect and unmount just re-throw - disconnect() cancels the choice, waits for the in-flight attempt to settle, then resets - a choice arriving while disconnect or unmount kills the attempt rejects instead of pending - unmount cancels the pending choice, so the attempt settles without unhandled rejections - fold the init-failure path onto resetToDisconnected(err)
- new connectedWallet.ts: the record under canton-connect:connected-wallet - written only when an attempt lands: connect success or the probe ending connected-and-locked - restored sessions read the record; a probe finding no session deletes it, as does disconnect() - a supplied walletPicker is wrapped only to note its result; the wrapper identity stays stable - corrupt or partial records read as absent, never throw - useParty() gains wallet (wagmi: useAccount().connector); popup mode reports none by design
- an inline walletPicker or additionalAdapters rebuilds the SDK per re-render (kept, documented) - a replacement instance whose restore probe finds nothing now resets status and party too - replacement keys on instance identity, so a StrictMode re-run never resets a first mount - an idle provider stays idle across swaps: only non-idle state is stale enough to clear - pin in-page mode: rerenders keep one SDK instance and a single init() - tests: dispose each fake wallet when its role ends, so a failing assertion leaks no listeners
- delete comments that restate the identifier below them: file headers, toParty, toParties - shorten every survivor running past ~100 characters - remove a stray /** that doubled the walletPicker JSDoc opener
…seam - add useParties, useWalletPicker, and walletSelection to the README tables - render connectError outside the isConnected branch in the quickstart - state that a rejected wallet is indistinguishable from a locked one from outside the SDK - note that an inline walletPicker or additionalAdapters rebuilds the SDK every render - state the two limits: wallet list only during an attempt, no wallet identity in popup mode - architecture: the themed picker is issue #50; describe the in-page context seam
…g state - a wallet window closed without answering leaves the connect RPC pending forever; disconnect() awaited that attempt and deadlocked before any reset. The kill is now recorded on the attempt itself and a condemnation race settles the caller, so disconnect cleans up immediately (found by hand in the harness with a real extension wallet, task 12) - the same fences close what the review pass then found: an in-page choice arriving after disconnect no longer opens over the clean slate (the picker fails closed and each choice dooms its own attempt); the cancel-recovery probe re-checks its fate after every await instead of only at catch entry; a mount restore resolving mid-attempt yields instead of wiring beneath it - guards runConnect's success path too, so a late wallet answer writes no state and never evicts a newer attempt - also braces the single-line guards and trues up the JSDoc the rework left stale
- the config identity note claimed additionalAdapters churn rebuilds the SDK; only walletPicker does, a new adapters array re-runs init() on the instance - the mount bullet now carries the replacement-instance rule from 51c096a - useConnect's disconnect JSDoc says it settles an unanswered connect
c3f7c30 to
f293a46
Compare
| const result = await sdk.connect() // opens the picker | ||
| // A condemned attempt may still get the wallet's answer later; it owns no state by then. | ||
| if (attempt.cancelledBy !== undefined) { | ||
| throw new UserRejectedError('Connect attempt cancelled') | ||
| } |
There was a problem hiding this comment.
(CC feedback)
Disconnect leaves a live SDK session behind
disconnect() (L587-L600) condemns the attempt and runs sdk.disconnect() while the wallet's connect RPC is still pending. When the wallet answers later, sdk.connect() resolves and establishes a session; runConnect sees cancelledBy and throws here without tearing that session down. Measured after disconnect + late answer:
sdk.status() → {"connection":{"isConnected":true,...}}
localStorage → splice_discovery_client_session, splice_wallet_kernel_discovery
UI reads disconnected, SDK is connected, and a reload restores the session the user just left. This is the exact scenario 928423b set out to fix; the existing test (a late answer from an abandoned attempt writes no state) asserts only React state.
Reproduced against this branch with a silent-wallet probe (the same createSilentWallet shape the suite already uses) asserting sdk.status() rather than React state.
| cancelAttempt('unmount') | ||
| teardownWiring() | ||
| } | ||
| }, [sdk, additionalAdapters, syncFromStatus, teardownWiring]) | ||
| }, [ | ||
| sdk, | ||
| additionalAdapters, | ||
| syncFromStatus, | ||
| teardownWiring, | ||
| cancelAttempt, | ||
| resetToDisconnected, | ||
| forgetConnectedWallet, | ||
| ]) |
There was a problem hiding this comment.
(CC feedback)
Any re-render with inline additionalAdapters kills an in-flight connect
The mount effect's cleanup now calls cancelAttempt('unmount'). Its deps include additionalAdapters, so a fresh array identity re-runs the effect and aborts the pending choice. Probe result: one rerender() while the picker was open → rejected: Wallet selection cancelled, picker closed. Previously an identity churn only re-ran init(); now it destroys the user's connect. The README note about memoizing is no longer sufficient given the new consequence.
Reproduced against this branch: provider rendered with config={{ appName, walletSelection: 'in-page', additionalAdapters: [adapter] }} written inline, connect() started, picker open, then a single rerender().
There was a problem hiding this comment.
Manual testing
- ✅ Connect renders the offered wallet entries in-page with no popup
- ✅ picker.wallets is empty before any connect attempt
- ✅ select() connects: party is the primary, parties are usable-only, wallet is named
- ✅ Reload restores the session, the parties and the wallet name
- ✅ Cancel settles the attempt, re-enables Connect, and rejects the caller with the same error
- ✅ Cancel-then-reload keeps the session
- ✅ Cancelling a chooser opened over a live session recovers to connected
- ✅ A chooser over a live session reads isConnected: false with party and wallet still populated
- ✅ Disconnect with the chooser open ends disconnected, no error, no party, caller rejects
- ✅ select("not-offered") rejects naming the id, not as a cancellation
- ✅ A second connect() joins the first, only one choice opens
- ✅ Unmounting mid-choice rejects connect() with no unhandled rejection
- ✅ walletSelection: 'popup' opens the SDK popup and no in-page choice
- ✅ An explicit walletPicker wins over in-page mode
- ✅ Disconnect clears the connected-wallet record
- ✅ Clearing storage leaves no stale wallet record
- ✅ A replacement SDK instance that restores nothing resets connection state
- ✅ A mount restore landing during an attempt leaves consistent state
- ✅ A rejection after selection surfaces the documented Wallet picker is not open limit
- ❌ Disconnect while a selected wallet has not answered leaves no live session. Expected result: after
disconnect()resolves, no session survives:sdk.status()reportsisConnected: falseor throws, nosplice_discovery_client_sessionorsplice_wallet_kernel_discoverykeys remain inlocalStorage, and reloading the page lands disconnected.- Load the dApp with walletSelection: 'in-page' and a CIP-0103 extension wallet whose origin has not been approved yet.
- Click Connect and select the extension wallet from the in-page list.
- When the wallet's approval prompt appears, leave it unanswered.
- In the dApp, click Disconnect.
- Return to the wallet and approve the still-pending request.
- Call sdk.status() and inspect localStorage.
- Reload the page.
- ❌ A parent re-render with inline additionalAdapters does not kill the connect attempt. Expected result: The in-page choice opens and stays open across parent re-renders, and selecting a wallet completes the connection.
- Render CantonConnectProvider with walletSelection: 'in-page' and additionalAdapters written as an inline array literal in JSX, so its identity is fresh on every parent render.
- Make the parent component re-render on ordinary activity during a connect (state update, spinner, toast, log line).
- Click Connect.
- Observe whether the in-page choice opens.
- If it opens, trigger one parent re-render while it is open.
Also left a few code review comments.
|
put on hold, recalibrating |
Summary
Closes #51
Top of the #51 stack (#47, #48, #66, #67, this). #66 landed
useParties()and the party mapping;#67 the #57 lifecycle fixes. This closes #51's other half: the offered wallet list on the public
surface, opt-in in-page selection, the session's wallet surviving a reload, and a guard on the
SDK-instance identity.
Changes
walletSelection: 'in-page'publishes the offered entries and aselect/cancelpair throughuseWalletPicker(), so a dApp can draw its own chooser. Thedefault popup path is untouched, and an explicit
walletPickerstill wins.state after the thing that killed it finished.
useParty()gainswallet: which wallet the session belongs to, surviving a reload.disconnect()settles even when a selected wallet never answers: the kill is recorded on theattempt itself and a condemnation race settles the caller, so a wallet window closed without
answering can no longer deadlock
disconnect()(found by hand in the harness, see Manualverification). The pre-undraft review pass hardened the same fences: the in-page picker fails
closed after a kill (a choice can no longer open over the clean slate), cancel-recovery
re-checks its fate after every await, and a mount restore yields to an attempt in flight.
canton-connect/src, and README/architecture coverage of wallet selection,its limits, and the picker seam.
Acceptance criteria
From #51 (two criteria landed in #66 and are listed here for the closing picture):
useParties()exists, returns every usable account asParty, updates onaccountsChanged(feat(connect): add useParties() for every party the wallet holds #66)props, no kit-to-connect dependency
selectPrimaryAccountandtoPartystay internal (feat(connect): add useParties() for every party the wallet holds #66)README.md's hook table coversusePartiesanduseWalletPickerDiverged from #51 as written: #50's criteria say the picker wires through the existing
walletPickerseam and thatcanton-connectneeds no new API. This routes it through a new modeand hook instead, deliberately: a React panel is rendered and waits, where that seam wants a
function that answers. #50 needs its criteria updated before it is picked up.
#47 review threads answered here
connectedstate51c096aThe other two threads (unhandled startup rejections; the error half of rejected-lands-as-locked)
are answered by #67.
Accepted limits
Documented rather than worked around, because every available workaround means guessing at wallet
state we cannot observe:
helper throws "Wallet picker is not open", swallowing the real cause.
connect()andexposes them nowhere else.
direct dependency on its UI bundle.
An earlier draft of this PR listed a fourth limit: a cancelled attempt costing the user their
persisted session. The harness walkthrough disproved it; see the correction under Manual
verification.
Stopgaps, with their removal triggers
canton-connect:connected-walletinlocalStorage. We remember which wallet a sessionbelongs to only because
getConnectedProvider()discards the session's provider id. Remove itthe moment the SDK exposes the restored session's provider id. The record is never
authoritative; the SDK's session is, and the record is deleted whenever no session backs it.
useState, not a reducer. Thatwas analysed and deliberately deferred. Revisit trigger: a seventh piece of work touching these
transitions, then fold
status/party/parties/isLocked/connectedWalletinto one reducerwith named transitions, keeping
lastTx,offeredWalletsand the refs outside it.Test plan
Automated tests
pnpm -C canton-connect test: 57 to 85 tests (the stack as a whole takes the suite from 38). Theones that matter here are the concurrency cases the old suite structurally could not catch:
disconnect()during a pending choice ends disconnected with no error and no partydisconnect()while a selected wallet never answers resolves, resets, and rejects the callerconnect()after abandoning a never-answering wallet starts a fresh attempt, and a lateanswer from the abandoned one writes no state
disconnect()killed the attempt never opensdisconnect()landing during cancel-recovery keeps the clean slateidleRoot gate at undraft time: lint, typecheck, build, knip clean; 387 tests across five suites, exit
0. That total includes the two
node:testsuites (91 and 83) whose output aTests Ngrepsilently hides. Shipped-surface check:
distresolves for both the root and./testingentries,useParties/useWalletPicker/ConnectedWalletare indist/index.d.ts, and the JSDoc survivedinto the declarations; runtime behaviour of the built bundle is what the harness exercised (the SDK
graph is browser-only, so a plain-Node smoke import cannot execute it). #59 tracks committing this
check.
Manual verification
Task 12 walked the harness (Vite serving the built dist, the real SDK, the mock adapter,
walletSelection: 'in-page') through the six planned flows:mock,walletconnect); thewindow.openinstrumentation stayed silent, so no popup opened.
partiesholds onlyallocatedaccounts (an
initializedone is dropped), anduseParty().walletnames the wallet.canton-connect:connected-walletrecord).connectErrorreads "Wallet selectioncancelled" and the caller's
connect()rejected with that same error.connect()rejects to its caller.
without answering left the attempt pending forever, and
disconnect()deadlocked awaiting it —the page was stuck on "connecting" with no recovery. Fixed in
fc696cf(see Changes); thewallet-side half (an extension should reject pending requests when its window closes) belongs to
the wallet, not this package.
Correction to a claimed limit. The draft body and the plan said a cancelled attempt costs the
user their persisted session because
connect()clears the SDK's persisted state before the pickeropens. Observed instead:
connect()clears only the kernel-session hints(
splice_wallet_kernel_session,splice_wallet_kernel_discovery); the discovery session record(
splice_discovery_client_session) and the live in-memory session both survive a cancelledattempt. Cancelling a chooser opened over a connected session recovers to connected in-page, and a
reload after the cancel restores the session for any adapter implementing
restore(), which allthree SDK adapters do.
A transitional state worth knowing: while an attempt is open over a live session,
isConnectedreads false with
party/walletstill populated until the attempt settles.Caveat:
createMockAdapter()implements norestore(), so the reload flows were driven through aharness-local wrapper that persists the mock's connection and answers
restore(); the package pathexercised (init, discovery restore, status, listAccounts, record read) is the real one. #69 tracks
giving the mock opt-in persistence so the shim can be retired.
Breaking changes
None.
walletSelectiondefaults to'popup', which is exactly today's behaviour.Checklist
Screenshots
None.