Skip to content

Commit 34ffb5c

Browse files
montelaidevmontelaidev
authored
feat: wire qr code errors (#29741)
## **Description** <!-- 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? --> Wires the QR scan errors from PR 2 into pairing and signing flows. QR scan failures now surface through the hardware wallet bottom sheet with retry support, signing confirmations can reopen the scanner from the error CTA, and replacement transaction gas params are normalized through the shared helper. This is PR 3 of 3 and is stacked on #29388. ## **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: Improved retry behavior when QR hardware wallet signing scans fail ## **Related issues** Refs: MUL-1665 Resolves: https://consensyssoftware.atlassian.net/browse/MUL-1513?atlOrigin=eyJpIjoiZThlODA2MTIwODU4NDA5ZmI1YjdlMjJkM2Q2ODdlOWQiLCJwIjoiaiJ9 ## **Manual testing steps** ```gherkin Feature: QR hardware signing retry Scenario: user retries a failed QR signing scan Given the user is signing with a QR-based hardware wallet When the user scans an invalid QR code Then the hardware wallet bottom sheet displays the QR scan error When the user taps Try again Then the QR scanner reopens for another signing scan ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** ### **After** https://github.com/user-attachments/assets/f89bbb3f-d9a4-44c4-a99b-2564c315bf52 ## **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) - [x] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [x] 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 - [x] 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. Made with [Cursor](https://cursor.com) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches QR hardware-wallet signing/pairing flows and error-handling/retry behavior, which can impact user ability to connect/sign if regressions occur. Changes are mostly additive/guarded and covered by expanded unit tests, but they alter control flow around modal visibility and error deduping. > > **Overview** > Routes QR-hardware scan failures (non-UR, decode errors, wrong UR type, scan exceptions) out of `AnimatedQRScannerModal` into the Hardware Wallet bottom sheet via a new `useQrScanErrorForwarding` hook, enabling *Try again* retry to reopen scanners in pairing (`ConnectQRHardware`), signing (`QRSigningDetails`), and confirmation QR signing (`QRInfo`). > > Hardens scanner behavior by deduplicating repeated error frames (`isSameScanError`), avoiding repeated decoder resets/analytics, and resetting decoder/dedup state on `visible` transitions; also makes analytics sending resilient to failures. > > Improves QR transport handling by centralizing camera permission status constants (`CAMERA_PERMISSION_STATUS`) and updating `QRWalletAdapter` to request permission for any non-granted status; adds adapter factory support for `HardwareWalletType.Qr` and normalizes replacement-tx gas param typing via shared `ReplacementTxParams`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 49f5ad6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 8cfa941 commit 34ffb5c

23 files changed

Lines changed: 1534 additions & 69 deletions

‎app/__mocks__/react-native-vision-camera.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import React from 'react';
2+
import { CAMERA_PERMISSION_STATUS } from '../constants/permissions';
23

34
const mockDevice = {
45
id: 'back',
@@ -21,7 +22,9 @@ const mockDevice = {
2122

2223
const mockPermission = {
2324
hasPermission: true,
24-
requestPermission: jest.fn().mockResolvedValue('granted'),
25+
requestPermission: jest
26+
.fn()
27+
.mockResolvedValue(CAMERA_PERMISSION_STATUS.granted),
2528
};
2629

2730
let capturedOnCodeScanned:
@@ -76,8 +79,10 @@ const useCodeScanner = jest.fn((config) => {
7679
// Static methods on Camera - using Object.assign to avoid TypeScript issues
7780
// eslint-disable-next-line @typescript-eslint/no-explicit-any
7881
(Object as any).assign(Camera, {
79-
getCameraPermissionStatus: jest.fn(() => 'granted'),
80-
requestCameraPermission: jest.fn().mockResolvedValue('granted'),
82+
getCameraPermissionStatus: jest.fn(() => CAMERA_PERMISSION_STATUS.granted),
83+
requestCameraPermission: jest
84+
.fn()
85+
.mockResolvedValue(CAMERA_PERMISSION_STATUS.granted),
8186
});
8287

8388
export { Camera, useCameraDevice, useCameraPermission, useCodeScanner };

‎app/components/UI/QRHardware/AnimatedQRScanner.test.tsx‎

Lines changed: 315 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,27 @@ describe('AnimatedQRScannerModal - Metrics', () => {
128128
});
129129
};
130130

131+
const scanNonUR = () =>
132+
mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
133+
134+
function setupSuccessfulDecoder(urType = SUPPORTED_UR_TYPE.CRYPTO_HDKEY) {
135+
const instance = {
136+
receivePart: jest.fn(),
137+
getProgress: jest.fn(() => 1),
138+
isError: jest.fn(() => false),
139+
isSuccess: jest.fn(() => true),
140+
resultError: jest.fn(),
141+
resultUR: jest.fn(() => ({
142+
type: urType,
143+
cbor: Buffer.from([]),
144+
})),
145+
};
146+
mockURRegistryDecoder.mockImplementation(
147+
() => instance as unknown as URRegistryDecoder,
148+
);
149+
return instance;
150+
}
151+
131152
beforeEach(() => {
132153
jest.clearAllMocks();
133154
mockBuild.mockReturnValue({});
@@ -483,6 +504,48 @@ describe('AnimatedQRScannerModal - Metrics', () => {
483504
expect(mockTrackEvent).not.toHaveBeenCalled();
484505
});
485506

507+
it('resumes scanning when external QR hardware error callback keeps modal open', async () => {
508+
const validDecoderInstance = {
509+
receivePart: jest.fn(),
510+
getProgress: jest.fn(() => 1),
511+
isError: jest.fn(() => false),
512+
isSuccess: jest.fn(() => true),
513+
resultError: jest.fn(),
514+
resultUR: jest.fn(() => ({
515+
type: SUPPORTED_UR_TYPE.CRYPTO_HDKEY,
516+
cbor: Buffer.from([]),
517+
})),
518+
};
519+
520+
mockURRegistryDecoder.mockImplementation(
521+
() => validDecoderInstance as unknown as URRegistryDecoder,
522+
);
523+
524+
render(
525+
<AnimatedQRScannerModal
526+
{...defaultProps}
527+
onQRHardwareScanError={mockOnQRHardwareScanError}
528+
/>,
529+
);
530+
531+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
532+
533+
expect(mockOnQRHardwareScanError).toHaveBeenCalledWith(
534+
expect.objectContaining({
535+
message: 'Scanned QR code is not in UR format',
536+
}),
537+
);
538+
539+
await mockOnCodeScanned([{ value: 'ur:crypto-hdkey/1-1', type: 'qr' }]);
540+
541+
await waitFor(() => {
542+
expect(mockOnScanSuccess).toHaveBeenCalledWith({
543+
type: SUPPORTED_UR_TYPE.CRYPTO_HDKEY,
544+
cbor: Buffer.from([]),
545+
});
546+
});
547+
});
548+
486549
it('resumes scanning after forwarding an existing inline error to an external callback', async () => {
487550
const validDecoderInstance = {
488551
receivePart: jest.fn(),
@@ -544,7 +607,7 @@ describe('AnimatedQRScannerModal - Metrics', () => {
544607
});
545608
});
546609

547-
it('invokes QR hardware scan error callback once for repeated error frames', async () => {
610+
it('does not track metrics for repeated error frames after deduplicating QR hardware scan error callback', async () => {
548611
render(
549612
<AnimatedQRScannerModal
550613
{...defaultProps}
@@ -565,6 +628,111 @@ describe('AnimatedQRScannerModal - Metrics', () => {
565628
expect(mockAddProperties).toHaveBeenCalledTimes(1);
566629
});
567630

631+
it('does not reset the decoder for repeated error frames after forwarding QR hardware scan error', async () => {
632+
render(
633+
<AnimatedQRScannerModal
634+
{...defaultProps}
635+
onQRHardwareScanError={mockOnQRHardwareScanError}
636+
/>,
637+
);
638+
expect(mockURRegistryDecoder).toHaveBeenCalledTimes(1);
639+
640+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
641+
642+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(1);
643+
const decoderCallsAfterFirstError =
644+
mockURRegistryDecoder.mock.calls.length;
645+
646+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
647+
648+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(1);
649+
expect(mockURRegistryDecoder).toHaveBeenCalledTimes(
650+
decoderCallsAfterFirstError,
651+
);
652+
});
653+
654+
it('forwards the same QR hardware scan error again after the scanner reopens', async () => {
655+
const { rerender } = render(
656+
<AnimatedQRScannerModal
657+
{...defaultProps}
658+
onQRHardwareScanError={mockOnQRHardwareScanError}
659+
/>,
660+
);
661+
662+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
663+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(1);
664+
665+
rerender(
666+
<AnimatedQRScannerModal
667+
{...defaultProps}
668+
visible={false}
669+
onQRHardwareScanError={mockOnQRHardwareScanError}
670+
/>,
671+
);
672+
rerender(
673+
<AnimatedQRScannerModal
674+
{...defaultProps}
675+
visible
676+
onQRHardwareScanError={mockOnQRHardwareScanError}
677+
/>,
678+
);
679+
680+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
681+
682+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(2);
683+
});
684+
685+
it('forwards the same error again after a successful scan clears dedup state', async () => {
686+
setupSuccessfulDecoder();
687+
688+
render(
689+
<AnimatedQRScannerModal
690+
{...defaultProps}
691+
onQRHardwareScanError={mockOnQRHardwareScanError}
692+
/>,
693+
);
694+
695+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
696+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(1);
697+
698+
await mockOnCodeScanned([{ value: 'ur:crypto-hdkey/1-1', type: 'qr' }]);
699+
await waitFor(() => {
700+
expect(mockOnScanSuccess).toHaveBeenCalledTimes(1);
701+
});
702+
703+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
704+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(2);
705+
});
706+
707+
it('forwards different error types independently even when one was already forwarded', async () => {
708+
setupSuccessfulDecoder(SUPPORTED_UR_TYPE.ETH_SIGNATURE);
709+
710+
render(
711+
<AnimatedQRScannerModal
712+
{...defaultProps}
713+
onQRHardwareScanError={mockOnQRHardwareScanError}
714+
/>,
715+
);
716+
717+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
718+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(1);
719+
expect(mockOnQRHardwareScanError).toHaveBeenLastCalledWith(
720+
expect.objectContaining({
721+
message: 'Scanned QR code is not in UR format',
722+
}),
723+
);
724+
725+
await mockOnCodeScanned([
726+
{ value: 'ur:crypto-account/mock-part', type: 'qr' },
727+
]);
728+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(2);
729+
expect(mockOnQRHardwareScanError).toHaveBeenLastCalledWith(
730+
expect.objectContaining({
731+
message: 'Received UR type is not valid for pairing flow',
732+
}),
733+
);
734+
});
735+
568736
it('reopens the scanner when try again is pressed', async () => {
569737
const validDecoderInstance = {
570738
receivePart: jest.fn(),
@@ -690,6 +858,28 @@ describe('AnimatedQRScannerModal - Metrics', () => {
690858
});
691859
});
692860

861+
it('handles analytics failure gracefully during QR scan error', async () => {
862+
const { withQrKeyring } = jest.requireMock(
863+
'../../../core/QrKeyring/QrKeyring',
864+
);
865+
const originalImpl = withQrKeyring.getMockImplementation();
866+
withQrKeyring.mockRejectedValue(new Error('analytics failure'));
867+
868+
render(<AnimatedQRScannerModal {...defaultProps} />);
869+
870+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
871+
872+
await waitFor(() => {
873+
expect(mockAddProperties).toHaveBeenCalledWith(
874+
expect.objectContaining({
875+
device_model: 'Unknown',
876+
}),
877+
);
878+
});
879+
880+
withQrKeyring.mockImplementation(originalImpl);
881+
});
882+
693883
it('does not process QR code when codes array is empty', async () => {
694884
const mockDecoderInstance = {
695885
receivePart: jest.fn(),
@@ -1219,6 +1409,130 @@ describe('AnimatedQRScannerModal - Metrics', () => {
12191409
});
12201410
});
12211411

1412+
describe('showScannerError scanErrorActiveRef guard', () => {
1413+
it('ignores scan errors while an inline error is already being displayed', async () => {
1414+
const { getByText } = render(
1415+
<AnimatedQRScannerModal {...defaultProps} />,
1416+
);
1417+
1418+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
1419+
1420+
await waitFor(() => {
1421+
expect(
1422+
getByText(
1423+
'hardware_wallet.qr_scan_errors.non_ur_qr_scanned.pair.title',
1424+
),
1425+
).toBeOnTheScreen();
1426+
});
1427+
1428+
const decoderCallsAfterFirstError =
1429+
mockURRegistryDecoder.mock.calls.length;
1430+
1431+
await mockOnCodeScanned([{ value: 'not-a-ur', type: 'qr' }]);
1432+
1433+
expect(mockURRegistryDecoder).toHaveBeenCalledTimes(
1434+
decoderCallsAfterFirstError,
1435+
);
1436+
expect(mockTrackEvent).toHaveBeenCalledTimes(1);
1437+
});
1438+
});
1439+
1440+
describe('Reset on visible transition', () => {
1441+
it('resets decoder and dedup state when scanner transitions from hidden to visible', async () => {
1442+
setupSuccessfulDecoder();
1443+
1444+
const { rerender } = render(
1445+
<AnimatedQRScannerModal
1446+
{...defaultProps}
1447+
onQRHardwareScanError={mockOnQRHardwareScanError}
1448+
/>,
1449+
);
1450+
1451+
await scanNonUR();
1452+
expect(mockOnQRHardwareScanError).toHaveBeenCalledTimes(1);
1453+
1454+
rerender(
1455+
<AnimatedQRScannerModal
1456+
{...defaultProps}
1457+
visible={false}
1458+
onQRHardwareScanError={mockOnQRHardwareScanError}
1459+
/>,
1460+
);
1461+
rerender(
1462+
<AnimatedQRScannerModal
1463+
{...defaultProps}
1464+
visible
1465+
onQRHardwareScanError={mockOnQRHardwareScanError}
1466+
/>,
1467+
);
1468+
1469+
await waitFor(() => {
1470+
expect(mockURRegistryDecoder.mock.calls.length).toBeGreaterThan(1);
1471+
});
1472+
1473+
await mockOnCodeScanned([
1474+
{ value: 'ur:crypto-hdkey/mock-part', type: 'qr' },
1475+
]);
1476+
1477+
await waitFor(() => {
1478+
expect(mockOnScanSuccess).toHaveBeenCalledWith({
1479+
type: SUPPORTED_UR_TYPE.CRYPTO_HDKEY,
1480+
cbor: Buffer.from([]),
1481+
});
1482+
});
1483+
});
1484+
1485+
it('clears inline scan error when scanner transitions from hidden to visible', async () => {
1486+
const errorTitle =
1487+
'hardware_wallet.qr_scan_errors.non_ur_qr_scanned.pair.title';
1488+
const { getByText, queryByText, rerender } = render(
1489+
<AnimatedQRScannerModal {...defaultProps} />,
1490+
);
1491+
1492+
await scanNonUR();
1493+
1494+
await waitFor(() => {
1495+
expect(getByText(errorTitle)).toBeOnTheScreen();
1496+
});
1497+
1498+
rerender(<AnimatedQRScannerModal {...defaultProps} visible={false} />);
1499+
rerender(<AnimatedQRScannerModal {...defaultProps} visible />);
1500+
1501+
await waitFor(() => {
1502+
expect(queryByText(errorTitle)).toBeNull();
1503+
});
1504+
});
1505+
1506+
it('does not reset when visible stays true across rerenders', async () => {
1507+
const mockDecoderInstance = {
1508+
receivePart: jest.fn(),
1509+
getProgress: jest.fn(() => 0.5),
1510+
isError: jest.fn(() => false),
1511+
isSuccess: jest.fn(() => false),
1512+
resultError: jest.fn(),
1513+
resultUR: jest.fn(),
1514+
};
1515+
1516+
mockURRegistryDecoder.mockImplementation(
1517+
() => mockDecoderInstance as unknown as URRegistryDecoder,
1518+
);
1519+
1520+
const { rerender } = render(<AnimatedQRScannerModal {...defaultProps} />);
1521+
1522+
await mockOnCodeScanned([
1523+
{ value: 'ur:crypto-hdkey/mock-part', type: 'qr' },
1524+
]);
1525+
1526+
const decoderCallsAfterScan = mockURRegistryDecoder.mock.calls.length;
1527+
1528+
rerender(<AnimatedQRScannerModal {...defaultProps} />);
1529+
1530+
expect(mockURRegistryDecoder).toHaveBeenCalledTimes(
1531+
decoderCallsAfterScan,
1532+
);
1533+
});
1534+
});
1535+
12221536
describe('showScannerError without onQRHardwareScanError callback', () => {
12231537
it('renders inline error UI when onQRHardwareScanError is not provided', async () => {
12241538
const { getByText, getByTestId } = render(

0 commit comments

Comments
 (0)