fix: wallet state corruption round 4 #8519

woodenfurniture · 2025-01-09T03:55:26Z

Description

Patch for state corruption in wallet provider when user backs out of switching wallet. See #7904 for context.

The fix here is a patch to prevent corrupt state by preventing "back-out" of native connect into corrupt state by forcing the previous wallet to first disconnect before the password prompt is displayed.

Previous issue in simplified terms:

User is connected to native wallet
User clicks "switch wallet"
User clicks different native wallet and is presented with password prompt
WalletProvider updates the deviceId with the new wallet device ID
User exits the password prompt without entering the password
WalletProvider now has the wrong deviceId until they reconnect the wallet

This PR:

User is connected to native wallet
User clicks "switch wallet"
User clicks different native wallet and is presented with password prompt
Both the WalletProvider deviceId and the redux wallet ID are cleared, wallet is disconnected
User exits the password prompt without entering the password
User is in a disconnected state without mismatch of wallet device IDs

See below jam exaplaining in more detail.

Issue (if applicable)

closes #7904

Risk

High Risk PRs Require 2 approvals

High risk because it modifies WalletProvider and could theoretically result in inability to open a wallet.

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

All wallets, especially native.

Testing

Thoroughly test opening/closing/switching wallets, with more emphasis on native wallet.
Sanity check signin, broadcasting, trading etc is not somehow unexpectedly broken.

Engineering

Operations

🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Have a watch of my home movie explaining everything (audio on)
https://jam.dev/c/22a2572a-aeb6-4a82-a6c2-bd79bac3079d

…ected

woodenfurniture · 2025-01-09T03:56:26Z

src/context/WalletProvider/WalletViewsSwitch.tsx

@@ -63,14 +61,11 @@ export const WalletViewsSwitch = () => {
    if (disposition === 'initializing' || disposition === 'recovering') {
      await wallet?.cancel()
      disconnect()
-      store.dispatch(localWalletSlice.actions.clearLocalWallet())


disconnect() calls RESET_STATE, which calls clearLocalWallet()

woodenfurniture · 2025-01-09T03:56:31Z

src/context/WalletProvider/WalletViewsSwitch.tsx

      dispatch({ type: WalletActions.OPEN_KEEPKEY_DISCONNECT })
    } else {
      history.replace(INITIAL_WALLET_MODAL_ROUTE)
      if (disconnectOnCloseModal) {
        disconnect()
-        dispatch({ type: WalletActions.RESET_STATE })
-        store.dispatch(localWalletSlice.actions.clearLocalWallet())


disconnect() calls RESET_STATE, which calls clearLocalWallet()

woodenfurniture · 2025-01-09T03:56:39Z

src/context/WalletProvider/WalletProvider.tsx

@@ -413,7 +433,6 @@ export const WalletProvider = ({ children }: { children: React.ReactNode }): JSX
     */
    state.wallet?.disconnect?.()
    dispatch({ type: WalletActions.RESET_STATE })
-    store.dispatch(localWalletSlice.actions.clearLocalWallet())


disconnect() calls RESET_STATE, which calls clearLocalWallet()

woodenfurniture · 2025-01-09T03:57:25Z

src/context/WalletProvider/WalletConnectV2/useWalletConnectV2EventHandler.ts

@@ -21,8 +21,7 @@ export const useWalletConnectV2EventHandler = (
     */
    state.wallet?.disconnect?.()
    dispatch({ type: WalletActions.RESET_STATE })
-    localWallet.clearLocalWallet()


disconnect() calls RESET_STATE, which calls clearLocalWallet()

woodenfurniture · 2025-01-09T03:59:43Z

src/context/WalletProvider/NativeWallet/components/NativeLoad.tsx

+        dispatch({
+          type: WalletActions.SET_NATIVE_PENDING_DEVICE_ID,
+          payload: deviceId,
+        })


Step 1: SET_NATIVE_PENDING_DEVICE_ID

woodenfurniture · 2025-01-09T04:01:36Z

src/context/WalletProvider/NativeWallet/components/EnterPassword.tsx

@@ -83,6 +84,7 @@ export const EnterPassword = () => {
          type: WalletActions.SET_IS_CONNECTED,
          payload: true,
        })
+        dispatch({ type: WalletActions.RESET_NATIVE_PENDING_DEVICE_ID })


Step 2: Open the vault (line 64)
Step 3: RESET_NATIVE_PENDING_DEVICE_ID

woodenfurniture · 2025-01-09T04:02:02Z

src/context/WalletProvider/NativeWallet/components/NativeLoad.tsx

@@ -103,6 +110,7 @@ export const NativeLoad = ({ history }: RouteComponentProps) => {
            type: WalletActions.SET_IS_CONNECTED,
            payload: true,
          })
+          dispatch({ type: WalletActions.RESET_NATIVE_PENDING_DEVICE_ID })


Step 2 (alternate): Vault already open, RESET_NATIVE_PENDING_DEVICE_ID

gomesalexandre · 2025-01-10T09:51:43Z

src/App.tsx

gomesalexandre · 2025-01-10T09:54:19Z

src/components/ManageAccountsDrawer/components/ImportAccounts.tsx

+    if (!walletDeviceId) {
+      console.error('Missing walletDeviceId')
+      return
+    }


suggestion: early return before isDemoWallet guard, since demo also has a deviceId:

web/src/context/WalletProvider/WalletProvider.tsx

Line 821 in d690560

// For the demo wallet, we use the name, DemoWallet, as the deviceId

gomesalexandre · 2025-01-10T10:02:48Z

src/components/MultiHopTrade/hooks/useReceiveAddress.tsx

@@ -66,7 +66,7 @@ export const useReceiveAddress = ({
      ? skipToken
      : async () => {
          // Already partially covered in isInitializing, but TypeScript lyfe mang.
-          if (!buyAsset || !wallet || !buyAccountId || !buyAccountMetadata) {
+          if (!buyAsset || !wallet || !buyAccountId || !buyAccountMetadata || !deviceId) {


Not introduced by this diff, but prefer this guy (possibly for a follow-up, given this will change behaviour at runtime):

queryFn: !isInitializing && buyAsset && wallet && buyAccountId && buyAccountMetadata && deviceId ? inlineQueryFn : skipToken

Rationale: all those checks should mostly be for the sake of type-safety, and we should not end up here, so realistically, should not make a difference, but we should always use skipToken for type guards vs. early returns.
With the current version, there is a theoreticaly world where we have query runs where we return undefined, vs. not running the query altogether.
In this specific case, I don't think there should be any runtime issue with the current way (isLoading going from true to false should be so fast it should be unnoticable), mostly for consistency.

gomesalexandre · 2025-01-10T10:36:08Z

src/context/WalletProvider/KeepKey/components/Pin.tsx

@@ -38,7 +38,7 @@ export const KeepKeyPin = ({
    },
    dispatch,
  } = useWallet()
-  const wallet = keyring.get(deviceId)
+  const wallet = keyring.get(deviceId ?? '')


two qs: absolutely fine for the time being but

do we want to pen a PR in hdwallet to make this guy a maybe instead of an optional to better reflect runtime types? https://github.com/shapeshift/hdwallet/blob/bd15119aad5a3b0410f4783dae946e146e960cf1/packages/hdwallet-core/src/keyring.ts#L50

diff --git a/packages/hdwallet-core/src/keyring.ts b/packages/hdwallet-core/src/keyring.ts index 4701eb65..e8e43b54 100644 --- a/packages/hdwallet-core/src/keyring.ts +++ b/packages/hdwallet-core/src/keyring.ts @@ -47,7 +47,7 @@ export class Keyring extends eventemitter2.EventEmitter2 { ); } - public get<T extends HDWallet>(deviceID?: string): T | null { + public get<T extends HDWallet>(deviceID: string | null | undefined): T | null { if (deviceID && this.aliases[deviceID] && this.wallets[this.aliases[deviceID]]) return this.wallets[this.aliases[deviceID]] as T; if (deviceID && this.wallets[deviceID]) return this.wallets[deviceID] as T;

do we want to useMemo this guy and early return, which will make it work both with current types and updated ones (if we decide to go with that) without nullish coalescing?

Suggested change

const wallet = keyring.get(deviceId ?? '')

const wallet = useMemo(() => {

if (!deviceId) return

return keyring.get(deviceId)

}, [deviceId, keyring])

Also as far as I can see with LSP this is the only occurence of a maybe deviceId, so only this specific occurence should be concerned by this comment

gomesalexandre · 2025-01-10T10:40:30Z

src/context/WalletProvider/KeepKey/components/Pin.tsx

+    keyring.on(['KeepKey', deviceId ?? '', String(MessageType.FAILURE)], handleError)

    return () => {
-      keyring.off(['KeepKey', deviceId, String(MessageType.FAILURE)], handleError)
+      keyring.off(['KeepKey', deviceId ?? '', String(MessageType.FAILURE)], handleError)


q: is setting event listeners without a deviceId (theoretically runtime-wise, at least) intended? Since this effect is reactive on deviceId, we should be able to early return, and to not return an unmount callback for the effect if (!deviceId)`

gomesalexandre · 2025-01-10T10:45:13Z

src/context/WalletProvider/Ledger/components/Chains.tsx

@@ -52,7 +52,8 @@ export const LedgerChains = () => {

  const handleConnectClick = useCallback(
    async (chainId: ChainId) => {
-      if (!walletState?.wallet) {
+      const { wallet, deviceId } = walletState ?? {}


i.e state should never be undefined in IWalletContext, as it is instantiated to an InitialState

Suggested change

const { wallet, deviceId } = walletState ?? {}

const { wallet, deviceId } = walletState

gomesalexandre · 2025-01-10T10:46:08Z

src/context/WalletProvider/NativeWallet/components/EnterPassword.tsx

@@ -58,6 +58,7 @@ export const EnterPassword = () => {
  const onSubmit = useCallback(
    async (values: FieldValues) => {
      try {
+        if (!deviceId) return


gomesalexandre · 2025-01-10T10:49:48Z

src/context/WalletProvider/NativeWallet/components/NativeLoad.tsx

@@ -81,6 +81,13 @@ export const NativeLoad = ({ history }: RouteComponentProps) => {
    if (adapter) {
      const { name, icon } = NativeConfig
      try {
+        // Set a pending device ID so the event handler doesn't redirect the user to password input


gomesalexandre · 2025-01-10T10:50:19Z

src/context/WalletProvider/NativeWallet/hooks/useNativeEventHandler.ts

        case NativeEvents.MNEMONIC_REQUIRED:
          if (!deviceId) break
+
+          // Don't show password input for previous wallet when switching


gomesalexandre · 2025-01-10T10:54:51Z

src/context/WalletProvider/NativeWallet/hooks/useNativeEventHandler.ts

          // If we're on the native mobile app we don't need to handle the MNEMONIC_REQUIRED event as we use the device's native authentication instead
          // Reacting to this event will incorrectly open the native password modal after authentication completes when on the mobile app
          if (isMobileApp) break
-          dispatch({ type: WalletActions.NATIVE_PASSWORD_OPEN, payload: { modal: true, deviceId } })
+          dispatch({ type: WalletActions.NATIVE_PASSWORD_OPEN, payload: { modal: true } })


gomesalexandre · 2025-01-10T10:56:28Z

src/context/WalletProvider/WalletProvider.tsx

@@ -140,14 +142,15 @@ const reducer = (state: InitialState, action: ActionTypes): InitialState => {
      if (currentConnectedType === 'walletconnectv2') {
        state.wallet?.disconnect?.()
        store.dispatch(localWalletSlice.actions.clearLocalWallet())
+        store.dispatch(portfolioSlice.actions.setWalletMeta(undefined))


gomesalexandre

Diff-wise looks excellent.
A few comments mostly re: types and cleanup, but shouldn't really change anything at runtime (the KK event listeners would just be EventEmitter spew rather than an actual bug, assuming we ever end up there).

Runtime pass incoming, stay tuned 🎸

gomesalexandre

Tested localy, purposedly starting from a non nuked cache with these guys cached

two native wallet
two MIPD wallets
Ledger
KK

Wallet connection flow - right accounts derived and no race shenanigns

Native ✅

Native is happy e2e: unlock, refresh and unlock, disconnect/reconnect, keystore import ✅

https://jam.dev/c/3a4e5f08-8f0b-4e04-943b-f04beb3f2974

Signing

Native ✅

https://jam.dev/c/c29ed56d-1df7-44fc-98ff-7bad41b18294

MIPD ✅

https://jam.dev/c/c951c98e-ab0b-4326-8a38-ea420f7c787e

KK ✅

https://jam.dev/c/c7be8103-d524-406b-adb8-a90856481e30

Ledger ✅

https://jam.dev/c/89b5c194-f71b-49a6-ad3f-e149ba213150

Noted one smolish bug however, which I can also repro in develop, not introduced by this diff. Captured in #8535.

When connecting a mipd wallet with a given ENS associated to it, then connecting to Ledger (to another EVM address set, without ENS associated to it), stale ENS is displayed
this diff: https://jam.dev/c/d9a18722-74cb-41bc-8f38-aa8daa917d7f
develop: https://jam.dev/c/4635c6ea-dc26-4222-bec6-8c10f62345e1

woodenfurniture added 3 commits January 9, 2025 14:22

wip: dont set wallet context device id until the wallet is fully conn…

7b1438e

…ected

fix: reset deviceId when disconnecting wallet

14c6dcb

chore: cleanup

57391b6

woodenfurniture commented Jan 9, 2025

View reviewed changes

fix: prompt user to open native wallet on reload

78f3ae8

woodenfurniture added high risk max-pain labels Jan 9, 2025

Merge branch 'develop' into wallet-state-corruption-4

09e4b7d

woodenfurniture marked this pull request as ready for review January 9, 2025 05:15

woodenfurniture requested a review from a team as a code owner January 9, 2025 05:15

gomesalexandre self-requested a review January 9, 2025 15:49

Merge branch 'develop' into wallet-state-corruption-4

ca7169e

gomesalexandre reviewed Jan 10, 2025

View reviewed changes

src/App.tsx

Copy link

Contributor

gomesalexandre Jan 10, 2025

Choose a reason for hiding this comment

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

revert me

gomesalexandre reviewed Jan 10, 2025

View reviewed changes

gomesalexandre approved these changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wallet state corruption round 4 #8519

fix: wallet state corruption round 4 #8519

woodenfurniture commented Jan 9, 2025 •

edited

Loading

woodenfurniture Jan 9, 2025

woodenfurniture Jan 9, 2025

woodenfurniture Jan 9, 2025

woodenfurniture Jan 9, 2025

woodenfurniture Jan 9, 2025

woodenfurniture Jan 9, 2025

woodenfurniture Jan 9, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025 •

edited

Loading

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025 •

edited

Loading

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre Jan 10, 2025

gomesalexandre left a comment

gomesalexandre left a comment

-  const wallet = keyring.get(deviceId ?? '')
+  const wallet = useMemo(() => {
+    if (!deviceId) return
+    return keyring.get(deviceId)
+  }, [deviceId, keyring])

	const { wallet, deviceId } = walletState ?? {}
	const { wallet, deviceId } = walletState

fix: wallet state corruption round 4 #8519

Are you sure you want to change the base?

fix: wallet state corruption round 4 #8519

Conversation

woodenfurniture commented Jan 9, 2025 • edited Loading

Description

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomesalexandre Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomesalexandre Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomesalexandre left a comment

Choose a reason for hiding this comment

gomesalexandre left a comment

Choose a reason for hiding this comment

Wallet connection flow - right accounts derived and no race shenanigns

Native ✅

Ledger ✅

KeepKey ✅

MIPD Wallets ✅

Phantom ✅

Signing

Native ✅

MIPD ✅

KK ✅

Ledger ✅

woodenfurniture commented Jan 9, 2025 •

edited

Loading

gomesalexandre Jan 10, 2025 •

edited

Loading

gomesalexandre Jan 10, 2025 •

edited

Loading