diff --git a/packages/perps-controller/CHANGELOG.md b/packages/perps-controller/CHANGELOG.md index 91d702fa56a..b26dd4c575a 100644 --- a/packages/perps-controller/CHANGELOG.md +++ b/packages/perps-controller/CHANGELOG.md @@ -52,6 +52,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Covers orders, edits, single and batch cancels (TWAP, scale and chase cancels included), position closes, TP/SL updates and clears, margin updates, withdrawals and transfers between DEXs, including the HIP-3 transfers around an order - HyperLiquid `cancelOrders` reports each order of a batch with its own result when an entry fails: orders the venue cancelled are no longer reported as failed with the batch's error ([#10559](https://github.com/MetaMask/core/pull/10559)) - HyperLiquid orders that set leverage without `marginMode` now keep the open position's margin mode instead of switching to isolated, so flipping a Cross position no longer fails with "Cannot switch leverage type with open position" ([#10588](https://github.com/MetaMask/core/pull/10588)) +- Wait up to `PERPS_CONSTANTS.ConnectionTimeoutMs` for the client's follow-up `init()` when a controller action (such as `placeOrder`) was waiting on a `disconnect()`, so an order submitted during a disconnect-then-init reconnect is placed instead of failing with `CLIENT_NOT_INITIALIZED` ([#10589](https://github.com/MetaMask/core/pull/10589)) + - If that reconnect switched the selected account, the network or the active provider, the action fails with `PROVIDER_LIFECYCLE_STALE` instead of running under the new context. ## [18.0.1] diff --git a/packages/perps-controller/src/PerpsController.ts b/packages/perps-controller/src/PerpsController.ts index d754d639404..f227bb42389 100644 --- a/packages/perps-controller/src/PerpsController.ts +++ b/packages/perps-controller/src/PerpsController.ts @@ -1039,6 +1039,9 @@ export class PerpsController extends BaseController< #initializationPromise: Promise | null = null; + // Actions that saw a disconnect wait here for the client's follow-up init(). + readonly #initializationStartWaiters = new Set<() => void>(); + #isReinitializing = false; #reinitializationOperationPromise: Promise | null = null; @@ -2192,9 +2195,33 @@ export class PerpsController extends BaseController< } this.#initializationPromise = this.#performInitialization(); + this.#initializationStartWaiters.forEach((notifyStarted) => + notifyStarted(), + ); return this.#initializationPromise; } + /** + * Resolve once a new initialization starts, or after the timeout. + * + * @param timeoutMs - Longest time to wait for init() to be called. + * @returns A promise that resolves when init starts or the timeout elapses. + */ + async #waitForInitializationStart(timeoutMs: number): Promise { + let notifyStarted = (): void => undefined; + const started = new Promise((resolve) => { + notifyStarted = resolve; + }); + this.#initializationStartWaiters.add(notifyStarted); + const timeout = setTimeout(notifyStarted, timeoutMs); + try { + await started; + } finally { + clearTimeout(timeout); + this.#initializationStartWaiters.delete(notifyStarted); + } + } + /** * Track a network or provider reinitialization so disconnect can serialize * behind the whole operation, including work before and after init(). @@ -2717,9 +2744,16 @@ export class PerpsController extends BaseController< * @returns The active provider once initialization completes. */ async #getActiveProviderWhenReady(): Promise { + // The context the action was issued under. A client reconnect + // (disconnect then init) that switches account, network or provider must + // not carry the action into the new context. + const issuedContext = this.#getActionContext(); + let awaitedDisconnect = false; + let awaitedInitializationStart = false; while (true) { const pendingDisconnect = this.#disconnectOperationPromise; if (pendingDisconnect) { + awaitedDisconnect = true; await pendingDisconnect; continue; } @@ -2739,10 +2773,45 @@ export class PerpsController extends BaseController< continue; } + // Clients reconnect with disconnect() followed by init(), and the + // disconnect settles before init() is called. Give that init a bounded + // window to start rather than failing an action the reconnect will + // serve. Nothing here starts a connection the client did not ask for. + if ( + awaitedDisconnect && + !awaitedInitializationStart && + !this.isInitialized && + !pendingInitialization + ) { + awaitedInitializationStart = true; + await this.#waitForInitializationStart( + PERPS_CONSTANTS.ConnectionTimeoutMs, + ); + continue; + } + + if (awaitedDisconnect && this.#getActionContext() !== issuedContext) { + throw new Error(PERPS_ERROR_CODES.PROVIDER_LIFECYCLE_STALE); + } + return this.getActiveProvider(); } } + /** + * Identify the account, network and provider an action runs under. + * + * @returns A key that changes when any of them changes. + */ + #getActionContext(): string { + const address = getSelectedEvmAccountFromMessenger(this.messenger)?.address; + return [ + address?.toLowerCase() ?? '', + this.state.isTestnet ? 'testnet' : 'mainnet', + this.state.activeProvider, + ].join('|'); + } + /** * Get the currently active provider, returning null if not available * Use this method when the caller can gracefully handle a missing provider diff --git a/packages/perps-controller/tests/src/PerpsController.lifecycle.test.ts b/packages/perps-controller/tests/src/PerpsController.lifecycle.test.ts index 53b92a6a07a..ad31088b8cb 100644 --- a/packages/perps-controller/tests/src/PerpsController.lifecycle.test.ts +++ b/packages/perps-controller/tests/src/PerpsController.lifecycle.test.ts @@ -15,7 +15,10 @@ import { jest.mock('@nktkas/hyperliquid', () => ({})); -import { PERPS_DISK_CACHE_MARKETS } from '../../src/constants/perpsConfig.js'; +import { + PERPS_CONSTANTS, + PERPS_DISK_CACHE_MARKETS, +} from '../../src/constants/perpsConfig.js'; import { PerpsController, getDefaultPerpsControllerState, @@ -1282,23 +1285,179 @@ describe('PerpsController', () => { return { success: true }; }); - const disconnectPromise = controller.disconnect(); - await disconnectStarted.promise; - const orderPromise = controller.placeOrder({ - symbol: 'BTC', - isBuy: true, - size: '0.1', - orderType: 'market', + jest.useFakeTimers(); + try { + const disconnectPromise = controller.disconnect(); + await disconnectStarted.promise; + const orderPromise = controller.placeOrder({ + symbol: 'BTC', + isBuy: true, + size: '0.1', + orderType: 'market', + }); + const orderRejection = expect(orderPromise).rejects.toThrow( + PERPS_ERROR_CODES.CLIENT_NOT_INITIALIZED, + ); + await Promise.resolve(); + + expect(mockTradingServiceInstance.placeOrder).not.toHaveBeenCalled(); + pendingDisconnect.resolve(); + await disconnectPromise; + // No init follows this disconnect: the order fails once the bounded + // wait for a reconnect runs out. + await jest.advanceTimersByTimeAsync( + PERPS_CONSTANTS.ConnectionTimeoutMs, + ); + await orderRejection; + expect(mockTradingServiceInstance.placeOrder).not.toHaveBeenCalled(); + } finally { + jest.useRealTimers(); + } + }); + + describe('TAT-4041: order submitted during a client reconnect', () => { + it('places an order submitted while a disconnect is followed by init', async () => { + await controller.init(); + const disconnectStarted = createDeferred(); + const pendingDisconnect = createDeferred(); + mockProvider.disconnect.mockImplementationOnce(async () => { + disconnectStarted.resolve(); + await pendingDisconnect.promise; + return { success: true }; + }); + jest + .spyOn(mockTradingServiceInstance, 'placeOrder') + .mockResolvedValue({ success: true, orderId: '123' }); + + // Clients reconnect with disconnect() then init(), with async work + // (for example a cleanup delay) between the two calls. + const reconnect = (async (): Promise => { + await controller.disconnect(); + await new Promise((resolve) => setTimeout(resolve, 50)); + await controller.init(); + })(); + await disconnectStarted.promise; + const orderPromise = controller.placeOrder({ + symbol: 'BTC', + isBuy: true, + size: '0.1', + orderType: 'market', + }); + pendingDisconnect.resolve(); + await reconnect; + + await expect(orderPromise).resolves.toStrictEqual( + expect.objectContaining({ success: true, orderId: '123' }), + ); + expect(mockTradingServiceInstance.placeOrder).toHaveBeenCalledTimes(1); }); - await Promise.resolve(); - expect(mockTradingServiceInstance.placeOrder).not.toHaveBeenCalled(); - pendingDisconnect.resolve(); - await disconnectPromise; - await expect(orderPromise).rejects.toThrow( - PERPS_ERROR_CODES.CLIENT_NOT_INITIALIZED, + it('bounds the wait for init after a disconnect', async () => { + await controller.init(); + jest.useFakeTimers(); + try { + const disconnectPromise = controller.disconnect(); + const orderPromise = controller.placeOrder({ + symbol: 'BTC', + isBuy: true, + size: '0.1', + orderType: 'market', + }); + let settled = false; + orderPromise + .catch(() => undefined) + .finally(() => { + settled = true; + }); + await disconnectPromise; + + await jest.advanceTimersByTimeAsync( + PERPS_CONSTANTS.ConnectionTimeoutMs - 1, + ); + expect(settled).toBe(false); + + await jest.advanceTimersByTimeAsync(1); + await expect(orderPromise).rejects.toThrow( + PERPS_ERROR_CODES.CLIENT_NOT_INITIALIZED, + ); + expect(mockTradingServiceInstance.placeOrder).not.toHaveBeenCalled(); + } finally { + jest.useRealTimers(); + } + }); + + it.each([ + [ + 'account', + (): void => { + const call = mockMessenger.call as unknown as jest.Mock; + const baseCall = call.getMockImplementation(); + call.mockImplementation((action: string, ...args: unknown[]) => + action === + 'AccountTreeController:getAccountsFromSelectedAccountGroup' + ? [ + { + address: '0x9999999999999999999999999999999999999999', + type: 'eip155:eoa', + id: 'account-2', + options: {}, + scopes: ['eip155:1'], + methods: [], + metadata: { + name: 'Other', + importTime: 0, + keyring: { type: 'HD Key Tree' }, + }, + }, + ] + : baseCall?.(action, ...args), + ); + }, + ], + [ + 'network', + (): void => { + controller.testUpdate((state) => { + state.isTestnet = !state.isTestnet; + }); + }, + ], + ])( + 'refuses an order parked across a reconnect that switched the %s', + async (_context, switchContext) => { + await controller.init(); + const disconnectStarted = createDeferred(); + const pendingDisconnect = createDeferred(); + mockProvider.disconnect.mockImplementationOnce(async () => { + disconnectStarted.resolve(); + await pendingDisconnect.promise; + return { success: true }; + }); + jest + .spyOn(mockTradingServiceInstance, 'placeOrder') + .mockResolvedValue({ success: true, orderId: '123' }); + + const reconnect = (async (): Promise => { + await controller.disconnect(); + switchContext(); + await controller.init(); + })(); + await disconnectStarted.promise; + const orderPromise = controller.placeOrder({ + symbol: 'BTC', + isBuy: true, + size: '0.1', + orderType: 'market', + }); + pendingDisconnect.resolve(); + await reconnect; + + await expect(orderPromise).rejects.toThrow( + PERPS_ERROR_CODES.PROVIDER_LIFECYCLE_STALE, + ); + expect(mockTradingServiceInstance.placeOrder).not.toHaveBeenCalled(); + }, ); - expect(mockTradingServiceInstance.placeOrder).not.toHaveBeenCalled(); }); it('keeps init queued when disconnect starts during reinitialization', async () => {