refactor/switch network and local storage controls #2888
Conversation
🦋 Changeset detectedLatest commit: 468ec8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 34 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
♻️ Vite-Size ♻️Size Difference
Current Size
Base Size
|
| this.appKit?.setCaipAddress(caipAddress, this.chainNamespace) | ||
| this.appKit?.setCaipNetwork(caipNetwork) |
There was a problem hiding this comment.
We should do all the caipNetwork setters from a single place and do it only when it's necessary.
| : Number(networkId) | ||
| const caipNetwork = this.caipNetworks.find(c => c.chainId === networkIdNumber) | ||
| const chainChangedHandler = (chainId: string) => { | ||
| const chainIdNumber = |
There was a problem hiding this comment.
We should control if the received chainId is already active, otherwise - still couldn't figure it out why - this listener is getting triggered multiple times in a single network switch and breaking the state
Same logic applied to all clients
| @@ -584,7 +580,7 @@ describe('EthersAdapter', () => { | |||
| const chainChangedHandler = mockProvider.on.mock.calls.find( | |||
| (call: string[]) => call[0] === 'chainChanged' | |||
| )[1] | |||
| await chainChangedHandler('0x1') | |||
| await chainChangedHandler('0x137') | |||
There was a problem hiding this comment.
This will be trigger the setCaipNetwork only if the network has changed. So passing Polygon id now
| : Number(networkId) | ||
| const caipNetwork = this.caipNetworks.find(c => c.chainId === networkIdNumber) | ||
| const chainChangedHandler = (chainId: string) => { | ||
| const chainIdNumber = |
| @@ -588,7 +586,7 @@ describe('EthersAdapter', () => { | |||
| const chainChangedHandler = mockProvider.on.mock.calls.find( | |||
| (call: string[]) => call[0] === 'chainChanged' | |||
| )[1] | |||
| await chainChangedHandler('0x1') | |||
| await chainChangedHandler('0x2') | |||
There was a problem hiding this comment.
Same - updating the value to make sure it's triggering setCaipNetwork
| @@ -311,10 +310,6 @@ export class SolanaAdapter implements ChainAdapter { | |||
|
|
|||
| this.syncRequestedNetworks(this.caipNetworks) | |||
|
|
|||
| AssetController.subscribeNetworkImages(() => { | |||
There was a problem hiding this comment.
This is unnecessary. I'll handle the network images fetching in another PR
| @@ -647,7 +645,6 @@ export class WagmiAdapter implements ChainAdapter { | |||
| } | |||
| } else if (status === 'connected' && address && chainId) { | |||
| const caipAddress = `eip155:${chainId}:${address}` as CaipAddress | |||
| this.appKit?.resetAccount(this.chainNamespace) | |||
There was a problem hiding this comment.
If we've connected, we don't have to resetAccount. This should have been removed long ago - I remember we removed these
| @@ -68,8 +68,7 @@ describe('UniversalAdapter', () => { | |||
| caipNetwork: undefined, | |||
| requestedCaipNetworks: [mainnet, solana], | |||
| approvedCaipNetworkIds: [], | |||
| supportsAllNetworks: true, | |||
| isDefaultCaipNetwork: false | |||
There was a problem hiding this comment.
Unused state value
There was a problem hiding this comment.
are there any other unused values here?
| if (!caipNetwork) { | ||
| NetworkController.setActiveCaipNetwork({ | ||
| chainId: Number(chainId), | ||
| id: `eip155:${chainId}`, |
There was a problem hiding this comment.
Why we setting only eip155? We should refactor this as that it'll get different namespaces as well.
What could be the chainId other than EVM or Solana chain IDs?
| '@appkit/active_caip_network_id': string | ||
| '@appkit/connected_connector': string | ||
| '@appkit/connected_social': string | ||
| '@appkit/connected_social_username': string | ||
| '@appkit/recent_wallets': string | ||
| '@appkit/deeplink_choice': { href: string; name: string } | ||
| '@appkit/deeplink_choice': string |
There was a problem hiding this comment.
We should store only string typed values on the localStorage. Keeping complex values will bring the complexity of stringify'ing and parsing problem and also we'll bring TS issues.
There was a problem hiding this comment.
if we are doing this I'd replace all usages of storing CaipNetwork for caipNetworkId (getting the netwrok on a diff context) and not stringify anything as a rule; only store primitive types
| } | ||
|
|
||
| public override disconnectedCallback() { | ||
| this.externalViewUnsubscribe.forEach(unsubscribe => unsubscribe()) |
There was a problem hiding this comment.
This was missing, we should remove listeners on unmount
| this.appKit?.setCaipAddress(caipAddress, this.chainNamespace) | ||
| this.appKit?.setCaipNetwork(caipNetwork) |
| @@ -68,8 +68,7 @@ describe('UniversalAdapter', () => { | |||
| caipNetwork: undefined, | |||
| requestedCaipNetworks: [mainnet, solana], | |||
| approvedCaipNetworkIds: [], | |||
| supportsAllNetworks: true, | |||
| isDefaultCaipNetwork: false | |||
There was a problem hiding this comment.
are there any other unused values here?
| const value = localStorage.getItem(key) | ||
|
|
||
| if (value) { | ||
| try { | ||
| return JSON.parse(value) | ||
| } catch (e) { | ||
| console.warn('Error parsing value from localStorage', key, e) | ||
|
|
||
| return undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
We'd done this change to have typed values + prevent having to parse and stringify all the time
Why remove this?
…tor/switch-network-and-local-storage-controls
enesozturk
force-pushed
the
refactor/switch-network-and-local-storage-controls
branch
from
abd3a10
to
dd1c571
Compare
enesozturk
deleted the
refactor/switch-network-and-local-storage-controls
branch
Description
Please include a brief summary of the change.
Type of change
Associated Issues
For Linear issues: Closes APKT-xxx
For GH issues: closes #...
Showcase (Optional)
If there is a UI change include the screenshots with before and after state.
If new feature is being introduced, include the link to demo recording.
Checklist