diff --git a/app/components/Approvals/TransactionApproval/TransactionApproval.test.tsx b/app/components/Approvals/TransactionApproval/TransactionApproval.test.tsx index b534d116a225..2034161327e5 100644 --- a/app/components/Approvals/TransactionApproval/TransactionApproval.test.tsx +++ b/app/components/Approvals/TransactionApproval/TransactionApproval.test.tsx @@ -85,7 +85,10 @@ describe('TransactionApproval', () => { } as any); const wrapper = shallow( - , + , ); expect(wrapper).toMatchSnapshot(); diff --git a/app/components/Approvals/TransactionApproval/TransactionApproval.tsx b/app/components/Approvals/TransactionApproval/TransactionApproval.tsx index 8a52bb36c7cd..a6ba56e41b2c 100644 --- a/app/components/Approvals/TransactionApproval/TransactionApproval.tsx +++ b/app/components/Approvals/TransactionApproval/TransactionApproval.tsx @@ -5,8 +5,8 @@ import Approval from '../../Views/confirmations/legacy/Approval'; import Approve from '../../Views/confirmations/legacy/ApproveView/Approve'; import QRSigningModal from '../../UI/QRHardware/QRSigningModal'; import withQRHardwareAwareness from '../../UI/QRHardware/withQRHardwareAwareness'; -import { IQRState } from '../../UI/QRHardware/types'; import { useConfirmationRedesignEnabled } from '../../Views/confirmations/hooks/useConfirmationRedesignEnabled'; +import { QrScanRequest, QrScanRequestType } from '@metamask/eth-qr-keyring'; export enum TransactionModalType { Transaction = 'transaction', @@ -19,15 +19,22 @@ export interface TransactionApprovalProps { // eslint-disable-next-line @typescript-eslint/no-explicit-any navigation: any; onComplete: () => void; - QRState?: IQRState; + pendingScanRequest?: QrScanRequest; isSigningQRObject?: boolean; } const TransactionApprovalInternal = (props: TransactionApprovalProps) => { const { approvalRequest } = useApprovalRequest(); const { isRedesignedEnabled } = useConfirmationRedesignEnabled(); - const { onComplete: propsOnComplete } = props; - const isQRSigning = props.isSigningQRObject && !props.transactionType; + const { + onComplete: propsOnComplete, + pendingScanRequest, + isSigningQRObject, + } = props; + const isQRSigning = + isSigningQRObject && + pendingScanRequest?.type === QrScanRequestType.SIGN && + !props.transactionType; const onComplete = useCallback(() => { propsOnComplete(); @@ -64,9 +71,7 @@ const TransactionApprovalInternal = (props: TransactionApprovalProps) => { return ( -`; +exports[`TransactionApproval renders QR signing modal if signing QR object is exists 1`] = `""`; exports[`TransactionApproval renders approval component if transaction type is dapp 1`] = ` { // Queue txMetaId to listen for confirmation event addTransactionMetaIdForListening(transactionMeta.id); - await KeyringController.resetQRKeyringState(); - const isLedgerAccount = isHardwareAccount( transactionMeta.txParams.from, [ExtendedKeyringTypes.ledger], ); + // As the `TransactionController:unapprovedTransactionAdded` event is emitted + // before the approval request is added to `ApprovalController`, we need to wait + // for the next tick to make sure the approval request is present when auto-approve it + await new Promise((resolve) => setTimeout(resolve, 0)); + // For Ledger Accounts we handover the signing to the confirmation flow if (isLedgerAccount) { const deviceId = await getDeviceId(); diff --git a/app/components/UI/EvmAccountSelectorList/EvmAccountSelectorList.tsx b/app/components/UI/EvmAccountSelectorList/EvmAccountSelectorList.tsx index 92e8ad446115..ef8ebcdb7de0 100644 --- a/app/components/UI/EvmAccountSelectorList/EvmAccountSelectorList.tsx +++ b/app/components/UI/EvmAccountSelectorList/EvmAccountSelectorList.tsx @@ -420,6 +420,7 @@ const EvmAccountSelectorList = ({ } = item.data; const internalAccount = internalAccountsById[id]; + const shortAddress = formatAddress(address, 'short'); const tagLabel = isMultichainAccountsState1Enabled ? undefined diff --git a/app/components/UI/QRHardware/AnimatedQRScanner.tsx b/app/components/UI/QRHardware/AnimatedQRScanner.tsx index 2b5bd581e79e..ffb83949c051 100644 --- a/app/components/UI/QRHardware/AnimatedQRScanner.tsx +++ b/app/components/UI/QRHardware/AnimatedQRScanner.tsx @@ -32,6 +32,7 @@ import Icon, { IconName, IconSize, } from '../../../component-library/components/Icons/Icon'; +import { QrScanRequestType } from '@metamask/eth-qr-keyring'; const createStyles = (theme: Theme) => StyleSheet.create({ @@ -99,7 +100,7 @@ const frameImage = require('../../../images/frame.png'); // eslint-disable-line interface AnimatedQRScannerProps { visible: boolean; - purpose: 'sync' | 'sign'; + purpose: QrScanRequestType; onScanSuccess: (ur: UR) => void; onScanError: (error: string) => void; hideModal: () => void; @@ -126,7 +127,7 @@ const AnimatedQRScannerModal = (props: AnimatedQRScannerProps) => { const { hasPermission, requestPermission } = useCameraPermission(); let expectedURTypes: string[]; - if (purpose === 'sync') { + if (purpose === QrScanRequestType.PAIR) { expectedURTypes = [ SUPPORTED_UR_TYPE.CRYPTO_HDKEY, SUPPORTED_UR_TYPE.CRYPTO_ACCOUNT, @@ -152,7 +153,7 @@ const AnimatedQRScannerModal = (props: AnimatedQRScannerProps) => { {strings('connect_qr_hardware.hint_text')} {strings( - purpose === 'sync' + purpose === QrScanRequestType.PAIR ? 'connect_qr_hardware.purpose_connect' : 'connect_qr_hardware.purpose_sign', )} @@ -202,7 +203,7 @@ const AnimatedQRScannerModal = (props: AnimatedQRScannerProps) => { onScanSuccess(ur); setProgress(0); setURDecoder(new URRegistryDecoder()); - } else if (purpose === 'sync') { + } else if (purpose === QrScanRequestType.PAIR) { trackEvent( createEventBuilder(MetaMetricsEvents.HARDWARE_WALLET_ERROR) .addProperties({ diff --git a/app/components/UI/QRHardware/QRSigningDetails.tsx b/app/components/UI/QRHardware/QRSigningDetails.tsx index 7582fd82bc34..2f42d740f57c 100644 --- a/app/components/UI/QRHardware/QRSigningDetails.tsx +++ b/app/components/UI/QRHardware/QRSigningDetails.tsx @@ -1,10 +1,4 @@ -import React, { - Fragment, - useCallback, - useEffect, - useMemo, - useState, -} from 'react'; +import React, { Fragment, useCallback, useEffect, useState } from 'react'; import Engine from '../../../core/Engine'; import { StyleSheet, @@ -23,7 +17,6 @@ import AnimatedQRScannerModal from './AnimatedQRScanner'; import { fontStyles } from '../../../styles/common'; import AccountInfoCard from '../AccountInfoCard'; import ActionView from '../ActionView'; -import { IQRState } from './types'; import { UR } from '@ngraveio/bc-ur'; import { ETHSignature } from '@keystonehq/bc-ur-registry-eth'; import { stringify as uuidStringify } from 'uuid'; @@ -34,9 +27,10 @@ import { useNavigation } from '@react-navigation/native'; import { useTheme } from '../../../util/theme'; import Device from '../../../util/device'; import { useMetrics } from '../../../components/hooks/useMetrics'; +import { QrScanRequest, QrScanRequestType } from '@metamask/eth-qr-keyring'; interface IQRSigningDetails { - QRState: IQRState; + pendingScanRequest: QrScanRequest; successCallback?: () => void; failureCallback?: (error: string) => void; cancelCallback?: () => void; @@ -117,7 +111,7 @@ const createStyles = (colors: any) => }); const QRSigningDetails = ({ - QRState, + pendingScanRequest, successCallback, failureCallback, cancelCallback, @@ -133,12 +127,6 @@ const QRSigningDetails = ({ const { trackEvent, createEventBuilder } = useMetrics(); const styles = createStyles(colors); const navigation = useNavigation(); - const KeyringController = useMemo(() => { - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { KeyringController: keyring } = Engine.context as any; - return keyring; - }, []); const [scannerVisible, setScannerVisible] = useState(false); const [errorMessage, setErrorMessage] = useState(''); const [shouldPause, setShouldPause] = useState(false); @@ -195,11 +183,14 @@ const QRSigningDetails = ({ return; } e.preventDefault(); - KeyringController.cancelQRSignRequest().then(() => { - navigation.dispatch(e.data.action); - }); + if (pendingScanRequest) { + Engine.getQrKeyringScanner().rejectPendingScan( + new Error('Scan canceled'), + ); + } + navigation.dispatch(e.data.action); }); - }, [KeyringController, hasSentOrCanceled, navigation]); + }, [pendingScanRequest, hasSentOrCanceled, navigation]); const resetError = () => { setErrorMessage(''); @@ -215,11 +206,15 @@ const QRSigningDetails = ({ }; const onCancel = useCallback(async () => { - await KeyringController.cancelQRSignRequest(); + if (pendingScanRequest) { + Engine.getQrKeyringScanner().rejectPendingScan( + new Error('Scan canceled'), + ); + } setSentOrCanceled(true); hideScanner(); cancelCallback?.(); - }, [KeyringController, cancelCallback]); + }, [pendingScanRequest, cancelCallback]); const onScanSuccess = useCallback( (ur: UR) => { @@ -228,11 +223,11 @@ const QRSigningDetails = ({ const buffer = signature.getRequestId(); if (buffer) { const requestId = uuidStringify(buffer); - if (QRState.sign.request?.requestId === requestId) { - KeyringController.submitQRSignature( - QRState.sign.request?.requestId as string, - ur.cbor.toString('hex'), - ); + if (pendingScanRequest?.request?.requestId === requestId) { + Engine.getQrKeyringScanner().resolvePendingScan({ + type: ur.type, + cbor: ur.cbor.toString('hex'), + }); setSentOrCanceled(true); successCallback?.(); return; @@ -250,8 +245,7 @@ const QRSigningDetails = ({ failureCallback?.(strings('transaction.mismatched_qr_request_id')); }, [ - KeyringController, - QRState.sign.request?.requestId, + pendingScanRequest?.request?.requestId, failureCallback, successCallback, trackEvent, @@ -287,7 +281,7 @@ const QRSigningDetails = ({ return ( - {QRState?.sign?.request && ( + {pendingScanRequest?.request && ( void; onCancel?: () => void; onFailure?: (error: string) => void; @@ -37,7 +37,7 @@ const createStyles = (colors: any) => const QRSigningModal = ({ isVisible, - QRState, + pendingScanRequest, onSuccess, onCancel, onFailure, @@ -85,7 +85,7 @@ const QRSigningModal = ({ > , @@ -12,43 +13,18 @@ const withQRHardwareAwareness = ( // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any const QRHardwareAwareness = (props: any) => { - const [QRState, SetQRState] = useState({ - sync: { - reading: false, - }, - sign: {}, - }); - - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const subscribeKeyringState = (value: any) => { - SetQRState(value); - }; - - useEffect(() => { - // This ensures that a QR keyring gets created if it doesn't already exist. - // This is intentionally not awaited (the subscription still gets setup correctly if called - // before the keyring is created). - // TODO: Stop automatically creating keyrings - Engine.context.KeyringController.getOrAddQRKeyring(); - Engine.controllerMessenger.subscribe( - 'KeyringController:qrKeyringStateChange', - subscribeKeyringState, - ); - return () => { - Engine.controllerMessenger.unsubscribe( - 'KeyringController:qrKeyringStateChange', - subscribeKeyringState, - ); - }; - }, []); + const { pendingScanRequest } = useSelector( + (state: RootState) => state.qrKeyringScanner, + ); return ( ); }; diff --git a/app/components/UI/Transactions/index.js b/app/components/UI/Transactions/index.js index 0c8f96a29c1a..e88e5ac91a9b 100644 --- a/app/components/UI/Transactions/index.js +++ b/app/components/UI/Transactions/index.js @@ -556,8 +556,7 @@ class Transactions extends PureComponent { }; signQRTransaction = async (tx) => { - const { KeyringController, ApprovalController } = Engine.context; - await KeyringController.resetQRKeyringState(); + const { ApprovalController } = Engine.context; await ApprovalController.accept(tx.id, undefined, { waitForResult: true }); }; diff --git a/app/components/UI/Transactions/index.test.tsx b/app/components/UI/Transactions/index.test.tsx index 8c752f73a90d..603545ea53b6 100644 --- a/app/components/UI/Transactions/index.test.tsx +++ b/app/components/UI/Transactions/index.test.tsx @@ -57,9 +57,6 @@ jest.mock('../../../util/transaction-controller', () => ({ jest.mock('../../../core/Engine', () => ({ context: { - KeyringController: { - resetQRKeyringState: jest.fn(), - }, ApprovalController: { accept: jest.fn(), reject: jest.fn(), @@ -641,9 +638,6 @@ describe('Transactions', () => { }); it('should test Engine context methods', () => { - expect( - Engine.context.KeyringController.resetQRKeyringState, - ).toBeDefined(); expect(Engine.context.ApprovalController.accept).toBeDefined(); expect( Engine.context.TransactionController.stopTransaction, diff --git a/app/components/Views/AccountActions/AccountActions.tsx b/app/components/Views/AccountActions/AccountActions.tsx index 0efa3d6b9f38..85e470298963 100644 --- a/app/components/Views/AccountActions/AccountActions.tsx +++ b/app/components/Views/AccountActions/AccountActions.tsx @@ -47,6 +47,7 @@ import { useEIP7702Networks } from '../confirmations/hooks/7702/useEIP7702Networ import { isEvmAccountType } from '@metamask/keyring-api'; import { toHex } from '@metamask/controller-utils'; import { getMultichainBlockExplorer } from '../../../core/Multichain/networks'; +import { forgetQrDevice } from '../../../core/QrKeyring/QrKeyring'; interface AccountActionsParams { selectedAccount: InternalAccount; @@ -308,7 +309,7 @@ const AccountActions = () => { ); break; case ExtendedKeyringTypes.qr: - await controllers.KeyringController.forgetQRDevice(); + await forgetQrDevice(); trackEvent( createEventBuilder(MetaMetricsEvents.HARDWARE_WALLET_FORGOTTEN) .addProperties({ diff --git a/app/components/Views/ActivityView/index.test.tsx b/app/components/Views/ActivityView/index.test.tsx index 280fcfc54da9..cc114ddb70fb 100644 --- a/app/components/Views/ActivityView/index.test.tsx +++ b/app/components/Views/ActivityView/index.test.tsx @@ -47,11 +47,6 @@ jest.mock('@react-navigation/native', () => ({ })); jest.mock('../../../core/Engine', () => ({ - context: { - KeyringController: { - getOrAddQRKeyring: jest.fn(), - }, - }, controllerMessenger: { subscribe: jest.fn(), unsubscribe: jest.fn(), diff --git a/app/components/Views/Asset/index.test.js b/app/components/Views/Asset/index.test.js index d0b17b5488bf..7d5dcbedd61d 100644 --- a/app/components/Views/Asset/index.test.js +++ b/app/components/Views/Asset/index.test.js @@ -235,7 +235,6 @@ jest.mock('../../../core/Engine', () => { return { context: { KeyringController: { - getOrAddQRKeyring: async () => ({ subscribe: () => ({}) }), state: { keyrings: [ { diff --git a/app/components/Views/ConnectQRHardware/index.test.tsx b/app/components/Views/ConnectQRHardware/index.test.tsx index 74c795d5fa77..5a94be652e63 100644 --- a/app/components/Views/ConnectQRHardware/index.test.tsx +++ b/app/components/Views/ConnectQRHardware/index.test.tsx @@ -6,12 +6,12 @@ import { fireEvent } from '@testing-library/react-native'; import { QR_CONTINUE_BUTTON } from '../../../../wdio/screen-objects/testIDs/Components/ConnectQRHardware.testIds'; import { backgroundState } from '../../../util/test/initial-root-state'; import { act } from '@testing-library/react-hooks'; -import PAGINATION_OPERATIONS from '../../../constants/pagination'; import { ACCOUNT_SELECTOR_FORGET_BUTTON, ACCOUNT_SELECTOR_NEXT_BUTTON, ACCOUNT_SELECTOR_PREVIOUS_BUTTON, } from '../../../../wdio/screen-objects/testIDs/Components/AccountSelector.testIds'; +import { QrKeyringBridge } from '@metamask/eth-qr-keyring'; import { removeAccountsFromPermissions } from '../../../core/Permissions'; jest.mock('../../../core/Permissions', () => ({ @@ -29,8 +29,8 @@ const mockedNavigate = { const mockPage0Accounts = [ { - address: '0x4x678901234567890123456789012345678901210', - shortenedAddress: '0x4x678...01210', + address: '0x4678901234567890123456789012345678901210', + shortenedAddress: '0x46789...01210', balance: '0x0', index: 0, }, @@ -93,6 +93,23 @@ const mockPage1Accounts = [ }, ]; +const mockQrKeyring = { + getFirstPage: jest.fn(), + getNextPage: jest.fn(), + getPreviousPage: jest.fn(), + forgetDevice: jest.fn(), + getAccounts: jest + .fn() + .mockReturnValue([ + '0x4678901234567890123456789012345678901210', + '0x49A10E12ceaacC302548d3c1C72836C9298d180e', + ]), +}; + +const mockQrKeyringBridge: QrKeyringBridge = { + requestScan: jest.fn(), +}; + jest.mock('@react-navigation/native', () => { const actualNav = jest.requireActual('@react-navigation/native'); return { @@ -111,30 +128,11 @@ jest.mock('../../../core/Engine', () => ({ keyrings: [], }, getAccounts: jest.fn(), - getOrAddQRKeyring: jest.fn(), - withKeyring: jest - .fn() - .mockImplementation( - (_selector: unknown, operation: (args: unknown) => void) => - operation({ - keyring: { - cancelSync: jest.fn(), - submitCryptoAccount: jest.fn(), - submitCryptoHDKey: jest.fn(), - getAccounts: jest - .fn() - .mockReturnValue([ - '0x4x678901234567890123456789012345678901210', - '0xa1e359811322d97991e03f863a0c30c2cf029cd24', - ]), - }, - metadata: { id: '1234' }, - }), - ), - connectQRHardware: jest.fn(), - forgetQRDevice: jest - .fn() - .mockReturnValue({ remainingAccounts: ['0xdeadbeef'] }), + withKeyring: (_selector: unknown, operation: (args: unknown) => void) => + operation({ + keyring: mockQrKeyring, + metadata: { id: '1234' }, + }), }, AccountTrackerController: { syncBalanceWithAddresses: jest.fn(), @@ -144,6 +142,7 @@ jest.mock('../../../core/Engine', () => ({ subscribe: jest.fn(), unsubscribe: jest.fn(), }, + qrKeyringScanner: mockQrKeyringBridge, setSelectedAddress: jest.fn(), })); const MockEngine = jest.mocked(Engine); @@ -158,19 +157,9 @@ const mockInitialState = { describe('ConnectQRHardware', () => { const mockKeyringController = MockEngine.context.KeyringController; - mockKeyringController.connectQRHardware.mockImplementation((page) => { - switch (page) { - case PAGINATION_OPERATIONS.GET_NEXT_PAGE: - return Promise.resolve(mockPage1Accounts); - - case PAGINATION_OPERATIONS.GET_PREVIOUS_PAGE: - return Promise.resolve(mockPage0Accounts); - - default: - // return account lists in first page. - return Promise.resolve(mockPage0Accounts); - } - }); + mockQrKeyring.getFirstPage.mockResolvedValue(mockPage0Accounts); + mockQrKeyring.getNextPage.mockResolvedValue(mockPage1Accounts); + mockQrKeyring.getPreviousPage.mockResolvedValue(mockPage0Accounts); const mockAccountTrackerController = MockEngine.context.AccountTrackerController; @@ -219,10 +208,7 @@ describe('ConnectQRHardware', () => { fireEvent.press(button); }); - expect(mockKeyringController.connectQRHardware).toHaveBeenCalledTimes(1); - expect(mockKeyringController.connectQRHardware).toHaveBeenCalledWith( - PAGINATION_OPERATIONS.GET_FIRST_PAGE, - ); + expect(mockQrKeyring.getFirstPage).toHaveBeenCalledTimes(1); mockPage0Accounts.forEach((account) => { expect(getByText(account.shortenedAddress)).toBeDefined(); @@ -250,10 +236,7 @@ describe('ConnectQRHardware', () => { fireEvent.press(nextButton); }); - expect(mockKeyringController.connectQRHardware).toHaveBeenCalledTimes(2); - expect(mockKeyringController.connectQRHardware).toHaveBeenCalledWith( - PAGINATION_OPERATIONS.GET_NEXT_PAGE, - ); + expect(mockQrKeyring.getNextPage).toHaveBeenCalledTimes(1); mockPage1Accounts.forEach((account) => { expect(getByText(account.shortenedAddress)).toBeDefined(); @@ -287,10 +270,7 @@ describe('ConnectQRHardware', () => { fireEvent.press(prevButton); }); - expect(mockKeyringController.connectQRHardware).toHaveBeenCalledTimes(3); - expect(mockKeyringController.connectQRHardware).toHaveBeenCalledWith( - PAGINATION_OPERATIONS.GET_PREVIOUS_PAGE, - ); + expect(mockQrKeyring.getPreviousPage).toHaveBeenCalledTimes(1); mockPage0Accounts.forEach((account) => { expect(getByText(account.shortenedAddress)).toBeDefined(); @@ -299,6 +279,7 @@ describe('ConnectQRHardware', () => { it('removes any hardware wallet accounts from existing permissions', async () => { mockKeyringController.getAccounts.mockResolvedValue([]); + const withKeyringSpy = jest.spyOn(mockKeyringController, 'withKeyring'); const { getByTestId } = renderWithProvider( , @@ -318,11 +299,11 @@ describe('ConnectQRHardware', () => { fireEvent.press(forgetButton); }); - expect(mockKeyringController.withKeyring).toHaveBeenCalled(); + expect(withKeyringSpy).toHaveBeenCalled(); expect(MockRemoveAccountsFromPermissions).toHaveBeenCalledWith([ - '0x4x678901234567890123456789012345678901210', - '0xa1e359811322d97991e03f863a0c30c2cf029cd24', + '0x4678901234567890123456789012345678901210', + '0x49A10E12ceaacC302548d3c1C72836C9298d180e', ]); - expect(mockKeyringController.forgetQRDevice).toHaveBeenCalled(); + expect(mockQrKeyring.forgetDevice).toHaveBeenCalled(); }); }); diff --git a/app/components/Views/ConnectQRHardware/index.tsx b/app/components/Views/ConnectQRHardware/index.tsx index 1837f8484f6f..3f451ee67bc6 100644 --- a/app/components/Views/ConnectQRHardware/index.tsx +++ b/app/components/Views/ConnectQRHardware/index.tsx @@ -3,7 +3,6 @@ import React, { useCallback, useEffect, useMemo, - useRef, useState, } from 'react'; import { StyleSheet, Text, TouchableOpacity, View } from 'react-native'; @@ -24,18 +23,17 @@ import { MetaMetricsEvents } from '../../../core/Analytics'; import MaterialIcon from 'react-native-vector-icons/MaterialIcons'; import { useTheme } from '../../../util/theme'; -import { SUPPORTED_UR_TYPE } from '../../../constants/qr'; import { fontStyles } from '../../../styles/common'; import Logger from '../../../util/Logger'; import { removeAccountsFromPermissions } from '../../../core/Permissions'; import { useMetrics } from '../../../components/hooks/useMetrics'; -import type { MetaMaskKeyring as QRKeyring } from '@keystonehq/metamask-airgapped-keyring'; -import { KeyringTypes } from '@metamask/keyring-controller'; import ExtendedKeyringTypes, { HardwareDeviceTypes, } from '../../../constants/keyringTypes'; import { ThemeColors } from '@metamask/design-tokens'; -import PAGINATION_OPERATIONS from '../../../constants/pagination'; +import { QrScanRequestType } from '@metamask/eth-qr-keyring'; +import { withQrKeyring } from '../../../core/QrKeyring/QrKeyring'; +import { getChecksumAddress } from '@metamask/utils'; interface IConnectQRHardwareProps { // TODO: Replace "any" with type @@ -85,75 +83,16 @@ const createStyles = (colors: ThemeColors, insets: EdgeInsets) => }, }); -/** - * Initiate a QR hardware wallet connection - * - * This returns a tuple containing a set of QR interactions, followed by a Promise representing - * the QR connection process overall. - * - * The QR interactions are returned here to ensure that we get the correct references for the - * specific keyring instance we initiated the scan with. This is to ensure that we always resolve - * the `connectQRHardware` Promise. There are equivalent methods on the `KeyringController` class - * that we could use (e.g. `KeyringController.cancelQRSynchronization` would call - * `keyring.cancelSync` for us), but these methods are unsafe to use because they might end up - * calling the method on the wrong keyring instance (e.g. if the user had locked and unlocked the - * app since initiating the scan). They will be deprecated in a future `KeyringController` version. - * - * TODO: Refactor the QR Keyring to separate interaction methods from keyring operations, so that - * interactions are not affected by our keyring operation locks and by our lock/unlock operations. - * - * @param page - The page of accounts to request (either 0, 1, or -1), for "first page", - * "next page", and "previous page" respectively. - * @returns A tuple of QR keyring interactions, and a Promise representing the QR hardware wallet - * connection process. - */ -async function initiateQRHardwareConnection( - page: 0 | 1 | -1, -): Promise< - [ - Pick, - ReturnType< - (typeof Engine)['context']['KeyringController']['connectQRHardware'] - >, - ] -> { - const KeyringController = Engine.context.KeyringController; - - const qrInteractions = await KeyringController.withKeyring( - { type: KeyringTypes.qr }, - // @ts-expect-error The QR Keyring type is not compatible with our keyring type yet - async ({ keyring }: QRKeyring) => ({ - cancelSync: keyring.cancelSync.bind(keyring), - submitCryptoAccount: keyring.submitCryptoAccount.bind(keyring), - submitCryptoHDKey: keyring.submitCryptoHDKey.bind(keyring), - }), - { createIfMissing: true }, - ); - - const connectQRHardwarePromise = KeyringController.connectQRHardware(page); - - return [qrInteractions, connectQRHardwarePromise]; -} - const ConnectQRHardware = ({ navigation }: IConnectQRHardwareProps) => { const { colors } = useTheme(); const { trackEvent, createEventBuilder } = useMetrics(); const insets = useSafeAreaInsets(); const styles = createStyles(colors, insets); - const KeyringController = useMemo(() => { - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const { KeyringController: keyring } = Engine.context as any; - return keyring; - }, []); + const [isScanning, setIsScanning] = useState(false); + + const KeyringController = useMemo(() => Engine.context.KeyringController, []); - const [QRState, setQRState] = useState({ - sync: { - reading: false, - }, - }); - const [scannerVisible, setScannerVisible] = useState(false); const [blockingModalVisible, setBlockingModalVisible] = useState(false); const [accounts, setAccounts] = useState< { address: string; index: number; balance: string }[] @@ -165,66 +104,13 @@ const ConnectQRHardware = ({ navigation }: IConnectQRHardwareProps) => { const [existingAccounts, setExistingAccounts] = useState([]); - const qrInteractionsRef = - useRef< - Pick< - QRKeyring, - 'cancelSync' | 'submitCryptoAccount' | 'submitCryptoHDKey' - > - >(); - - const showScanner = useCallback(() => { - setScannerVisible(true); - }, []); - - const hideScanner = useCallback(() => { - setScannerVisible(false); - }, []); - - const hideModal = useCallback(() => { - qrInteractionsRef.current?.cancelSync(); - hideScanner(); - }, [hideScanner]); - useEffect(() => { KeyringController.getAccounts().then((value: string[]) => { setExistingAccounts(value); }); }, [KeyringController]); - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const subscribeKeyringState = useCallback((storeValue: any) => { - setQRState(storeValue); - }, []); - - useEffect(() => { - // This ensures that a QR keyring gets created if it doesn't already exist. - // This is intentionally not awaited (the subscription still gets setup correctly if called - // before the keyring is created). - // TODO: Stop automatically creating keyrings - Engine.context.KeyringController.getOrAddQRKeyring(); - Engine.controllerMessenger.subscribe( - 'KeyringController:qrKeyringStateChange', - subscribeKeyringState, - ); - return () => { - Engine.controllerMessenger.unsubscribe( - 'KeyringController:qrKeyringStateChange', - subscribeKeyringState, - ); - }; - }, [KeyringController, subscribeKeyringState]); - - useEffect(() => { - if (QRState.sync.reading) { - showScanner(); - } else { - hideScanner(); - } - }, [QRState.sync, hideScanner, showScanner]); - - const onConnectHardware = useCallback(async () => { + const onConnectHardware = async () => { trackEvent( createEventBuilder(MetaMetricsEvents.CONTINUE_QR_HARDWARE_WALLET) .addProperties({ @@ -233,19 +119,23 @@ const ConnectQRHardware = ({ navigation }: IConnectQRHardwareProps) => { .build(), ); resetError(); - const [qrInteractions, connectQRHardwarePromise] = - await initiateQRHardwareConnection(PAGINATION_OPERATIONS.GET_FIRST_PAGE); - - qrInteractionsRef.current = qrInteractions; - const firstPageAccounts = await connectQRHardwarePromise; - delete qrInteractionsRef.current; - - setAccounts(firstPageAccounts); - }, [resetError, trackEvent, createEventBuilder]); + try { + setIsScanning(true); + const firstAccountsPage = await withQrKeyring(({ keyring }) => + keyring.getFirstPage(), + ); + setAccounts( + // TODO: Add `balance` to the QR Keyring accounts or remove it from the expected type + firstAccountsPage.map((account) => ({ ...account, balance: '0x0' })), + ); + } finally { + setIsScanning(false); + } + }; const onScanSuccess = useCallback( - (ur: UR) => { - hideScanner(); + async (ur: UR) => { + setIsScanning(false); trackEvent( createEventBuilder(MetaMetricsEvents.CONNECT_HARDWARE_WALLET_SUCCESS) .addProperties({ @@ -253,56 +143,47 @@ const ConnectQRHardware = ({ navigation }: IConnectQRHardwareProps) => { }) .build(), ); - if (!qrInteractionsRef.current) { - const errorMessage = 'Missing QR keyring interactions'; - setErrorMsg(errorMessage); - throw new Error(errorMessage); - } - if (ur.type === SUPPORTED_UR_TYPE.CRYPTO_HDKEY) { - qrInteractionsRef.current.submitCryptoHDKey(ur.cbor.toString('hex')); - } else { - qrInteractionsRef.current.submitCryptoAccount(ur.cbor.toString('hex')); - } + Engine.getQrKeyringScanner().resolvePendingScan({ + type: ur.type, + cbor: ur.cbor.toString('hex'), + }); resetError(); }, - [hideScanner, resetError, trackEvent, createEventBuilder], + [resetError, trackEvent, createEventBuilder], ); - const onScanError = useCallback( - async (error: string) => { - hideScanner(); - setErrorMsg(error); - if (qrInteractionsRef.current) { - qrInteractionsRef.current.cancelSync(); - } - }, - [hideScanner], - ); + const onScanError = useCallback(async (error: string) => { + setErrorMsg(error); + Engine.getQrKeyringScanner().rejectPendingScan(new Error(error)); + }, []); + + const cancelScan = useCallback(() => { + Engine.getQrKeyringScanner().rejectPendingScan(new Error('Scan cancelled')); + }, []); const nextPage = useCallback(async () => { resetError(); - const [qrInteractions, connectQRHardwarePromise] = - await initiateQRHardwareConnection(PAGINATION_OPERATIONS.GET_NEXT_PAGE); - - qrInteractionsRef.current = qrInteractions; - const nextPageAccounts = await connectQRHardwarePromise; - delete qrInteractionsRef.current; - - setAccounts(nextPageAccounts); + const nextAccountsPage = await withQrKeyring(async ({ keyring }) => + keyring.getNextPage(), + ); + setAccounts( + // TODO: Add `balance` to the QR Keyring accounts or remove it from the expected type + nextAccountsPage.map((account) => ({ ...account, balance: '0x0' })), + ); }, [resetError]); const prevPage = useCallback(async () => { resetError(); - const [qrInteractions, connectQRHardwarePromise] = - await initiateQRHardwareConnection( - PAGINATION_OPERATIONS.GET_PREVIOUS_PAGE, - ); - - qrInteractionsRef.current = qrInteractions; - const previousPageAccounts = await connectQRHardwarePromise; - delete qrInteractionsRef.current; - - setAccounts(previousPageAccounts); + const previousAccountsPage = await withQrKeyring(async ({ keyring }) => + keyring.getPreviousPage(), + ); + setAccounts( + // TODO: Add `balance` to the QR Keyring accounts or remove it from the expected type + previousAccountsPage.map((account) => ({ + ...account, + balance: '0x0', + })), + ); }, [resetError]); const onCheck = useCallback(() => { @@ -314,35 +195,39 @@ const ConnectQRHardware = ({ navigation }: IConnectQRHardwareProps) => { resetError(); setBlockingModalVisible(true); try { - for (const index of accountIndexs) { - await KeyringController.unlockQRHardwareWalletAccount(index); - } + await withQrKeyring(async ({ keyring }) => { + for (const index of accountIndexs) { + keyring.setAccountToUnlock(index); + await keyring.addAccounts(1); + } + }); } catch (err) { Logger.log('Error: Connecting QR hardware wallet', err); } setBlockingModalVisible(false); navigation.pop(2); }, - [KeyringController, navigation, resetError], + [navigation, resetError], ); const onForget = useCallback(async () => { resetError(); - // Permissions need to be updated before the hardware wallet is forgotten. - // This is because `removeAccountsFromPermissions` relies on the account - // existing in AccountsController in order to resolve a hex address - // back into CAIP Account Id. Hex addresses are used in - // `removeAccountsFromPermissions` because too many places in the UI still - // operate on hex addresses rather than CAIP Account Id. - await Engine.context.KeyringController.withKeyring( - { type: ExtendedKeyringTypes.qr }, - async ({ keyring }) => { - const keyringAccounts = await keyring.getAccounts(); - removeAccountsFromPermissions(keyringAccounts); - }, - ); - const { remainingAccounts } = await KeyringController.forgetQRDevice(); - Engine.setSelectedAddress(remainingAccounts.at(-1)); + const remainingAccounts = KeyringController.state.keyrings + .filter((keyring) => keyring.type !== ExtendedKeyringTypes.qr) + .flatMap((keyring) => keyring.accounts); + Engine.setSelectedAddress(remainingAccounts[remainingAccounts.length - 1]); + await withQrKeyring(async ({ keyring }) => { + const existingQrAccounts = await keyring.getAccounts(); + // Permissions need to be updated before the hardware wallet is forgotten. + // This is because `removeAccountsFromPermissions` relies on the account + // existing in AccountsController in order to resolve a hex address + // back into CAIP Account Id. Hex addresses are used in + // `removeAccountsFromPermissions` because too many places in the UI still + // operate on hex addresses rather than CAIP Account Id. + removeAccountsFromPermissions(existingQrAccounts.map(getChecksumAddress)); + await keyring.forgetDevice(); + return existingQrAccounts; + }); navigation.pop(2); }, [KeyringController, navigation, resetError]); @@ -392,11 +277,11 @@ const ConnectQRHardware = ({ navigation }: IConnectQRHardwareProps) => { )} {strings('common.please_wait')} diff --git a/app/components/Views/TransactionsView/index.test.tsx b/app/components/Views/TransactionsView/index.test.tsx index 9d880aeefb32..68a6e57dda64 100644 --- a/app/components/Views/TransactionsView/index.test.tsx +++ b/app/components/Views/TransactionsView/index.test.tsx @@ -174,8 +174,6 @@ jest.mock('../../UI/Transactions', () => jest.fn()); jest.mock('../../../core/Engine', () => ({ context: { KeyringController: { - getOrAddQRKeyring: jest.fn(), - cancelQRSignRequest: jest.fn().mockResolvedValue(undefined), state: { keyrings: [], }, diff --git a/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.test.ts b/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.test.ts index 31824883c160..710967076b1c 100644 --- a/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.test.ts +++ b/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.test.ts @@ -74,9 +74,6 @@ jest.mock('../../../core/Engine', () => ({ accept: jest.fn(), reject: jest.fn(), }, - KeyringController: { - resetQRKeyringState: jest.fn(), - }, }, })); @@ -93,7 +90,6 @@ describe('useUnifiedTxActions', () => { interface EngineContextMock { TransactionController: { stopTransaction: jest.Mock }; ApprovalController: { accept: jest.Mock; reject: jest.Mock }; - KeyringController: { resetQRKeyringState: jest.Mock }; } const engineContext = Engine.context as unknown as EngineContextMock; @@ -421,9 +417,6 @@ describe('useUnifiedTxActions', () => { await result.current.signQRTransaction(tx); }); - expect( - engineContext.KeyringController.resetQRKeyringState, - ).toHaveBeenCalled(); expect(engineContext.ApprovalController.accept).toHaveBeenCalledWith( '12', undefined, diff --git a/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.ts b/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.ts index fdd561c16c6b..269ea93cf864 100644 --- a/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.ts +++ b/app/components/Views/UnifiedTransactionsView/useUnifiedTxActions.ts @@ -297,8 +297,7 @@ export function useUnifiedTxActions() { }; const signQRTransaction = async (tx: TransactionMeta) => { - const { KeyringController, ApprovalController } = Engine.context; - await KeyringController.resetQRKeyringState(); + const { ApprovalController } = Engine.context; await ApprovalController.accept(tx.id, undefined, { waitForResult: true }); }; diff --git a/app/components/Views/confirmations/components/confirm/confirm-component.test.tsx b/app/components/Views/confirmations/components/confirm/confirm-component.test.tsx index 07b0c6e5b580..60be72a70415 100644 --- a/app/components/Views/confirmations/components/confirm/confirm-component.test.tsx +++ b/app/components/Views/confirmations/components/confirm/confirm-component.test.tsx @@ -109,7 +109,6 @@ jest.mock('../../../../../core/Engine', () => ({ }, ], }, - getOrAddQRKeyring: jest.fn(), }, NetworkController: { getNetworkConfigurationByNetworkClientId: jest.fn(), diff --git a/app/components/Views/confirmations/components/footer/footer.test.tsx b/app/components/Views/confirmations/components/footer/footer.test.tsx index 3513960e7736..9e01d9302a8f 100644 --- a/app/components/Views/confirmations/components/footer/footer.test.tsx +++ b/app/components/Views/confirmations/components/footer/footer.test.tsx @@ -134,7 +134,7 @@ describe('Footer', () => { it('renders confirm button text "Get Signature" if QR signing is in progress', () => { jest.spyOn(QRHardwareHook, 'useQRHardwareContext').mockReturnValue({ - isQRSigningInProgress: true, + isSigningQRObject: true, } as QRHardwareHook.QRHardwareContextType); const { getByText } = renderWithProvider(