feat(perps): sync PerpsController implementation for npm publishing#7941
feat(perps): sync PerpsController implementation for npm publishing#7941abretonc7s wants to merge 28 commits intomainfrom
Conversation
Add all required dependencies to perps-controller package.json (account-tree-controller, bridge-controller, keyring-controller, network-controller, transaction-controller, etc.) and corresponding tsconfig.build.json project references so the package builds correctly when source files are synced from mobile.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution MetaMask internal reviewing guidelines:
Ignoring alerts on:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Synced from mobile PR #26110: - toggleTestnet: check InitializationState.Failed after init() and rollback isTestnet on failure (matching switchProvider pattern) - depositWithConfirmation: replace never-resolving promise with Promise.resolve(transactionMeta.id) when placeOrder=true
Hoist currentDepositId before try block so it is accessible in the outer catch. When a pre-submission error occurs (e.g. missing networkClientId), the deposit request is now marked as 'failed' instead of staying permanently 'pending' in state.
Prevent standalone preload from leaking WebSocket providers by caching the HyperLiquidProvider instance across standalone calls and cleaning it up at lifecycle boundaries (init, disconnect, toggleTestnet, etc.). Reorder switchProvider() to check "already active" before validating the providers map, so it returns a no-op success before init().
Add uuidv4() fallback for currentDepositId which TypeScript cannot narrow inside the update() callback after the variable was hoisted to let binding with string | undefined type.
The base tsconfig.json was missing project references that tsconfig.build.json already had, causing TS2345 errors for imports like @metamask/account-tree-controller during type checking.
Jest exits with code 1 when no test files exist. Add a minimal placeholder test in tests/ (outside src/ so the Mobile sync script does not delete it) and scope collectCoverageFrom to that file only, preventing 0% coverage failures on the synced source.
…cription Register all 34 PerpsControllerActions via registerMethodActionHandlers() so inter-controller communication works through the messenger in core. Store the RemoteFeatureFlagController:stateChange subscription handle and clean it up in disconnect() to prevent leaks.
… and account switch - Remove feature-flag unsubscribe from disconnect() so geo-blocking and HIP-3 flag changes keep propagating after reconnect cycles - Add deposit request lifecycle tracking for the deposit+order flow so requests transition from pending to completed/failed/cancelled - Clear cached user data when switching to a non-EVM account group to prevent stale positions/orders from the previous EVM account
|
@SocketSecurity ignore npm/@nktkas/hyperliquid@0.30.3 |
|
@SocketSecurity ignore npm/@inquirer/external-editor@2.0.3 |
|
@SocketSecurity ignore npm/@myx-trade/sdk@0.1.265 |
|
@SocketSecurity ignore npm/@noble/curves@2.0.1 |
|
@SocketSecurity ignore npm/ox@0.12.1 |
|
@SocketSecurity ignore npm/viem@2.45.3 |
|
@SocketSecurity ignore npm/@scure/bip39@1.6.0 |
…-migration-sync # Conflicts: # packages/perps-controller/package.json # yarn.lock
| state.depositInProgress = false; | ||
| state.lastDepositTransactionId = null; | ||
| // Don't set lastDepositResult - no toast needed | ||
| }); |
There was a problem hiding this comment.
Deposit request stays 'pending' forever on user cancellation
Medium Severity
When the user cancels a deposit transaction in the !placeOrder (submit) path, the result.catch() handler clears depositInProgress and lastDepositTransactionId but does not update the corresponding entry in state.depositRequests. The deposit request created earlier remains with status: 'pending' indefinitely. By contrast, the depositOrderResult .catch() handler correctly transitions the request to 'cancelled' or 'failed'. This inconsistency causes orphaned pending deposit records in the persistent transaction history.
Additional Locations (1)
| limitPrice?: string; // Limit price (for limit orders) | ||
| orderType?: OrderType; // Market vs limit | ||
| timestamp: number; // When the config was saved (for expiration check) | ||
| }; |
There was a problem hiding this comment.
State type missing selectedPaymentToken in pendingConfig
Low Severity
The pendingConfig shape in PerpsControllerState (for both testnet and mainnet) does not include selectedPaymentToken, yet savePendingTradeConfiguration writes it via spread and getPendingTradeConfiguration declares it in its return type. The value exists at runtime but the state type is unaware of it, which means direct state reads, serialization validation, or future state migrations could silently drop it.
Additional Locations (1)
|
|
||
| // Reset initialization state to ensure proper reconnection | ||
| this.isInitialized = false; | ||
| this.#initializationPromise = null; |
There was a problem hiding this comment.
disconnect() leaves initializationState stale in persisted state
Medium Severity
disconnect() resets this.isInitialized and this.#initializationPromise but never updates state.initializationState, leaving it as Initialized even though the controller is disconnected. Since initializationState is marked usedInUi: true, UI consumers reading this state would incorrectly show the controller as connected. By contrast, switchProvider() correctly sets state.initializationState to Uninitialized before re-initializing.
Additional Locations (1)
| [PERPS_EVENT_PROPERTY.WITHDRAWAL_AMOUNT]: | ||
| Number.parseFloat(withdrawalAmount), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Analytics call inside Immer producer risks aborting state update
Low Severity
this.#getMetrics().trackPerpsEvent() is called inside the this.update() Immer producer in updateWithdrawalStatus. If that call throws, Immer rolls back the entire draft, so the withdrawal status change, success flag, and progress reset are all silently lost. The analytics side effect couples a non-critical operation with a critical state mutation.
| requestToUpdate.status = ( | ||
| isCancellation ? 'cancelled' : 'failed' | ||
| ) as TransactionStatus; | ||
| requestToUpdate.success = false; |
There was a problem hiding this comment.
Invalid 'cancelled' status bypasses TransactionStatus type
Medium Severity
The deposit+order cancellation path sets requestToUpdate.status to 'cancelled' via an as TransactionStatus cast, but TransactionStatus is 'pending' | 'bridging' | 'completed' | 'failed' — it does not include 'cancelled'. This means cancelled deposit requests persist in state with an unrecognized status value. Since depositRequests has persist: true, these ghost records accumulate forever and are never cleaned up by clearPendingTransactionRequests (which only filters 'pending' and 'bridging').
Additional Locations (1)
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
## **Description** Addresses 5 confirmed Bugbot findings from [Core PR #7941](MetaMask/core#7941). ### What changed 1. **Missing 'canceled' spelling** — deposit+order `.catch()` only checked `'cancelled'`, not `'canceled'` 2. **Cancelled deposits stay pending** — non-placeOrder cancellation cleared UI state but left the deposit request as `'pending'` 3. **Stale `activeProviderInstance` after disconnect** — standalone reads could route through a disconnected provider 4. **Disconnect skips preload teardown** — preload timer and messenger subscriptions were not stopped, causing background work during teardown 5. **Analytics side effect inside immer producer** — `trackPerpsEvent()` and `#debugLog()` moved out of `this.update()` ### Disconnect design note Fix 4 inlines the preload cleanup logic from `stopMarketDataPreload()` rather than calling it directly, because `stopMarketDataPreload()` calls `#cleanupStandaloneProvider()` fire-and-forget, while `disconnect()` needs the awaited version to prevent races with reconnection. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: Bugbot findings on MetaMask/core#7941 ## **Manual testing steps** ```gherkin Feature: Perps deposit cancellation and disconnect cleanup Scenario: user cancels a deposit transaction Given user is on the Perps deposit screen with a pending deposit When user rejects the transaction in the wallet prompt Then the deposit request status should be 'cancelled' (not stuck as 'pending') Scenario: user disconnects from Perps Given user is on Perps screens with active market data preload When user navigates away triggering disconnect Then preload timer is stopped, provider instance is nulled, and no stale subscriptions remain ``` ## **Screenshots/Recordings** N/A — internal controller logic, no UI changes. ## **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** - [ ] 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** > Touches deposit/withdrawal status transitions and disconnect teardown ordering, which can affect transaction UX, metrics emission, and background polling behavior if regressions occur. > > **Overview** > Fixes several PerpsController lifecycle edge cases around deposits, withdrawals, and teardown. > > Deposit tracking now correctly treats both `cancelled` and `canceled` error strings as user cancellation, and ensures non-order deposit cancellations update the corresponding `depositRequests` entry to `cancelled` instead of leaving it `pending`. > > `updateWithdrawalStatus` no longer performs analytics/logging side-effects inside the `this.update()` immer producer; it computes whether tracking/logging is needed during the state update and runs `trackPerpsEvent`/`#debugLog` afterward. > > `disconnect()` now proactively stops market-data preload intervals and unsubscribes messenger listeners, resets cached preload config, clears `activeProviderInstance` to avoid stale routing, and awaits standalone-provider cleanup. A new unit test asserts preload work does not continue after disconnect. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 330b560. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Sync from Mobile (b6279ca996). Replace PerpsPlatformDependencies.controllers.* with messenger.call() / messenger.subscribe() for all cross-controller communication. Add typed PerpsControllerAllowedActions/Events. Rewards stays as top-level DI (no RewardsController in Core yet). Adds sync-state tracking with source checksum for conflict detection.
| (req) => req.id === currentDepositId, | ||
| ); | ||
| if (requestToUpdate) { | ||
| requestToUpdate.status = 'cancelled' as TransactionStatus; |
There was a problem hiding this comment.
Assigning 'cancelled' to TransactionStatus field bypasses type safety
Medium Severity
TransactionStatus is defined as 'pending' | 'bridging' | 'completed' | 'failed' in transactionTypes.ts, but the deposit flow assigns 'cancelled' using as TransactionStatus to bypass the type checker. The depositRequests state field declares status: TransactionStatus, so at runtime these records contain a value outside the type's domain. Any downstream code that exhaustively matches on TransactionStatus values (UI filters, history displays, status checks) will never match 'cancelled' records — they become invisible ghost entries in persisted state.
Additional Locations (1)
Messenger types (PerpsControllerAllowedActions, PerpsControllerAllowedEvents, PerpsControllerMessengerBase) are now in types/messenger.ts instead of types/index.ts. This prevents them from leaking into the public API via the barrel export, avoiding forcing consumers to install 6 controller packages as transitive dependencies.
Replace inline parameter types with the existing structural types PerpsTransactionParams and PerpsAddTransactionOptions, removing dead code.
|
@metamaskbot publish-previews |
1 similar comment
|
@metamaskbot publish-previews |
| (req) => req.id === currentDepositId, | ||
| ); | ||
| if (requestToUpdate) { | ||
| requestToUpdate.status = 'cancelled' as TransactionStatus; |
There was a problem hiding this comment.
'cancelled' is not a valid TransactionStatus value
Medium Severity
The TransactionStatus type is defined as 'pending' | 'bridging' | 'completed' | 'failed', but the code writes 'cancelled' to the status field using as TransactionStatus casts in two locations within depositWithConfirmation. This bypasses the type system, placing an invalid value into persisted state (depositRequests). Any downstream code that performs exhaustive matching or filtering on TransactionStatus won't recognize 'cancelled', leading to silent misclassification. The 'cancelled' variant needs to be added to the TransactionStatus union type.
Additional Locations (2)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| (req) => req.id === currentDepositId, | ||
| ); | ||
| if (requestToUpdate) { | ||
| requestToUpdate.status = 'cancelled' as TransactionStatus; |
There was a problem hiding this comment.
Invalid 'cancelled' status cast bypasses TransactionStatus type
Medium Severity
The code assigns 'cancelled' as TransactionStatus to deposit request statuses, but TransactionStatus is defined as 'pending' | 'bridging' | 'completed' | 'failed' and does not include 'cancelled'. The as cast silences the type checker, meaning persisted controller state can contain a value that no consumer expects. Any exhaustive switch/check on TransactionStatus will miss this case, and filtering logic (e.g., clearPendingTransactionRequests) doesn't account for it.
Additional Locations (2)
| limitPrice?: string; // Limit price (for limit orders) | ||
| orderType?: OrderType; // Market vs limit | ||
| timestamp: number; // When the config was saved (for expiration check) | ||
| }; |
There was a problem hiding this comment.
State type missing selectedPaymentToken in pendingConfig definition
Medium Severity
The pendingConfig type in PerpsControllerState (for both testnet and mainnet) omits selectedPaymentToken, but savePendingTradeConfiguration spreads the full config object (which includes selectedPaymentToken) into pendingConfig via ...config. Both getPendingTradeConfiguration and selectPendingTradeConfiguration declare selectedPaymentToken in their return types. The field is persisted at runtime but invisible to TypeScript, so any typed access to state.tradeConfigurations[network][symbol].pendingConfig.selectedPaymentToken would fail to compile, and the state metadata won't document this persisted field.
Additional Locations (2)
|
@metamaskbot publish-previews |
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.
| (req) => req.id === currentDepositId, | ||
| ); | ||
| if (requestToUpdate) { | ||
| requestToUpdate.status = 'cancelled' as TransactionStatus; |
There was a problem hiding this comment.
'cancelled' is not a valid TransactionStatus value
Medium Severity
The TransactionStatus type is defined as 'pending' | 'bridging' | 'completed' | 'failed', but depositWithConfirmation assigns 'cancelled' as TransactionStatus to deposit requests in multiple places. The as cast silently bypasses type safety, storing an invalid value in persisted state. Downstream, clearPendingTransactionRequests only filters 'pending' and 'bridging' statuses, so cancelled deposit requests accumulate indefinitely in depositRequests without any cleanup path.
Additional Locations (2)
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.
| (req) => req.id === currentDepositId, | ||
| ); | ||
| if (requestToUpdate) { | ||
| requestToUpdate.status = 'cancelled' as TransactionStatus; |
There was a problem hiding this comment.
'cancelled' value doesn't exist in TransactionStatus union type
Medium Severity
The TransactionStatus type is defined as 'pending' | 'bridging' | 'completed' | 'failed', but the code stores 'cancelled' into status fields using as TransactionStatus casts, bypassing type safety. This means persisted state contains a value that no consumer expects. Any downstream code exhaustively handling TransactionStatus (e.g., switch statements, equality checks, UI display logic) will silently miss the 'cancelled' case, potentially showing incorrect status or falling through to unexpected defaults.
Additional Locations (2)
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
3 participants
Explanation
Syncs
PerpsControllerfrom Mobile into Core as@metamask/perps-controllerfor npm publishing.Architecture (latest): Cross-controller communication uses the standard
@metamask/messengerpattern —messenger.call()andmessenger.subscribe(). TypedPerpsControllerAllowedActions/PerpsControllerAllowedEventsdefine the contract. The only DI remaining isPerpsPlatformDependencies.rewards(RewardsController is not yet in Core) and platform-specific adapters (logger, metrics, tracer, stream manager, etc.).This addresses Mark Stacey's concern from the ADR-42 review: PerpsController uses the same messenger architecture as all other Core controllers, with no special DI bridging for controller-to-controller calls.
Messenger actions used:
AccountTreeController:getAccountsFromSelectedAccountGroupKeyringController:getStateKeyringController:signTypedMessageNetworkController:getStateNetworkController:getNetworkClientByIdNetworkController:findNetworkClientIdByChainIdTransactionController:addTransactionAuthenticationController:getBearerTokenRemoteFeatureFlagController:getStateMessenger events subscribed:
RemoteFeatureFlagController:stateChangeAccountTreeController:selectedAccountGroupChangeApproach: Mobile remains the source of truth. A sync script (
validate-core-sync.sh) copies controller source to this package, verifies build + lint, and writes a.sync-state.jsonwith commit hashes and source checksum for conflict detection. This is a transitional mechanism that goes away when Mobile folds into Core asapps/mobile.Key components:
PerpsController— main controller with state management, messenger integration, and multi-provider orchestrationHyperLiquidProvider/MYXProvider— DEX-specific provider implementationsAggregatedPerpsProvider— multi-provider aggregation layerProviderRouter— routes operations to the appropriate providerSubscriptionMultiplexer— real-time WebSocket data aggregationPerpsPlatformDependencies— platform-agnostic injection interface (rewards, logger, metrics, tracer, stream manager, feature flags, market data formatters, cache invalidator)CI / Testing: A minimal placeholder test (
tests/placeholder.test.ts) is included to satisfy Core's CI requirement. Full unit tests remain in Mobile (27 suites, 1295 tests passing) and will be migrated when development moves fully to Core.References
perps-controllerpackage scaffolding)validate-core-sync.sh)Checklist
Note
High Risk
Large new controller implementation that orchestrates perps trading, WebSocket live data, feature-flag gating, and transaction submission via other controllers; bugs here could impact trading actions, network selection, and user funds flows. The change is also high-risk due to its size and new cross-controller messenger surface area.
Overview
Syncs
packages/perps-controllerfrom a scaffold into a full perps trading controller:PerpsControllernow has a rich persisted state, initialization/reinit logic, messenger-exposed methods for trading/account/market operations, and live WebSocket subscription APIs.Adds multi-provider plumbing (e.g.,
AggregatedPerpsProvider+ newSubscriptionMultiplexer) and portable constants (e.g.,constants/chartConfig,constants/eventNames) to support routing/aggregation and analytics.Updates packaging and test/CI wiring: adds provider/SDK dependencies in
package.json, narrows Jest coverage collection totests/placeholder.test.ts, removes the prior stub unit test, and introduces.sync-state.jsonplus changelog entries documenting the synced implementation.Written by Cursor Bugbot for commit b014892. This will update automatically on new commits. Configure here.