Skip to content

Commit 2a5d2de

Browse files
tommasiniMarioAslau
tommasini
and
MarioAslau
authored
fix: re order migrations 105, 106, 107 cp-7.59.0 (#22276)
<!-- 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** Re-order migrations 105, 106, 107 Removal of migration 104 redux-persist slicing. No longer needed <!-- 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: ## **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** - [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] > Reorders and repurposes migrations (104–106), removes 107, updates inflation/deflation thresholds, and aligns tests with the new behaviors. > > - **Migrations**: > - **104**: Simplified to only reset `PhishingController.urlScanCache`; adds error reporting for invalid state. > - **105**: Now removes `RatesController` from `engine.backgroundState`; no-ops if absent; standardized error handling. > - **106**: Now cleans PPOM MMKV storage (`PPOMDB`); logic moved from former `107`; returns unchanged state; error captured. > - **Removed**: `107` migration and its tests. > - **Index** (`migrations/index.ts`): Dropped `107` from `migrationList`; changed inflate trigger to `> 106` and deflate on last version `>= 106`. > - **Tests**: > - Updated/added tests for 104–106 reflecting new responsibilities and error messages. > - Adjusted async migration flow tests to new version thresholds; removed old 107 tests; renamed for concision. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit a52e123. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Aslau Mario-Daniel <[email protected]>
1 parent 327cf31 commit 2a5d2de

File tree

10 files changed

+335
-651
lines changed

10 files changed

+335
-651
lines changed

‎app/store/migrations/104.test.ts‎

Lines changed: 118 additions & 169 deletions
Original file line numberDiff line numberDiff line change
@@ -1,213 +1,162 @@
11
import migrate from './104';
2-
import FilesystemStorage from 'redux-persist-filesystem-storage';
3-
import Device from '../../util/device';
2+
import { ensureValidState } from './util';
3+
import { captureException } from '@sentry/react-native';
44

5-
jest.mock('redux-persist-filesystem-storage');
6-
const mockFilesystemStorage = FilesystemStorage as jest.Mocked<
7-
typeof FilesystemStorage
8-
>;
5+
jest.mock('@sentry/react-native', () => ({
6+
captureException: jest.fn(),
7+
}));
98

10-
jest.mock('../../util/device');
11-
const mockDevice = Device as jest.Mocked<typeof Device>;
9+
jest.mock('./util', () => ({
10+
ensureValidState: jest.fn(),
11+
}));
1212

13-
describe('Migration 104', () => {
13+
const mockedCaptureException = jest.mocked(captureException);
14+
const mockedEnsureValidState = jest.mocked(ensureValidState);
15+
16+
describe('Migration 104: Reset PhishingController urlScanCache', () => {
1417
beforeEach(() => {
15-
jest.clearAllMocks();
16-
mockDevice.isIos.mockReturnValue(true);
17-
mockFilesystemStorage.setItem.mockResolvedValue();
18+
jest.resetAllMocks();
1819
});
1920

20-
it('should migrate existing engine data to individual controller storage', async () => {
21-
const mockState = {
22-
engine: {
23-
backgroundState: {
24-
KeyringController: {
25-
vault: 'encrypted-vault-data',
26-
isUnlocked: false,
27-
},
28-
NetworkController: {
29-
network: 'mainnet',
30-
chainId: '1',
31-
},
32-
TransactionController: {
33-
transactions: [{ id: '1', status: 'pending' }],
34-
methodData: { '0x123': { method: 'transfer' } },
35-
},
36-
},
37-
},
38-
};
39-
40-
const result = await migrate(mockState);
41-
42-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(3);
43-
44-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith(
45-
'persist:KeyringController',
46-
JSON.stringify({
47-
vault: 'encrypted-vault-data',
48-
isUnlocked: false,
49-
}),
50-
true,
51-
);
21+
it('returns state unchanged if ensureValidState fails', () => {
22+
const state = { some: 'state' };
5223

53-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith(
54-
'persist:NetworkController',
55-
JSON.stringify({
56-
network: 'mainnet',
57-
chainId: '1',
58-
}),
59-
true,
60-
);
24+
mockedEnsureValidState.mockReturnValue(false);
6125

62-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith(
63-
'persist:TransactionController',
64-
JSON.stringify({
65-
transactions: [{ id: '1', status: 'pending' }],
66-
methodData: { '0x123': { method: 'transfer' } },
67-
}),
68-
true,
69-
);
26+
const migratedState = migrate(state);
7027

71-
expect(result).toEqual({
72-
engine: {
73-
backgroundState: {},
74-
},
75-
});
28+
expect(migratedState).toBe(state);
29+
expect(mockedCaptureException).not.toHaveBeenCalled();
7630
});
7731

78-
it('should completely clear backgroundState when all controllers migrate successfully', async () => {
79-
const mockState = {
32+
it('captures exception if PhishingController state is invalid', () => {
33+
const state = {
8034
engine: {
8135
backgroundState: {
82-
KeyringController: {
83-
vault: 'encrypted-vault-data',
84-
},
85-
NetworkController: {
86-
network: 'mainnet',
87-
},
36+
// PhishingController is missing
8837
},
8938
},
9039
};
9140

92-
// All migrations succeed
93-
mockFilesystemStorage.setItem.mockResolvedValue();
41+
mockedEnsureValidState.mockReturnValue(true);
9442

95-
const result = await migrate(mockState);
43+
const migratedState = migrate(state);
9644

97-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(2);
98-
99-
// Should completely clear backgroundState when all controllers migrate successfully
100-
expect(result).toEqual({
101-
engine: {
102-
backgroundState: {},
103-
},
104-
});
45+
expect(migratedState).toEqual(state);
46+
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
47+
expect(mockedCaptureException.mock.calls[0][0].message).toContain(
48+
'Migration 104: Invalid PhishingController state',
49+
);
10550
});
10651

107-
it('should handle empty engine data gracefully', async () => {
108-
const mockState = {
52+
it('resets PhishingController urlScanCache to empty object while preserving other fields', () => {
53+
interface TestState {
10954
engine: {
110-
backgroundState: {},
111-
},
112-
};
113-
114-
const result = await migrate(mockState);
115-
116-
expect(mockFilesystemStorage.setItem).not.toHaveBeenCalled();
117-
118-
expect(result).toEqual(mockState);
119-
});
120-
121-
it('should handle missing engine data gracefully', async () => {
122-
const mockState = {
123-
engine: {},
124-
};
125-
126-
const result = await migrate(mockState);
127-
128-
expect(mockFilesystemStorage.setItem).not.toHaveBeenCalled();
129-
// Should return state unchanged
130-
expect(result).toEqual(mockState);
131-
});
132-
133-
it('should handle partial controller data', async () => {
134-
const mockState = {
55+
backgroundState: {
56+
PhishingController: {
57+
c2DomainBlocklistLastFetched: number;
58+
phishingLists: string[];
59+
whitelist: string[];
60+
hotlistLastFetched: number;
61+
stalelistLastFetched: number;
62+
urlScanCache: Record<string, unknown>;
63+
extraProperty?: string;
64+
};
65+
OtherController: {
66+
shouldStayUntouched: boolean;
67+
};
68+
};
69+
};
70+
}
71+
72+
const state: TestState = {
13573
engine: {
13674
backgroundState: {
137-
KeyringController: {
138-
vault: 'encrypted-vault-data',
75+
PhishingController: {
76+
c2DomainBlocklistLastFetched: 123456789,
77+
phishingLists: ['list1', 'list2'],
78+
whitelist: ['site1', 'site2'],
79+
hotlistLastFetched: 987654321,
80+
stalelistLastFetched: 123123123,
81+
urlScanCache: {
82+
'example.com': { result: 'safe', timestamp: 1234567890 },
83+
'phishing.com': { result: 'malicious', timestamp: 9876543210 },
84+
},
85+
extraProperty: 'should remain',
13986
},
140-
TransactionController: {
141-
transactions: [],
87+
OtherController: {
88+
shouldStayUntouched: true,
14289
},
14390
},
14491
},
14592
};
14693

147-
const result = await migrate(mockState);
148-
149-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(2);
150-
151-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith(
152-
'persist:KeyringController',
153-
JSON.stringify({ vault: 'encrypted-vault-data' }),
154-
true,
155-
);
156-
157-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith(
158-
'persist:TransactionController',
159-
JSON.stringify({ transactions: [] }),
160-
true,
161-
);
162-
163-
expect(result).toEqual({
164-
engine: {
165-
backgroundState: {},
166-
},
94+
mockedEnsureValidState.mockReturnValue(true);
95+
96+
const migratedState = migrate(state) as typeof state;
97+
98+
// urlScanCache should be reset to empty object
99+
expect(
100+
migratedState.engine.backgroundState.PhishingController.urlScanCache,
101+
).toEqual({});
102+
103+
// Other fields should remain unchanged
104+
expect(
105+
migratedState.engine.backgroundState.PhishingController
106+
.c2DomainBlocklistLastFetched,
107+
).toBe(123456789);
108+
expect(
109+
migratedState.engine.backgroundState.PhishingController.phishingLists,
110+
).toEqual(['list1', 'list2']);
111+
expect(
112+
migratedState.engine.backgroundState.PhishingController.whitelist,
113+
).toEqual(['site1', 'site2']);
114+
expect(
115+
migratedState.engine.backgroundState.PhishingController
116+
.hotlistLastFetched,
117+
).toBe(987654321);
118+
expect(
119+
migratedState.engine.backgroundState.PhishingController
120+
.stalelistLastFetched,
121+
).toBe(123123123);
122+
expect(
123+
migratedState.engine.backgroundState.PhishingController.extraProperty,
124+
).toBe('should remain');
125+
126+
expect(migratedState.engine.backgroundState.OtherController).toEqual({
127+
shouldStayUntouched: true,
167128
});
129+
130+
expect(mockedCaptureException).not.toHaveBeenCalled();
168131
});
169132

170-
it('should handle storage errors gracefully and preserve failed controller state', async () => {
171-
const mockState = {
133+
it('handles error during migration', () => {
134+
// Create state with a PhishingController that throws when urlScanCache is accessed
135+
const state = {
172136
engine: {
173137
backgroundState: {
174-
KeyringController: {
175-
vault: 'encrypted-vault-data',
176-
},
177-
NetworkController: {
178-
network: 'mainnet',
179-
},
138+
PhishingController: Object.defineProperty({}, 'urlScanCache', {
139+
get: () => {
140+
throw new Error('Test error');
141+
},
142+
set: () => {
143+
throw new Error('Test error');
144+
},
145+
configurable: true,
146+
enumerable: true,
147+
}),
180148
},
181149
},
182150
};
183151

184-
mockFilesystemStorage.setItem
185-
.mockRejectedValueOnce(new Error('Storage error'))
186-
.mockResolvedValueOnce();
187-
188-
const result = await migrate(mockState);
152+
mockedEnsureValidState.mockReturnValue(true);
189153

190-
expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(2);
154+
const migratedState = migrate(state);
191155

192-
// Should preserve failed controller state to prevent data loss
193-
expect(result).toEqual({
194-
engine: {
195-
backgroundState: {
196-
KeyringController: {
197-
vault: 'encrypted-vault-data',
198-
},
199-
// NetworkController should be migrated successfully and removed from backgroundState
200-
},
201-
},
202-
});
203-
});
204-
205-
it('should handle invalid state gracefully', async () => {
206-
const invalidState = null;
207-
208-
const result = await migrate(invalidState);
209-
210-
expect(result).toBe(invalidState);
211-
expect(mockFilesystemStorage.setItem).not.toHaveBeenCalled();
156+
expect(migratedState).toEqual(state);
157+
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
158+
expect(mockedCaptureException.mock.calls[0][0].message).toContain(
159+
'Migration 104: cleaning PhishingController state failed with error',
160+
);
212161
});
213162
});

0 commit comments

Comments
 (0)