From b0471699a7a32639ffe6e34dfdd67b84bae65e82 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 15 Jan 2025 23:07:39 +0530 Subject: [PATCH] fix: re-designs signatures, continue to use old designs when signing with hardware wallets (#12976) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Render old designs if hardware wallet or WR aware device is used for signing. ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3870 ## **Manual testing steps** 1. Enable re-designs locally 2. Try to sign signature request using hardware wallet 3. It should work in old designs ## **Screenshots/Recordings** TODO ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .../confirmations/Confirm/Confirm.test.tsx | 11 +++ .../SignatureRequest/Root/Root.test.tsx | 4 ++ .../useConfirmationRedesignEnabled.test.ts | 72 +++++++++++++++++++ .../hooks/useConfirmationRedesignEnabled.ts | 21 +++++- .../hooks/useQRHardwareAwareness.ts | 38 ++++++++++ 5 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.test.ts create mode 100644 app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts 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', () => ({ 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', () => ({ 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(); + }); +}); diff --git a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts index 8d3ff6f05b6..140c58dcde4 100644 --- a/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts +++ b/app/components/Views/confirmations/hooks/useConfirmationRedesignEnabled.ts @@ -2,14 +2,20 @@ 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: {}, }; @@ -17,11 +23,22 @@ const useConfirmationRedesignEnabled = () => { () => (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, confirmation_redesign], + [ + approvalRequestType, + confirmation_redesign, + fromAddress, + isSigningQRObject, + isSyncingQRHardware, + ], ); 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..3767789d3ef --- /dev/null +++ b/app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts @@ -0,0 +1,38 @@ +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: {}, + }); + + const subscribe = (value: IQRState) => { + setQRState(value); + }; + + useEffect(() => { + Engine.context.KeyringController.getOrAddQRKeyring(); + Engine.controllerMessenger.subscribe( + 'KeyringController:qrKeyringStateChange', + subscribe, + ); + return () => { + Engine.controllerMessenger.unsubscribe( + 'KeyringController:qrKeyringStateChange', + subscribe, + ); + }; + }, []); + + return { + isSigningQRObject: !!qrState.sign?.request, + isSyncingQRHardware: qrState.sync.reading, + }; +}; + +export default useQRHardwareAwareness;