Skip to content

Commit a6f4d69

Browse files
infiniteflowerinfiniteflower
authored
fix: remove whole redux state pass through for bridge selectors (#33529)
<!-- 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. --> <!-- mms-check directive vocabulary — read by .github/scripts/shared/pr-template-checks.ts at module load to build the validation plan. Directives are invisible in rendered markdown and must NOT be removed or edited without updating the validator registry. type=text Section must contain non-placeholder prose. type=changelog Section must have a valid CHANGELOG entry: line. type=issue-link Section must have a Fixes:/Closes:/Refs: line with a value. type=manual-testing Section must have real testing steps or an explicit N/A. type=screenshot Section must have evidence (image/URL) or an explicit N/A. type=checklist Section must have all checkboxes consciously checked. required=true|false Whether a missing/invalid section runs the validator at all. blocking=true|false Whether a failure of this check fails the CI workflow. Default: false — failures are shown as warnings in the sticky comment but do not block the PR. Sections without a directive are checked for structural presence only. --> ## **Description** <!-- mms-check: type=text required=true --> <!-- 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? --> This PR removes whole Redux-state passthrough inputs from three Bridge account selectors. The selectors now compose granular token and account inputs, preventing result recomputation on unrelated dispatches while preserving selected account-group lookup, destination scope matching, and the existing `Set<AccountId>` API. It also adds EVM and non-EVM value coverage plus recomputation and reference-stability tests. ## **Changelog** <!-- mms-check: type=changelog required=true blocking=true --> <!-- 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** <!-- mms-check: type=issue-link required=true --> Fixes: #31269 ## **Manual testing steps** <!-- mms-check: type=manual-testing required=true --> N/A — this is an internal selector memoization change with no UI behavior change. Automated verification: run `yarn jest app/selectors/bridge.test.ts` and confirm all 24 tests pass, including the unrelated-state recomputation and stable Set-reference cases. ## **Screenshots/Recordings** <!-- mms-check: type=screenshot required=true --> <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> N/A — no UI changes. ### **After** <!-- [screenshots/recordings] --> N/A — no UI changes. ## **Pre-merge author checklist** <!-- mms-check: type=checklist required=true --> <!-- 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`. --> - [ ] 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** > Destination account eligibility logic was rewritten (different data source and `anyScopesMatch` vs per-scope queries); behavior should match but account picking for bridge is user-facing if wrong. > > **Overview** > **Refactors three Bridge account selectors** so they no longer depend on the entire `RootState`, which was forcing recomputation on every unrelated dispatch. > > `selectSourceWalletAddress` and `selectBatchSellSourceWalletAddress` now compose `selectSourceToken` / `selectBatchSellSourceTokens` with `selectSelectedInternalAccountByScope` instead of calling scope lookup inside a `(state) => state` input. **`selectValidDestInternalAccountIds`** drops `createDeepEqualSelector` and `selectInternalAccountsByScope` in favor of `selectInternalAccountsById` plus `anyScopesMatch` for destination scope and EVM wildcard (`eip155:0`) filtering, while still returning a `Set<AccountId>`. > > **Tests** add shared bridge selector mock state, EVM/Solana coverage, and assertions that unrelated bridge fields (e.g. `sourceAmount`) do not trigger recomputation or new `Set` references until the relevant token changes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f604031. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent b0c757a commit a6f4d69

2 files changed

Lines changed: 255 additions & 37 deletions

File tree

‎app/selectors/bridge.test.ts‎

Lines changed: 237 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,26 @@ jest.mock('./smartTransactionsController', () => ({
44
getGaslessBridgeWith7702EnabledForChain: jest.fn().mockReturnValue(false),
55
}));
66

7-
import { initialState } from '../core/redux/slices/bridge';
7+
import { initialState as bridgeInitialState } from '../core/redux/slices/bridge';
88
import {
9+
selectBatchSellSourceWalletAddress,
910
selectGasIncludedQuoteParams,
1011
selectIsGasIncluded7702BridgeEnabled,
12+
selectSourceWalletAddress,
13+
selectValidDestInternalAccountIds,
1114
} from './bridge';
1215
import { BridgeToken } from '../components/UI/Bridge/types';
1316
import { Hex } from '@metamask/utils';
1417
import { RootState } from '../reducers';
18+
import {
19+
evmAccountAddress,
20+
evmAccountId,
21+
initialState as bridgeSelectorInitialState,
22+
solanaAccountAddress,
23+
solanaAccountId,
24+
solanaNativeTokenAddress,
25+
} from '../components/UI/Bridge/_mocks_/initialState';
26+
import { SolScope } from '@metamask/keyring-api';
1527

1628
const mockToken: BridgeToken = {
1729
address: '0x123',
@@ -35,7 +47,218 @@ const mockDestToken: BridgeToken = {
3547
balanceFiat: '100',
3648
};
3749

50+
const mockSolanaToken: BridgeToken = {
51+
...mockToken,
52+
address: solanaNativeTokenAddress,
53+
symbol: 'SOL',
54+
chainId: SolScope.Mainnet,
55+
name: 'Solana',
56+
};
57+
58+
const selectorInitialState = bridgeSelectorInitialState as unknown as RootState;
59+
60+
function createSelectorState(
61+
bridgeOverrides: Partial<RootState['bridge']> = {},
62+
): RootState {
63+
return {
64+
...selectorInitialState,
65+
bridge: {
66+
...selectorInitialState.bridge,
67+
...bridgeOverrides,
68+
},
69+
};
70+
}
71+
3872
describe('bridge selectors', () => {
73+
describe('selectSourceWalletAddress', () => {
74+
beforeEach(() => {
75+
selectSourceWalletAddress.clearCache();
76+
selectSourceWalletAddress.memoizedResultFunc.clearCache();
77+
selectSourceWalletAddress.resetRecomputations();
78+
});
79+
80+
it('returns undefined when the source token is missing', () => {
81+
expect(selectSourceWalletAddress(createSelectorState())).toBeUndefined();
82+
});
83+
84+
it.each([
85+
['EVM', mockToken, evmAccountAddress],
86+
['non-EVM', mockSolanaToken, solanaAccountAddress],
87+
])(
88+
'returns the selected account group address for the %s source token',
89+
(_label, sourceToken, expectedAddress) => {
90+
const state = createSelectorState({ sourceToken });
91+
92+
expect(selectSourceWalletAddress(state)).toBe(expectedAddress);
93+
},
94+
);
95+
96+
it('does not recompute for unrelated state changes but does for a source token change', () => {
97+
const state = createSelectorState({
98+
sourceToken: mockToken,
99+
sourceAmount: '1',
100+
});
101+
const firstResult = selectSourceWalletAddress(state);
102+
const recomputations = selectSourceWalletAddress.recomputations();
103+
const unrelatedState = {
104+
...state,
105+
bridge: {
106+
...state.bridge,
107+
sourceAmount: '2',
108+
},
109+
};
110+
111+
const secondResult = selectSourceWalletAddress(unrelatedState);
112+
113+
expect(secondResult).toBe(firstResult);
114+
expect(selectSourceWalletAddress.recomputations()).toBe(recomputations);
115+
116+
const changedTokenState = {
117+
...unrelatedState,
118+
bridge: {
119+
...unrelatedState.bridge,
120+
sourceToken: mockSolanaToken,
121+
},
122+
};
123+
124+
expect(selectSourceWalletAddress(changedTokenState)).toBe(
125+
solanaAccountAddress,
126+
);
127+
expect(selectSourceWalletAddress.recomputations()).toBe(
128+
recomputations + 1,
129+
);
130+
});
131+
});
132+
133+
describe('selectBatchSellSourceWalletAddress', () => {
134+
beforeEach(() => {
135+
selectBatchSellSourceWalletAddress.clearCache();
136+
selectBatchSellSourceWalletAddress.memoizedResultFunc.clearCache();
137+
selectBatchSellSourceWalletAddress.resetRecomputations();
138+
});
139+
140+
it('returns undefined when the batch has no source tokens', () => {
141+
expect(
142+
selectBatchSellSourceWalletAddress(createSelectorState()),
143+
).toBeUndefined();
144+
});
145+
146+
it('returns the account address for the first batch source token', () => {
147+
const state = createSelectorState({
148+
batchSellSourceTokens: [mockSolanaToken, mockToken],
149+
});
150+
151+
expect(selectBatchSellSourceWalletAddress(state)).toBe(
152+
solanaAccountAddress,
153+
);
154+
});
155+
156+
it('does not recompute for unrelated state changes but does for a batch token change', () => {
157+
const state = createSelectorState({
158+
batchSellSourceTokens: [mockToken],
159+
sourceAmount: '1',
160+
});
161+
const firstResult = selectBatchSellSourceWalletAddress(state);
162+
const recomputations =
163+
selectBatchSellSourceWalletAddress.recomputations();
164+
const unrelatedState = {
165+
...state,
166+
bridge: {
167+
...state.bridge,
168+
sourceAmount: '2',
169+
},
170+
};
171+
172+
const secondResult = selectBatchSellSourceWalletAddress(unrelatedState);
173+
174+
expect(secondResult).toBe(firstResult);
175+
expect(selectBatchSellSourceWalletAddress.recomputations()).toBe(
176+
recomputations,
177+
);
178+
179+
const changedTokenState = {
180+
...unrelatedState,
181+
bridge: {
182+
...unrelatedState.bridge,
183+
batchSellSourceTokens: [mockSolanaToken],
184+
},
185+
};
186+
187+
expect(selectBatchSellSourceWalletAddress(changedTokenState)).toBe(
188+
solanaAccountAddress,
189+
);
190+
expect(selectBatchSellSourceWalletAddress.recomputations()).toBe(
191+
recomputations + 1,
192+
);
193+
});
194+
});
195+
196+
describe('selectValidDestInternalAccountIds', () => {
197+
beforeEach(() => {
198+
selectValidDestInternalAccountIds.clearCache();
199+
selectValidDestInternalAccountIds.memoizedResultFunc.clearCache();
200+
selectValidDestInternalAccountIds.resetRecomputations();
201+
});
202+
203+
it('returns an empty Set when the destination token is missing', () => {
204+
expect(selectValidDestInternalAccountIds(createSelectorState())).toEqual(
205+
new Set(),
206+
);
207+
});
208+
209+
it.each([
210+
['EVM', mockDestToken, [evmAccountId]],
211+
['non-EVM', mockSolanaToken, [solanaAccountId]],
212+
])(
213+
'returns valid internal account IDs for the %s destination',
214+
(_label, destToken, expectedAccountIds) => {
215+
const state = createSelectorState({ destToken });
216+
217+
expect(selectValidDestInternalAccountIds(state)).toEqual(
218+
new Set(expectedAccountIds),
219+
);
220+
},
221+
);
222+
223+
it('keeps the Set reference stable for unrelated state changes and recomputes for a destination token change', () => {
224+
const state = createSelectorState({
225+
destToken: mockDestToken,
226+
sourceAmount: '1',
227+
});
228+
const firstResult = selectValidDestInternalAccountIds(state);
229+
const recomputations = selectValidDestInternalAccountIds.recomputations();
230+
const unrelatedState = {
231+
...state,
232+
bridge: {
233+
...state.bridge,
234+
sourceAmount: '2',
235+
},
236+
};
237+
238+
const secondResult = selectValidDestInternalAccountIds(unrelatedState);
239+
240+
expect(secondResult).toBe(firstResult);
241+
expect(selectValidDestInternalAccountIds.recomputations()).toBe(
242+
recomputations,
243+
);
244+
245+
const changedTokenState = {
246+
...unrelatedState,
247+
bridge: {
248+
...unrelatedState.bridge,
249+
destToken: mockSolanaToken,
250+
},
251+
};
252+
253+
expect(selectValidDestInternalAccountIds(changedTokenState)).toEqual(
254+
new Set([solanaAccountId]),
255+
);
256+
expect(selectValidDestInternalAccountIds.recomputations()).toBe(
257+
recomputations + 1,
258+
);
259+
});
260+
});
261+
39262
describe('selectIsGasIncluded7702BridgeEnabled', () => {
40263
beforeEach(() => {
41264
jest
@@ -45,7 +268,7 @@ describe('bridge selectors', () => {
45268

46269
it('returns false when sourceToken is undefined', () => {
47270
const mockState = {
48-
bridge: { ...initialState, sourceToken: undefined },
271+
bridge: { ...bridgeInitialState, sourceToken: undefined },
49272
} as RootState;
50273

51274
expect(selectIsGasIncluded7702BridgeEnabled(mockState)).toBe(false);
@@ -54,7 +277,7 @@ describe('bridge selectors', () => {
54277
it('returns false when sourceToken has no chainId', () => {
55278
const mockState = {
56279
bridge: {
57-
...initialState,
280+
...bridgeInitialState,
58281
sourceToken: { ...mockToken, chainId: undefined },
59282
},
60283
} as unknown as RootState;
@@ -68,7 +291,7 @@ describe('bridge selectors', () => {
68291
.mockReturnValue(true);
69292

70293
const mockState = {
71-
bridge: { ...initialState, sourceToken: mockToken },
294+
bridge: { ...bridgeInitialState, sourceToken: mockToken },
72295
} as RootState;
73296

74297
expect(selectIsGasIncluded7702BridgeEnabled(mockState)).toBe(true);
@@ -90,7 +313,7 @@ describe('bridge selectors', () => {
90313
it('returns gasIncluded true with 7702 false when STX send bundle is supported', () => {
91314
const mockState = {
92315
bridge: {
93-
...initialState,
316+
...bridgeInitialState,
94317
isGasIncludedSTXSendBundleSupported: true,
95318
isGasIncluded7702Supported: true,
96319
},
@@ -104,7 +327,7 @@ describe('bridge selectors', () => {
104327
it('returns gasIncluded true with 7702 true for swap when 7702 is supported', () => {
105328
const mockState = {
106329
bridge: {
107-
...initialState,
330+
...bridgeInitialState,
108331
sourceToken: mockToken,
109332
destToken: { ...mockDestToken, chainId: mockToken.chainId },
110333
isGasIncludedSTXSendBundleSupported: false,
@@ -120,7 +343,7 @@ describe('bridge selectors', () => {
120343
it('returns gasIncluded false with 7702 false for swap without 7702 support', () => {
121344
const mockState = {
122345
bridge: {
123-
...initialState,
346+
...bridgeInitialState,
124347
sourceToken: mockToken,
125348
destToken: { ...mockDestToken, chainId: mockToken.chainId },
126349
isGasIncludedSTXSendBundleSupported: false,
@@ -136,7 +359,7 @@ describe('bridge selectors', () => {
136359
it('returns gasIncluded false with 7702 false for bridge mode if disabled via flag', () => {
137360
const mockState = {
138361
bridge: {
139-
...initialState,
362+
...bridgeInitialState,
140363
sourceToken: mockToken,
141364
destToken: mockDestToken,
142365
isGasIncludedSTXSendBundleSupported: false,
@@ -156,7 +379,7 @@ describe('bridge selectors', () => {
156379

157380
const mockState = {
158381
bridge: {
159-
...initialState,
382+
...bridgeInitialState,
160383
sourceToken: mockToken,
161384
destToken: mockDestToken,
162385
isGasIncludedSTXSendBundleSupported: false,
@@ -176,7 +399,7 @@ describe('bridge selectors', () => {
176399

177400
const mockState = {
178401
bridge: {
179-
...initialState,
402+
...bridgeInitialState,
180403
sourceToken: mockToken,
181404
destToken: mockDestToken,
182405
isGasIncludedSTXSendBundleSupported: false,
@@ -196,7 +419,7 @@ describe('bridge selectors', () => {
196419

197420
const mockState = {
198421
bridge: {
199-
...initialState,
422+
...bridgeInitialState,
200423
sourceToken: mockToken,
201424
destToken: mockDestToken,
202425
isGasIncludedSTXSendBundleSupported: false,
@@ -217,7 +440,7 @@ describe('bridge selectors', () => {
217440
};
218441
const mockState = {
219442
bridge: {
220-
...initialState,
443+
...bridgeInitialState,
221444
sourceToken: solanaToken,
222445
isGasIncludedSTXSendBundleSupported: false,
223446
isGasIncluded7702Supported: false,
@@ -237,7 +460,7 @@ describe('bridge selectors', () => {
237460
};
238461
const mockState = {
239462
bridge: {
240-
...initialState,
463+
...bridgeInitialState,
241464
sourceToken: solanaToken,
242465
isGasIncludedSTXSendBundleSupported: true,
243466
isGasIncluded7702Supported: true,
@@ -256,7 +479,7 @@ describe('bridge selectors', () => {
256479

257480
const mockState = {
258481
bridge: {
259-
...initialState,
482+
...bridgeInitialState,
260483
sourceToken: mockToken,
261484
destToken: mockDestToken,
262485
isGasIncludedSTXSendBundleSupported: true,

0 commit comments

Comments
 (0)