Skip to content
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

fix: wallet state corruption round 4 #8519

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/App.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

revert me

Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,13 @@ import { IconCircle } from 'components/IconCircle'
import { useBridgeClaimNotification } from 'hooks/useBridgeClaimNotification/useBridgeClaimNotification'
import { useHasAppUpdated } from 'hooks/useHasAppUpdated/useHasAppUpdated'
import { useModal } from 'hooks/useModal/useModal'
import { selectShowConsentBanner, selectShowWelcomeModal } from 'state/slices/selectors'
import { useWallet } from 'hooks/useWallet/useWallet'
import {
selectShowConsentBanner,
selectShowWelcomeModal,
selectWalletId,
} from 'state/slices/selectors'
import { useAppSelector } from 'state/store'

const flexGap = { base: 2, md: 3 }
const flexDir: ResponsiveValue<Property.FlexDirection> = { base: 'column', md: 'row' }
Expand Down Expand Up @@ -69,6 +75,19 @@ export const App = () => {
}
}, [isNativeOnboardOpen, openNativeOnboard, showWelcomeModal])

const portfolioWalletId = useAppSelector(selectWalletId)

const {
state: { deviceId: contextDeviceId },
} = useWallet()

useEffect(() => {
console.log({
portfolioWalletId,
contextDeviceId,
})
}, [contextDeviceId, portfolioWalletId])

return (
<>
{showConsentBanner && <ConsentBanner />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,11 @@ export const ImportAccounts = ({ chainId, onClose }: ImportAccountsProps) => {
return
}

if (!walletDeviceId) {
console.error('Missing walletDeviceId')
return
}
Comment on lines +320 to +323
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


setIsSubmitting(true)

// For every new account that is active, fetch the account and upsert it into the redux state
Expand Down
2 changes: 1 addition & 1 deletion src/components/MultiHopTrade/hooks/useReceiveAddress.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

@gomesalexandre gomesalexandre Jan 10, 2025

Choose a reason for hiding this comment

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

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.

return undefined
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const KeepKeyPassphrase = () => {
state: { deviceId, keyring },
dispatch,
} = useWallet()
const wallet = keyring.get(deviceId)
const wallet = keyring.get(deviceId ?? '')
const walletId = useAppSelector(selectWalletId)
const appDispatch = useAppDispatch()

Expand Down
6 changes: 3 additions & 3 deletions src/context/WalletProvider/KeepKey/components/Pin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const KeepKeyPin = ({
},
dispatch,
} = useWallet()
const wallet = keyring.get(deviceId)
const wallet = keyring.get(deviceId ?? '')
Copy link
Contributor

Choose a reason for hiding this comment

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

two qs: absolutely fine for the time being but

  1. 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;
  1. 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])

Copy link
Contributor

Choose a reason for hiding this comment

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

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


const pinFieldRef = useRef<HTMLInputElement | null>(null)

Expand Down Expand Up @@ -138,10 +138,10 @@ export const KeepKeyPin = ({
}
}

keyring.on(['KeepKey', deviceId, String(MessageType.FAILURE)], handleError)
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)
Comment on lines +141 to +144
Copy link
Contributor

@gomesalexandre gomesalexandre Jan 10, 2025

Choose a reason for hiding this comment

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

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)`

}
}, [deviceId, keyring])

Expand Down
9 changes: 5 additions & 4 deletions src/context/WalletProvider/Ledger/components/Chains.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ export const LedgerChains = () => {

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

Choose a reason for hiding this comment

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

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

image
Suggested change
const { wallet, deviceId } = walletState ?? {}
const { wallet, deviceId } = walletState

if (!wallet || !deviceId) {
console.error('No wallet found')
return
}
Expand All @@ -67,7 +68,7 @@ export const LedgerChains = () => {
]({
accountNumber: 0,
chainIds,
wallet: walletState.wallet,
wallet,
isSnapInstalled: false,
})

Expand Down Expand Up @@ -102,7 +103,7 @@ export const LedgerChains = () => {
const accountMetadata = accountMetadataByAccountId[accountId]
const payload = {
accountMetadataByAccountId: { [accountId]: accountMetadata },
walletId: walletState.deviceId,
walletId: deviceId,
}

dispatch(portfolio.actions.upsertAccountMetadata(payload))
Expand All @@ -120,7 +121,7 @@ export const LedgerChains = () => {
setLoadingChains(prevLoading => ({ ...prevLoading, [chainId]: false }))
}
},
[dispatch, walletState.deviceId, walletState.wallet],
[dispatch, walletState],
)

const chainsRows = useMemo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const EnterPassword = () => {
const translate = useTranslate()
const { state, dispatch, disconnect } = useWallet()
const localWallet = useLocalWallet()
const { deviceId, keyring } = state
const { nativeWalletPendingDeviceId: deviceId, keyring } = state

const [showPw, setShowPw] = useState<boolean>(false)

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

Choose a reason for hiding this comment

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

💜

const wallet = keyring.get<NativeHDWallet>(deviceId)
const Vault = await import('@shapeshiftoss/hdwallet-native-vault').then(m => m.Vault)
const vault = await Vault.open(deviceId, values.password)
Expand All @@ -83,6 +84,7 @@ export const EnterPassword = () => {
type: WalletActions.SET_IS_CONNECTED,
payload: true,
})
dispatch({ type: WalletActions.RESET_NATIVE_PENDING_DEVICE_ID })
Copy link
Member Author

Choose a reason for hiding this comment

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

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

dispatch({ type: WalletActions.SET_LOCAL_WALLET_LOADING, payload: false })
dispatch({ type: WalletActions.SET_WALLET_MODAL, payload: false })
} catch (e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

🧠

// for the previous wallet
dispatch({
type: WalletActions.SET_NATIVE_PENDING_DEVICE_ID,
payload: deviceId,
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Step 1: SET_NATIVE_PENDING_DEVICE_ID


const wallet = await adapter.pairDevice(deviceId)
if (!(await wallet?.isInitialized())) {
// This will trigger the password modal and the modal will set the wallet on state
Expand All @@ -103,6 +110,7 @@ export const NativeLoad = ({ history }: RouteComponentProps) => {
type: WalletActions.SET_IS_CONNECTED,
payload: true,
})
dispatch({ type: WalletActions.RESET_NATIVE_PENDING_DEVICE_ID })
Copy link
Member Author

Choose a reason for hiding this comment

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

Step 2 (alternate): Vault already open, RESET_NATIVE_PENDING_DEVICE_ID

// The wallet is already initialized so we can close the modal
dispatch({ type: WalletActions.SET_WALLET_MODAL, payload: false })
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,28 @@ import type { ActionTypes } from 'context/WalletProvider/actions'
import { WalletActions } from 'context/WalletProvider/actions'
import type { InitialState } from 'context/WalletProvider/WalletProvider'
import { isMobile as isMobileApp } from 'lib/globals'
import { assertUnreachable } from 'lib/utils'

export const useNativeEventHandler = (state: InitialState, dispatch: Dispatch<ActionTypes>) => {
const { keyring, modal, modalType } = state

useEffect(() => {
const handleEvent = (e: [deviceId: string, message: Event]) => {
const deviceId = e[0]
switch (e[1].message_type) {
const messageType = e[1].message_type as NativeEvents
switch (messageType) {
case NativeEvents.MNEMONIC_REQUIRED:
if (!deviceId) break

// Don't show password input for previous wallet when switching
Copy link
Contributor

Choose a reason for hiding this comment

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

🧠

if (deviceId !== state.nativeWalletPendingDeviceId) {
break
}

// 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 } })
Copy link
Contributor

Choose a reason for hiding this comment

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

💜


break
case NativeEvents.READY:
Expand All @@ -28,8 +36,7 @@ export const useNativeEventHandler = (state: InitialState, dispatch: Dispatch<Ac
}
break
default:
// If there wasn't an enum value, then we'll check the message type
break
assertUnreachable(messageType)
}
}

Expand All @@ -48,5 +55,5 @@ export const useNativeEventHandler = (state: InitialState, dispatch: Dispatch<Ac
keyring.off(['Native', '*', NativeEvents.MNEMONIC_REQUIRED], handleEvent)
keyring.off(['Native', '*', NativeEvents.READY], handleEvent)
}
}, [modalType, dispatch, keyring, modal])
}, [modalType, dispatch, keyring, modal, state.nativeWalletPendingDeviceId, state.deviceId])
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ export const useWalletConnectV2EventHandler = (
*/
state.wallet?.disconnect?.()
dispatch({ type: WalletActions.RESET_STATE })
localWallet.clearLocalWallet()
Copy link
Member Author

Choose a reason for hiding this comment

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

disconnect() calls RESET_STATE, which calls clearLocalWallet()

}, [dispatch, localWallet, state.wallet])
}, [dispatch, state.wallet])

useEffect(() => {
// This effect should never run for wallets other than WalletConnectV2 since we explicitly tap into @walletconnect/ethereum-provider provider
Expand Down
36 changes: 28 additions & 8 deletions src/context/WalletProvider/WalletProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
selectWalletRdns,
selectWalletType,
} from 'state/slices/localWalletSlice/selectors'
import { portfolio } from 'state/slices/portfolioSlice/portfolioSlice'
import { portfolio as portfolioSlice } from 'state/slices/portfolioSlice/portfolioSlice'
import { store } from 'state/store'

import type { ActionTypes } from './actions'
Expand Down Expand Up @@ -93,11 +93,12 @@ export type InitialState = {
isLocked: boolean
modal: boolean
isLoadingLocalWallet: boolean
deviceId: string
deviceId: string | null
showBackButton: boolean
keepKeyPinRequestType: PinMatrixRequestType | null
deviceState: DeviceState
disconnectOnCloseModal: boolean
nativeWalletPendingDeviceId: string | null
} & (
| {
modalType: KeyManager | null
Expand All @@ -124,11 +125,12 @@ const initialState: InitialState = {
isLocked: false,
modal: false,
isLoadingLocalWallet: false,
deviceId: '',
deviceId: null,
showBackButton: true,
keepKeyPinRequestType: null,
deviceState: initialDeviceState,
disconnectOnCloseModal: false,
nativeWalletPendingDeviceId: null,
}

const reducer = (state: InitialState, action: ActionTypes): InitialState => {
Expand All @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

🧠

}
const { deviceId, name, wallet, icon, meta, isDemoWallet, connectedType } = action.payload
// set wallet metadata in redux store
const walletMeta = {
walletId: deviceId,
walletName: name,
}
store.dispatch(portfolio.actions.setWalletMeta(walletMeta))
store.dispatch(portfolioSlice.actions.setWalletMeta(walletMeta))
return {
...state,
deviceId,
Expand Down Expand Up @@ -221,7 +224,8 @@ const reducer = (state: InitialState, action: ActionTypes): InitialState => {
modal: action.payload.modal,
modalType: KeyManager.Native,
showBackButton: !state.isLoadingLocalWallet,
deviceId: action.payload.deviceId,
deviceId: null,
walletInfo: null,
initialRoute: NativeWalletRoutes.EnterPassword,
}
case WalletActions.OPEN_KEEPKEY_PIN: {
Expand Down Expand Up @@ -290,9 +294,10 @@ const reducer = (state: InitialState, action: ActionTypes): InitialState => {
case WalletActions.SET_LOCAL_WALLET_LOADING:
return { ...state, isLoadingLocalWallet: action.payload }
case WalletActions.RESET_STATE:
const resetProperties = omit(initialState, ['keyring', 'adapters', 'modal', 'deviceId'])
const resetProperties = omit(initialState, ['keyring', 'adapters', 'modal'])
// reset wallet meta in redux store
store.dispatch(portfolio.actions.setWalletMeta(undefined))
store.dispatch(localWalletSlice.actions.clearLocalWallet())
store.dispatch(portfolioSlice.actions.setWalletMeta(undefined))
return { ...state, ...resetProperties }
// TODO: Remove this once we update SET_DEVICE_STATE to allow explicitly setting falsey values
case WalletActions.RESET_LAST_DEVICE_INTERACTION_STATE: {
Expand Down Expand Up @@ -320,6 +325,21 @@ const reducer = (state: InitialState, action: ActionTypes): InitialState => {
modalType: KeyManager.KeepKey,
initialRoute: KeepKeyRoutes.Disconnect,
}
case WalletActions.SET_NATIVE_PENDING_DEVICE_ID:
store.dispatch(localWalletSlice.actions.clearLocalWallet())
store.dispatch(portfolioSlice.actions.setWalletMeta(undefined))
return {
...state,
isConnected: false,
deviceId: null,
walletInfo: null,
nativeWalletPendingDeviceId: action.payload,
}
case WalletActions.RESET_NATIVE_PENDING_DEVICE_ID:
return {
...state,
nativeWalletPendingDeviceId: null,
}
default:
return state
}
Expand All @@ -334,6 +354,7 @@ const getInitialState = () => {
*/
return {
...initialState,
nativeWalletPendingDeviceId: localWalletDeviceId,
isLoadingLocalWallet: true,
}
}
Expand Down Expand Up @@ -413,7 +434,6 @@ export const WalletProvider = ({ children }: { children: React.ReactNode }): JSX
*/
state.wallet?.disconnect?.()
dispatch({ type: WalletActions.RESET_STATE })
store.dispatch(localWalletSlice.actions.clearLocalWallet())
Copy link
Member Author

Choose a reason for hiding this comment

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

disconnect() calls RESET_STATE, which calls clearLocalWallet()

}, [state.wallet])

const load = useCallback(() => {
Expand Down
5 changes: 0 additions & 5 deletions src/context/WalletProvider/WalletViewsSwitch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ import { Route, Switch, useHistory, useLocation, useRouteMatch } from 'react-rou
import { SlideTransition } from 'components/SlideTransition'
import { WalletActions } from 'context/WalletProvider/actions'
import { useWallet } from 'hooks/useWallet/useWallet'
import { localWalletSlice } from 'state/slices/localWalletSlice/localWalletSlice'
import { store } from 'state/store'

import { SUPPORTED_WALLETS } from './config'
import { KeyManager } from './KeyManager'
Expand Down Expand Up @@ -63,14 +61,11 @@ export const WalletViewsSwitch = () => {
if (disposition === 'initializing' || disposition === 'recovering') {
await wallet?.cancel()
disconnect()
store.dispatch(localWalletSlice.actions.clearLocalWallet())
Copy link
Member Author

Choose a reason for hiding this comment

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

disconnect() calls RESET_STATE, which calls clearLocalWallet()

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())
Copy link
Member Author

Choose a reason for hiding this comment

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

disconnect() calls RESET_STATE, which calls clearLocalWallet()

} else {
dispatch({ type: WalletActions.SET_WALLET_MODAL, payload: false })
}
Expand Down
8 changes: 7 additions & 1 deletion src/context/WalletProvider/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export enum WalletActions {
OPEN_KEEPKEY_RECOVERY = 'OPEN_KEEPKEY_RECOVERY',
OPEN_KEEPKEY_CHARACTER_REQUEST = 'OPEN_KEEPKEY_CHARACTER_REQUEST',
DOWNLOAD_UPDATER = 'DOWNLOAD_UPDATER',
SET_NATIVE_PENDING_DEVICE_ID = 'SET_NATIVE_PENDING_DEVICE_ID',
RESET_NATIVE_PENDING_DEVICE_ID = 'RESET_NATIVE_PENDING_DEVICE_ID',
}

export type ActionTypes =
Expand Down Expand Up @@ -62,7 +64,6 @@ export type ActionTypes =
type: WalletActions.NATIVE_PASSWORD_OPEN
payload: {
modal: boolean
deviceId: string
}
}
| {
Expand Down Expand Up @@ -107,3 +108,8 @@ export type ActionTypes =
}
}
| { type: WalletActions.OPEN_KEEPKEY_DISCONNECT }
| {
type: WalletActions.SET_NATIVE_PENDING_DEVICE_ID
payload: string | null
}
| { type: WalletActions.RESET_NATIVE_PENDING_DEVICE_ID }
Loading