Skip to content

Commit

Permalink
fix: re-designs signatures, continue to use old designs when signing …
Browse files Browse the repository at this point in the history
…with hardware wallets (MetaMask#12976)

## **Description**

Render old designs if hardware wallet or WR aware device is used for
signing.

## **Related issues**

Fixes: MetaMask/MetaMask-planning#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.
  • Loading branch information
jpuri authored Jan 15, 2025
1 parent 9b0b6e7 commit b047169
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 2 deletions.
11 changes: 11 additions & 0 deletions app/components/Views/confirmations/Confirm/Confirm.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ jest.mock('../../../../../../core/Engine', () => ({
getQRKeyringState: jest.fn(() =>
Promise.resolve({ subscribe: jest.fn(), unsubscribe: jest.fn() }),
),
getOrAddQRKeyring: jest.fn(),
state: {
keyrings: [],
},
Expand All @@ -33,6 +34,9 @@ jest.mock('../../../../../../core/Engine', () => ({
},
},
},
controllerMessenger: {
subscribe: jest.fn(),
},
}));

jest.mock('@react-navigation/native', () => ({
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,43 @@ 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<string, string>)?.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 };
Expand Down
38 changes: 38 additions & 0 deletions app/components/Views/confirmations/hooks/useQRHardwareAwareness.ts
Original file line number Diff line number Diff line change
@@ -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<IQRState>({
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;

0 comments on commit b047169

Please sign in to comment.