fix(connect): guard re-entrant connect and surface silent lifecycle failures - #67
fix(connect): guard re-entrant connect and surface silent lifecycle failures#67fernandomg wants to merge 3 commits into
Conversation
|
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 hardens the wallet-connection lifecycle in canton-connect's CantonConnectProvider, fixing three defects surfaced by adversarial review of #47 (tracked as #57). It sits within the #51 stack (merge order #47, #48, #66, this, #63) and improves reliability of the wagmi-shaped hooks that wrap the dapp-sdk facade. The changes are refactored into small, named state-transition helpers plus a promise-based re-entrancy guard, and are backed by new concurrency-focused tests.
Changes:
- Adds a re-entrancy guard so a second
connect()during an in-flight attempt joins the first (viaattemptRefand a non-asyncconnectwrapper) rather than orphaning event wiring. - Surfaces previously silent failures: a failing
init()or session restore now lands asconnectError+disconnected(account read happens before claimingconnected), and a failed connect always re-throws its original cause after recovery. disconnect()now clearsconnectError(and tears down wiring) via the newresetToDisconnectedhelper; JSDoc updated in bothCantonConnectProvider.tsxanduseConnect.ts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
canton-connect/src/CantonConnectProvider.tsx |
Introduces teardownWiring/markConnected/markConnectedLocked/resetToDisconnected helpers, a non-throwing syncFromStatus, init-failure handling on mount, and the attemptRef-based re-entrancy guard around runConnect. |
canton-connect/src/hooks/useConnect.ts |
Updates UseConnectResult JSDoc to note connect idempotency and that disconnect clears the connect error. |
canton-connect/src/CantonConnectProvider.test.tsx |
Adds tests for failed restore/account-read teardown, session-vanish teardown, re-entrant/overlapping connect, preserved original error on recovery failure, disconnect() clearing the error, and surfaced init() failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
767318c to
38a18c0
Compare
38a18c0 to
2a7e7e0
Compare
- overlapping connect() calls each stored a teardown closure; the second overwrote the first - the orphaned wiring survived disconnect() and kept applying wallet pushes (#57) - guard with an attempt ref ahead of setStatus('connecting') and the listener teardown - non-async guard: an async wrapper would re-wrap the shared promise per caller, rejecting unhandled - keep one handler on the shared attempt so a fire-and-forget joiner cannot reject unhandled - document the idempotent-while-in-flight contract on the context value and useConnect
…nnected state - add a rejection handler to the mount init chain: connectError + disconnected, cancelled-aware - syncFromStatus claims connected only after the account read completes - a failed read tears down wiring, clears party/parties, surfaces the error, lands disconnected - syncFromStatus never throws, so connect()'s failure path keeps its original error - locked restore keeps its shape: no accounts to read, connected + isLocked directly - extract the repeated transitions into named helpers instead of setter piles
… failed connects - resetToDisconnected sets connectError unconditionally, so disconnect() clears a stale error - tear down wiring in runConnect's no-session branch: no live listeners on a disconnected provider - record the original error after recovery, so connectError matches what connect() rejects with
2a7e7e0 to
6e064b3
Compare
| const disconnect = useCallback(async (): Promise<void> => { | ||
| teardownRef.current?.() | ||
| teardownRef.current = undefined | ||
| teardownWiring() |
There was a problem hiding this comment.
(CC feedback)
High priority: disconnect() during an in-flight connect() is silently undone
disconnect() does not clear attemptRef or signal the in-flight attempt. With the picker open, a disconnect() resets to disconnected, then the picker resolves and runConnect runs wireEvents() + markConnected() (lines 291-294).
Verified with a probe test: after disconnect(), state went back to status: 'connected', party: 'alice::1', and a subsequent accountsChanged push was still delivered (party: 'carol::9'). This contradicts the PR's own acceptance criterion "no wiring outlives disconnect()".
| cancelled = true | ||
| teardownRef.current?.() | ||
| teardownRef.current = undefined | ||
| teardownWiring() |
There was a problem hiding this comment.
(CC feedback)
High priority: unmount during an in-flight connect() permanently leaks the listeners
runConnect tears wiring down at line 283 before opening the picker, so at unmount teardownRef.current is undefined and this cleanup is a no-op. The attempt then resolves and re-wires on a dead provider (line 291); nothing ever unwires it.
Verified with a probe test: onAccountsChanged called 1x, removeOnAccountsChanged called 0x.
Summary
Closes #57
Three lifecycle defects in
CantonConnectProvider, found by adversarial review of #47 after it wentgreen: overlapping
connect()calls leaked event wiring, startup and restore failures passedsilently, and
disconnect()left a staleconnectErrorbehind. One slice because they live in thesame three functions. Sits on #66 in the #51 stack; merge order is #47, #48, #66, this, #63.
Changes
connect(): a second call during an attempt joins the in-flight attemptinstead of starting a second one, so no wiring is ever orphaned.
init()or session restore surfaces asconnectErrorinstead of a silentidleor abogus
connected; the account read happens before the provider claimsconnected.disconnect()clearsconnectErrorand the current wiring.Acceptance criteria
From #57:
connect()during an attempt starts no second attempt, and no wiring outlivesdisconnect()init()or restore surfaces an error instead of a silent or bogus statedisconnect()clears the connect errordisconnect()also has to clear the remembered wallet and any pending in-page choice; both statesare introduced by #63, so they are cleared there.
#47 review threads answered here
init()and the account read; status stuck atidle304c9d5767318cThe rejected-lands-as-locked thread is only half closed here: the error the caller catches now
matches the error rendered. The state itself cannot be fixed from outside the SDK, because a
rejected wallet is indistinguishable from a locked one (
status()reportsisConnected: falseforboth and the session persists either way). The README paragraph saying so lands with #63's doc pass.
Test plan
Automated tests
pnpm -C canton-connect test: 50 to 57 tests. The ones that matter are concurrency cases the oldsuite structurally could not catch: two overlapping
connect()calls leave no orphaned wiring(asserted behaviourally, by pushing an
accountsChangedafterdisconnect()and requiring nothingto change), and a restore whose account read fails lands disconnected and tears its wiring down.
Verified at this commit: 57 passed,
tscandbiome checkclean.Manual verification
No manual steps required.
Breaking changes
None.
Checklist
Screenshots
None.