From daad9ee39cd7f531fa3279d2ec1fc4df040ed03e Mon Sep 17 00:00:00 2001 From: jpuri Date: Tue, 14 Jan 2025 16:01:37 +0530 Subject: [PATCH 1/7] Re-design signature changes for QR hardware support --- .../hooks/useConfirmationRedesignEnabled.ts | 7 ++- .../hooks/useQRHardwareAwareness.ts | 45 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts diff --git a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts index 2a1b63132f3..dbeec152725 100644 --- a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts +++ b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts @@ -2,9 +2,11 @@ import { useMemo } from 'react'; import { ApprovalTypes } from '../../../../core/RPCMethods/RPCMethodMiddleware'; import useApprovalRequest from './useApprovalRequest'; +import useQRHardwareAwareness from './useQRHardwareAwareness'; const useConfirmationRedesignEnabled = () => { const { approvalRequest } = useApprovalRequest(); + const { isSigningQRObject, isSyncingQRHardware } = useQRHardwareAwareness(); const { type: approvalRequestType } = approvalRequest ?? { requestData: {}, @@ -12,12 +14,13 @@ const useConfirmationRedesignEnabled = () => { const isRedesignedEnabled = useMemo( () => + !isSyncingQRHardware && + !isSigningQRObject && approvalRequestType && - process.env.REDESIGNED_SIGNATURE_REQUEST === 'true' && [ApprovalTypes.PERSONAL_SIGN, ApprovalTypes.ETH_SIGN_TYPED_DATA].includes( approvalRequestType as ApprovalTypes, ), - [approvalRequestType], + [approvalRequestType, isSyncingQRHardware, isSigningQRObject], ); return { isRedesignedEnabled }; diff --git a/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts b/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts new file mode 100644 index 00000000000..e2bd3e5d24f --- /dev/null +++ b/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts @@ -0,0 +1,45 @@ +import { useState, useEffect } from 'react'; + +import Engine from '../../../../core/Engine'; +import { IQRState } from '../../../UI/QRHardware/types'; + +const useQRHardwareAwareness = () => { + 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, + ); + }; + }, []); + + return { + isSigningQRObject: !!QRState.sign?.request, + isSyncingQRHardware: QRState.sync.reading, + QRState, + }; +}; + +export default useQRHardwareAwareness; From 19be28e09481cf9d02e3f32da92108e8fe6548f0 Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 15 Jan 2025 14:04:32 +0530 Subject: [PATCH 2/7] fix --- .../confirmations/hooks/useConfirmActions.ts | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/app/components/Views/confirmations/hooks/useConfirmActions.ts b/app/components/Views/confirmations/hooks/useConfirmActions.ts index 01ab81fe5c0..f1431bf4d27 100644 --- a/app/components/Views/confirmations/hooks/useConfirmActions.ts +++ b/app/components/Views/confirmations/hooks/useConfirmActions.ts @@ -4,6 +4,9 @@ import { MetaMetricsEvents } from '../../../hooks/useMetrics'; import { isSignatureRequest } from '../utils/confirm'; import useApprovalRequest from './useApprovalRequest'; import { useSignatureMetrics } from './useSignatureMetrics'; +import { isExternalHardwareAccount } from '../../../../util/address'; +import createExternalSignModelNav from '../../../../util/hardwareWallet/signatureUtils'; +import { useNavigation } from '@react-navigation/native'; export const useConfirmActions = () => { const { @@ -12,20 +15,39 @@ export const useConfirmActions = () => { approvalRequest, } = useApprovalRequest(); const { captureSignatureMetrics } = useSignatureMetrics(); + const navigation = useNavigation(); const signatureRequest = approvalRequest?.type && isSignatureRequest(approvalRequest?.type); const onConfirm = useCallback(async () => { - await onRequestConfirm({ - waitForResult: true, - deleteAfterResult: true, - handleErrors: false, - }); + if (isExternalHardwareAccount(approvalRequest?.requestData?.from)) { + navigation.navigate( + ...(await createExternalSignModelNav( + onRequestReject, + onRequestConfirm, + approvalRequest?.requestData, + 'personal_sign', + )), + ); + } else { + await onRequestConfirm({ + waitForResult: true, + deleteAfterResult: true, + handleErrors: false, + }); + } if (signatureRequest) { captureSignatureMetrics(MetaMetricsEvents.SIGNATURE_APPROVED); } - }, [captureSignatureMetrics, onRequestConfirm, signatureRequest]); + }, [ + captureSignatureMetrics, + onRequestConfirm, + navigation, + signatureRequest, + approvalRequest, + onRequestReject, + ]); const onReject = useCallback(() => { onRequestReject(); From bf2bda64e1a9c0fd7b9c37082bbeece7ef2197f7 Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 15 Jan 2025 14:33:51 +0530 Subject: [PATCH 3/7] update --- .../confirmations/hooks/useConfirmActions.ts | 34 ++++--------------- .../hooks/useConfirmationRedesignEnabled.ts | 22 ++++++++++-- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/app/components/Views/confirmations/hooks/useConfirmActions.ts b/app/components/Views/confirmations/hooks/useConfirmActions.ts index f1431bf4d27..01ab81fe5c0 100644 --- a/app/components/Views/confirmations/hooks/useConfirmActions.ts +++ b/app/components/Views/confirmations/hooks/useConfirmActions.ts @@ -4,9 +4,6 @@ import { MetaMetricsEvents } from '../../../hooks/useMetrics'; import { isSignatureRequest } from '../utils/confirm'; import useApprovalRequest from './useApprovalRequest'; import { useSignatureMetrics } from './useSignatureMetrics'; -import { isExternalHardwareAccount } from '../../../../util/address'; -import createExternalSignModelNav from '../../../../util/hardwareWallet/signatureUtils'; -import { useNavigation } from '@react-navigation/native'; export const useConfirmActions = () => { const { @@ -15,39 +12,20 @@ export const useConfirmActions = () => { approvalRequest, } = useApprovalRequest(); const { captureSignatureMetrics } = useSignatureMetrics(); - const navigation = useNavigation(); const signatureRequest = approvalRequest?.type && isSignatureRequest(approvalRequest?.type); const onConfirm = useCallback(async () => { - if (isExternalHardwareAccount(approvalRequest?.requestData?.from)) { - navigation.navigate( - ...(await createExternalSignModelNav( - onRequestReject, - onRequestConfirm, - approvalRequest?.requestData, - 'personal_sign', - )), - ); - } else { - await onRequestConfirm({ - waitForResult: true, - deleteAfterResult: true, - handleErrors: false, - }); - } + await onRequestConfirm({ + waitForResult: true, + deleteAfterResult: true, + handleErrors: false, + }); if (signatureRequest) { captureSignatureMetrics(MetaMetricsEvents.SIGNATURE_APPROVED); } - }, [ - captureSignatureMetrics, - onRequestConfirm, - navigation, - signatureRequest, - approvalRequest, - onRequestReject, - ]); + }, [captureSignatureMetrics, onRequestConfirm, signatureRequest]); const onReject = useCallback(() => { onRequestReject(); diff --git a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts index dbeec152725..140c58dcde4 100644 --- a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts +++ b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts @@ -1,26 +1,44 @@ import { useMemo } from 'react'; +import { useSelector } from 'react-redux'; import { ApprovalTypes } from '../../../../core/RPCMethods/RPCMethodMiddleware'; +import { isExternalHardwareAccount } from '../../../../util/address'; +import { selectRemoteFeatureFlags } from '../../../../selectors/featureFlagController'; import useApprovalRequest from './useApprovalRequest'; import useQRHardwareAwareness from './useQRHardwareAwareness'; const useConfirmationRedesignEnabled = () => { const { approvalRequest } = useApprovalRequest(); const { isSigningQRObject, isSyncingQRHardware } = useQRHardwareAwareness(); + const { confirmation_redesign } = useSelector(selectRemoteFeatureFlags); - const { type: approvalRequestType } = approvalRequest ?? { + const { + type: approvalRequestType, + requestData: { from: fromAddress }, + } = approvalRequest ?? { requestData: {}, }; const isRedesignedEnabled = useMemo( () => + (confirmation_redesign as Record)?.signatures && + process.env.REDESIGNED_SIGNATURE_REQUEST === 'true' && + // following condition will ensure that user is redirected to old designs is using QR scan aware hardware !isSyncingQRHardware && !isSigningQRObject && + // following condition will ensure that user is redirected to old designs for hardware wallets + !isExternalHardwareAccount(fromAddress) && approvalRequestType && [ApprovalTypes.PERSONAL_SIGN, ApprovalTypes.ETH_SIGN_TYPED_DATA].includes( approvalRequestType as ApprovalTypes, ), - [approvalRequestType, isSyncingQRHardware, isSigningQRObject], + [ + approvalRequestType, + confirmation_redesign, + fromAddress, + isSigningQRObject, + isSyncingQRHardware, + ], ); return { isRedesignedEnabled }; From c108f59245132104d146474af4b1be152726809e Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 15 Jan 2025 15:24:56 +0530 Subject: [PATCH 4/7] update --- .../Views/confirmations/Confirm/Confirm.test.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/components/Views/confirmations/Confirm/Confirm.test.tsx b/app/components/Views/confirmations/Confirm/Confirm.test.tsx index dc1c3bdbd6b..8007a4c9866 100644 --- a/app/components/Views/confirmations/Confirm/Confirm.test.tsx +++ b/app/components/Views/confirmations/Confirm/Confirm.test.tsx @@ -10,6 +10,17 @@ import Confirm from './index'; jest.mock('../../../../core/Engine', () => ({ getTotalFiatAccountBalance: () => ({ tokenFiat: 10 }), + context: { + KeyringController: { + state: { + keyrings: [], + }, + getOrAddQRKeyring: jest.fn(), + }, + }, + controllerMessenger: { + subscribe: jest.fn(), + }, })); jest.mock('../../../../util/address', () => ({ From 46aeb36d64072140719decb7017593fc2c5168ce Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 15 Jan 2025 15:43:20 +0530 Subject: [PATCH 5/7] update --- .../components/SignatureRequest/Root/Root.test.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/components/Views/confirmations/components/SignatureRequest/Root/Root.test.tsx b/app/components/Views/confirmations/components/SignatureRequest/Root/Root.test.tsx index cc7c6b7675b..84c9ab2520f 100644 --- a/app/components/Views/confirmations/components/SignatureRequest/Root/Root.test.tsx +++ b/app/components/Views/confirmations/components/SignatureRequest/Root/Root.test.tsx @@ -23,6 +23,7 @@ jest.mock('../../../../../../core/Engine', () => ({ getQRKeyringState: jest.fn(() => Promise.resolve({ subscribe: jest.fn(), unsubscribe: jest.fn() }), ), + getOrAddQRKeyring: jest.fn(), state: { keyrings: [], }, @@ -33,6 +34,9 @@ jest.mock('../../../../../../core/Engine', () => ({ }, }, }, + controllerMessenger: { + subscribe: jest.fn(), + }, })); jest.mock('@react-navigation/native', () => ({ From 014f1ee2998e9a94ee31673f33d7d9602c3ade85 Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 15 Jan 2025 16:07:43 +0530 Subject: [PATCH 6/7] update --- .../hooks/useQRHardwareAwareness.ts | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts b/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts index e2bd3e5d24f..3767789d3ef 100644 --- a/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts +++ b/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts @@ -4,41 +4,34 @@ import Engine from '../../../../core/Engine'; import { IQRState } from '../../../UI/QRHardware/types'; const useQRHardwareAwareness = () => { - const [QRState, SetQRState] = useState({ + 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); + const subscribe = (value: IQRState) => { + 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, + subscribe, ); return () => { Engine.controllerMessenger.unsubscribe( 'KeyringController:qrKeyringStateChange', - subscribeKeyringState, + subscribe, ); }; }, []); return { - isSigningQRObject: !!QRState.sign?.request, - isSyncingQRHardware: QRState.sync.reading, - QRState, + isSigningQRObject: !!qrState.sign?.request, + isSyncingQRHardware: qrState.sync.reading, }; }; From d2f60696b87368a1592d8a03d99c6b611df8249c Mon Sep 17 00:00:00 2001 From: jpuri Date: Wed, 15 Jan 2025 19:00:40 +0530 Subject: [PATCH 7/7] adding test coverage --- .../useConfirmationRedesignEnabled.test.ts | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.test.ts diff --git a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.test.ts b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.test.ts new file mode 100644 index 00000000000..ffa03104883 --- /dev/null +++ b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.test.ts @@ -0,0 +1,72 @@ +// eslint-disable-next-line import/no-namespace +import * as AddressUtils from '../../../../util/address'; +import { renderHookWithProvider } from '../../../../util/test/renderWithProvider'; +import { personalSignatureConfirmationState } from '../../../../util/test/confirm-data-helpers'; + +// eslint-disable-next-line import/no-namespace +import * as QRHardwareAwareness from './useQRHardwareAwareness'; +import useConfirmationRedesignEnabled from './useConfirmationRedesignEnabled'; + +jest.mock('../../../../core/Engine', () => ({ + getTotalFiatAccountBalance: () => ({ tokenFiat: 10 }), + context: { + KeyringController: { + state: { + keyrings: [], + }, + getOrAddQRKeyring: jest.fn(), + }, + }, + controllerMessenger: { + subscribe: jest.fn(), + }, +})); + +describe('useConfirmationRedesignEnabled', () => { + it('return true for personal sign request', async () => { + const { result } = renderHookWithProvider( + () => useConfirmationRedesignEnabled(), + { + state: personalSignatureConfirmationState, + }, + ); + expect(result?.current.isRedesignedEnabled).toBeTruthy(); + }); + + it('return false for external accounts', async () => { + jest.spyOn(AddressUtils, 'isExternalHardwareAccount').mockReturnValue(true); + const { result } = renderHookWithProvider( + () => useConfirmationRedesignEnabled(), + { + state: personalSignatureConfirmationState, + }, + ); + expect(result?.current.isRedesignedEnabled).toBeFalsy(); + }); + + it('return false if QR hardware is syncing', async () => { + jest + .spyOn(QRHardwareAwareness, 'default') + .mockReturnValue({ isSigningQRObject: true, isSyncingQRHardware: false }); + const { result } = renderHookWithProvider( + () => useConfirmationRedesignEnabled(), + { + state: personalSignatureConfirmationState, + }, + ); + expect(result?.current.isRedesignedEnabled).toBeFalsy(); + }); + + it('return false if QR hardware has synced successfully', async () => { + jest + .spyOn(QRHardwareAwareness, 'default') + .mockReturnValue({ isSigningQRObject: false, isSyncingQRHardware: true }); + const { result } = renderHookWithProvider( + () => useConfirmationRedesignEnabled(), + { + state: personalSignatureConfirmationState, + }, + ); + expect(result?.current.isRedesignedEnabled).toBeFalsy(); + }); +});