Wallet Connection & Network-Switch Guard with Persistent Session - #45
Conversation
…Closes workman-labs#25) Extends useWallet() with session restore that verifies Freighter's isAllowed() grant (not just a localStorage flag, so a revoked/stale session self-heals) and a live WatchWalletChanges-backed guard that detects the wrong Stellar network and guides the user to switch inside the extension, since Freighter has no API to switch networks on a dApp's behalf. NetworkGuard surfaces this app-wide, and EscrowFundingWizard now blocks progress past wallet-connect on the wrong network. Also wires WalletButton into the navbar, where it was defined but never mounted.
|
@libby-coder is attempting to deploy a commit to the Meshack Yaro's projects Team on Vercel. A member of the Team first needs to authorize it. |
This looks good to me but not without some minor changes — this is a solid, well-tested feature that meaningfully improves wallet UX and session correctness. Merge readiness and risk assessment
Possible improvements
Overall, this PR is well-scoped and well-tested. Addressing the cleanup/accessibility/clarity items above will reduce risk further and make the feature more robust in CI and production. |
…edback Fixes a real bug the review surfaced: useWallet() previously ran a fully independent instance (session restore + its own WatchWalletChanges poller) per call site, so the navbar's desktop/mobile WalletButton and NetworkGuard were each polling Freighter separately and could drift out of sync. Renamed the engine to useWalletState() and added WalletProvider (mirroring ThemeProvider's existing pattern) so the whole app shares one instance. Also: NetworkGuard moves focus to itself and gives its recheck action an explicit aria-label; WalletButton's collapsed trigger gets an aria-label covering the wrong-network state and decorative icons are aria-hidden; expectedNetwork is now typed as StellarNetwork instead of string; documented NEXT_PUBLIC_STELLAR_NETWORK's valid values and security implications in .env.example/README; added a regression test proving WalletProvider shares one instance/poller across consumers and an explicit extension-uninstalled-mid-session test.
|
Thanks for the thorough review! Pushed a follow-up commit addressing this: Cleanup / duplicate polling — good catch. NetworkGuard accessibility — it now moves focus to itself ( WalletButton — added an Test flakiness — worth flagging: the tests never used real timers or the library's actual polling loop. Typing — .env.example / README — both now spell out the three valid values and call out explicitly that this is security-relevant (it's what the guard checks before letting funds move), not just a label. Telemetry hook — left this out; it's explicitly optional and there's no analytics infrastructure in this project yet, so adding one would be speculative for this PR. CI chores — Verified again after these changes: |
meshackyaro
left a comment
There was a problem hiding this comment.
Excellent follow-up. This commit fixes a real bug and substantially improves the code quality on several fronts:
What this does well:
Architecture: The WalletProvider context pattern (mirroring ThemeProvider) is the right fix for the dual-instance problem. I appreciate how clearly you've documented the WHY in the hook's comment block — that failure mode (independent desktop/mobile buttons polling separately and drifting) is exactly the kind of subtle bug that ships into production undetected.
Accessibility: Solid additions across the board:
NetworkGuardnow moves focus when it appears — that's correct for an urgentrole="alert"region.WalletButton's aria-labels are precise and context-aware (e.g., "wrong network, action needed" conditional).- Decorative icons are now properly
aria-hidden. - The recheck button's aria-label includes state ("checking now") — nice touch.
Tests: The new walletProvider.test.tsx directly validates the one-instance invariant with two consumers in the same provider — exactly the scenario that would have exposed this bug in CI if the original code had made it through. The "extension uninstalled mid-session" test in wallet.test.tsx covers an edge case I wouldn't have thought to verify.
Documentation: The .env.example and README updates appropriately flag NEXT_PUBLIC_STELLAR_NETWORK as security-relevant, not just cosmetic. That's important guidance for the next person who deploys this.
Minor suggestions (not blockers):
-
Type export in wallet.ts: You're re-exporting
StellarNetworkfrom wallet.ts for consumer convenience, which is good. Consider doing the same for the fullWalletContextValuetype in the WalletProvider module itself — right now a consumer who wants to type-annotate a ref to the context has to reach into lib/wallet. (Not a real pain point, just a polish detail.) -
NetworkGuard's address display: The line
truncateAddress(address)in the banner is helpful, but I'd make sure the truncation is consistent with what the WalletButton uses — if someone sees "G...YZ" in the banner and "G...AB" in the button's label (due to a truncation mismatch), that confusion outweighs the added verbosity. A quick scan suggests they're already aligned, but worth a comment noting it.
Meanwhile, this is ship-ready. It's a solid architectural correction backed by thoughtful testing and accessibility work. Well done responding to the review feedback.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary
Closes #25.
useWallet()(src/lib/wallet.ts) now restores a session on reload by verifying Freighter's ownisAllowed()grant instead of trusting a localStorage flag alone, so a session revoked inside the extension self-heals instead of showing stale "connected" UI.WatchWalletChangespoller, exposesisWrongNetwork/expectedNetworkfor the lifetime of a connected session and reacts the moment the user switches networks or accounts inside the extension — no reload required.NetworkGuard(src/components/wallet/NetworkGuard.tsx), mounted app-wide in the root layout, shows a banner guiding the user through switching networks inside Freighter (there's no API for a dApp to switch it on their behalf) and clears itself automatically once the watcher detects the change; a "check again" action short-circuits the wait.EscrowFundingWizard's connect-wallet step now blocks progress while the wallet is on the wrong network, since funding escrow there isn't recoverable after the fact.WalletButtonbeing defined but never mounted in the navbar (its styling was also updated to match the current sand/light navbar — it was written for an older dark navbar).NEXT_PUBLIC_STELLAR_NETWORK(defaults toTESTNET; see.env.example).No new dependencies —
WatchWalletChangesandisAllowed()are both already part of the installed@stellar/freighter-apiversion. Documented in the README's "Web3 / Stellar touches" section.Test plan
npm run lint— 0 errors (pre-existingreact-hooks/set-state-in-effectwarnings only, consistent with the rest of the codebase)npx tsc --noEmit— no TypeScript errorsnpm test— 85/85 passing, including a newsrc/lib/test/wallet.test.tsxcovering connect, freighter-missing, declined access, wrong-network detection, session restore (allowed/revoked/uninstalled), the live watcher's network-change and access-revoked paths, manual recheck, and disconnectnpm run build— production build succeeds