Skip to content

Commit 839c3ae

Browse files
runway-github[bot]montelaidevchloeYuegantunesr
authored
chore(runway): cherry-pick fix: qr scanner appearing before the confirmations screen (#30193)
- fix: qr scanner appearing before the confirmations screen cp-7.77.0 (#30088) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until this PR meets the canonical Definition of Ready For Review in `docs/readme/ready-for-review.md`. In short: the template must be materially complete (not just section titles present), all status checks must be currently passing, and the only expected follow-up commits must be reviewer-driven. --> ## **Description** This PR fixes the issue where the qr scanning appears before the confirmation screen. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: fixes the the qr scanner appearing before confirmations page ## **Related issues** Fixes: #29949 ## **Manual testing steps** ```gherkin Scenario: QR hardware wallet transaction shows confirmation screen first Given the user has a QR hardware wallet account (e.g. Keystone) set up And the user is on that QR account in MetaMask When the user initiates a send transaction to an address Then the confirmation screen is displayed (NOT the QR scanner) And the user sees transaction details (amount, recipient, gas) Scenario: QR hardware wallet user confirms a transaction Given the user is on the confirmation screen for a QR account transaction When the user taps "Confirm" Then the QR scanner screen appears And the app waits for the QR code scan to complete the transaction Scenario: QR hardware wallet user rejects a transaction Given the user is on the confirmation screen for a QR account transaction When the user taps "Reject" Then the transaction is cancelled And the QR scanner does NOT appear Scenario: QR hardware wallet signature request shows confirmation first Given the user is on a QR hardware wallet account When a dApp requests a signature (personal_sign or eth_signTypedData) Then the confirmation screen is displayed And the QR scanner does NOT appear until the user taps "Confirm" Scenario: QR hardware wallet signature request shows QRInfo after confirm Given the user is on the confirmation screen for a QR account signature request When the user taps "Confirm" Then the QR scanner / QRInfo view is shown And the user can scan the QR code to complete signing ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** https://github.com/user-attachments/assets/ee6004b2-2f63-47b2-b007-d623b7ed990e https://github.com/user-attachments/assets/9798e0f2-6756-4e4b-87fd-ee2cc1ebf98b <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** <!-- Every checklist item must be consciously assessed before marking this PR as "Ready for review". A checked box means you deliberately considered that responsibility, not that you literally performed every action listed. Unchecked boxes are ambiguous: they are not an implicit "N/A" and they are not a silent "skip". See `docs/readme/ready-for-review.md` for the full checklist semantics. --> - [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. #### Performance checks (if applicable) - [ ] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [ ] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** <!-- Reviewer checklist items follow the same semantics as the author checklist: an unchecked box is ambiguous, a checked box means the reviewer consciously assessed that responsibility. See `docs/readme/ready-for-review.md`. --> - [ ] 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes confirmation submit flow for QR hardware wallets by adding new state and altering when scanning UI is shown; mistakes could block or prematurely trigger signing for QR accounts. Scope is contained to confirmations/QR hardware paths with updated tests. > > **Overview** > Prevents the QR scanner/`QRInfo` view from appearing before the confirmations screen by introducing `signingConfirmed` in `QRHardwareContext` and only rendering `QRInfo` when QR signing is active *and* the user has confirmed. > > Updates `useConfirmActions` so tapping Confirm during an in-progress QR signing session sets `signingConfirmed` and explicitly shows the scanner, while still delegating to `useQrConfirm` for QR-hardware accounts; also ensures `signingConfirmed` is set before executing the default approval/transaction paths. > > Adjusts QR confirm UX so `useQrConfirm` re-shows the awaiting-confirmation bottom sheet when QR signing is already in progress (instead of opening the `QRInfo` scanner), and updates footer behavior/tests so the button label remains `Confirm` during QR signing. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 864f724. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> [a9c779f](a9c779f) --------- Co-authored-by: Monte Lai <monte.lai@consensys.net> Co-authored-by: chloeYue <105063779+chloeYue@users.noreply.github.com> Co-authored-by: gantunesr <17601467+gantunesr@users.noreply.github.com>
1 parent e50d2dd commit 839c3ae

11 files changed

Lines changed: 168 additions & 30 deletions

File tree

app/components/Views/confirmations/components/footer/footer.test.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,14 +156,14 @@ describe('Footer', () => {
156156
});
157157
});
158158

159-
it('renders confirm button text "Get signature" if QR signing is in progress', () => {
159+
it('renders confirm button text "Confirm" even if QR signing is in progress', () => {
160160
jest.spyOn(QRHardwareHook, 'useQRHardwareContext').mockReturnValue({
161161
isSigningQRObject: true,
162162
} as QRHardwareHook.QRHardwareContextType);
163163
const { getByText } = renderWithProvider(<Footer />, {
164164
state: personalSignatureConfirmationState,
165165
});
166-
expect(getByText('Get signature')).toBeTruthy();
166+
expect(getByText('Confirm')).toBeTruthy();
167167
});
168168

169169
it('confirm button is disabled if `needsCameraPermission` is true', () => {

app/components/Views/confirmations/components/footer/footer.tsx

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import { PredictClaimFooter } from '../predict-confirmations/predict-claim-foote
3939
import { useIsTransactionPayLoading } from '../../hooks/pay/useTransactionPayData';
4040
import { Skeleton } from '../../../../../component-library/components-temp/Skeleton';
4141
import { useQRHardwareContext } from '../../context/qr-hardware-context';
42+
import { useIsConfirmationFromQrAccount } from '../../../../../core/HardwareWallet/hooks/useIsConfirmationFromQrAccount';
4243
import { useIsGaslessLoading } from '../../hooks/gas/useIsGaslessLoading';
4344

4445
const HIDE_FOOTER_BY_DEFAULT_TYPES = [
@@ -61,7 +62,7 @@ export const Footer = () => {
6162
hasUnconfirmedDangerAlerts,
6263
} = useAlerts();
6364
const { onConfirm, onReject } = useConfirmActions();
64-
const { isSigningQRObject, needsCameraPermission } = useQRHardwareContext();
65+
const { needsCameraPermission } = useQRHardwareContext();
6566
const { securityAlertResponse } = useSecurityAlertResponse();
6667
const transactionMetadata = useTransactionMetadataRequest();
6768
const { trackAlertMetrics } = useConfirmationAlertMetrics();
@@ -121,10 +122,6 @@ export const Footer = () => {
121122
});
122123

123124
const confirmButtonLabel = () => {
124-
if (isSigningQRObject) {
125-
return strings('confirm.qr_get_sign');
126-
}
127-
128125
if (isPayLoading) {
129126
return strings('confirm.confirm');
130127
}

app/components/Views/confirmations/components/info-root/info-root.test.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,10 @@ describe('Info', () => {
173173
).toBeDefined();
174174
});
175175

176-
it('renders QRInfo if user is signing using QR hardware', () => {
176+
it('renders QRInfo if user is signing using QR hardware and has confirmed', () => {
177177
jest.spyOn(QRHardwareHook, 'useQRHardwareContext').mockReturnValue({
178178
isSigningQRObject: true,
179+
signingConfirmed: true,
179180
} as unknown as QRHardwareHook.QRHardwareContextType);
180181
const { getByTestId } = renderWithProvider(<Info />, {
181182
state: personalSignatureConfirmationState,

app/components/Views/confirmations/components/info-root/info-root.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ interface InfoProps {
8787
const Info = ({ route }: InfoProps) => {
8888
const { approvalRequest } = useApprovalRequest();
8989
const transactionMetadata = useTransactionMetadataRequest();
90-
const { isSigningQRObject } = useQRHardwareContext();
90+
const { isSigningQRObject, signingConfirmed } = useQRHardwareContext();
9191
const { isDowngrade, isUpgradeOnly } = use7702TransactionType();
9292
// Refresh STX liveness for the transaction's network
9393
useRefreshSmartTransactionsLiveness(transactionMetadata?.chainId);
@@ -100,7 +100,7 @@ const Info = ({ route }: InfoProps) => {
100100
return <SwitchAccountType />;
101101
}
102102

103-
if (isSigningQRObject) {
103+
if (isSigningQRObject && signingConfirmed) {
104104
return <QRInfo />;
105105
}
106106

app/components/Views/confirmations/context/qr-hardware-context/qr-hardware-context.test.tsx

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,17 @@ jest.mock('../../../../../core/HardwareWallet', () => ({
5757

5858
jest.mock('../../hooks/gas/useGasFeeToken');
5959

60+
jest.mock('../../hooks/useIsConfirmationFromLedgerAccount', () => ({
61+
useIsConfirmationFromLedgerAccount: jest.fn(() => false),
62+
}));
63+
64+
jest.mock(
65+
'../../../../../core/HardwareWallet/hooks/useIsConfirmationFromQrAccount',
66+
() => ({
67+
useIsConfirmationFromQrAccount: jest.fn(() => false),
68+
}),
69+
);
70+
6071
jest.mock('../../hooks/ui/useFullScreenConfirmation', () => ({
6172
useFullScreenConfirmation: jest.fn(() => ({
6273
isFullScreenConfirmation: true,
@@ -177,7 +188,7 @@ describe('QRHardwareContext', () => {
177188
expect(getByText('Scan with your hardware wallet')).toBeTruthy();
178189
});
179190

180-
it('passes correct value of scannerVisible to child components', async () => {
191+
it('renders Confirm button when QR signing is in progress', async () => {
181192
createCameraSpy({ cameraError: undefined, hasCameraPermission: true });
182193
createQRHardwareAwarenessSpy({
183194
isSigningQRObject: true,
@@ -194,10 +205,7 @@ describe('QRHardwareContext', () => {
194205
state: personalSignatureConfirmationState,
195206
},
196207
);
197-
await userEvent.press(getByText('Get signature'));
198-
expect(
199-
getByText('Scan your hardware wallet to confirm the transaction'),
200-
).toBeTruthy();
208+
expect(getByText('Confirm')).toBeTruthy();
201209
});
202210
});
203211

app/components/Views/confirmations/context/qr-hardware-context/qr-hardware-context.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ export interface QRHardwareContextType {
2222
scannerVisible: boolean;
2323
setRequestCompleted: () => void;
2424
setScannerVisible: (visibility: boolean) => void;
25+
setSigningConfirmed: () => void;
26+
signingConfirmed: boolean;
2527
}
2628

2729
export const QRHardwareContext = createContext<QRHardwareContextType>({
@@ -33,6 +35,8 @@ export const QRHardwareContext = createContext<QRHardwareContextType>({
3335
scannerVisible: false,
3436
setRequestCompleted: () => undefined,
3537
setScannerVisible: () => undefined,
38+
setSigningConfirmed: () => undefined,
39+
signingConfirmed: false,
3640
});
3741

3842
export const QRHardwareContextProvider: React.FC<{
@@ -43,6 +47,7 @@ export const QRHardwareContextProvider: React.FC<{
4347
const { cameraError, hasCameraPermission } = useCamera(isSigningQRObject);
4448
const [scannerVisible, setScannerVisible] = useState(false);
4549
const [isRequestCompleted, setRequestCompleted] = useState(false);
50+
const [signingConfirmed, setSigningConfirmed] = useState(false);
4651

4752
const cancelRequest = useCallback(
4853
(e: { preventDefault: () => void; data: { action: NavigationAction } }) => {
@@ -87,6 +92,8 @@ export const QRHardwareContextProvider: React.FC<{
8792
scannerVisible,
8893
setRequestCompleted: () => setRequestCompleted(true),
8994
setScannerVisible,
95+
setSigningConfirmed: () => setSigningConfirmed(true),
96+
signingConfirmed,
9097
}}
9198
>
9299
{children}

app/components/Views/confirmations/hooks/send/useSendActions.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,20 +77,20 @@ describe('useSendActions', () => {
7777
expect(result.current.handleCancelPress).toBeDefined();
7878
});
7979

80-
it('calls navigation.navigate with correct params when evm ', () => {
80+
it('calls navigation.navigate with correct params when evm ', async () => {
8181
const { result } = renderHookWithProvider(
8282
() => useSendActions(),
8383
mockState,
8484
);
8585
jest.spyOn(SendUtils, 'submitEvmTransaction').mockImplementation(jest.fn());
86-
result.current.handleSubmitPress();
86+
await result.current.handleSubmitPress();
8787
expect(mockNavigate).toHaveBeenCalledWith('RedesignedConfirmations', {
8888
params: { maxValueMode: undefined },
8989
loader: 'transfer',
9090
});
9191
});
9292

93-
it('normalizes trailing dot values before submitting evm transaction', () => {
93+
it('normalizes trailing dot values before submitting evm transaction', async () => {
9494
mockUseSendContext.mockReturnValue({
9595
asset: {
9696
chainId: '0x1',
@@ -111,7 +111,7 @@ describe('useSendActions', () => {
111111
() => useSendActions(),
112112
mockState,
113113
);
114-
result.current.handleSubmitPress();
114+
await result.current.handleSubmitPress();
115115

116116
expect(submitSpy).toHaveBeenCalledWith(
117117
expect.objectContaining({ value: '0' }),

app/components/Views/confirmations/hooks/useConfirmActions.test.ts

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,27 @@ jest.mock('./useIsConfirmationFromLedgerAccount', () => ({
1919
useIsConfirmationFromLedgerAccount: jest.fn().mockReturnValue(false),
2020
}));
2121

22+
jest.mock(
23+
'../../../../core/HardwareWallet/hooks/useIsConfirmationFromQrAccount',
24+
() => ({
25+
useIsConfirmationFromQrAccount: jest.fn().mockReturnValue(false),
26+
}),
27+
);
28+
2229
const mockOnLedgerConfirm = jest.fn().mockResolvedValue(undefined);
2330
jest.mock('./useLedgerConfirm', () => ({
2431
useLedgerConfirm: () => ({ onConfirm: mockOnLedgerConfirm }),
2532
}));
2633

34+
const mockOnQrConfirm = jest.fn().mockResolvedValue(undefined);
35+
jest.mock('../../../../core/HardwareWallet/hooks/useQrConfirm', () => ({
36+
useQrConfirm: () => ({ onConfirm: mockOnQrConfirm }),
37+
}));
38+
39+
const mockIsConfirmationFromQrAccount = jest.requireMock(
40+
'../../../../core/HardwareWallet/hooks/useIsConfirmationFromQrAccount',
41+
).useIsConfirmationFromQrAccount;
42+
2743
jest.mock('@react-navigation/native', () => ({
2844
...jest.requireActual('@react-navigation/native'),
2945
useNavigation: jest.fn(),
@@ -79,20 +95,23 @@ describe('useConfirmAction', () => {
7995
});
8096
});
8197

82-
it('call setScannerVisible if QR signing is in progress', async () => {
98+
it('sets signing confirmed and shows scanner when QR signing is in progress', async () => {
8399
const clearSecurityAlertResponseSpy = jest.spyOn(
84100
PPOMUtil,
85101
'clearSignatureSecurityAlertResponse',
86102
);
87103
const mockSetScannerVisible = jest.fn().mockResolvedValue(undefined);
104+
const mockSetSigningConfirmed = jest.fn();
88105
jest.spyOn(QRHardwareHook, 'useQRHardwareContext').mockReturnValue({
89106
isSigningQRObject: true,
90107
setScannerVisible: mockSetScannerVisible,
108+
setSigningConfirmed: mockSetSigningConfirmed,
91109
} as unknown as QRHardwareHook.QRHardwareContextType);
92110
const { result } = renderHookWithProvider(() => useConfirmActions(), {
93111
state: personalSignatureConfirmationState,
94112
});
95113
result?.current?.onConfirm();
114+
expect(mockSetSigningConfirmed).toHaveBeenCalledTimes(1);
96115
expect(mockSetScannerVisible).toHaveBeenCalledTimes(1);
97116
expect(mockSetScannerVisible).toHaveBeenLastCalledWith(true);
98117
expect(Engine.acceptPendingApproval).toHaveBeenCalledTimes(0);
@@ -101,6 +120,79 @@ describe('useConfirmAction', () => {
101120
expect(clearSecurityAlertResponseSpy).toHaveBeenCalledTimes(0);
102121
});
103122

123+
it('delegates to useQrConfirm when account is QR hardware and request is a transaction', async () => {
124+
mockIsConfirmationFromQrAccount.mockReturnValue(true);
125+
mockOnQrConfirm.mockClear();
126+
127+
const { result } = renderHookWithProvider(() => useConfirmActions(), {
128+
state: stakingDepositConfirmationState,
129+
});
130+
131+
await result?.current?.onConfirm();
132+
133+
expect(mockOnQrConfirm).toHaveBeenCalledTimes(1);
134+
expect(Engine.acceptPendingApproval).not.toHaveBeenCalled();
135+
136+
mockIsConfirmationFromQrAccount.mockReturnValue(false);
137+
});
138+
139+
it('delegates to useQrConfirm when account is QR hardware and request is a signature', async () => {
140+
mockIsConfirmationFromQrAccount.mockReturnValue(true);
141+
mockOnQrConfirm.mockClear();
142+
143+
const { result } = renderHookWithProvider(() => useConfirmActions(), {
144+
state: personalSignatureConfirmationState,
145+
});
146+
147+
await result?.current?.onConfirm();
148+
149+
expect(mockOnQrConfirm).toHaveBeenCalledTimes(1);
150+
expect(Engine.acceptPendingApproval).not.toHaveBeenCalled();
151+
152+
mockIsConfirmationFromQrAccount.mockReturnValue(false);
153+
});
154+
155+
it('calls setSigningConfirmed before executeApproval on default confirm path', async () => {
156+
const mockSetSigningConfirmed = jest.fn();
157+
jest.spyOn(QRHardwareHook, 'useQRHardwareContext').mockReturnValue({
158+
isSigningQRObject: false,
159+
setScannerVisible: jest.fn(),
160+
setSigningConfirmed: mockSetSigningConfirmed,
161+
} as unknown as QRHardwareHook.QRHardwareContextType);
162+
const { result } = renderHookWithProvider(() => useConfirmActions(), {
163+
state: personalSignatureConfirmationState,
164+
});
165+
result?.current?.onConfirm();
166+
expect(mockSetSigningConfirmed).toHaveBeenCalledTimes(1);
167+
expect(Engine.acceptPendingApproval).toHaveBeenCalledTimes(1);
168+
await flushPromises();
169+
});
170+
171+
it('calls setSigningConfirmed before confirming a transaction', async () => {
172+
const mockSetSigningConfirmed = jest.fn();
173+
const mockTransactionConfirm = jest.fn().mockResolvedValue(undefined);
174+
useTransactionConfirmMock.mockReturnValue({
175+
onConfirm: mockTransactionConfirm,
176+
});
177+
jest.spyOn(QRHardwareHook, 'useQRHardwareContext').mockReturnValue({
178+
isSigningQRObject: false,
179+
setScannerVisible: jest.fn(),
180+
setSigningConfirmed: mockSetSigningConfirmed,
181+
} as unknown as QRHardwareHook.QRHardwareContextType);
182+
183+
const { result } = renderHookWithProvider(() => useConfirmActions(), {
184+
state: stakingDepositConfirmationState,
185+
});
186+
187+
await result?.current?.onConfirm();
188+
189+
expect(mockSetSigningConfirmed).toHaveBeenCalledTimes(1);
190+
expect(mockTransactionConfirm).toHaveBeenCalledTimes(1);
191+
expect(mockSetSigningConfirmed.mock.invocationCallOrder[0]).toBeLessThan(
192+
mockTransactionConfirm.mock.invocationCallOrder[0],
193+
);
194+
});
195+
104196
it('delegates to useLedgerConfirm when account is Ledger', async () => {
105197
const { useIsConfirmationFromLedgerAccount } = jest.requireMock(
106198
'./useIsConfirmationFromLedgerAccount',

0 commit comments

Comments
 (0)