feat(perps): wire dedicated aggregated order-book socket per UI connection - #45035
Conversation
…ction Isolate the order-book panel's nSigFigs l2Book subscription on AggregatedOrderBookConnection so it cannot cross-contaminate the controller's shared raw book socket.
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
✨ Files requiring CODEOWNER review ✨🫰 @MetaMask/core-platform (1 files, +13 -0)
👨🔧 @MetaMask/extension-platform (1 files, +13 -0)
👨🔧 @MetaMask/perps (2 files, +466 -24)
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ea3c074. Configure here.
Builds ready [ea3c074]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 10 warn · 🔴 0 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Route the aggregated order-book channel through the injected dedicated connection so raw and grouped l2Book streams no longer share one socket.
|
Builds ready [ccd9fe8]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 9 warn · 🔴 1 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
gauthierpetetin
left a comment
There was a problem hiding this comment.
lgtm for @MetaMask/extension-platform CO file:
- app/scripts/metamask-controller.js
| perpsToggleTestnet: this.messengerClientApi.perpsToggleTestnet, | ||
| isConnectionAlive: () => !outStream.mmFinished, | ||
| subscribeAggregatedOrderBook: (params) => | ||
| aggregatedOrderBookConnection.subscribe(params), |
There was a problem hiding this comment.
non-blocking nit: The aggregatedOrderBookConnection / perpsStream pair are both gated on the same perpsController ternary, so the .subscribe() call can never hit a null reference in practice. However, since aggregatedOrderBookConnection is typed as AggregatedOrderBookConnection | null, this will become a compile error the day metamask-controller.js is migrated to TypeScript. Worth adding either a non-null assertion (aggregatedOrderBookConnection!.subscribe(params)) or optional chaining with a no-op fallback now to avoid the issue later.
🧪 Validation RunVerdict: ✅ nothing introduced — every new retention primitive is torn down — Claim: the dedicated aggregated order-book socket releases what it acquires. head Note Trial run of the MetaMask evidence skills, Wiring a new socket is where subscriptions and pending-teardown maps accumulate, so every retention primitive in the changed file was paired against a release in the same file, run in CI at the merge commit: Ran to completion (exit 0) — read the output, no verdict assertedClaim under test: the dedicated order-book socket releases everything it acquires $ sh -c git diff 0610a607c7480022b7d4c201875f294b38d8b45f^ 0610a607c7480022b7d4c201875f294b38d8b45f > d.patch && python3 .evidence-skills/domains/stability/skills/memory-leak/scripts/retention-scan.py app/scripts/controllers/perps/perps-stream-bridge.ts:d.patch
RETENTION REVIEW — scoped to the supplied diff (re-run: retention-scan.py <file>:<patch> [...])
==========================================================================
perps-stream-bridge.ts
[pre-exist] ok L140: #pendingCandleTeardowns (.set L389)
-> .delete L330
[pre-exist] ok L154: #pendingCandleActivations (.set L363)
-> .delete L368
VERDICT: no retention path INTRODUCED — every NEW primitive is torn down; no heap snapshot warrantedProduced by Follows from the scan above
Open for review: static pairing shows a release exists in the file, not that it runs on every path — an early return or a thrown error between |




Description
Context
The Perps order-book ladder needs a server-aggregated
l2Booksubscription (nSigFigs/mantissa/ fast). PerpsController already keeps a shared Hyperliquid WebSocket for the rawl2Bookused by spread / mid / slippage. The Hyperliquid SDK dispatchesl2Bookevents by coin only, so running raw and aggregated subscriptions for the same coin on one socket cross-contaminates them.Problem
Without a dedicated connection per UI stream, aggregated and raw order-book data for the same market can clobber each other on the shared controller socket.
Solution
In the per-UI-connection setup path of
metamask-controller.js:AggregatedOrderBookConnectionper UI outStream (not a controller singleton), with liveisTestnetreads fromperpsController.state.subscribeAggregatedOrderBookintoPerpsStreamBridge.outstreamEndHandlerafterperpsStream?.destroy().Also stub
AggregatedOrderBookConnectionin the Jest mock for@metamask/perps-controllerso unit tests do not open a real socket.Changelog
CHANGELOG entry: null
Related issues
Relates to #44254
Manual testing steps
Screenshots/Recordings
N/A — background socket wiring only; no UI visual change in this PR.
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Changes real-time perps streaming and WebSocket lifecycle per UI connection; incorrect teardown or subscription identity could leak sockets or show wrong ladder data, though behavior is heavily covered by new bridge tests.
Overview
Wires server-aggregated order book (
nSigFigs/mantissa) through a dedicated Hyperliquid WebSocket per UI outStream, separate from the controller’s shared socket used for the rawl2Book. That avoids SDKl2Bookdispatch-by-coin cross-talk between the grouped ladder and spread/slippage paths.PerpsStreamBridgegainsperpsActivateOrderBookAggregatedStream/perpsDeactivateOrderBookAggregatedStream, injectingsubscribeAggregatedOrderBookand emittingorderBookAggregatedplusorderBookAggregatedStatuswith an optionalsubscriptionIdso the UI can ignore stale packets after grouping changes.Price, raw order book, and aggregated streams now share
#activateDynamicWhenReady/#deactivateDynamicChannelgeneration guards so a deactivate (ordestroy) during pendingperpsInitdoes not resurrect a hidden subscription after teardown.metamask-controller.jsconstructsAggregatedOrderBookConnectionwhen the stream opens and **close()**s it afterperpsStream.destroy(); Jest mocks stub the connection for unit tests.Reviewed by Cursor Bugbot for commit ccd9fe8. Bugbot is set up for automated code reviews on this repo. Configure here.