From 78cab4a52f6ff3226f23e8a89648603150072fd0 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 18 Dec 2024 09:48:05 -0700 Subject: [PATCH 1/4] Prevent Engine.destroy test intermittent failure Some unit tests for the Engine do not properly wait for the NetworkController provider to be fully initialized after initializing the engine before attempting to destroy it. This prevents NetworkController's `lookupNetwork` from functioning because it removes an event listener that NetworkController had previously created, causing an error. This commit patches NetworkController to not throw if the controller is destroyed before `lookupNetwork` has a chance to remove the event listener. It also updates Engine to create a promise that resolves when NetworkController's provider finishes initialization. This promise is then awaited in `Engine.destroyEngineInstance`. Finally this commit also ensures that any calls to `Engine.destroyEngineInstance` in tests are properly awaited. --- app/core/Engine/Engine.test.ts | 8 +++-- app/core/Engine/Engine.ts | 18 +++++++++++ .../@metamask+network-controller+22.1.0.patch | 30 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 patches/@metamask+network-controller+22.1.0.patch diff --git a/app/core/Engine/Engine.test.ts b/app/core/Engine/Engine.test.ts index 5af5d8965c8..36ba3aff3fc 100644 --- a/app/core/Engine/Engine.test.ts +++ b/app/core/Engine/Engine.test.ts @@ -108,7 +108,9 @@ describe('Engine', () => { describe('getTotalFiatAccountBalance', () => { let engine: EngineClass; - afterEach(() => engine?.destroyEngineInstance()); + afterEach(async () => { + await engine?.destroyEngineInstance(); + }); const selectedAddress = '0x9DeE4BF1dE9E3b930E511Db5cEBEbC8d6F855Db0'; const chainId: Hex = '0x1'; @@ -372,8 +374,8 @@ describe('Transaction event handlers', () => { jest.spyOn(store, 'getState').mockReturnValue({} as RootState); }); - afterEach(() => { - engine?.destroyEngineInstance(); + afterEach(async () => { + await engine?.destroyEngineInstance(); jest.clearAllMocks(); }); diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index 95c23de7ba2..e9912233923 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -268,6 +268,8 @@ export class Engine { keyringController: KeyringController; + #promiseForNetworkProviderInitialization: Promise; + /** * Creates a CoreController instance */ @@ -333,6 +335,21 @@ export class Engine { }; const networkController = new NetworkController(networkControllerOpts); + this.#promiseForNetworkProviderInitialization = new Promise( + (resolve) => { + this.controllerMessenger.subscribe( + 'NetworkController:stateChange', + (networksMetadata: NetworkState['networksMetadata']) => { + // `networksMetadata` starts out empty and is then updated for the + // currently selected network, which is Mainnet by default + if ('mainnet' in networksMetadata) { + resolve(); + } + }, + (networkControllerState) => networkControllerState.networksMetadata, + ); + }, + ); networkController.initializeProvider(); const assetsContractController = new AssetsContractController({ @@ -1948,6 +1965,7 @@ export class Engine { } async destroyEngineInstance() { + await this.#promiseForNetworkProviderInitialization; // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any Object.values(this.context).forEach((controller: any) => { diff --git a/patches/@metamask+network-controller+22.1.0.patch b/patches/@metamask+network-controller+22.1.0.patch new file mode 100644 index 00000000000..0158b455da7 --- /dev/null +++ b/patches/@metamask+network-controller+22.1.0.patch @@ -0,0 +1,30 @@ +diff --git a/node_modules/@metamask/network-controller/dist/NetworkController.cjs b/node_modules/@metamask/network-controller/dist/NetworkController.cjs +index cc9793f..b94112a 100644 +--- a/node_modules/@metamask/network-controller/dist/NetworkController.cjs ++++ b/node_modules/@metamask/network-controller/dist/NetworkController.cjs +@@ -525,7 +525,11 @@ class NetworkController extends base_controller_1.BaseController { + let networkChanged = false; + const listener = () => { + networkChanged = true; +- this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); ++ try { ++ this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); ++ } catch { ++ // The controller may have been destroyed, so don't worry about it ++ } + }; + this.messagingSystem.subscribe('NetworkController:networkDidChange', listener); + let updatedNetworkStatus; +@@ -575,7 +579,11 @@ class NetworkController extends base_controller_1.BaseController { + // in the process of being called, so we don't need to go further. + return; + } +- this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); ++ try { ++ this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); ++ } catch { ++ // The controller may have been destroyed, so don't worry about it ++ } + this.update((state) => { + const meta = state.networksMetadata[state.selectedNetworkClientId]; + meta.status = updatedNetworkStatus; From 52f4433e779eb48ed3cf75bb8f2e95723cb03b61 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 18 Dec 2024 12:52:24 -0700 Subject: [PATCH 2/4] Be more specific about catching the error --- .../@metamask+network-controller+22.1.0.patch | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/patches/@metamask+network-controller+22.1.0.patch b/patches/@metamask+network-controller+22.1.0.patch index 0158b455da7..112add5aa58 100644 --- a/patches/@metamask+network-controller+22.1.0.patch +++ b/patches/@metamask+network-controller+22.1.0.patch @@ -1,29 +1,47 @@ diff --git a/node_modules/@metamask/network-controller/dist/NetworkController.cjs b/node_modules/@metamask/network-controller/dist/NetworkController.cjs -index cc9793f..b94112a 100644 +index cc9793f..e7334e4 100644 --- a/node_modules/@metamask/network-controller/dist/NetworkController.cjs +++ b/node_modules/@metamask/network-controller/dist/NetworkController.cjs -@@ -525,7 +525,11 @@ class NetworkController extends base_controller_1.BaseController { +@@ -525,7 +525,20 @@ class NetworkController extends base_controller_1.BaseController { let networkChanged = false; const listener = () => { networkChanged = true; - this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); + try { + this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); -+ } catch { -+ // The controller may have been destroyed, so don't worry about it ++ } catch (error) { ++ if ( ++ typeof error === 'object' && ++ error !== null && ++ 'message' in error && ++ error.message === 'Subscription not found for event: NetworkController:networkDidChange' ++ ) { ++ // The controller may have been destroyed, so don't worry about it ++ } else { ++ throw error; ++ } + } }; this.messagingSystem.subscribe('NetworkController:networkDidChange', listener); let updatedNetworkStatus; -@@ -575,7 +579,11 @@ class NetworkController extends base_controller_1.BaseController { +@@ -575,7 +588,20 @@ class NetworkController extends base_controller_1.BaseController { // in the process of being called, so we don't need to go further. return; } - this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); + try { + this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); -+ } catch { -+ // The controller may have been destroyed, so don't worry about it ++ } catch (error) { ++ if ( ++ typeof error === 'object' && ++ error !== null && ++ 'message' in error && ++ error.message === 'Subscription not found for event: NetworkController:networkDidChange' ++ ) { ++ // The controller may have been destroyed, so don't worry about it ++ } else { ++ throw error; ++ } + } this.update((state) => { const meta = state.networksMetadata[state.selectedNetworkClientId]; From aa8b0706a4f21913e8010ed3e5c7f41cae994f10 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 18 Dec 2024 13:03:38 -0700 Subject: [PATCH 3/4] No need to wait for provider initialization --- app/core/Engine/Engine.ts | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index e9912233923..95c23de7ba2 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -268,8 +268,6 @@ export class Engine { keyringController: KeyringController; - #promiseForNetworkProviderInitialization: Promise; - /** * Creates a CoreController instance */ @@ -335,21 +333,6 @@ export class Engine { }; const networkController = new NetworkController(networkControllerOpts); - this.#promiseForNetworkProviderInitialization = new Promise( - (resolve) => { - this.controllerMessenger.subscribe( - 'NetworkController:stateChange', - (networksMetadata: NetworkState['networksMetadata']) => { - // `networksMetadata` starts out empty and is then updated for the - // currently selected network, which is Mainnet by default - if ('mainnet' in networksMetadata) { - resolve(); - } - }, - (networkControllerState) => networkControllerState.networksMetadata, - ); - }, - ); networkController.initializeProvider(); const assetsContractController = new AssetsContractController({ @@ -1965,7 +1948,6 @@ export class Engine { } async destroyEngineInstance() { - await this.#promiseForNetworkProviderInitialization; // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any Object.values(this.context).forEach((controller: any) => { From 2587cbaff0f9e182fedf758b1e2fb0620a23d71f Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 18 Dec 2024 13:06:09 -0700 Subject: [PATCH 4/4] No need to await destroy in tests either --- app/core/Engine/Engine.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/core/Engine/Engine.test.ts b/app/core/Engine/Engine.test.ts index 36ba3aff3fc..5af5d8965c8 100644 --- a/app/core/Engine/Engine.test.ts +++ b/app/core/Engine/Engine.test.ts @@ -108,9 +108,7 @@ describe('Engine', () => { describe('getTotalFiatAccountBalance', () => { let engine: EngineClass; - afterEach(async () => { - await engine?.destroyEngineInstance(); - }); + afterEach(() => engine?.destroyEngineInstance()); const selectedAddress = '0x9DeE4BF1dE9E3b930E511Db5cEBEbC8d6F855Db0'; const chainId: Hex = '0x1'; @@ -374,8 +372,8 @@ describe('Transaction event handlers', () => { jest.spyOn(store, 'getState').mockReturnValue({} as RootState); }); - afterEach(async () => { - await engine?.destroyEngineInstance(); + afterEach(() => { + engine?.destroyEngineInstance(); jest.clearAllMocks(); });