Skip to content

Commit 0528810

Browse files
FrederikBoldingMrtenz
FrederikBolding
and
Mrtenz
authored
fix: Filter out duplicate JSON-RPC requests (#24748)
<!-- 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** Copies `createDupeReqFilterStream.ts` from the extension repo and applies it to the provider stream in `BackgroundBridge`. This filters out any duplicate requests received from the dapp. It is possible for the RPC requests to be duplicated due to the "replay functionality" found here: https://github.com/MetaMask/metamask-mobile/blob/main/scripts/inpage-bridge/src/provider.js#L131 Feedback required. ## **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 <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Prevents duplicate JSON-RPC requests from reaching the engines. > > - Add `createDupeReqFilterStream` with a three-minute expiry for seen request ids; comprehensive unit tests in `createDupeReqFilterStream.test.ts` > - Insert filter into `BackgroundBridge` pump chain for both `setupProviderConnectionEip1193` and `setupProviderConnectionCaip` (between `outStream` and `providerStream`) > - Add `@types/readable-stream` to dependencies and update lockfile; annotate snaps execution setup with `@ts-expect-error` for stream type mismatch > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 31f5493. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
1 parent c7be495 commit 0528810

7 files changed

Lines changed: 424 additions & 2 deletions

File tree

‎app/core/BackgroundBridge/BackgroundBridge.js‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ import { isRelaySupported } from '../../util/transactions/transaction-relay';
9999
import { selectSmartTransactionsEnabled } from '../../selectors/smartTransactionsController';
100100
import { AccountTreeController } from '@metamask/account-tree-controller';
101101
import { createTrustSignalsMiddleware } from '../RPCMethods/TrustSignalsMiddleware';
102+
import createDupeReqFilterStream from './createDupeReqFilterStream';
102103

103104
const legacyNetworkId = () => {
104105
const { networksMetadata, selectedNetworkClientId } =
@@ -551,7 +552,9 @@ export class BackgroundBridge extends EventEmitter {
551552
// setup connection
552553
const providerStream = createEngineStream({ engine: this.engine });
553554

554-
pump(outStream, providerStream, outStream, (err) => {
555+
const filterStream = createDupeReqFilterStream();
556+
557+
pump(outStream, filterStream, providerStream, outStream, (err) => {
555558
// handle any middleware cleanup
556559
this.engine.destroy();
557560
if (err) Logger.log('Error with provider stream conn', err);
@@ -582,7 +585,9 @@ export class BackgroundBridge extends EventEmitter {
582585
this.notifyTronAccountChangedForCurrentAccount();
583586
///: END:ONLY_INCLUDE_IF
584587

585-
pump(outStream, providerStream, outStream, (err) => {
588+
const filterStream = createDupeReqFilterStream();
589+
590+
pump(outStream, filterStream, providerStream, outStream, (err) => {
586591
// handle any middleware cleanup
587592
this.multichainEngine.destroy();
588593
if (err) Logger.log('Error with provider stream conn', err);

‎app/core/BackgroundBridge/createDupeReqFilterStream.test.ts‎

Lines changed: 324 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,324 @@
1+
import { Transform } from 'readable-stream';
2+
3+
import type { JsonRpcNotification, JsonRpcRequest } from '@metamask/utils';
4+
import createDupeReqFilterStream, {
5+
THREE_MINUTES,
6+
} from './createDupeReqFilterStream';
7+
8+
function createTestStream(output: JsonRpcRequest[] = [], S = Transform) {
9+
const transformStream = createDupeReqFilterStream();
10+
const testOutStream = new S({
11+
transform: (chunk: JsonRpcRequest, _, cb) => {
12+
output.push(chunk);
13+
cb();
14+
},
15+
objectMode: true,
16+
});
17+
18+
transformStream.pipe(testOutStream);
19+
20+
return transformStream;
21+
}
22+
23+
function runStreamTest(
24+
requests: (JsonRpcRequest | JsonRpcNotification)[] = [],
25+
advanceTimersTime = 10,
26+
S = Transform,
27+
) {
28+
return new Promise((resolve, reject) => {
29+
const output: JsonRpcRequest[] = [];
30+
const testStream = createTestStream(output, S);
31+
32+
testStream
33+
.on('finish', () => resolve(output))
34+
.on('error', (err) => reject(err));
35+
36+
requests.forEach((request) => testStream.write(request));
37+
testStream.end();
38+
39+
jest.advanceTimersByTime(advanceTimersTime);
40+
});
41+
}
42+
43+
describe('createDupeReqFilterStream', () => {
44+
beforeEach(() => {
45+
jest.useFakeTimers({ now: 10 });
46+
});
47+
48+
it('lets through requests with ids being seen for the first time', async () => {
49+
const requests = [
50+
{ id: 1, method: 'foo' },
51+
{ id: 2, method: 'bar' },
52+
].map((request) => ({ ...request, jsonrpc: '2.0' as const }));
53+
54+
const expectedOutput = [
55+
{ id: 1, method: 'foo' },
56+
{ id: 2, method: 'bar' },
57+
].map((output) => ({ ...output, jsonrpc: '2.0' }));
58+
59+
const output = await runStreamTest(requests);
60+
expect(output).toEqual(expectedOutput);
61+
});
62+
63+
it('does not let through the request if the id has been seen before', async () => {
64+
const requests = [
65+
{ id: 1, method: 'foo' },
66+
{ id: 1, method: 'foo' }, // duplicate
67+
].map((request) => ({ ...request, jsonrpc: '2.0' as const }));
68+
69+
const expectedOutput = [{ id: 1, method: 'foo' }].map((output) => ({
70+
...output,
71+
jsonrpc: '2.0',
72+
}));
73+
74+
const output = await runStreamTest(requests);
75+
expect(output).toEqual(expectedOutput);
76+
});
77+
78+
it("lets through requests if they don't have an id", async () => {
79+
const requests = [{ method: 'notify1' }, { method: 'notify2' }].map(
80+
(request) => ({ ...request, jsonrpc: '2.0' as const }),
81+
);
82+
83+
const expectedOutput = [{ method: 'notify1' }, { method: 'notify2' }].map(
84+
(output) => ({ ...output, jsonrpc: '2.0' }),
85+
);
86+
87+
const output = await runStreamTest(requests);
88+
expect(output).toEqual(expectedOutput);
89+
});
90+
91+
it('handles a mix of request types', async () => {
92+
const requests = [
93+
{ id: 1, method: 'foo' },
94+
{ method: 'notify1' },
95+
{ id: 1, method: 'foo' },
96+
{ id: 2, method: 'bar' },
97+
{ method: 'notify2' },
98+
{ id: 2, method: 'bar' },
99+
{ id: 3, method: 'baz' },
100+
].map((request) => ({ ...request, jsonrpc: '2.0' as const }));
101+
102+
const expectedOutput = [
103+
{ id: 1, method: 'foo' },
104+
{ method: 'notify1' },
105+
{ id: 2, method: 'bar' },
106+
{ method: 'notify2' },
107+
{ id: 3, method: 'baz' },
108+
].map((output) => ({ ...output, jsonrpc: '2.0' }));
109+
110+
const output = await runStreamTest(requests);
111+
expect(output).toEqual(expectedOutput);
112+
});
113+
114+
it('expires single id after three minutes', () => {
115+
const output: JsonRpcRequest[] = [];
116+
const testStream = createTestStream(output);
117+
118+
const requests1 = [
119+
{ id: 1, method: 'foo' },
120+
{ id: 1, method: 'foo' },
121+
{ id: 1, method: 'foo' },
122+
];
123+
const expectedOutputBeforeExpiryTime = [{ id: 1, method: 'foo' }];
124+
125+
requests1.forEach((request) => testStream.write(request));
126+
expect(output).toEqual(expectedOutputBeforeExpiryTime);
127+
128+
const requests2 = [
129+
{ id: 1, method: 'foo' },
130+
{ id: 1, method: 'foo' },
131+
{ id: 1, method: 'foo' },
132+
];
133+
const expectedOutputAfterExpiryTime = [
134+
{ id: 1, method: 'foo' },
135+
{ id: 1, method: 'foo' },
136+
];
137+
138+
jest.advanceTimersByTime(THREE_MINUTES);
139+
140+
requests2.forEach((request) => testStream.write(request));
141+
expect(output).toEqual(expectedOutputAfterExpiryTime);
142+
});
143+
144+
it('does not expire single id after less than three', () => {
145+
const output: JsonRpcRequest[] = [];
146+
const testStream = createTestStream(output);
147+
148+
const requests1 = [
149+
{ id: 1, method: 'foo' },
150+
{ id: 1, method: 'foo' },
151+
{ id: 1, method: 'foo' },
152+
];
153+
const expectedOutputBeforeTimeElapses = [{ id: 1, method: 'foo' }];
154+
155+
requests1.forEach((request) => testStream.write(request));
156+
expect(output).toEqual(expectedOutputBeforeTimeElapses);
157+
158+
const requests2 = [
159+
{ id: 1, method: 'foo' },
160+
{ id: 1, method: 'foo' },
161+
{ id: 1, method: 'foo' },
162+
];
163+
const expectedOutputAfterTimeElapses = expectedOutputBeforeTimeElapses;
164+
165+
jest.advanceTimersByTime(THREE_MINUTES - 1);
166+
167+
requests2.forEach((request) => testStream.write(request));
168+
expect(output).toEqual(expectedOutputAfterTimeElapses);
169+
});
170+
171+
it('expires multiple ids after three minutes', () => {
172+
const output: JsonRpcRequest[] = [];
173+
const testStream = createTestStream(output);
174+
175+
const requests1 = [
176+
{ id: 1, method: 'foo' },
177+
{ id: 1, method: 'foo' },
178+
{ id: 2, method: 'bar' },
179+
{ id: 2, method: 'bar' },
180+
{ id: 3, method: 'baz' },
181+
{ id: 3, method: 'baz' },
182+
];
183+
const expectedOutputBeforeExpiryTime = [
184+
{ id: 1, method: 'foo' },
185+
{ id: 2, method: 'bar' },
186+
{ id: 3, method: 'baz' },
187+
];
188+
189+
requests1.forEach((request) => testStream.write(request));
190+
expect(output).toEqual(expectedOutputBeforeExpiryTime);
191+
192+
const requests2 = [
193+
{ id: 3, method: 'baz' },
194+
{ id: 3, method: 'baz' },
195+
{ id: 2, method: 'bar' },
196+
{ id: 2, method: 'bar' },
197+
{ id: 1, method: 'foo' },
198+
{ id: 1, method: 'foo' },
199+
];
200+
const expectedOutputAfterExpiryTime = [
201+
{ id: 1, method: 'foo' },
202+
{ id: 2, method: 'bar' },
203+
{ id: 3, method: 'baz' },
204+
{ id: 3, method: 'baz' },
205+
{ id: 2, method: 'bar' },
206+
{ id: 1, method: 'foo' },
207+
];
208+
209+
jest.advanceTimersByTime(THREE_MINUTES);
210+
211+
requests2.forEach((request) => testStream.write(request));
212+
expect(output).toEqual(expectedOutputAfterExpiryTime);
213+
});
214+
215+
it('expires single id in three minute intervals', () => {
216+
const output: JsonRpcRequest[] = [];
217+
const testStream = createTestStream(output);
218+
219+
const requests1 = [
220+
{ id: 1, method: 'foo' },
221+
{ id: 1, method: 'foo' },
222+
{ id: 1, method: 'foo' },
223+
];
224+
const expectedOutputBeforeExpiryTime = [{ id: 1, method: 'foo' }];
225+
226+
requests1.forEach((request) => testStream.write(request));
227+
expect(output).toEqual(expectedOutputBeforeExpiryTime);
228+
229+
const requests2 = [
230+
{ id: 1, method: 'foo' },
231+
{ id: 1, method: 'foo' },
232+
{ id: 1, method: 'foo' },
233+
];
234+
const expectedOutputAfterFirstExpiryTime = [
235+
{ id: 1, method: 'foo' },
236+
{ id: 1, method: 'foo' },
237+
];
238+
239+
jest.advanceTimersByTime(THREE_MINUTES);
240+
241+
requests2.forEach((request) => testStream.write(request));
242+
expect(output).toEqual(expectedOutputAfterFirstExpiryTime);
243+
244+
const requests3 = [
245+
{ id: 1, method: 'foo' },
246+
{ id: 1, method: 'foo' },
247+
{ id: 1, method: 'foo' },
248+
];
249+
const expectedOutputAfterSecondExpiryTime = [
250+
{ id: 1, method: 'foo' },
251+
{ id: 1, method: 'foo' },
252+
{ id: 1, method: 'foo' },
253+
];
254+
255+
jest.advanceTimersByTime(THREE_MINUTES);
256+
257+
requests3.forEach((request) => testStream.write(request));
258+
expect(output).toEqual(expectedOutputAfterSecondExpiryTime);
259+
});
260+
261+
it('expires somes ids at intervals while not expiring others', () => {
262+
const output: JsonRpcRequest[] = [];
263+
const testStream = createTestStream(output);
264+
265+
const requests1 = [
266+
{ id: 1, method: 'foo' },
267+
{ id: 2, method: 'bar' },
268+
];
269+
const expectedOutputBeforeExpiryTime = [
270+
{ id: 1, method: 'foo' },
271+
{ id: 2, method: 'bar' },
272+
];
273+
274+
requests1.forEach((request) => testStream.write(request));
275+
expect(output).toEqual(expectedOutputBeforeExpiryTime);
276+
277+
const requests2 = [{ id: 3, method: 'baz' }];
278+
const expectedOutputAfterFirstExpiryTime = [
279+
{ id: 1, method: 'foo' },
280+
{ id: 2, method: 'bar' },
281+
{ id: 3, method: 'baz' },
282+
];
283+
284+
jest.advanceTimersByTime(THREE_MINUTES - 1);
285+
286+
requests2.forEach((request) => testStream.write(request));
287+
expect(output).toEqual(expectedOutputAfterFirstExpiryTime);
288+
289+
const requests3 = [
290+
{ id: 1, method: 'foo' },
291+
{ id: 2, method: 'bar' },
292+
{ id: 3, method: 'baz' },
293+
{ id: 4, method: 'buzz' },
294+
];
295+
const expectedOutputAfterSecondExpiryTime = [
296+
{ id: 1, method: 'foo' },
297+
{ id: 2, method: 'bar' },
298+
{ id: 3, method: 'baz' },
299+
{ id: 1, method: 'foo' },
300+
{ id: 2, method: 'bar' },
301+
{ id: 4, method: 'buzz' },
302+
];
303+
304+
jest.advanceTimersByTime(THREE_MINUTES - 1);
305+
306+
requests3.forEach((request) => testStream.write(request));
307+
expect(output).toEqual(expectedOutputAfterSecondExpiryTime);
308+
});
309+
310+
it('handles running expiry job without seeing any ids', () => {
311+
const output: JsonRpcRequest[] = [];
312+
const testStream = createTestStream(output);
313+
314+
const requests1 = [{ id: 1, method: 'foo' }];
315+
const expectedOutputBeforeExpiryTime = [{ id: 1, method: 'foo' }];
316+
317+
requests1.forEach((request) => testStream.write(request));
318+
expect(output).toEqual(expectedOutputBeforeExpiryTime);
319+
320+
jest.advanceTimersByTime(THREE_MINUTES + 1);
321+
322+
expect(output).toEqual(expectedOutputBeforeExpiryTime);
323+
});
324+
});

0 commit comments

Comments
 (0)