Skip to content

Commit a8a95d8

Browse files
runway-github[bot]agangladagambinish
authored
chore(runway): cherry-pick fix: hw account abstraction migration (#30261)
- fix: hw account abstraction migration (#30114) <!-- 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 prevents Perps from triggering browsing-time signing flows for hardware wallet users while the screen is idle. Previously, Unified Account setup only deferred the `dexAbstraction` migration when user signing was not allowed. Other signing-backed HyperLiquid abstraction modes could still attempt setup during passive Perps initialization, which could surface repeated hardware wallet prompts without an explicit user action. The fix adds a shared helper for deciding when Unified Account setup must be deferred, applies it during HyperLiquid provider initialization, and expands hardware wallet detection to cover MetaMask's hardware keyring types: Ledger, Trezor, OneKey, Lattice, and QR hardware wallets. Software wallets can still complete setup during initialization so the first trade sees unified collateral, while hardware wallets defer setup until an explicit trading or withdrawal action. ## **Changelog** CHANGELOG entry: Fixed a bug that could repeatedly prompt hardware wallet users while Perps was idle. ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: Perps hardware wallet idle setup Scenario: Hardware wallet user opens Perps and leaves it idle Given the user has selected a Ledger, Trezor, OneKey, Lattice, or QR hardware wallet account And Perps is enabled and available When the user opens the Perps screen And leaves the Perps screen idle without placing an order, withdrawing, or otherwise starting a signing action Then the app does not repeatedly show hardware wallet signing prompts ``` ```gherkin Feature: Perps software wallet setup Scenario: Software wallet user opens Perps Given the user has selected a software wallet account that needs Unified Account setup When the user opens the Perps screen Then Perps can complete the setup flow during initialization And the first trade can use unified collateral without an extra setup step ``` ## **Screenshots/Recordings** N/A. This is a controller behavior change with no intended UI changes. ### **Before** N/A ### **After** N/A ## **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`. --> - [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** > Changes when unified-account migration and other signing-backed setup runs, which can affect Perps initialization and migration behavior for hardware wallet users. Risk is moderated by added unit tests and a narrow decision helper, but incorrect deferral could delay required setup until action time. > > **Overview** > Prevents browsing-time hardware wallet signing prompts by deferring HyperLiquid unified-account migration during Perps initialization whenever the current abstraction mode would require a signing-backed transition and user signing is not allowed. > > Adds `shouldDeferUnifiedAccountSetup` to centralize this decision (with new unit tests), updates `HyperLiquidProvider` to use it, and expands `HyperLiquidWalletService.isSelectedHardwareWallet()` to recognize additional MetaMask hardware keyring types (Ledger, Trezor, OneKey, Lattice, QR). Also updates related tests and log/comment wording from *QR popup spam* to *hardware wallet signing prompt spam*. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6f8cfe2. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> [4669b3a](4669b3a) --------- Co-authored-by: Alejandro Garcia Anglada <aganglada@gmail.com> Co-authored-by: Nicholas Gambino <nicholas.gambino@consensys.net>
1 parent 3e598ba commit a8a95d8

8 files changed

Lines changed: 142 additions & 71 deletions

app/controllers/perps/providers/HyperLiquidProvider.test.ts

Lines changed: 34 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -5963,7 +5963,7 @@ describe('HyperLiquidProvider', () => {
59635963
const result = await provider.placeOrder(orderParams);
59645964

59655965
// PR #25334: Builder fee approval is now non-blocking (fire-and-forget)
5966-
// to prevent QR popup spam for hardware wallets.
5966+
// to prevent repeated signing prompts for hardware wallets.
59675967
// Order should proceed even if builder fee approval fails.
59685968
expect(result.success).toBe(true);
59695969
expect(result.orderId).toBeDefined();
@@ -8957,12 +8957,12 @@ describe('HyperLiquidProvider', () => {
89578957
});
89588958

89598959
// ─────────────────────────────────────────────────
8960-
// dexAbstraction → unifiedAccount migration on init
8960+
// Signing-backed unifiedAccount migration on init
89618961
//
8962-
// The transition requires an EIP-712 prompt (HL blocks the agent path),
8963-
// so software-wallet users migrate during initial setup to ensure the
8964-
// first trade sees unified collateral. Hardware wallets remain deferred
8965-
// to avoid QR / Ledger prompt spam while browsing.
8962+
// Some transitions require an EIP-712 prompt, so software-wallet users
8963+
// migrate during initial setup to ensure the first trade sees unified
8964+
// collateral. Hardware wallets remain deferred to avoid repeated signing
8965+
// prompts while browsing.
89668966
// ─────────────────────────────────────────────────
89678967

89688968
it('calls userSetAbstraction on init for software-wallet dexAbstraction users', async () => {
@@ -9100,33 +9100,36 @@ describe('HyperLiquidProvider', () => {
91009100
).toHaveBeenCalledWith(USER_ADDRESS, 'unifiedAccount');
91019101
});
91029102

9103-
it('defers dexAbstraction migration on init for hardware wallets', async () => {
9104-
// Arrange
9105-
mockWalletService.isSelectedHardwareWallet.mockReturnValue(true);
9106-
const mockExchangeClient = createMockExchangeClient();
9107-
mockClientService.getInfoClient = jest.fn().mockReturnValue(
9108-
createMockInfoClient({
9109-
userAbstraction: jest.fn().mockResolvedValue('dexAbstraction'),
9110-
}),
9111-
);
9112-
mockClientService.getExchangeClient = jest
9113-
.fn()
9114-
.mockReturnValue(mockExchangeClient);
9103+
it.each(['dexAbstraction', 'default', 'disabled'] as const)(
9104+
'defers %s migration on init for hardware wallets',
9105+
async (currentMode) => {
9106+
// Arrange
9107+
mockWalletService.isSelectedHardwareWallet.mockReturnValue(true);
9108+
const mockExchangeClient = createMockExchangeClient();
9109+
mockClientService.getInfoClient = jest.fn().mockReturnValue(
9110+
createMockInfoClient({
9111+
userAbstraction: jest.fn().mockResolvedValue(currentMode),
9112+
}),
9113+
);
9114+
mockClientService.getExchangeClient = jest
9115+
.fn()
9116+
.mockReturnValue(mockExchangeClient);
91159117

9116-
// Act - init path
9117-
await provider.getMarketDataWithPrices();
9118+
// Act - init path
9119+
await provider.getMarketDataWithPrices();
91189120

9119-
// Assert - no browsing-time hardware prompt; action-time setup can still run.
9120-
expect(mockExchangeClient.userSetAbstraction).not.toHaveBeenCalled();
9121-
expect(mockExchangeClient.agentSetAbstraction).not.toHaveBeenCalled();
9122-
expect(
9123-
(TradingReadinessCache as jest.Mocked<typeof TradingReadinessCache>)
9124-
.set,
9125-
).not.toHaveBeenCalled();
9126-
expect(
9127-
mockSubscriptionService.setUserAbstractionMode,
9128-
).not.toHaveBeenCalled();
9129-
});
9121+
// Assert - no browsing-time hardware prompt; action-time setup can still run.
9122+
expect(mockExchangeClient.userSetAbstraction).not.toHaveBeenCalled();
9123+
expect(mockExchangeClient.agentSetAbstraction).not.toHaveBeenCalled();
9124+
expect(
9125+
(TradingReadinessCache as jest.Mocked<typeof TradingReadinessCache>)
9126+
.set,
9127+
).not.toHaveBeenCalled();
9128+
expect(
9129+
mockSubscriptionService.setUserAbstractionMode,
9130+
).not.toHaveBeenCalled();
9131+
},
9132+
);
91309133

91319134
it('does NOT call setUserAbstractionMode when migration fails', async () => {
91329135
const mockExchangeClient = createMockExchangeClient();

app/controllers/perps/providers/HyperLiquidProvider.ts

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ import {
128128
addSpotBalanceToAccountState,
129129
aggregateAccountStates,
130130
} from '../utils/accountUtils';
131-
import { ensureError } from '../utils/errorUtils';
131+
import { ensureError, isKeyringLockedError } from '../utils/errorUtils';
132+
import { shouldDeferUnifiedAccountSetup } from '../utils/hyperLiquidAbstraction';
132133
import {
133134
adaptAccountStateFromSDK,
134135
adaptHyperLiquidLedgerUpdateToUserHistoryItem,
@@ -627,7 +628,8 @@ export class HyperLiquidProvider implements PerpsProvider {
627628
const network = this.#clientService.isTestnetMode() ? 'testnet' : 'mainnet';
628629

629630
// Check global cache first to avoid repeated signing requests
630-
// This is CRITICAL for hardware wallets to prevent QR popup spam
631+
// This is CRITICAL for hardware wallets to prevent repeated signing prompts
632+
// while browsing.
631633
const cachedStatus = TradingReadinessCache.get(network, userAddress);
632634
if (cachedStatus?.attempted) {
633635
this.#deps.debugLogger.log(
@@ -726,14 +728,14 @@ export class HyperLiquidProvider implements PerpsProvider {
726728
return;
727729
}
728730

729-
// Defer the user-signed transition until the user attempts an action.
731+
// Defer signing-backed transitions until the user attempts an action.
730732
// Cache is intentionally left untouched so the next entry re-evaluates;
731733
// the read-only userAbstraction call is cheap and gated by the in-flight
732734
// lock, preventing concurrent prompts.
733-
if (currentMode === 'dexAbstraction' && !allowUserSigning) {
735+
if (shouldDeferUnifiedAccountSetup(currentMode, allowUserSigning)) {
734736
this.#deps.debugLogger.log(
735-
'HyperLiquidProvider: Deferring dexAbstraction → unifiedAccount migration to action time',
736-
{ user: userAddress, network },
737+
'HyperLiquidProvider: Deferring unified account migration to action time',
738+
{ user: userAddress, network, mode: currentMode },
737739
);
738740
completeInFlight();
739741
return;
@@ -815,7 +817,7 @@ export class HyperLiquidProvider implements PerpsProvider {
815817
completeInFlight();
816818
} catch (error) {
817819
// If keyring is locked, don't cache so it retries when unlocked
818-
if (ensureError(error).message === PERPS_ERROR_CODES.KEYRING_LOCKED) {
820+
if (isKeyringLockedError(error)) {
819821
this.#deps.debugLogger.log(
820822
'[ensureUnifiedAccountEnabled] Keyring locked, will retry later',
821823
);
@@ -927,10 +929,10 @@ export class HyperLiquidProvider implements PerpsProvider {
927929
}
928930

929931
// Attempt Unified Account migration as early as possible so users aren't
930-
// blocked when they try to trade. Software-wallet dexAbstraction users can
931-
// complete the one-time EIP-712 migration during initial setup so the first
932-
// trade sees the unified balance. Hardware wallets remain deferred to
933-
// action time to avoid QR / Ledger prompt spam while browsing.
932+
// blocked when they try to trade. Software wallets can complete the
933+
// signing-backed migration during initial setup so the first trade sees
934+
// the unified balance. Hardware wallets remain deferred to action time to
935+
// avoid repeated signing prompts while browsing.
934936
await this.#ensureUnifiedAccountEnabled({
935937
allowUserSigning: !this.#walletService.isSelectedHardwareWallet(),
936938
});
@@ -963,7 +965,7 @@ export class HyperLiquidProvider implements PerpsProvider {
963965
* - Builder fee approval (required for orders)
964966
* - Referral code setup (attribution)
965967
*
966-
* These operations are DEFERRED from ensureReady() to avoid QR popup spam
968+
* These operations are DEFERRED from ensureReady() to avoid hardware wallet prompt spam
967969
* when users are just viewing the Perps section (critical for hardware wallets).
968970
*
969971
* Call this method before any trading operation (placeOrder, cancelOrder, etc.)
@@ -2542,11 +2544,12 @@ export class HyperLiquidProvider implements PerpsProvider {
25422544
const cacheKey = this.#getCacheKey(network, userAddress);
25432545

25442546
// Check GLOBAL cache first to avoid repeated signing requests across reconnections
2545-
// This is CRITICAL for hardware wallets to prevent QR popup spam
2547+
// This is CRITICAL for hardware wallets to prevent repeated signing prompts
2548+
// while browsing.
25462549
const globalCached = PerpsSigningCache.getBuilderFee(network, userAddress);
25472550
if (globalCached?.attempted) {
25482551
this.#deps.debugLogger.log(
2549-
'[ensureBuilderFeeApproval] Using global cache (prevents QR popup spam)',
2552+
'[ensureBuilderFeeApproval] Using global cache (prevents hardware wallet prompt spam)',
25502553
{ network, success: globalCached.success },
25512554
);
25522555
if (globalCached.success) {
@@ -8377,7 +8380,7 @@ export class HyperLiquidProvider implements PerpsProvider {
83778380
const globalCached = PerpsSigningCache.getReferral(network, userAddress);
83788381
if (globalCached?.attempted) {
83798382
this.#deps.debugLogger.log(
8380-
'[ensureReferralSet] Using global cache (prevents QR popup spam)',
8383+
'[ensureReferralSet] Using global cache (prevents hardware wallet prompt spam)',
83818384
{ network, success: globalCached.success },
83828385
);
83838386
return;

app/controllers/perps/services/HyperLiquidWalletService.test.ts

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,13 @@ describe('HyperLiquidWalletService', () => {
322322
expect(service.isSelectedHardwareWallet()).toBe(false);
323323
});
324324

325-
it('returns true for Ledger hardware wallet', () => {
325+
it.each([
326+
'Ledger Hardware',
327+
'Trezor Hardware',
328+
'OneKey Hardware',
329+
'Lattice Hardware',
330+
'QR Hardware Wallet Device',
331+
])('returns true for %s wallet', (keyringType) => {
326332
(mockMessenger.call as jest.Mock).mockImplementation((action: string) => {
327333
if (
328334
action === 'AccountTreeController:getAccountsFromSelectedAccountGroup'
@@ -332,28 +338,7 @@ describe('HyperLiquidWalletService', () => {
332338
...mockEvmAccount,
333339
metadata: {
334340
...mockEvmAccount.metadata,
335-
keyring: { type: 'Ledger Hardware' },
336-
},
337-
},
338-
];
339-
}
340-
return undefined;
341-
});
342-
343-
expect(service.isSelectedHardwareWallet()).toBe(true);
344-
});
345-
346-
it('returns true for QR hardware wallet', () => {
347-
(mockMessenger.call as jest.Mock).mockImplementation((action: string) => {
348-
if (
349-
action === 'AccountTreeController:getAccountsFromSelectedAccountGroup'
350-
) {
351-
return [
352-
{
353-
...mockEvmAccount,
354-
metadata: {
355-
...mockEvmAccount.metadata,
356-
keyring: { type: 'QR Hardware Wallet Device' },
341+
keyring: { type: keyringType },
357342
},
358343
},
359344
];

app/controllers/perps/services/HyperLiquidWalletService.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ import { findEvmAccount, getSelectedEvmAccount } from '../utils/accountUtils';
1818
// service portable between mobile and the core monorepo.
1919
const HARDWARE_KEYRING_TYPES = new Set<string>([
2020
'Ledger Hardware',
21+
'Trezor Hardware',
22+
'OneKey Hardware',
23+
'Lattice Hardware',
2124
'QR Hardware Wallet Device',
2225
]);
2326

@@ -55,7 +58,7 @@ export class HyperLiquidWalletService {
5558
/**
5659
* Check whether the selected EVM account is backed by hardware.
5760
*
58-
* @returns True for Ledger / QR hardware keyrings; false for software accounts.
61+
* @returns True for MetaMask hardware keyrings; false for software accounts.
5962
*/
6063
public isSelectedHardwareWallet(): boolean {
6164
const selectedEvmAccount = findEvmAccount(

app/controllers/perps/services/TradingReadinessCache.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@
22
* Global singleton cache for Perps signing operations
33
*
44
* This cache persists across provider reconnections to prevent repeated
5-
* signing requests for hardware wallets. Critical for preventing QR popup spam.
5+
* signing requests for hardware wallets. Critical for preventing repeated
6+
* hardware wallet signing prompts.
67
*
78
* Cache is intentionally kept separate from provider instances because providers
89
* are recreated on account/network changes, which would reset instance-level caches.

app/controllers/perps/utils/errorUtils.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
*/
55
import { hasProperty } from '@metamask/utils';
66

7+
import { PERPS_ERROR_CODES } from '../perpsErrorCodes';
8+
79
/**
810
* Detects expected cancellation/abort errors that should not be reported to Sentry.
911
* These occur during normal navigation or view teardown when in-flight fetch requests
@@ -23,6 +25,30 @@ export function isAbortError(error: unknown): boolean {
2325
return false;
2426
}
2527

28+
/**
29+
* Detects keyring-locked errors, including SDK-wrapped errors that preserve the
30+
* original error in `cause`.
31+
*
32+
* @param error - The error to check.
33+
* @returns True if any error in the cause chain is KEYRING_LOCKED.
34+
*/
35+
export function isKeyringLockedError(error: unknown): boolean {
36+
let current: unknown = error;
37+
const seen = new Set<unknown>();
38+
39+
while (current instanceof Error && !seen.has(current)) {
40+
seen.add(current);
41+
42+
if (current.message === PERPS_ERROR_CODES.KEYRING_LOCKED) {
43+
return true;
44+
}
45+
46+
current = (current as { cause?: unknown }).cause;
47+
}
48+
49+
return false;
50+
}
51+
2652
/**
2753
* Ensures we have a proper Error object for logging.
2854
* Converts unknown/string errors to proper Error instances.
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { shouldDeferUnifiedAccountSetup } from './hyperLiquidAbstraction';
2+
3+
describe('shouldDeferUnifiedAccountSetup', () => {
4+
it.each(['dexAbstraction', 'default', 'disabled'] as const)(
5+
'defers %s setup when signing is not allowed',
6+
(currentMode) => {
7+
expect(shouldDeferUnifiedAccountSetup(currentMode, false)).toBe(true);
8+
},
9+
);
10+
11+
it.each(['dexAbstraction', 'default', 'disabled'] as const)(
12+
'allows %s setup when signing is allowed',
13+
(currentMode) => {
14+
expect(shouldDeferUnifiedAccountSetup(currentMode, true)).toBe(false);
15+
},
16+
);
17+
18+
it.each(['unifiedAccount', 'portfolioMargin', undefined] as const)(
19+
'does not defer %s because no migration is required',
20+
(currentMode) => {
21+
expect(shouldDeferUnifiedAccountSetup(currentMode, false)).toBe(false);
22+
},
23+
);
24+
});
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import type { HyperLiquidAbstractionMode } from '../types/hyperliquid-types';
2+
3+
const MIGRATABLE_ABSTRACTION_MODES = new Set<HyperLiquidAbstractionMode>([
4+
'dexAbstraction',
5+
'default',
6+
'disabled',
7+
]);
8+
9+
/**
10+
* Determine whether unified-account setup should be deferred until a user
11+
* explicitly starts a trading or withdrawal action.
12+
*
13+
* @param currentMode - The user's current HyperLiquid abstraction mode.
14+
* @param allowUserSigning - Whether the caller is allowed to trigger wallet signing.
15+
* @returns True when migration would require a signing-backed mutation that should be deferred.
16+
*/
17+
export function shouldDeferUnifiedAccountSetup(
18+
currentMode: HyperLiquidAbstractionMode | undefined,
19+
allowUserSigning: boolean,
20+
): boolean {
21+
return (
22+
!allowUserSigning &&
23+
currentMode !== undefined &&
24+
MIGRATABLE_ABSTRACTION_MODES.has(currentMode)
25+
);
26+
}

0 commit comments

Comments
 (0)