Skip to content

improve(sdk): preserve EOA wallet connections on logout#482

Open
aqt-dev wants to merge 1 commit intomainfrom
improve/logout-preserve-eoa
Open

improve(sdk): preserve EOA wallet connections on logout#482
aqt-dev wants to merge 1 commit intomainfrom
improve/logout-preserve-eoa

Conversation

@aqt-dev
Copy link
Contributor

@aqt-dev aqt-dev commented Mar 2, 2026

Summary

  • Preserves EOA wallet connections (MetaMask, Coinbase Wallet) on logout by clearing thirdweb's active-wallet stores directly via useConnectionManager() instead of calling disconnect()
  • Ecosystem/smart wallets still get full disconnect() for proper auth cleanup
  • Blank modal fix from fix(sdk): disconnect active wallet on logout regardless of wallet type #481 is preserved — activeAccount is cleared to undefined in both paths
  • DRYs up authenticateUser by extracting finalizeAuth helper

How it works

On logout, the active wallet is now handled differently based on type:

  • Ecosystem/smart: disconnect() — revokes auth, removes from connectedWallets
  • EOA: clears activeAccountStore, activeWalletStore, activeWalletChainStore, activeWalletConnectionStatusStore directly — wallet stays in connectedWallets, thirdweb:active-wallet-id stays in localStorage, autoConnectCore silently reconnects via eth_accounts on next login (no approval popup)

Test plan

  • Login with email → logout → verify login form appears (not blank modal)
  • Login with Coinbase Wallet → logout → login again → should NOT see wallet approval popup
  • Login with MetaMask → logout → login again → should NOT see wallet approval popup
  • After logout: isAuthenticated=false, isConnected=false, isAuthenticating=false, user=undefined
  • Ecosystem wallet is fully disconnected after logout

🤖 Generated with Claude Code

Instead of unconditionally calling disconnect() on the active wallet
(which nukes EOA wallets from connectedWallets and revokes permissions),
use useConnectionManager() to clear only the active-wallet stores for
EOA wallets. This preserves the wallet in connectedWallets and keeps
thirdweb:active-wallet-id in localStorage, allowing autoConnectCore to
silently reconnect via eth_accounts on next login — no approval popup.

Ecosystem/smart wallets still get full disconnect() for proper auth
cleanup. The blank modal fix is preserved since activeAccount is still
cleared to undefined in both paths.

Also DRYs up authenticateUser by extracting finalizeAuth helper.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aqt-dev aqt-dev requested a review from a team as a code owner March 2, 2026 06:22
@claude
Copy link

claude bot commented Mar 2, 2026

Claude finished @aqt-dev's task in 2m 4s —— View job


PR Review: improve(sdk): preserve EOA wallet connections on logout

  • Gather context (CLAUDE.md, PR diff, relevant files)
  • Analyze the changes
  • Post review feedback

The approach is sound and the finalizeAuth DRY refactor is clean. A few things worth flagging:

Issue 1 — Bypassing thirdweb's internal stores (Medium)

useAuthentication.ts:190-193

mgr.activeAccountStore.setValue(undefined);
mgr.activeWalletStore.setValue(undefined);
mgr.activeWalletChainStore.setValue(undefined);
mgr.activeWalletConnectionStatusStore.setValue("disconnected");

This directly mutates thirdweb's internal store objects rather than going through any public API. While these stores are exposed on ConnectionManager, the setValue() bypass skips any side effects that disconnect() normally triggers — e.g. event emitter teardown, subscription cleanup, or other internal bookkeeping inside thirdweb. If thirdweb renames these stores or changes their semantics in a patch release, this will break silently and be hard to diagnose. Worth adding a comment referencing the specific thirdweb version (5.116.1) this was tested against, and ideally a test that asserts the four stores are null after logout so a future thirdweb upgrade surfaces any regression immediately.

Issue 2 — Inconsistent EOA handling in the onConnect error path (Low)

useAuthentication.ts:268-271

if (_walleAutoConnectedWith && _walleAutoConnectedWith !== connectedEcosystemWallet) {
  disconnect(_walleAutoConnectedWith);
}

When auto-connect succeeds, EOA wallets are preserved on logout. But when auto-connect fails (e.g. auth error on page load), _walleAutoConnectedWith gets a full disconnect() regardless of wallet type — removing it from connectedWallets and requiring an approval popup next time. The behavior is asymmetric:

  • Auth success → logout → no popup ✓
  • Auth failure on auto-connect → next login → popup required ✗

This is arguably an acceptable edge case (failed auth is unusual), but it's worth documenting or aligning. Fix this →

Nit — Unnecessary alias

useAuthentication.ts:189

const mgr = connectionManager;

mgr is only used 4 lines below and adds no clarity. Use connectionManager directly.


Overall the change is correct for the happy path. The internal API concern (Issue 1) is the main risk — it's worth pinning a comment or adding a smoke test to catch thirdweb upgrade breakage early.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the authentication and logout mechanisms within the SDK, primarily focusing on improving the user experience for EOA wallet users. It introduces a nuanced logout process that preserves EOA wallet connections, allowing for silent re-authentication on subsequent logins, while maintaining robust disconnection for other wallet types. Additionally, it addresses a blank modal display issue and streamlines the authentication code through refactoring.

Highlights

  • EOA Wallet Connection Preservation: Modified logout behavior to preserve connections for EOA wallets (MetaMask, Coinbase Wallet) by directly clearing thirdweb's active-wallet stores instead of calling disconnect(), allowing for silent re-authentication.
  • Differentiated Logout Logic: Implemented distinct logout processes: ecosystem/smart wallets undergo a full disconnect() to revoke authentication, while EOA wallets only have their active-wallet stores cleared, maintaining their presence in connectedWallets.
  • Blank Modal Fix: Ensured that activeAccount is consistently cleared to undefined across all logout paths, resolving an issue where a blank modal might appear instead of the login form.
  • Authentication Code Refactoring: Introduced a finalizeAuth helper function to reduce code duplication and improve readability within the authenticateUser logic.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • packages/sdk/src/global-account/react/hooks/useAuthentication.ts
    • Imported useConnectionManager from thirdweb/react.
    • Initialized connectionManager for direct access to connection stores.
    • Refactored authenticateUser to use a new finalizeAuth helper for cleaner code.
    • Updated the logout function to conditionally clear active wallet stores for EOA wallets or call disconnect() for ecosystem/smart wallets.
    • Added connectionManager to the dependency array of the useCallback hook for the logout function.
Activity
  • No specific activity (comments, reviews, progress updates) was found in the provided context.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves the logout flow by preserving EOA wallet connections, which avoids requiring users to re-approve connections on subsequent logins. This is achieved by directly clearing thirdweb's active-wallet stores for EOA wallets instead of performing a full disconnect. The changes also include a good refactoring to reduce duplication in the authentication logic. I have one minor suggestion to improve code clarity.

Comment on lines +189 to +193
const mgr = connectionManager;
mgr.activeAccountStore.setValue(undefined);
mgr.activeWalletStore.setValue(undefined);
mgr.activeWalletChainStore.setValue(undefined);
mgr.activeWalletConnectionStatusStore.setValue("disconnected");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve readability and avoid an unnecessary local variable, you can use connectionManager directly.

Suggested change
const mgr = connectionManager;
mgr.activeAccountStore.setValue(undefined);
mgr.activeWalletStore.setValue(undefined);
mgr.activeWalletChainStore.setValue(undefined);
mgr.activeWalletConnectionStatusStore.setValue("disconnected");
connectionManager.activeAccountStore.setValue(undefined);
connectionManager.activeWalletStore.setValue(undefined);
connectionManager.activeWalletChainStore.setValue(undefined);
connectionManager.activeWalletConnectionStatusStore.setValue("disconnected");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant