fix(siwe): sign-out semantics and case-normalized query keys by fionnachan · Pull Request #120 · OffchainLabs/tally-zero

fionnachan · 2026-08-27T18:38:09Z

Stacked on #102, base siwe/client-layer. Addresses the review findings on that PR, and then the findings on this one.

Sign-out (`6d58de3`, `a1a5046`)

#102 routed logout() through send(), so it throws on any non-2xx, but the consumer was not adapted: useSiwe cleared the session only in onSuccess, and the sole caller swallowed the rejection. Clicking Sign out on a 401 (cookie already gone server side) or on the proxy's 502/503 did nothing observable at all: stale session on screen, no error.

logout() now treats 401 as a completed sign-out (the second documented exception, mirroring me()); genuine failures still throw. isSigningOut / signOutError are exposed and rendered inline.

6d58de3 also moved the cache clear to onSettled, and a1a5046 walks that half back, because it reached further than it needed to and broke the error it was meant to surface. The signOutError paragraph renders inside SiweGate, so clearing the session closes the gate that displays it. When the reason for the failed logout is that the indexer is unreachable, the reconciling /api/me read fails too (me() only maps 401 to null), react-query retains the cleared value, and the user lands on the sign-in screen with no explanation and a live session cookie.

The 401 carve-out already covers the case that motivated onSettled, since logout() now resolves on both 204 and 401. So the clear is back on onSuccess, a failed sign-out leaves the session alone, and the gate stays open to show why. No component change was needed for that, which is the clearest sign it addressed the cause.

The setQueryData-then-invalidateQueries pair now lives in clearAndReconcileSession() (lib/siwe/session-cache.ts) so it can be tested: neither call is redundant and the order is not interchangeable, and nothing guarded that before.

Query keys (`7e0b0fa`, `ea4b913`)

siweKeys embedded the address verbatim, so wagmi's checksummed address and the indexer's lowercase effectiveAddress could open two cache entries for one subject. Now lowercased in subjectKey(), safes(), and publicCandidateProfile(), matching the sibling hooks.

7e0b0fa exempted election ids as "an opaque composite id", which ea4b913 corrects: types.ts documents them as ${governorAddress}:${proposalId}, so they carry an address with the same problem. Governor addresses arrive lowercase from the indexer and checksummed from config/governors.ts. The governor half is now normalized in candidateProfile and publicCandidateProfile; the proposal id is passed through untouched.

No callers of the subject-scoped or candidate-profile keys yet, so no cache migration.

Candidate profiles (`745dbc4`)

getPublicCandidateProfile was typed CandidateProfileVersion | null but routed through send(), so the null branch was unreachable: the indexer 404s a candidate who has never filed a profile, and a 404 threw, on what for a public candidate page is the ordinary case. It now returns null on 404 and throws on everything else, the third documented exception in that file. This rests on an indexer contract not verifiable from this repo.

Same commit corrects the claim in keys.ts and in the act-as block of client.ts that a subject switch is a single removeQueries(SUBJECT_SCOPE). It is two calls: siweKeys.me sits outside SUBJECT_SCOPE but carries the effective subject's resolved profile, so the scope removal leaves it behind. Verified against react-query 5.91.2.

Notes

.gitignore gains e2e/.auth/ (373fbaa). Combined with 93fa271 on the base, which untracked the five dumps, the directory is now both untracked and ignored. Their cookie values do remain in 2f57921's pushed history.
The sign-out wiring is covered at the cache level (lib/siwe/session-cache.test.ts), which needs no jsdom because what matters there is cache behaviour rather than React's. Rendering is not covered: the repo has no jsdom or @testing-library, and e2e coverage for this surface lands with the Playwright specs in later PRs in this stack.
Rebased onto siwe/client-layer to pick up 93fa271, so this branch is linear on the base.

Verified: 1300 unit tests, tsc, and eslint clean.

🤖 Generated with Claude Code

vercel · 2026-08-27T18:38:16Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
tally-zero	Ready	Preview	Aug 28, 2026 2:32pm

Context: 2f57921 routed logout() through send(), so it throws SiweApiError on any non-2xx where it previously never threw. The consumer was not adapted: useSiwe cleared the cached session only in onSuccess, and the app's only sign-out control discarded the rejection with signOut().catch(() => {}). The result was that clicking Sign out on a 401 (cookie already gone server side) or on the proxy's 502/503 paths did nothing observable at all: the ["siwe","me"] cache kept the stale session, SiweGate kept rendering the signed-in form, and no error reached the screen. The commit claimed to fix "a failed sign-out looked successful" and instead swapped it for a sign-out that looks like nothing happened. Why: Two different failures were being treated as one. A 401 means the server has no session, which is exactly what sign-out asked for, so it is a completed sign-out and not an error worth surfacing. A 502, 503, or timeout means we could not ask, which the user does need to know about. Handling the 401 in the client keeps the HTTP semantics where the rest of them live (me() already documents the same exception) and needs no component change. For the genuine failures, clearing local state alone was rejected: the me query stays mounted with staleTime 30_000, so a clear without a re-read gets reversed by the next refetch and the user watches themselves sign back in. Clearing and immediately invalidating means the UI never asserts a state the server contradicts, in either direction. Leaving the session untouched and only showing an error was the other candidate, but /api/me is the authority either way, so reconciling against it is the shorter path to the same answer. What: - lib/siwe/client.ts: logout() returns normally on 401 and still throws on everything else. This is the second documented exception to the file's "non-2xx throws" rule, so it steps back out of send() and calls fetch/parse directly, with a comment naming why. - hooks/use-siwe.ts: the signOut mutation moves from onSuccess to onSettled, which clears ME_KEY and then invalidates it so the session is re-read from /api/me at once rather than 30s later. isSigningOut and signOutError are now returned. - components/delegate/DelegateRegistrationForm.tsx: disables the button and shows "Signing out..." while pending, and renders signOutError.message inline (data-testid siwe-sign-out-error), the way SiweGate already renders signInError. The .catch(() => {}) stays, now only to keep the rejection from escaping unhandled, and says so. - lib/siwe/client.test.ts: the existing "surfaces a failed logout" case moves from 401 to 502 so it still guards what it was written to guard, plus a new case pinning that 401 resolves. - lib/siwe/client.ts also rewords the act-as comment that pointed at hooks/use-act-as.ts, a file that does not exist until the next PR in this stack. Left out: hook-level tests of the onSettled wiring across 2xx/401/502. The repo has no jsdom or @testing-library and vitest runs with environment "node", so no hook is tested directly anywhere here; adding React hook testing is a separate decision. The client semantics the fix turns on (401 resolves, 502 throws) are covered, and the happy-path sign-out stays covered by e2e/profile.spec.ts. Verified: npx vitest run (89 files, 1291 tests) green, tsc --noEmit clean, eslint clean on the changed files. References: - PR #102 (feat(siwe): client methods, types, and subject-scoped query keys for the full mount), review finding 2 - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: siweKeys embedded the address verbatim, but addresses reach these keys in two casings: the indexer returns effectiveAddress lowercase, wagmi returns a checksummed address. The safety-critical direction was never at risk, since two different subjects cannot collide and prefix eviction on SUBJECT_SCOPE catches every casing. The deduplication direction was: a caller reaching for wagmi's address instead of effectiveAddress opens a second cache entry for the same subject, and a write through one then leaves the other stale. Only a doc comment on useSiwe ("Use this, not address") stood between the two, and a doc comment is not an invariant. Why: The module's whole premise is that the key is the boundary you can reason about, so it should not depend on callers picking the right one of two spellings. Lowercasing also matches what the sibling hooks in this repo already do (use-user-vote, use-delegate-votes, and use-proposal-delegate-votes all lowercase the address in the key), which made keys.ts the outlier in the one module whose stated purpose is key correctness. No subject-scoped key has a caller on this branch yet (only siweKeys.me is consumed, and its value is unchanged), so changing the key values has no runtime effect today and needs no cache migration. What: - lib/siwe/keys.ts: add a local addr() helper and apply it to the subject in subjectKey(), the signer in safes(), and the address in publicCandidateProfile(). safes() uses `signer && addr(signer)` so the documented pre-session undefined still passes through instead of throwing on .toLowerCase(). electionId is deliberately left alone: it is an opaque composite id, not a bare address. - lib/siwe/keys.test.ts: two cases, one pinning that a checksummed and a lowercase address key identically across profile, drafts, draft, safes, and publicCandidateProfile, one pinning that safes(undefined) still does not throw. - lib/siwe/keys.test.ts also rewords the comment that described hooks/use-act-as.ts in the present tense; that file lands in the next PR of this stack. Not changed: safes(undefined) hashes the same as a hypothetical safes(null), because react-query's stable stringify serializes undefined in an array position to null. It stays harmless as long as the parameter is typed string | undefined and no caller can pass null, so the new test guards that property rather than the code changing to defend it. Verified: npx vitest run (89 files, 1291 tests) green, tsc --noEmit clean, eslint clean on the changed files. References: - PR #102, review findings 3, 4, and 5 - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: 2f57921 carried five e2e/.auth/*.json Playwright storageState dumps, each holding a real siwe_session cookie value against the local dev indexer (domain localhost, secure false). Nothing references them: playwright.config.ts declares no storageState, and e2e/profile.spec.ts authenticates from scratch every run. They were local artifacts that got swept in, and because the path was tracked, every local run that rewrites them produces diffs in unrelated PRs. Why: The exposure from these particular values is low and most have expired, but an unignored path means a future run against a shared or staging backend silently commits a valid session cookie, where it survives deletion. The reason to act is the pattern, not these values. What: Add e2e/.auth/ to the existing playwright block in .gitignore, with a comment naming the reason so nobody re-adds the path later. Note on the current state: the five files were deleted and then restored at the user's request, so they remain tracked at 2f57921 and this ignore rule does not apply to them (git ignores only untracked paths). It takes effect for any newly written storageState file, and a later `git rm --cached e2e/.auth/*.json` would bring the existing five under it while leaving them on disk. Either way the cookie values stay in pushed history at 2f57921 unless that commit is rewritten. References: - PR #102, review finding 1 - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: 2f57921 wrapped GET /api/elections/:id/candidate-profiles/:address as returning CandidateProfileVersion | null, with a comment promising "the bare version (or null)". It routed through send(), and parse() yields null only for a 2xx with an empty or literal-null body: every non-2xx goes through extractError and throws. The indexer 404s a candidate who has never filed a profile, so the null branch was unreachable and the documented contract was wrong. This fix was written once before, during the review round that produced 6fb2b1d and 4fd0ec0, and was lost when the branch was re-pointed onto the current main. It exists on no pushed ref, confirmed by grepping lib/siwe/client.ts across every origin ref for a 404 carve-out. Why: On a public candidate page a candidate with no profile is the ordinary case, not an edge one, so the common path was the one that rejected. A caller trusting the signature writes `if (!profile)` and gets an unhandled rejection instead of the empty state. Handling it in the client keeps the HTTP semantics where the other two exceptions already live, and needs no caller to catch anything. The route steps back out of send() and calls fetch/parse directly, the same shape me() and logout() already use. That is now three documented exceptions to the file's "non-2xx throws" rule, so they are numbered first/second/third and each says why it is one, to keep the set legible rather than accumulated. What: - lib/siwe/client.ts: getPublicCandidateProfile returns null on 404 and throws on everything else. me()'s comment drops "The one intentional exception" for "The first", which logout() had already made false. - lib/siwe/client.test.ts: three cases, one pinning that a 404 resolves to null, one that a 502 still rejects with SiweApiError, one that both path segments are encoded. The 404 case was verified to fail against the pre-fix implementation and pass after it. - lib/siwe/client.test.ts also renames lastCall() to firstCall(). It reads spy.mock.calls[0], so the name invited a future two-call test to assert against the wrong request; correct at all five call sites today because each asserts against a freshly created spy. - lib/siwe/keys.ts and the act-as block in client.ts: correct the claim that a subject switch is "a single removeQueries(SUBJECT_SCOPE)". It is two calls. siweKeys.me sits outside SUBJECT_SCOPE, yet /api/me answers with the effective subject's resolved profile and ownedFields, so removing the scope root leaves a stale subject profile cached. Verified against @tanstack/react-query 5.91.2: after removeQueries(SUBJECT_SCOPE), profile and drafts are evicted, safes survives as designed, and me survives holding the previous subject. Comments only, no key values changed. Not changed: the two-call sequence stays described rather than encoded. hooks/use-act-as.ts on siwe/act-as-ui already performs it in the right order (invalidate me, then remove the scope), so the only caller is correct, and a helper owning the ordering would ship here unused while its adopter lives on another branch. Worth revisiting when a second subject-switch path appears. Verified: npx vitest run (89 files, 1294 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the three changed files. References: - PR #102 review findings 1 and 4, and the lastCall nit - PR #120 (this branch), which carries the other findings from that round - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: 6fb2b1d moved the signOut mutation from onSuccess to onSettled so that a sign-out failing at the proxy could not leave the UI asserting a session the user had just ended. That fixed one thing and broke another. The error message the same commit added renders inside SiweGate (DelegateRegistrationForm.tsx:99-317), and clearing the session closes that gate, so the message was only visible if the reconciling /api/me read happened to succeed. When the reason for the failed logout is that the indexer is unreachable, /api/me returns 502 too: me() only maps 401 to null, so the refetch throws, react-query retains the cleared value, and the user lands on the sign-in screen with no explanation and a session cookie still very much alive. Verified against @tanstack/react-query 5.91.2 rather than assumed: after setQueryData(null) plus a throwing refetch, getQueryData is null and the query status is error. Why: onSettled was reaching further than it needed to. The case that motivated it, "clicking Sign out on a 401 did nothing", is already handled by the 401 carve-out 6fb2b1d added to logout() in the same commit: logout() now resolves on both 204 and 401, so onSuccess covers every outcome that actually signed the user out. What onSettled added on top was the genuine-failure case, and that is precisely where clearing is wrong. The two changes were partially redundant, and collapsing them is what makes the error reachable. Leaving the session alone on failure also needs no component change: the gate stays open, so the existing signOutError paragraph renders where it already sits. A screen that says "still signed in, here is why" is honest; one that says "signed out" while the cookie lives is not. What: - lib/siwe/session-cache.ts (new): clearAndReconcileSession() holds the setQueryData-then-invalidateQueries pair. It is a separate module so keys.ts stays a dependency-free data module with no react-query import, and a function rather than two inline lines so it can be tested. The comment names all three ways the pair can be got wrong. - hooks/use-siwe.ts: signOut clears via onSuccess again, delegating to clearAndReconcileSession. - lib/siwe/session-cache.test.ts (new): three cases driving a real QueryClient with a mounted QueryObserver. No jsdom needed, because what is worth pinning here is cache behaviour, not React's. Both halves of the pair are mutation-tested: replacing the invalidateQueries with Promise.resolve() fails "clears the session and refetches it in one call", and deleting the setQueryData fails "does not leave the old session readable while the refetch is in flight". Nothing guarded that ordering before, and it is silently reversible. The third case pins the react-query behaviour the design rests on, that setQueryData leaves the entry fresh under staleTime. If an upgrade changes that, the invalidation has become redundant and this fails rather than going unnoticed. Left out: no test that the error paragraph is reachable, which is React rendering rather than logic. The repo has no jsdom or @testing-library, and Playwright coverage for this surface lands with later PRs in this stack, so an interception pattern here would only duplicate it. Verified: npx vitest run (90 files, 1300 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the changed files. References: - PR #120 review findings 1 and 2, resolved as the plan's Option B - code-review-siwe-client-layer-followups.md, fix-plan-siwe-client-layer-followups.md - 6fb2b1d fix(siwe): make a failed sign-out clear, reconcile, and show why Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: 4fd0ec0 lowercased bare addresses in siweKeys so one subject could not occupy two cache entries, and exempted election ids on the grounds that "electionId is deliberately left alone: it is an opaque composite id, not a bare address". It is not opaque. types.ts documents it twice as `${governorAddress}:${proposalId}`, so it carries an address and has exactly the two-casings problem the rest of that commit fixed. The casings are real here, not hypothetical. Governor addresses arrive lowercase from the indexer, and config/governors.ts holds them checksummed (ADDRESSES.CONSTITUTIONAL_GOVERNOR is 0xf07DeD9dC292157749B6Fd268E37DF6EA38395B9); config imports addressesEqual and findByAddress from lib/address-utils precisely because that difference bites. Why: The convention argument 4fd0ec0 used to justify normalizing bare addresses points the same way for composites, and the repo demonstrates it: six places build a composite key holding a governor or contract address, and five lowercase it (use-delegate-votes.ts:25, DelegateVotesTable.tsx:58, use-multi-governor-search.ts:109, VoteForm.tsx:85, fetch-vote-history.ts:77 and :84). Only use-top-delegates-not-voted.ts:97 does not, and that looks like an oversight rather than a precedent. Nothing is broken today: grep finds no caller of the candidate-profile keys outside lib/siwe, and ids that come from listElections() share one source so they are internally consistent. The divergence needs a caller that builds an id locally, and the natural way to do that is from config/governors.ts, which is checksummed. Fixing it while there are no callers costs nothing and needs no cache migration, the same reasoning 4fd0ec0 already relied on. What: - lib/siwe/keys.ts: an electionKey() helper beside addr() lowercases only the segment before the first colon, applied in candidateProfile and publicCandidateProfile. The proposal id is passed through untouched rather than lowercased, so nothing depends on its alphabet, and an id with no colon is returned unchanged rather than guessed at. - lib/siwe/keys.test.ts: three cases, covering casing equivalence across both builders, the proposal id surviving untouched, and the no-colon input. Mutation-tested: reverting electionKey to the identity function fails two of the three. Verified: npx vitest run (90 files, 1300 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the changed files. References: - PR #120 review finding 3, resolved as the plan's Option A - code-review-siwe-client-layer-followups.md, fix-plan-siwe-client-layer-followups.md - 4fd0ec0 fix(siwe): normalize address casing inside the subject-scoped keys Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

douglance

Approve the substance — sign-out semantics, the 401/404 carve-outs, and the casing normalization are all right, and the two-step eviction the comments describe is exactly what use-act-as.ts does in #103. One change requested inline so the stack above rebases cleanly; the other two suggestions are the matching signatures.

The branches above this one widen the subject to string | null | undefined, because the key is built before the session resolves and the fetch is held back with skipToken. addr() lowercased unconditionally, so that shape failed tsc and threw on null at runtime. Pass an unresolved part through untouched, matching how use-user-vote and use-delegate-votes key an address.

Context: 2f57921 wrapped GET /api/elections/:id/candidate-profiles/:address as returning CandidateProfileVersion | null, with a comment promising "the bare version (or null)". It routed through send(), and parse() yields null only for a 2xx with an empty or literal-null body: every non-2xx goes through extractError and throws. The indexer 404s a candidate who has never filed a profile, so the null branch was unreachable and the documented contract was wrong. This fix was written once before, during the review round that produced 6fb2b1d and 4fd0ec0, and was lost when the branch was re-pointed onto the current main. It exists on no pushed ref, confirmed by grepping lib/siwe/client.ts across every origin ref for a 404 carve-out. Why: On a public candidate page a candidate with no profile is the ordinary case, not an edge one, so the common path was the one that rejected. A caller trusting the signature writes `if (!profile)` and gets an unhandled rejection instead of the empty state. Handling it in the client keeps the HTTP semantics where the other two exceptions already live, and needs no caller to catch anything. The route steps back out of send() and calls fetch/parse directly, the same shape me() and logout() already use. That is now three documented exceptions to the file's "non-2xx throws" rule, so they are numbered first/second/third and each says why it is one, to keep the set legible rather than accumulated. What: - lib/siwe/client.ts: getPublicCandidateProfile returns null on 404 and throws on everything else. me()'s comment drops "The one intentional exception" for "The first", which logout() had already made false. - lib/siwe/client.test.ts: three cases, one pinning that a 404 resolves to null, one that a 502 still rejects with SiweApiError, one that both path segments are encoded. The 404 case was verified to fail against the pre-fix implementation and pass after it. - lib/siwe/client.test.ts also renames lastCall() to firstCall(). It reads spy.mock.calls[0], so the name invited a future two-call test to assert against the wrong request; correct at all five call sites today because each asserts against a freshly created spy. - lib/siwe/keys.ts and the act-as block in client.ts: correct the claim that a subject switch is "a single removeQueries(SUBJECT_SCOPE)". It is two calls. siweKeys.me sits outside SUBJECT_SCOPE, yet /api/me answers with the effective subject's resolved profile and ownedFields, so removing the scope root leaves a stale subject profile cached. Verified against @tanstack/react-query 5.91.2: after removeQueries(SUBJECT_SCOPE), profile and drafts are evicted, safes survives as designed, and me survives holding the previous subject. Comments only, no key values changed. Not changed: the two-call sequence stays described rather than encoded. hooks/use-act-as.ts on siwe/act-as-ui already performs it in the right order (invalidate me, then remove the scope), so the only caller is correct, and a helper owning the ordering would ship here unused while its adopter lives on another branch. Worth revisiting when a second subject-switch path appears. Verified: npx vitest run (89 files, 1294 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the three changed files. References: - PR #102 review findings 1 and 4, and the lastCall nit - PR #120 (this branch), which carries the other findings from that round - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: 6fb2b1d moved the signOut mutation from onSuccess to onSettled so that a sign-out failing at the proxy could not leave the UI asserting a session the user had just ended. That fixed one thing and broke another. The error message the same commit added renders inside SiweGate (DelegateRegistrationForm.tsx:99-317), and clearing the session closes that gate, so the message was only visible if the reconciling /api/me read happened to succeed. When the reason for the failed logout is that the indexer is unreachable, /api/me returns 502 too: me() only maps 401 to null, so the refetch throws, react-query retains the cleared value, and the user lands on the sign-in screen with no explanation and a session cookie still very much alive. Verified against @tanstack/react-query 5.91.2 rather than assumed: after setQueryData(null) plus a throwing refetch, getQueryData is null and the query status is error. Why: onSettled was reaching further than it needed to. The case that motivated it, "clicking Sign out on a 401 did nothing", is already handled by the 401 carve-out 6fb2b1d added to logout() in the same commit: logout() now resolves on both 204 and 401, so onSuccess covers every outcome that actually signed the user out. What onSettled added on top was the genuine-failure case, and that is precisely where clearing is wrong. The two changes were partially redundant, and collapsing them is what makes the error reachable. Leaving the session alone on failure also needs no component change: the gate stays open, so the existing signOutError paragraph renders where it already sits. A screen that says "still signed in, here is why" is honest; one that says "signed out" while the cookie lives is not. What: - lib/siwe/session-cache.ts (new): clearAndReconcileSession() holds the setQueryData-then-invalidateQueries pair. It is a separate module so keys.ts stays a dependency-free data module with no react-query import, and a function rather than two inline lines so it can be tested. The comment names all three ways the pair can be got wrong. - hooks/use-siwe.ts: signOut clears via onSuccess again, delegating to clearAndReconcileSession. - lib/siwe/session-cache.test.ts (new): three cases driving a real QueryClient with a mounted QueryObserver. No jsdom needed, because what is worth pinning here is cache behaviour, not React's. Both halves of the pair are mutation-tested: replacing the invalidateQueries with Promise.resolve() fails "clears the session and refetches it in one call", and deleting the setQueryData fails "does not leave the old session readable while the refetch is in flight". Nothing guarded that ordering before, and it is silently reversible. The third case pins the react-query behaviour the design rests on, that setQueryData leaves the entry fresh under staleTime. If an upgrade changes that, the invalidation has become redundant and this fails rather than going unnoticed. Left out: no test that the error paragraph is reachable, which is React rendering rather than logic. The repo has no jsdom or @testing-library, and Playwright coverage for this surface lands with later PRs in this stack, so an interception pattern here would only duplicate it. Verified: npx vitest run (90 files, 1300 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the changed files. References: - PR #120 review findings 1 and 2, resolved as the plan's Option B - code-review-siwe-client-layer-followups.md, fix-plan-siwe-client-layer-followups.md - 6fb2b1d fix(siwe): make a failed sign-out clear, reconcile, and show why Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Context: 4fd0ec0 lowercased bare addresses in siweKeys so one subject could not occupy two cache entries, and exempted election ids on the grounds that "electionId is deliberately left alone: it is an opaque composite id, not a bare address". It is not opaque. types.ts documents it twice as `${governorAddress}:${proposalId}`, so it carries an address and has exactly the two-casings problem the rest of that commit fixed. The casings are real here, not hypothetical. Governor addresses arrive lowercase from the indexer, and config/governors.ts holds them checksummed (ADDRESSES.CONSTITUTIONAL_GOVERNOR is 0xf07DeD9dC292157749B6Fd268E37DF6EA38395B9); config imports addressesEqual and findByAddress from lib/address-utils precisely because that difference bites. Why: The convention argument 4fd0ec0 used to justify normalizing bare addresses points the same way for composites, and the repo demonstrates it: six places build a composite key holding a governor or contract address, and five lowercase it (use-delegate-votes.ts:25, DelegateVotesTable.tsx:58, use-multi-governor-search.ts:109, VoteForm.tsx:85, fetch-vote-history.ts:77 and :84). Only use-top-delegates-not-voted.ts:97 does not, and that looks like an oversight rather than a precedent. Nothing is broken today: grep finds no caller of the candidate-profile keys outside lib/siwe, and ids that come from listElections() share one source so they are internally consistent. The divergence needs a caller that builds an id locally, and the natural way to do that is from config/governors.ts, which is checksummed. Fixing it while there are no callers costs nothing and needs no cache migration, the same reasoning 4fd0ec0 already relied on. What: - lib/siwe/keys.ts: an electionKey() helper beside addr() lowercases only the segment before the first colon, applied in candidateProfile and publicCandidateProfile. The proposal id is passed through untouched rather than lowercased, so nothing depends on its alphabet, and an id with no colon is returned unchanged rather than guessed at. - lib/siwe/keys.test.ts: three cases, covering casing equivalence across both builders, the proposal id surviving untouched, and the no-colon input. Mutation-tested: reverting electionKey to the identity function fails two of the three. Verified: npx vitest run (90 files, 1300 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the changed files. References: - PR #120 review finding 3, resolved as the plan's Option A - code-review-siwe-client-layer-followups.md, fix-plan-siwe-client-layer-followups.md - 4fd0ec0 fix(siwe): normalize address casing inside the subject-scoped keys Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…the full mount (#102) * feat(siwe): client methods, types, and subject-scoped query keys for the full mount The SIWE mount exposes 24 routes; the client wrapped 6. Add the remaining 18 (session, safes, act-as, drafts, elections, candidate profiles). Every call now goes through one send() helper, including the five that previously hand-rolled fetch + parse — one convention in the file rather than two. That also fixes logout(), which fire-and-forgot its response, so a failed sign-out looked successful while the server kept the session alive. Query keys: the indexer resolves every owned read/write against effectiveSubject = actingAs ?? address, so act-as is a server-side mode and the same URL means a different subject once it flips. Caching across that boundary can put one entity's data into a form whose Save writes it to another. Every subject-scoped key therefore nests under one shared SUBJECT_SCOPE prefix carrying the effective address, so eviction on a subject switch is a single removeQueries() and a newly added subject-scoped query is covered the moment it is written — no parallel list to keep in sync. keys.test.ts pins that invariant. Signer-scoped keys are kept deliberately separate: /api/auth/safes resolves against session.address, not the effective subject, so acting as a Safe must not move it. MeResponse.safes was typed unknown[] but is a bare string[]; the enriched KnownSafe[] only comes from /api/auth/safes. The draft governor type is DraftGovernorType rather than GovernorType because config/governors.ts already owns that name with different values. * chore(e2e): untrack the .auth storage-state files They are regenerated by the setup project on every run and were already gitignored; the ignore rule never applied because they were tracked. * fix(siwe): make a failed sign-out clear, reconcile, and show why Context: 2f57921 routed logout() through send(), so it throws SiweApiError on any non-2xx where it previously never threw. The consumer was not adapted: useSiwe cleared the cached session only in onSuccess, and the app's only sign-out control discarded the rejection with signOut().catch(() => {}). The result was that clicking Sign out on a 401 (cookie already gone server side) or on the proxy's 502/503 paths did nothing observable at all: the ["siwe","me"] cache kept the stale session, SiweGate kept rendering the signed-in form, and no error reached the screen. The commit claimed to fix "a failed sign-out looked successful" and instead swapped it for a sign-out that looks like nothing happened. Why: Two different failures were being treated as one. A 401 means the server has no session, which is exactly what sign-out asked for, so it is a completed sign-out and not an error worth surfacing. A 502, 503, or timeout means we could not ask, which the user does need to know about. Handling the 401 in the client keeps the HTTP semantics where the rest of them live (me() already documents the same exception) and needs no component change. For the genuine failures, clearing local state alone was rejected: the me query stays mounted with staleTime 30_000, so a clear without a re-read gets reversed by the next refetch and the user watches themselves sign back in. Clearing and immediately invalidating means the UI never asserts a state the server contradicts, in either direction. Leaving the session untouched and only showing an error was the other candidate, but /api/me is the authority either way, so reconciling against it is the shorter path to the same answer. What: - lib/siwe/client.ts: logout() returns normally on 401 and still throws on everything else. This is the second documented exception to the file's "non-2xx throws" rule, so it steps back out of send() and calls fetch/parse directly, with a comment naming why. - hooks/use-siwe.ts: the signOut mutation moves from onSuccess to onSettled, which clears ME_KEY and then invalidates it so the session is re-read from /api/me at once rather than 30s later. isSigningOut and signOutError are now returned. - components/delegate/DelegateRegistrationForm.tsx: disables the button and shows "Signing out..." while pending, and renders signOutError.message inline (data-testid siwe-sign-out-error), the way SiweGate already renders signInError. The .catch(() => {}) stays, now only to keep the rejection from escaping unhandled, and says so. - lib/siwe/client.test.ts: the existing "surfaces a failed logout" case moves from 401 to 502 so it still guards what it was written to guard, plus a new case pinning that 401 resolves. - lib/siwe/client.ts also rewords the act-as comment that pointed at hooks/use-act-as.ts, a file that does not exist until the next PR in this stack. Left out: hook-level tests of the onSettled wiring across 2xx/401/502. The repo has no jsdom or @testing-library and vitest runs with environment "node", so no hook is tested directly anywhere here; adding React hook testing is a separate decision. The client semantics the fix turns on (401 resolves, 502 throws) are covered, and the happy-path sign-out stays covered by e2e/profile.spec.ts. Verified: npx vitest run (89 files, 1291 tests) green, tsc --noEmit clean, eslint clean on the changed files. References: - PR #102 (feat(siwe): client methods, types, and subject-scoped query keys for the full mount), review finding 2 - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(siwe): normalize address casing inside the subject-scoped keys Context: siweKeys embedded the address verbatim, but addresses reach these keys in two casings: the indexer returns effectiveAddress lowercase, wagmi returns a checksummed address. The safety-critical direction was never at risk, since two different subjects cannot collide and prefix eviction on SUBJECT_SCOPE catches every casing. The deduplication direction was: a caller reaching for wagmi's address instead of effectiveAddress opens a second cache entry for the same subject, and a write through one then leaves the other stale. Only a doc comment on useSiwe ("Use this, not address") stood between the two, and a doc comment is not an invariant. Why: The module's whole premise is that the key is the boundary you can reason about, so it should not depend on callers picking the right one of two spellings. Lowercasing also matches what the sibling hooks in this repo already do (use-user-vote, use-delegate-votes, and use-proposal-delegate-votes all lowercase the address in the key), which made keys.ts the outlier in the one module whose stated purpose is key correctness. No subject-scoped key has a caller on this branch yet (only siweKeys.me is consumed, and its value is unchanged), so changing the key values has no runtime effect today and needs no cache migration. What: - lib/siwe/keys.ts: add a local addr() helper and apply it to the subject in subjectKey(), the signer in safes(), and the address in publicCandidateProfile(). safes() uses `signer && addr(signer)` so the documented pre-session undefined still passes through instead of throwing on .toLowerCase(). electionId is deliberately left alone: it is an opaque composite id, not a bare address. - lib/siwe/keys.test.ts: two cases, one pinning that a checksummed and a lowercase address key identically across profile, drafts, draft, safes, and publicCandidateProfile, one pinning that safes(undefined) still does not throw. - lib/siwe/keys.test.ts also rewords the comment that described hooks/use-act-as.ts in the present tense; that file lands in the next PR of this stack. Not changed: safes(undefined) hashes the same as a hypothetical safes(null), because react-query's stable stringify serializes undefined in an array position to null. It stays harmless as long as the parameter is typed string | undefined and no caller can pass null, so the new test guards that property rather than the code changing to defend it. Verified: npx vitest run (89 files, 1291 tests) green, tsc --noEmit clean, eslint clean on the changed files. References: - PR #102, review findings 3, 4, and 5 - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(e2e): ignore the Playwright storageState directory Context: 2f57921 carried five e2e/.auth/*.json Playwright storageState dumps, each holding a real siwe_session cookie value against the local dev indexer (domain localhost, secure false). Nothing references them: playwright.config.ts declares no storageState, and e2e/profile.spec.ts authenticates from scratch every run. They were local artifacts that got swept in, and because the path was tracked, every local run that rewrites them produces diffs in unrelated PRs. Why: The exposure from these particular values is low and most have expired, but an unignored path means a future run against a shared or staging backend silently commits a valid session cookie, where it survives deletion. The reason to act is the pattern, not these values. What: Add e2e/.auth/ to the existing playwright block in .gitignore, with a comment naming the reason so nobody re-adds the path later. Note on the current state: the five files were deleted and then restored at the user's request, so they remain tracked at 2f57921 and this ignore rule does not apply to them (git ignores only untracked paths). It takes effect for any newly written storageState file, and a later `git rm --cached e2e/.auth/*.json` would bring the existing five under it while leaving them on disk. Either way the cookie values stay in pushed history at 2f57921 unless that commit is rewritten. References: - PR #102, review finding 1 - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(siwe): return null for a candidate with no public profile Context: 2f57921 wrapped GET /api/elections/:id/candidate-profiles/:address as returning CandidateProfileVersion | null, with a comment promising "the bare version (or null)". It routed through send(), and parse() yields null only for a 2xx with an empty or literal-null body: every non-2xx goes through extractError and throws. The indexer 404s a candidate who has never filed a profile, so the null branch was unreachable and the documented contract was wrong. This fix was written once before, during the review round that produced 6fb2b1d and 4fd0ec0, and was lost when the branch was re-pointed onto the current main. It exists on no pushed ref, confirmed by grepping lib/siwe/client.ts across every origin ref for a 404 carve-out. Why: On a public candidate page a candidate with no profile is the ordinary case, not an edge one, so the common path was the one that rejected. A caller trusting the signature writes `if (!profile)` and gets an unhandled rejection instead of the empty state. Handling it in the client keeps the HTTP semantics where the other two exceptions already live, and needs no caller to catch anything. The route steps back out of send() and calls fetch/parse directly, the same shape me() and logout() already use. That is now three documented exceptions to the file's "non-2xx throws" rule, so they are numbered first/second/third and each says why it is one, to keep the set legible rather than accumulated. What: - lib/siwe/client.ts: getPublicCandidateProfile returns null on 404 and throws on everything else. me()'s comment drops "The one intentional exception" for "The first", which logout() had already made false. - lib/siwe/client.test.ts: three cases, one pinning that a 404 resolves to null, one that a 502 still rejects with SiweApiError, one that both path segments are encoded. The 404 case was verified to fail against the pre-fix implementation and pass after it. - lib/siwe/client.test.ts also renames lastCall() to firstCall(). It reads spy.mock.calls[0], so the name invited a future two-call test to assert against the wrong request; correct at all five call sites today because each asserts against a freshly created spy. - lib/siwe/keys.ts and the act-as block in client.ts: correct the claim that a subject switch is "a single removeQueries(SUBJECT_SCOPE)". It is two calls. siweKeys.me sits outside SUBJECT_SCOPE, yet /api/me answers with the effective subject's resolved profile and ownedFields, so removing the scope root leaves a stale subject profile cached. Verified against @tanstack/react-query 5.91.2: after removeQueries(SUBJECT_SCOPE), profile and drafts are evicted, safes survives as designed, and me survives holding the previous subject. Comments only, no key values changed. Not changed: the two-call sequence stays described rather than encoded. hooks/use-act-as.ts on siwe/act-as-ui already performs it in the right order (invalidate me, then remove the scope), so the only caller is correct, and a helper owning the ordering would ship here unused while its adopter lives on another branch. Worth revisiting when a second subject-switch path appears. Verified: npx vitest run (89 files, 1294 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the three changed files. References: - PR #102 review findings 1 and 4, and the lastCall nit - PR #120 (this branch), which carries the other findings from that round - code-review-siwe-client-layer.md, fix-plan-siwe-client-layer.md - 2f57921 feat(siwe): client methods, types, and subject-scoped query keys for the full mount Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(siwe): only drop the local session when the server confirms sign-out Context: 6fb2b1d moved the signOut mutation from onSuccess to onSettled so that a sign-out failing at the proxy could not leave the UI asserting a session the user had just ended. That fixed one thing and broke another. The error message the same commit added renders inside SiweGate (DelegateRegistrationForm.tsx:99-317), and clearing the session closes that gate, so the message was only visible if the reconciling /api/me read happened to succeed. When the reason for the failed logout is that the indexer is unreachable, /api/me returns 502 too: me() only maps 401 to null, so the refetch throws, react-query retains the cleared value, and the user lands on the sign-in screen with no explanation and a session cookie still very much alive. Verified against @tanstack/react-query 5.91.2 rather than assumed: after setQueryData(null) plus a throwing refetch, getQueryData is null and the query status is error. Why: onSettled was reaching further than it needed to. The case that motivated it, "clicking Sign out on a 401 did nothing", is already handled by the 401 carve-out 6fb2b1d added to logout() in the same commit: logout() now resolves on both 204 and 401, so onSuccess covers every outcome that actually signed the user out. What onSettled added on top was the genuine-failure case, and that is precisely where clearing is wrong. The two changes were partially redundant, and collapsing them is what makes the error reachable. Leaving the session alone on failure also needs no component change: the gate stays open, so the existing signOutError paragraph renders where it already sits. A screen that says "still signed in, here is why" is honest; one that says "signed out" while the cookie lives is not. What: - lib/siwe/session-cache.ts (new): clearAndReconcileSession() holds the setQueryData-then-invalidateQueries pair. It is a separate module so keys.ts stays a dependency-free data module with no react-query import, and a function rather than two inline lines so it can be tested. The comment names all three ways the pair can be got wrong. - hooks/use-siwe.ts: signOut clears via onSuccess again, delegating to clearAndReconcileSession. - lib/siwe/session-cache.test.ts (new): three cases driving a real QueryClient with a mounted QueryObserver. No jsdom needed, because what is worth pinning here is cache behaviour, not React's. Both halves of the pair are mutation-tested: replacing the invalidateQueries with Promise.resolve() fails "clears the session and refetches it in one call", and deleting the setQueryData fails "does not leave the old session readable while the refetch is in flight". Nothing guarded that ordering before, and it is silently reversible. The third case pins the react-query behaviour the design rests on, that setQueryData leaves the entry fresh under staleTime. If an upgrade changes that, the invalidation has become redundant and this fails rather than going unnoticed. Left out: no test that the error paragraph is reachable, which is React rendering rather than logic. The repo has no jsdom or @testing-library, and Playwright coverage for this surface lands with later PRs in this stack, so an interception pattern here would only duplicate it. Verified: npx vitest run (90 files, 1300 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the changed files. References: - PR #120 review findings 1 and 2, resolved as the plan's Option B - code-review-siwe-client-layer-followups.md, fix-plan-siwe-client-layer-followups.md - 6fb2b1d fix(siwe): make a failed sign-out clear, reconcile, and show why Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(siwe): normalize the governor address inside election-id keys Context: 4fd0ec0 lowercased bare addresses in siweKeys so one subject could not occupy two cache entries, and exempted election ids on the grounds that "electionId is deliberately left alone: it is an opaque composite id, not a bare address". It is not opaque. types.ts documents it twice as `${governorAddress}:${proposalId}`, so it carries an address and has exactly the two-casings problem the rest of that commit fixed. The casings are real here, not hypothetical. Governor addresses arrive lowercase from the indexer, and config/governors.ts holds them checksummed (ADDRESSES.CONSTITUTIONAL_GOVERNOR is 0xf07DeD9dC292157749B6Fd268E37DF6EA38395B9); config imports addressesEqual and findByAddress from lib/address-utils precisely because that difference bites. Why: The convention argument 4fd0ec0 used to justify normalizing bare addresses points the same way for composites, and the repo demonstrates it: six places build a composite key holding a governor or contract address, and five lowercase it (use-delegate-votes.ts:25, DelegateVotesTable.tsx:58, use-multi-governor-search.ts:109, VoteForm.tsx:85, fetch-vote-history.ts:77 and :84). Only use-top-delegates-not-voted.ts:97 does not, and that looks like an oversight rather than a precedent. Nothing is broken today: grep finds no caller of the candidate-profile keys outside lib/siwe, and ids that come from listElections() share one source so they are internally consistent. The divergence needs a caller that builds an id locally, and the natural way to do that is from config/governors.ts, which is checksummed. Fixing it while there are no callers costs nothing and needs no cache migration, the same reasoning 4fd0ec0 already relied on. What: - lib/siwe/keys.ts: an electionKey() helper beside addr() lowercases only the segment before the first colon, applied in candidateProfile and publicCandidateProfile. The proposal id is passed through untouched rather than lowercased, so nothing depends on its alphabet, and an id with no colon is returned unchanged rather than guessed at. - lib/siwe/keys.test.ts: three cases, covering casing equivalence across both builders, the proposal id surviving untouched, and the no-colon input. Mutation-tested: reverting electionKey to the identity function fails two of the three. Verified: npx vitest run (90 files, 1300 tests) green, tsc --noEmit clean, eslint and prettier --check clean on the changed files. References: - PR #120 review finding 3, resolved as the plan's Option A - code-review-siwe-client-layer-followups.md, fix-plan-siwe-client-layer-followups.md - 4fd0ec0 fix(siwe): normalize address casing inside the subject-scoped keys Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(siwe): let the normalized keys accept an unresolved subject The branches above this one widen the subject to string | null | undefined, because the key is built before the session resolves and the fetch is held back with skipToken. addr() lowercased unconditionally, so that shape failed tsc and threw on null at runtime. Pass an unresolved part through untouched, matching how use-user-vote and use-delegate-votes key an address. --------- Co-authored-by: dlance <dlance@offchainlabs.com> Co-authored-by: Fionna <13184582+fionnachan@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

vercel Bot deployed to Preview August 27, 2026 18:39 View deployment

fionnachan and others added 6 commits August 28, 2026 13:40

fionnachan force-pushed the siwe/client-layer-followups branch from 957fba7 to ea4b913 Compare August 28, 2026 13:15

vercel Bot deployed to Preview August 28, 2026 13:17 View deployment

douglance reviewed Aug 28, 2026

View reviewed changes

Comment thread lib/siwe/keys.ts

Comment thread lib/siwe/keys.ts Outdated

Comment thread lib/siwe/keys.ts Outdated

vercel Bot deployed to Preview August 28, 2026 14:32 View deployment

douglance merged commit e704daa into siwe/client-layer Aug 28, 2026
1 check passed

This was referenced Sep 2, 2026

feat(siwe): act as a Safe — switcher, persistent banner, subject-aware profile #103

Closed

test(e2e): pre-authenticated session harness to stay under the nonce rate limit #104

Closed

test(e2e): cover the act-as Safe flow end to end #105

Closed

Navigation Menu

Uh oh!

fix(siwe): sign-out semantics and case-normalized query keys - #120

fix(siwe): sign-out semantics and case-normalized query keys#120
douglance merged 7 commits into
siwe/client-layerfrom
siwe/client-layer-followups

fionnachan commented Aug 27, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Aug 27, 2026 •

edited

Loading

Uh oh!

douglance left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

fionnachan commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sign-out (6d58de3, a1a5046)

Query keys (7e0b0fa, ea4b913)

Candidate profiles (745dbc4)

Notes

Uh oh!

vercel Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglance left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fionnachan commented Aug 27, 2026 •

edited

Loading

Sign-out (`6d58de3`, `a1a5046`)

Query keys (`7e0b0fa`, `ea4b913`)

Candidate profiles (`745dbc4`)

vercel Bot commented Aug 27, 2026 •

edited

Loading