Skip to content

Commit ab4c4b7

Browse files
authored
fix: remove silent Ledger fallbacks in useHardwareWallet (MetaMask#27056)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **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? --> ## **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: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MUL-1532 ## **Manual testing steps** no manual testing steps ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **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** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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** > Removes multiple implicit `HardwareWalletType.Ledger` fallbacks across connection/error flows, which can change runtime behavior when `walletType` is unset and may surface new error/visibility paths. Risk is moderated by added unit tests but touches core hardware-wallet connection state handling. > > **Overview** > Eliminates silent defaulting to `HardwareWalletType.Ledger` when `walletType` is `null/undefined`, propagating `null` through error creation/parsing and transport/device-discovery handlers instead. > > Updates the connection flow to **require an explicit wallet type**: `ensureDeviceReady` now throws if neither `walletType` nor `targetWalletTypeRef` is available, and the hardware wallet bottom sheet **won’t render** unless `walletType` is set (even in active connection states). Adds targeted tests covering the new null-walletType behavior and edge cases (including null adapter/walletType error handling). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 96479ae. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent a092b62 commit ab4c4b7

12 files changed

Lines changed: 183 additions & 33 deletions

app/core/HardwareWallet/components/HardwareWalletBottomSheet/HardwareWalletBottomSheet.test.tsx

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,19 @@ describe('HardwareWalletBottomSheet', () => {
140140
expect(queryByTestId(HARDWARE_WALLET_BOTTOM_SHEET_TEST_ID)).toBeNull();
141141
});
142142

143+
it('does not render when walletType is null even if status is active', () => {
144+
mockConnectionState.status = ConnectionStatus.Scanning;
145+
const { queryByTestId } = render(
146+
<HardwareWalletBottomSheet
147+
{...createDefaultProps({ walletType: null })}
148+
/>,
149+
);
150+
151+
expect(
152+
queryByTestId(HARDWARE_WALLET_BOTTOM_SHEET_TEST_ID),
153+
).not.toBeOnTheScreen();
154+
});
155+
143156
it('renders when connected (polling may still be in progress)', () => {
144157
Object.assign(mockConnectionState, {
145158
status: ConnectionStatus.Connected,

app/core/HardwareWallet/components/HardwareWalletBottomSheet/HardwareWalletBottomSheet.tsx

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ export const HardwareWalletBottomSheet: React.FC<
8888
const { devices, selectedDevice, isScanning } = deviceSelection;
8989

9090
const shouldShow = useMemo(() => {
91+
if (!walletType) return false;
9192
switch (connectionState.status) {
9293
case ConnectionStatus.Scanning:
9394
case ConnectionStatus.Connecting:
@@ -100,7 +101,7 @@ export const HardwareWalletBottomSheet: React.FC<
100101
default:
101102
return false;
102103
}
103-
}, [connectionState.status]);
104+
}, [connectionState.status, walletType]);
104105

105106
useEffect(() => {
106107
DevLogger.log(
@@ -159,16 +160,13 @@ export const HardwareWalletBottomSheet: React.FC<
159160
onClose();
160161
}, [onClose]);
161162

162-
// The effective device type — only used when the sheet is visible,
163-
// so walletType should always be set by then.
164-
const deviceType = walletType ?? HardwareWalletType.Ledger;
165-
166163
const renderContent = () => {
164+
if (!walletType) return null;
167165
switch (connectionState.status) {
168166
case ConnectionStatus.Ready:
169167
return (
170168
<SuccessContent
171-
deviceType={deviceType}
169+
deviceType={walletType}
172170
onDismiss={handleSuccessDismiss}
173171
autoDismissMs={successAutoDismissMs}
174172
/>
@@ -180,7 +178,7 @@ export const HardwareWalletBottomSheet: React.FC<
180178
devices={devices}
181179
selectedDevice={selectedDevice ?? undefined}
182180
isScanning={isScanning}
183-
deviceType={deviceType}
181+
deviceType={walletType}
184182
onSelectDevice={handleSelectDevice}
185183
onConfirmSelection={handleConfirmDeviceSelection}
186184
onRescan={handleRescan}
@@ -190,12 +188,12 @@ export const HardwareWalletBottomSheet: React.FC<
190188

191189
case ConnectionStatus.Connecting:
192190
case ConnectionStatus.Connected:
193-
return <ConnectingContent deviceType={deviceType} />;
191+
return <ConnectingContent deviceType={walletType} />;
194192

195193
case ConnectionStatus.AwaitingApp:
196194
return (
197195
<AwaitingAppContent
198-
deviceType={deviceType}
196+
deviceType={walletType}
199197
requiredApp={connectionState.appName}
200198
onContinue={handleErrorContinue}
201199
/>
@@ -204,7 +202,7 @@ export const HardwareWalletBottomSheet: React.FC<
204202
case ConnectionStatus.AwaitingConfirmation:
205203
return (
206204
<AwaitingConfirmationContent
207-
deviceType={deviceType}
205+
deviceType={walletType}
208206
operationType={connectionState.operationType}
209207
onCancel={handleAwaitingConfirmationCancel}
210208
/>
@@ -214,7 +212,7 @@ export const HardwareWalletBottomSheet: React.FC<
214212
return (
215213
<ErrorContent
216214
error={connectionState.error}
217-
deviceType={deviceType}
215+
deviceType={walletType}
218216
onContinue={handleErrorContinue}
219217
onDismiss={handleErrorDismiss}
220218
/>

app/core/HardwareWallet/errors/factory.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ function getSDKErrorInfo(
5050
*/
5151
export function createHardwareWalletError(
5252
code: ErrorCode,
53-
walletType?: HardwareWalletType,
53+
walletType?: HardwareWalletType | null,
5454
technicalMessage?: string,
5555
options?: { cause?: Error; metadata?: Record<string, unknown> },
5656
): HardwareWalletError {

app/core/HardwareWallet/errors/mappings.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ interface MobileErrorExtension {
1111
recoveryAction: RecoveryAction;
1212
icon: IconName;
1313
iconColor: IconColor;
14-
getLocalizedTitle: (walletType?: HardwareWalletType) => string;
15-
getLocalizedMessage: (walletType?: HardwareWalletType) => string;
14+
getLocalizedTitle: (walletType?: HardwareWalletType | null) => string;
15+
getLocalizedMessage: (walletType?: HardwareWalletType | null) => string;
1616
}
1717

1818
/**

app/core/HardwareWallet/errors/parser.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ function extractStatusCode(error: unknown): number | null {
6060
*/
6161
function parseLedgerCommunicationError(
6262
error: LedgerCommunicationErrors,
63-
walletType: HardwareWalletType,
63+
walletType?: HardwareWalletType | null,
6464
): HardwareWalletError {
6565
switch (error) {
6666
case LedgerCommunicationErrors.LedgerDisconnected:
@@ -122,7 +122,7 @@ function parseLedgerCommunicationError(
122122
*/
123123
function parseLedgerStatusCode(
124124
statusCode: number,
125-
walletType: HardwareWalletType,
125+
walletType: HardwareWalletType | null | undefined,
126126
originalError?: Error,
127127
): HardwareWalletError {
128128
const hexCode = toHexStatusCode(statusCode);
@@ -163,7 +163,7 @@ function parseLedgerStatusCode(
163163
*/
164164
function parseErrorByName(
165165
error: Error,
166-
walletType: HardwareWalletType,
166+
walletType?: HardwareWalletType | null,
167167
): HardwareWalletError | null {
168168
const name = error.name;
169169

@@ -196,7 +196,7 @@ function parseErrorByName(
196196
*/
197197
function parseErrorByMessage(
198198
error: Error,
199-
walletType: HardwareWalletType,
199+
walletType?: HardwareWalletType | null,
200200
): HardwareWalletError | null {
201201
const message = error.message.toLowerCase();
202202
const name = error.name?.toLowerCase() ?? '';
@@ -285,7 +285,7 @@ function parseErrorByMessage(
285285
*/
286286
export function parseErrorByType(
287287
error: unknown,
288-
walletType: HardwareWalletType,
288+
walletType?: HardwareWalletType | null,
289289
): HardwareWalletError {
290290
if (error instanceof HardwareWalletError) {
291291
return error;

app/core/HardwareWallet/helpers.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { strings } from '../../../locales/i18n';
77
* Helper to get wallet type display name
88
*/
99
export const getHardwareWalletTypeName = (
10-
walletType?: HardwareWalletType,
10+
walletType?: HardwareWalletType | null,
1111
): string => {
1212
switch (walletType) {
1313
case HardwareWalletType.Ledger:
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import { renderHook, act } from '@testing-library/react-native';
2+
import { HardwareWalletType, ConnectionStatus } from '@metamask/hw-wallet-sdk';
3+
import { useDeviceConnectionFlow } from './useDeviceConnectionFlow';
4+
import {
5+
HardwareWalletRefs,
6+
HardwareWalletStateSetters,
7+
} from './useHardwareWalletStateManager';
8+
9+
const createMockRefs = (): HardwareWalletRefs => ({
10+
adapterRef: { current: null },
11+
isConnectingRef: { current: false },
12+
abortControllerRef: { current: null },
13+
targetWalletTypeRef: { current: null },
14+
});
15+
16+
const createMockSetters = (): HardwareWalletStateSetters => ({
17+
setConnectionState: jest.fn(),
18+
setDeviceId: jest.fn(),
19+
setTargetWalletType: jest.fn(),
20+
});
21+
22+
const createDefaultOptions = (overrides = {}) => ({
23+
refs: createMockRefs(),
24+
setters: createMockSetters(),
25+
walletType: HardwareWalletType.Ledger as HardwareWalletType | null,
26+
deviceId: null as string | null,
27+
handleError: jest.fn(),
28+
updateConnectionState: jest.fn(),
29+
createAdapterWithCallbacks: jest.fn(),
30+
initializeAdapter: jest.fn(),
31+
checkTransportEnabledOrShowError: jest.fn(),
32+
...overrides,
33+
});
34+
35+
describe('useDeviceConnectionFlow', () => {
36+
describe('ensureDeviceReady', () => {
37+
it('throws when no wallet type is available', async () => {
38+
const options = createDefaultOptions({ walletType: null });
39+
const { result } = renderHook(() => useDeviceConnectionFlow(options));
40+
41+
await expect(
42+
act(() => result.current.ensureDeviceReady()),
43+
).rejects.toThrow('ensureDeviceReady called without a wallet type');
44+
});
45+
46+
it('uses targetWalletTypeRef when walletType is null', async () => {
47+
const refs = createMockRefs();
48+
refs.targetWalletTypeRef.current = HardwareWalletType.Ledger;
49+
const mockAdapter = {
50+
walletType: HardwareWalletType.Ledger,
51+
resetFlowState: jest.fn(),
52+
isTransportAvailable: jest.fn().mockResolvedValue(true),
53+
startDeviceDiscovery: jest.fn(),
54+
stopDeviceDiscovery: jest.fn(),
55+
connect: jest.fn().mockResolvedValue(undefined),
56+
disconnect: jest.fn(),
57+
getConnectedDeviceId: jest.fn().mockReturnValue('device-123'),
58+
};
59+
const createAdapterWithCallbacks = jest.fn().mockReturnValue(mockAdapter);
60+
const options = createDefaultOptions({
61+
refs,
62+
walletType: null,
63+
deviceId: 'device-123',
64+
createAdapterWithCallbacks,
65+
checkTransportEnabledOrShowError: jest.fn().mockResolvedValue(false),
66+
});
67+
68+
const { result } = renderHook(() => useDeviceConnectionFlow(options));
69+
70+
let readyPromise: Promise<boolean>;
71+
await act(async () => {
72+
readyPromise = result.current.ensureDeviceReady('device-123');
73+
await Promise.resolve();
74+
});
75+
76+
expect(createAdapterWithCallbacks).toHaveBeenCalledWith(
77+
HardwareWalletType.Ledger,
78+
);
79+
80+
// Resolve the pending readiness promise so it doesn't leak
81+
await act(async () => {
82+
result.current.closeFlow();
83+
await readyPromise;
84+
});
85+
});
86+
});
87+
88+
describe('closeFlow', () => {
89+
it('clears targetWalletType', () => {
90+
const options = createDefaultOptions();
91+
const { result } = renderHook(() => useDeviceConnectionFlow(options));
92+
93+
act(() => {
94+
result.current.closeFlow();
95+
});
96+
97+
expect(options.setters.setTargetWalletType).toHaveBeenCalledWith(null);
98+
expect(options.updateConnectionState).toHaveBeenCalledWith({
99+
status: ConnectionStatus.Disconnected,
100+
});
101+
});
102+
});
103+
104+
describe('handleConnectionSuccess', () => {
105+
it('does not clear targetWalletType', () => {
106+
const options = createDefaultOptions();
107+
const { result } = renderHook(() => useDeviceConnectionFlow(options));
108+
109+
act(() => {
110+
result.current.handleConnectionSuccess();
111+
});
112+
113+
expect(options.setters.setTargetWalletType).not.toHaveBeenCalled();
114+
expect(options.updateConnectionState).toHaveBeenCalledWith({
115+
status: ConnectionStatus.Disconnected,
116+
});
117+
});
118+
});
119+
});

app/core/HardwareWallet/hooks/useDeviceConnectionFlow.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -195,10 +195,11 @@ export const useDeviceConnectionFlow = ({
195195
pendingReadyResolveRef.current = null;
196196
}
197197

198-
const targetType =
199-
refs.targetWalletTypeRef.current ??
200-
walletType ??
201-
HardwareWalletType.Ledger;
198+
const targetType = refs.targetWalletTypeRef.current ?? walletType;
199+
200+
if (!targetType) {
201+
throw new Error('ensureDeviceReady called without a wallet type');
202+
}
202203

203204
if (!targetDeviceId) {
204205
setters.setDeviceId(null);

app/core/HardwareWallet/hooks/useDeviceDiscovery.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export const useDeviceDiscovery = ({
8888
DevLogger.log('[HardwareWallet] Device discovery error:', error);
8989
const scanError = parseErrorByType(
9090
error,
91-
walletType ?? adapter.walletType ?? HardwareWalletType.Ledger,
91+
walletType ?? adapter.walletType,
9292
);
9393
updateConnectionState({
9494
status: ConnectionStatus.ErrorState,

app/core/HardwareWallet/hooks/useDeviceEventHandlers.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,30 @@ describe('useDeviceEventHandlers', () => {
423423

424424
expect(lastConnectionState.status).toBe(ConnectionStatus.ErrorState);
425425
});
426+
427+
it('handles error when both walletType and adapter walletType are null', () => {
428+
mockRefs.adapterRef.current = null;
429+
const { result } = createHook(null);
430+
431+
act(() => {
432+
result.current.handleError(new Error('Test'));
433+
});
434+
435+
expect(lastConnectionState.status).toBe(ConnectionStatus.ErrorState);
436+
});
437+
438+
it('handles DeviceLocked when both walletType and adapter walletType are null', () => {
439+
mockRefs.adapterRef.current = null;
440+
const { result } = createHook(null);
441+
442+
act(() => {
443+
result.current.handleDeviceEvent({
444+
event: DeviceEvent.DeviceLocked,
445+
});
446+
});
447+
448+
expect(lastConnectionState.status).toBe(ConnectionStatus.ErrorState);
449+
});
426450
});
427451

428452
describe('with null adapter', () => {

0 commit comments

Comments
 (0)