Skip to content

Commit 92b606d

Browse files
jiexiwenfix
andauthored
fix: Fix MM Connect resuming of sessions before NetworkController state is available for BackgroundBridge (MetaMask#22749)
<!-- 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** Fixes an issue where MM Connect deeplinks do not result in an approval being shown to the user from app cold start AND (?) if the app is an expo build that gets delayed by bundling. ~~1. For some reason we do not restore connections the first ReactNative AppState "active" event, which is the only one we receive. Guard has been removed~~ Tamas has clarified why this is the case. I have reverted this change 2. Our check to ensure the store is fully initialized is not complete enough and was resulting in BackgroundBridge throwing when trying to access state from the NetworkController that was not ready. ## **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` 3. 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 No changelog as MetaMask Connect has not been released to public yet ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **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] > Adds store readiness check (including NetworkController state) to RPC bridge init and increases readiness polling intervals; updates tests accordingly. > > - **SDKConnectV2 readiness**: > - `when-store-ready`: New util ensuring `store.dispatch` and `engine.backgroundState.NetworkController` exist before proceeding. > - `when-engine-ready`/`when-store-ready`: Increase polling interval from 10ms to 100ms. > - **RPC bridge initialization**: > - `RPCBridgeAdapter` tests updated to wait for `whenEngineReady`, `whenOnboardingComplete`, and new `whenStoreReady` before creating `BackgroundBridge` and subscribing to unlock events; ensure idempotent init and queuing behavior. > - **Tests**: > - Update timer advances to match slower polling (milliseconds → seconds) in `when-engine-ready.test.ts` and `when-store-ready.test.ts`. > - `connection-registry.test.ts`: Mock store state to include `NetworkController` for initialization-dependent flows. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 570282b. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: aphex <52055541+wenfix@users.noreply.github.com>
1 parent 94f0c35 commit 92b606d

6 files changed

Lines changed: 71 additions & 13 deletions

File tree

app/core/SDKConnectV2/adapters/rpc-bridge-adapter.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { ConnectionInfo } from '../types/connection-info';
55
import { RPCBridgeAdapter } from './rpc-bridge-adapter';
66
import { whenEngineReady } from '../utils/when-engine-ready';
77
import { whenOnboardingComplete } from '../utils/when-onboarding-complete';
8+
import { whenStoreReady } from '../utils/when-store-ready';
89

910
jest.mock('../../BackgroundBridge/BackgroundBridge');
1011
jest.mock('../utils/when-engine-ready', () => ({
@@ -13,6 +14,9 @@ jest.mock('../utils/when-engine-ready', () => ({
1314
jest.mock('../utils/when-onboarding-complete', () => ({
1415
whenOnboardingComplete: jest.fn(),
1516
}));
17+
jest.mock('../utils/when-store-ready', () => ({
18+
whenStoreReady: jest.fn(),
19+
}));
1620
jest.mock('../../Engine', () => ({
1721
controllerMessenger: {
1822
subscribe: jest.fn(),
@@ -28,6 +32,7 @@ jest.mock('../../Engine', () => ({
2832
const MockedBackgroundBridge = BackgroundBridge as any;
2933
const mockedWhenEngineReady = whenEngineReady as jest.Mock;
3034
const mockedWhenOnboardingComplete = whenOnboardingComplete as jest.Mock;
35+
const mockedWhenStoreReady = whenStoreReady as jest.Mock;
3136
const mockedEngine = Engine as any;
3237

3338
describe('RPCBridgeAdapter', () => {
@@ -62,6 +67,7 @@ describe('RPCBridgeAdapter', () => {
6267
mockedEngine.controllerMessenger = mockMessenger;
6368
mockedWhenEngineReady.mockResolvedValue(undefined);
6469
mockedWhenOnboardingComplete.mockResolvedValue(undefined);
70+
mockedWhenStoreReady.mockResolvedValue(undefined);
6571

6672
// Capture the instance and sendMessage callback from the mock constructor
6773
MockedBackgroundBridge.mockImplementation((args: any) => {
@@ -89,6 +95,8 @@ describe('RPCBridgeAdapter', () => {
8995
await new Promise(process.nextTick);
9096

9197
expect(mockedWhenEngineReady).toHaveBeenCalledTimes(1);
98+
expect(mockedWhenOnboardingComplete).toHaveBeenCalledTimes(1);
99+
expect(mockedWhenStoreReady).toHaveBeenCalledTimes(1);
92100
expect(mockMessenger.subscribe).toHaveBeenCalledWith(
93101
'KeyringController:unlock',
94102
expect.any(Function),
@@ -112,6 +120,8 @@ describe('RPCBridgeAdapter', () => {
112120
await new Promise(process.nextTick);
113121

114122
expect(mockedWhenEngineReady).toHaveBeenCalledTimes(1);
123+
expect(mockedWhenOnboardingComplete).toHaveBeenCalledTimes(1);
124+
expect(mockedWhenStoreReady).toHaveBeenCalledTimes(1);
115125
expect(mockMessenger.subscribe).toHaveBeenCalledTimes(1);
116126
expect(MockedBackgroundBridge).toHaveBeenCalledTimes(1);
117127
});

app/core/SDKConnectV2/services/connection-registry.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ jest.mock('../../Permissions');
1717
jest.mock('../../../store', () => ({
1818
store: {
1919
dispatch: jest.fn(),
20+
getState: jest.fn().mockImplementation(() => ({
21+
engine: { backgroundState: { NetworkController: {} } },
22+
})),
2023
},
2124
}));
2225

app/core/SDKConnectV2/utils/when-engine-ready.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ describe('when-engine-ready', () => {
4343
const promise = whenEngineReady();
4444

4545
// Fast-forward time to trigger the check intervals
46-
await jest.advanceTimersByTimeAsync(200);
46+
await jest.advanceTimersByTimeAsync(2000);
4747

4848
await expect(promise).resolves.toBeUndefined();
4949
});
@@ -65,7 +65,7 @@ describe('when-engine-ready', () => {
6565
const promise = whenEngineReady();
6666

6767
// Fast-forward time to trigger the check intervals
68-
await jest.advanceTimersByTimeAsync(300);
68+
await jest.advanceTimersByTimeAsync(3000);
6969

7070
await expect(promise).resolves.toBeUndefined();
7171
});

app/core/SDKConnectV2/utils/when-engine-ready.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,6 @@ const isEngineReady = (): boolean => {
1313

1414
export async function whenEngineReady() {
1515
while (!isEngineReady()) {
16-
await new Promise((resolve) => setTimeout(resolve, 10));
16+
await new Promise((resolve) => setTimeout(resolve, 100));
1717
}
1818
}

app/core/SDKConnectV2/utils/when-store-ready.test.ts

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@ describe('when-store-ready', () => {
2121
it('should resolve immediately if the store is already ready', async () => {
2222
const mockStore = {
2323
dispatch: jest.fn(),
24-
getState: jest.fn(),
24+
getState: jest.fn().mockImplementation(() => ({
25+
engine: { backgroundState: { NetworkController: true } },
26+
})),
2527
};
2628

2729
// eslint-disable-next-line @typescript-eslint/no-require-imports
@@ -41,7 +43,9 @@ describe('when-store-ready', () => {
4143
let callCount = 0;
4244
const mockStore = {
4345
dispatch: jest.fn(),
44-
getState: jest.fn(),
46+
getState: jest.fn().mockImplementation(() => ({
47+
engine: { backgroundState: { NetworkController: true } },
48+
})),
4549
};
4650

4751
// eslint-disable-next-line @typescript-eslint/no-require-imports
@@ -60,7 +64,7 @@ describe('when-store-ready', () => {
6064
const promise = whenStoreReady();
6165

6266
// Fast-forward time to trigger the check intervals
63-
await jest.advanceTimersByTimeAsync(200);
67+
await jest.advanceTimersByTimeAsync(2000);
6468

6569
await expect(promise).resolves.toBeUndefined();
6670
});
@@ -70,7 +74,9 @@ describe('when-store-ready', () => {
7074
let callCount = 0;
7175
const mockStore = {
7276
dispatch: jest.fn(),
73-
getState: jest.fn(),
77+
getState: jest.fn().mockImplementation(() => ({
78+
engine: { backgroundState: { NetworkController: true } },
79+
})),
7480
};
7581

7682
// eslint-disable-next-line @typescript-eslint/no-require-imports
@@ -89,7 +95,42 @@ describe('when-store-ready', () => {
8995
const promise = whenStoreReady();
9096

9197
// Fast-forward time to trigger the check intervals
92-
await jest.advanceTimersByTimeAsync(200);
98+
await jest.advanceTimersByTimeAsync(2000);
99+
100+
await expect(promise).resolves.toBeUndefined();
101+
});
102+
103+
it('should wait if NetworkController is not available in store state', async () => {
104+
let callCount = 0;
105+
const mockReadyStore = {
106+
dispatch: jest.fn(),
107+
getState: jest.fn().mockImplementation(() => ({
108+
engine: { backgroundState: { NetworkController: true } },
109+
})),
110+
};
111+
112+
// eslint-disable-next-line @typescript-eslint/no-require-imports
113+
const storeModule = require('../../../store');
114+
Object.defineProperty(storeModule, 'store', {
115+
get: () => {
116+
callCount++;
117+
if (callCount < 3) {
118+
return {
119+
dispatch: jest.fn(),
120+
getState: jest.fn().mockImplementation(() => ({
121+
engine: { backgroundState: {} }, // NetworkController not set
122+
})),
123+
};
124+
}
125+
return mockReadyStore;
126+
},
127+
configurable: true,
128+
});
129+
130+
const promise = whenStoreReady();
131+
132+
// Fast-forward time to trigger the check intervals
133+
await jest.advanceTimersByTimeAsync(2000);
93134

94135
await expect(promise).resolves.toBeUndefined();
95136
});

app/core/SDKConnectV2/utils/when-store-ready.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@ import { store } from '../../../store';
22

33
const isStoreReady = () => {
44
try {
5-
if (store && typeof store.dispatch === 'function') {
6-
return true;
7-
}
8-
return false;
5+
return Boolean(
6+
store &&
7+
typeof store.dispatch === 'function' &&
8+
// This check is needed because the BackgroundBridge requires the NetworkController state
9+
// for initialization. This check is brittle as reliance on other controller state during
10+
// resumption would not be accounted for. We should figure out something more robust.
11+
store.getState().engine.backgroundState.NetworkController,
12+
);
913
} catch {
1014
return false;
1115
}
@@ -20,6 +24,6 @@ const isStoreReady = () => {
2024
*/
2125
export const whenStoreReady = async (): Promise<void> => {
2226
while (!isStoreReady()) {
23-
await new Promise((resolve) => setTimeout(resolve, 10));
27+
await new Promise((resolve) => setTimeout(resolve, 100));
2428
}
2529
};

0 commit comments

Comments
 (0)