Skip to content

Commit eb330c5

Browse files
jiexijiexi
authored
fix: make WCv2 session restore sequential (MetaMask#27950)
<!-- 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** Changes WCv2 session/connection restores to be sequential. Previously they were concurrent. The concurrency was problematic because it would cause a large spike in relay traffic for users with numerous WCv2 connections. ## **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/WAPI-1357 ## **Manual testing steps** 1. Change `SESSION_RESTORE_STAGGER_MS` to be something larger like 10000 2. Make an ios expo build 3. Using safari native browser, connect to https://react-app.walletconnect.com/ AND https://wagmi-app.vercel.app/ using WalletConnect 4. Swipe away MetaMask 5. On the dapp you connected to last, trigger a personal sign 6. Ignore the deeplink 7. Reopen MetaMask manually from the homescreen 8. Login 9. It should take at least your `SESSION_RESTORE_STAGGER_MS` delay before you see the approval appear Why this way instead of using the js debugger? Unfortunately restarting the app causes the debugger to disconnect, and so it isn't a reliable way to probe startup based flows. ## **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** - [ ] 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). - [ ] I've completed the PR template to the best of my ability - [ ] I've included tests if applicable - [ ] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes WCv2 startup session restoration from concurrent async updates to a serialized loop with an added delay, which can impact connection readiness timing and expose ordering/regression issues in session/account syncing. > > **Overview** > WCv2 startup now restores existing WalletConnect sessions **sequentially** via a new `restoreSessions()` flow, adding a small stagger (`SESSION_RESTORE_STAGGER_MS`) between `updateSession` calls to avoid relay-traffic spikes. > > Initialization (`WC2Manager.init`) now triggers this restore pass after constructing the manager, and tests were extended to assert serial execution, the per-session delay, and that sessions with internal/invalid URLs are skipped. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 2c7c339. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent bcf7f71 commit eb330c5

2 files changed

Lines changed: 223 additions & 67 deletions

File tree

‎app/core/WalletConnect/WalletConnectV2.test.ts‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import { Core } from '@walletconnect/core';
1313
import Routes from '../../constants/navigation/Routes';
1414
import { store } from '../../store';
1515
import { ActionType } from '../../actions/sdk';
16+
// eslint-disable-next-line import-x/no-namespace
17+
import * as waitUtil from '../SDKConnect/utils/wait.util';
1618

1719
jest.mock('../AppConstants', () => ({
1820
WALLET_CONNECT: {
@@ -236,6 +238,11 @@ jest.mock('@walletconnect/core', () => ({
236238
})),
237239
}));
238240

241+
jest.mock('../SDKConnect/utils/wait.util', () => ({
242+
wait: jest.fn().mockResolvedValue(undefined),
243+
waitForKeychainUnlocked: jest.fn().mockResolvedValue(undefined),
244+
}));
245+
239246
describe('WC2Manager', () => {
240247
let manager: WC2Manager;
241248
let mockApproveSession: jest.SpyInstance;
@@ -442,6 +449,145 @@ describe('WC2Manager', () => {
442449
expect(secondInstance).toBe(firstInstance);
443450
expect(secondInstance).toBeInstanceOf(WC2Manager);
444451
});
452+
453+
describe('session restore', () => {
454+
const SESSION_RESTORE_STAGGER_MS = 200;
455+
456+
const makeSession = (index: number) => ({
457+
topic: `topic-${index}`,
458+
pairingTopic: `pairing-${index}`,
459+
peer: {
460+
metadata: {
461+
url: `https://dapp-${index}.example.com`,
462+
name: `DApp ${index}`,
463+
icons: [],
464+
},
465+
},
466+
});
467+
468+
// The WalletKit mock always resolves to the same singleton mockClient.
469+
// We access it through the manager created by the outer beforeEach so
470+
// we can configure getActiveSessions before each fresh init() call.
471+
let walletKitClient: IWalletKit;
472+
473+
beforeEach(() => {
474+
walletKitClient = (manager as unknown as { web3Wallet: IWalletKit })
475+
.web3Wallet;
476+
(WC2Manager as any).instance = undefined;
477+
(WC2Manager as any)._initialized = false;
478+
jest.clearAllMocks();
479+
});
480+
481+
it('calls updateSession for each active session on startup', async () => {
482+
const sessions = [makeSession(1), makeSession(2), makeSession(3)];
483+
(walletKitClient.getActiveSessions as jest.Mock).mockReturnValueOnce(
484+
Object.fromEntries(sessions.map((s) => [s.topic, s])),
485+
);
486+
487+
const mockUpdateSession = jest.fn().mockResolvedValue(undefined);
488+
(WalletConnect2Session as jest.Mock).mockImplementation(() => ({
489+
updateSession: mockUpdateSession,
490+
handleRequest: jest.fn(),
491+
removeListeners: jest.fn(),
492+
setDeeplink: jest.fn(),
493+
isHandlingRequest: jest.fn().mockReturnValue(false),
494+
getCurrentChainId: jest.fn().mockReturnValue('0x1'),
495+
getAllowedChainIds: ['eip155:1'],
496+
}));
497+
498+
await WC2Manager.init({});
499+
500+
expect(mockUpdateSession).toHaveBeenCalledTimes(sessions.length);
501+
});
502+
503+
it('does not start the next updateSession before the previous one resolves', async () => {
504+
const sessions = [makeSession(1), makeSession(2), makeSession(3)];
505+
(walletKitClient.getActiveSessions as jest.Mock).mockReturnValueOnce(
506+
Object.fromEntries(sessions.map((s) => [s.topic, s])),
507+
);
508+
509+
let concurrentCallCount = 0;
510+
let maxConcurrentCalls = 0;
511+
const mockUpdateSession = jest.fn().mockImplementation(async () => {
512+
concurrentCallCount++;
513+
maxConcurrentCalls = Math.max(
514+
maxConcurrentCalls,
515+
concurrentCallCount,
516+
);
517+
await Promise.resolve();
518+
concurrentCallCount--;
519+
});
520+
521+
(WalletConnect2Session as jest.Mock).mockImplementation(() => ({
522+
updateSession: mockUpdateSession,
523+
handleRequest: jest.fn(),
524+
removeListeners: jest.fn(),
525+
setDeeplink: jest.fn(),
526+
isHandlingRequest: jest.fn().mockReturnValue(false),
527+
getCurrentChainId: jest.fn().mockReturnValue('0x1'),
528+
getAllowedChainIds: ['eip155:1'],
529+
}));
530+
531+
await WC2Manager.init({});
532+
533+
expect(maxConcurrentCalls).toBe(1);
534+
});
535+
536+
it('inserts a delay between successive session restores', async () => {
537+
const sessions = [makeSession(1), makeSession(2), makeSession(3)];
538+
(walletKitClient.getActiveSessions as jest.Mock).mockReturnValueOnce(
539+
Object.fromEntries(sessions.map((s) => [s.topic, s])),
540+
);
541+
542+
const waitSpy = jest.spyOn(waitUtil, 'wait');
543+
544+
await WC2Manager.init({});
545+
546+
const staggerCalls = waitSpy.mock.calls.filter(
547+
([ms]) => ms === SESSION_RESTORE_STAGGER_MS,
548+
);
549+
// One stagger wait after each restored session
550+
expect(staggerCalls).toHaveLength(sessions.length);
551+
});
552+
553+
it('skips sessions with internal/invalid URLs without restoring them', async () => {
554+
// ORIGIN_METAMASK ('metamask') is a member of INTERNAL_ORIGINS
555+
const internalUrl = 'metamask';
556+
const sessions = [
557+
{
558+
topic: 'internal-topic',
559+
pairingTopic: 'internal-pairing',
560+
peer: {
561+
metadata: { url: internalUrl, name: 'Internal', icons: [] },
562+
},
563+
},
564+
makeSession(2),
565+
];
566+
(walletKitClient.getActiveSessions as jest.Mock).mockReturnValueOnce(
567+
Object.fromEntries(sessions.map((s) => [s.topic, s])),
568+
);
569+
570+
const mockUpdateSession = jest.fn().mockResolvedValue(undefined);
571+
(WalletConnect2Session as jest.Mock).mockImplementation(() => ({
572+
updateSession: mockUpdateSession,
573+
handleRequest: jest.fn(),
574+
removeListeners: jest.fn(),
575+
setDeeplink: jest.fn(),
576+
isHandlingRequest: jest.fn().mockReturnValue(false),
577+
getCurrentChainId: jest.fn().mockReturnValue('0x1'),
578+
getAllowedChainIds: ['eip155:1'],
579+
}));
580+
581+
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
582+
583+
await WC2Manager.init({});
584+
585+
// Only the valid session is restored
586+
expect(mockUpdateSession).toHaveBeenCalledTimes(1);
587+
588+
consoleSpy.mockRestore();
589+
});
590+
});
445591
});
446592

447593
describe('WC2Manager Sessions', () => {

‎app/core/WalletConnect/WalletConnectV2.ts‎

Lines changed: 77 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,22 @@ export class WC2Manager {
118118
delete this.sessions[event.topic];
119119
},
120120
);
121+
}
122+
123+
// Milliseconds to wait between consecutive session restorations on startup.
124+
private static readonly SESSION_RESTORE_STAGGER_MS = 200;
125+
126+
/**
127+
* Restores all active WalletConnect sessions one at a time.
128+
*
129+
* This method processes sessions serially with a small delay between
130+
* each one so that relay traffic is smoothed out over time.
131+
*/
132+
private async restoreSessions(): Promise<void> {
133+
const activeSessions = this.getSessions();
134+
if (!activeSessions?.length) {
135+
return;
136+
}
121137

122138
const accountsController = (
123139
Engine.context as {
@@ -137,83 +153,76 @@ export class WC2Manager {
137153
}
138154
).PermissionController;
139155

140-
const activeSessions = this.getSessions();
156+
for (const session of activeSessions) {
157+
if (INTERNAL_ORIGINS.includes(session.peer.metadata.url)) {
158+
console.warn(
159+
`WC2::init skipping session with invalid url: ${session.topic}`,
160+
);
161+
continue;
162+
}
141163

142-
if (activeSessions) {
143-
activeSessions.forEach(async (session) => {
144-
if (INTERNAL_ORIGINS.includes(session.peer.metadata.url)) {
145-
console.warn(
146-
`WC2::init skipping session with invalid url: ${session.topic}`,
147-
);
148-
return;
149-
}
164+
const sessionKey = session.topic;
165+
const pairingTopic = session.pairingTopic;
166+
try {
167+
const wcSession = new WalletConnect2Session({
168+
web3Wallet: this.web3Wallet,
169+
channelId: pairingTopic,
170+
navigation: this.navigation,
171+
deeplink:
172+
typeof this.deeplinkSessions[session.pairingTopic] !== 'undefined',
173+
session,
174+
});
175+
this.sessions[sessionKey] = wcSession;
150176

151-
const sessionKey = session.topic;
152-
const pairingTopic = session.pairingTopic;
153-
try {
154-
const wcSession = new WalletConnect2Session({
155-
web3Wallet,
156-
channelId: pairingTopic,
157-
navigation: this.navigation,
158-
deeplink:
159-
typeof deeplinkSessions[session.pairingTopic] !== 'undefined',
160-
session,
161-
});
162-
this.sessions[sessionKey] = wcSession;
163-
164-
// Find approvedAccounts for current sessions
165-
DevLogger.log(
166-
`WC2::init getPermittedAccounts for ${sessionKey} origin=${sessionKey}`,
167-
JSON.stringify(permissionController.state, null, 2),
168-
);
169-
const accountPermission = permissionController.getPermission(
170-
pairingTopic,
171-
'eth_accounts',
172-
);
177+
// Find approvedAccounts for current sessions
178+
DevLogger.log(
179+
`WC2::init getPermittedAccounts for ${sessionKey} origin=${sessionKey}`,
180+
JSON.stringify(permissionController.state, null, 2),
181+
);
182+
const accountPermission = permissionController.getPermission(
183+
pairingTopic,
184+
'eth_accounts',
185+
);
173186

174-
DevLogger.log(
175-
`WC2::init accountPermission`,
176-
JSON.stringify(accountPermission, null, 2),
177-
);
187+
DevLogger.log(
188+
`WC2::init accountPermission`,
189+
JSON.stringify(accountPermission, null, 2),
190+
);
178191

179-
let approvedAccounts = getPermittedAccounts(pairingTopic) ?? [];
192+
let approvedAccounts = getPermittedAccounts(pairingTopic) ?? [];
193+
194+
DevLogger.log(
195+
`WC2::init approvedAccounts id ${accountPermission?.id}`,
196+
approvedAccounts,
197+
);
180198

199+
if (approvedAccounts?.length === 0) {
181200
DevLogger.log(
182-
`WC2::init approvedAccounts id ${accountPermission?.id}`,
183-
approvedAccounts,
201+
`WC2::init fallback to parsing accountPermission`,
202+
accountPermission,
184203
);
204+
// FIXME: Why getPermitted accounts doesn't work???
205+
approvedAccounts = extractApprovedAccounts(accountPermission);
206+
DevLogger.log(`WC2::init approvedAccounts`, approvedAccounts);
207+
}
185208

186-
if (approvedAccounts?.length === 0) {
187-
DevLogger.log(
188-
`WC2::init fallback to parsing accountPermission`,
189-
accountPermission,
190-
);
191-
// FIXME: Why getPermitted accounts doesn't work???
192-
approvedAccounts = extractApprovedAccounts(accountPermission);
193-
DevLogger.log(`WC2::init approvedAccounts`, approvedAccounts);
194-
}
209+
updatePermittedChains(pairingTopic, wcSession.getAllowedChainIds, true);
195210

196-
updatePermittedChains(
197-
pairingTopic,
198-
wcSession.getAllowedChainIds,
199-
true,
200-
);
211+
const chainId = wcSession.getCurrentChainId();
201212

202-
const chainId = wcSession.getCurrentChainId();
203-
204-
const nChainId = parseInt(chainId, 16);
205-
DevLogger.log(
206-
`WC2::init updateSession session=${pairingTopic} chainId=${chainId} nChainId=${nChainId} selectedAddress=${selectedInternalAccountChecksummedAddress}`,
207-
approvedAccounts,
208-
);
209-
await this.sessions[sessionKey].updateSession({
210-
chainId: nChainId,
211-
accounts: approvedAccounts,
212-
});
213-
} catch (err) {
214-
console.warn(`WC2::init can't update session ${sessionKey}`);
215-
}
216-
});
213+
const nChainId = parseInt(chainId, 16);
214+
DevLogger.log(
215+
`WC2::init updateSession session=${pairingTopic} chainId=${chainId} nChainId=${nChainId} selectedAddress=${selectedInternalAccountChecksummedAddress}`,
216+
approvedAccounts,
217+
);
218+
await this.sessions[sessionKey].updateSession({
219+
chainId: nChainId,
220+
accounts: approvedAccounts,
221+
});
222+
} catch (err) {
223+
console.warn(`WC2::init can't update session ${sessionKey}`);
224+
}
225+
await wait(WC2Manager.SESSION_RESTORE_STAGGER_MS);
217226
}
218227
}
219228

@@ -317,6 +326,7 @@ export class WC2Manager {
317326
navigation,
318327
sessions,
319328
);
329+
await this.instance.restoreSessions();
320330
} catch (error) {
321331
Logger.error(error as Error, `WC2@init() failed to create instance`);
322332
}

0 commit comments

Comments
 (0)