-
Notifications
You must be signed in to change notification settings - Fork 1.6k
WIP: remove ConnectionControllerClient and move behavior account behavior to ConnectionController #5085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… and topken balances
…m/appkit into chore/remove-account-controller
…account-controller
…account-controller
This reverts commit 9300120.
…m/appkit into chore/remove-account-controller
…account-controller
…m/appkit into chore/remove-account-controller
…account-controller
| const namespaceNetworkId = ChainController.getNetworkData(chainNamespace)?.caipNetwork?.id | ||
| const syncAccountChainId = chainId || namespaceNetworkId | ||
|
|
||
| const caipNetworkId = `${chainNamespace}:${syncAccountChainId}` as const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: CAIP Network IDs Malformed by Missing Data
The caipNetworkId is constructed as a template literal with as const assertion, but when syncAccountChainId is undefined, it creates an invalid CAIP network ID like "eip155:undefined". This happens in the else branch at line 675 where syncAccountChainId might be undefined, leading to malformed CAIP network IDs being passed to ConnectionController.syncAccount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Connection Persistence Broken
The setConnections method no longer persists connections to storage. The removed line StorageUtil.setConnections(connections, chainNamespace) was not replaced, and ConnectionController.setConnections only updates in-memory state without persisting to localStorage. This breaks reconnection functionality on page reload since connections won't be stored.
packages/appkit/src/client/appkit-base-client.ts#L1250-L1256
appkit/packages/appkit/src/client/appkit-base-client.ts
Lines 1250 to 1256 in f872b74
| public setConnections: (typeof ConnectionController)['setConnections'] = ( | |
| connections, | |
| chainNamespace | |
| ) => { | |
| ConnectionController.setConnections(connections, chainNamespace) | |
| } |
Description
Overview
This PR implements a significant architectural refactoring that moves account-related logic from AppKitBaseClient to ConnectionController and completely removes the connectionControllerClient abstraction layer. This change aligns with the AppKit stability improvements initiative and moves us closer to atomic state management.
🎯 Motivation
As outlined in the AppKit Refactor POC Findings & Recommendations, the current architecture has several issues:
Circular Dependencies: Controllers have circular deps through client abstractions
Tangled State: Multiple cross-controller dependencies make atomic updates impossible
Blurry Boundaries: State responsibilities are unclear between controllers
Proxy Pattern Anti-pattern: connectionControllerClient acts as unnecessary middleware
🔄 Changes Made
✅ Removed connectionControllerClient Completely
views → controllers → adapters(instead of views → controllers → client → adapters)🏗️ Moved Account Logic to ConnectionController
The following methods were moved from AppKitBaseClient to ConnectionController:
Connection Management Methods:
Account Operations:
Provider execution
Utility Methods:
Updated Adapter Integration
Adapters now call ConnectionController methods directly instead of through client
Type of change
Associated Issues
Closes APKT-3837
Checklist
Note
Consolidates account and provider operations into ConnectionController, removes ConnectionControllerClient, and updates adapters, UI, tests, and examples to the new API.
connectExternal,reconnectExternal,disconnect,switchConnection,syncAccount,syncIdentity,updateBalance,signMessage,sendTransaction,estimateGas,writeContract,request) intoConnectionController.ConnectionControllerClient; simplify call chain and updateAppKitBaseClient,appkit-core, and universal adapter client.ConnectionController.wagmi,ethers/ethers5,solana,bitcoin,polkadot,tonclients to callConnectionControllerdirectly; refresh tests and mocks.ChainController,SendController,SwapController,BlockchainApiControllerto align with new responsibilities and API surface.appkit,controllers,adapters, andscaffold-ui; add/adjust cases for connection, balance, send/estimate, ENS, smart sessions.Written by Cursor Bugbot for commit f872b74. This will update automatically on new commits. Configure here.