-
-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: SelectedNetworkController
should return globally selected networkClient from getProviderAndBlockTracker
when domain not in state
#4063
Conversation
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
globallySelectedNetworkClient.blockTracker, | ||
); | ||
} else if (networkProxy) { | ||
this.#proxies.delete(domain); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still need to destroy the proxies to avoid a memory leak.
From @Gudahtt :
We can counteract this by storing the proxies as WeakRefs using the strategy described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_management#weakrefs_and_finalizationregistry
SelectedNetworkController
should return globally selected networkClient from getProviderAndBlockTracker
when domain not in state
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
], | ||
); | ||
|
||
// TODO(JL): replace this with a test that the proxy is removed from cache when the domainProxyMap PR is merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO after #4104 is merged
@@ -654,6 +665,21 @@ export class NetworkController extends BaseController< | |||
}; | |||
} | |||
|
|||
getSelectedNetworkClient(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should have a JSDoc description.
Maybe also worth adding a @deprecated
tag on getProviderAndBlockTracker
(it's equivalent to this, except more difficult to work with because of the return type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done here: b0f1670
|
||
this.update((state) => { | ||
state.domains[domain] = networkClientId; | ||
}); | ||
} | ||
|
||
#unsetNetworkClientIdForDomain(domain: Domain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could use a JSDoc description here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done here: b0f1670
…ructor params (#4104) ## Explanation In a [preceding PR](#4063), the SelectedNetworkController will start to keep proxy instances for all domains seen without a way to remove them when they become stale. This PR exposes the private proxies cache map as a constructor param which will enable callers to implement their own cache invalidation strategies without needing to concern the SelectedNetworkController of the implementation details themselves. This is needed because Mobile and Extension will need to follow different cache invalidation strategies. This PR should be merged before #4063 ## References Fixes: #4062 See: #4063 ## Changelog ### `@metamask/selected-network-controller` - **BREAKING**: `SelectedNetworkController` now expects the `domainProxyMap` param which is a `Map` of `Domain` to `NetworkProxy` - **ADDED**: exports the `Domain` type ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex Donesky <adonesky@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com>
5828800
to
f3fd820
Compare
f3fd820
to
af622c6
Compare
af622c6
to
b0f1670
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Explanation
The SelectedNetworkController currently constructs a proxy for any domain that has permissions. Other domains have no associated proxy, so the getProviderAndBlockTracker method would throw an error.
This is problematic because we grab the network client for an origin a single time when constructing an RPC pipeline for that origin in the MetaMask extension. We don't re-create the RPC pipeline when permissions change. That means that the pipeline is setup with the wrong network client (see here for more details on this bug: MetaMask/metamask-extension#23509 )
To resolve this problem, we can instead keep network client proxies for all origins, not just those with permissions. Origins without permissions still should not have a selected network client ID state, but they can have proxies that point at the globally selected network client. That way, when the origin is granted permissions, we can redirect this proxy to the selected network client and everything works smoothly, without needing to reconstruct the RPC pipeline.
This PR should be merged shortly after #4104
References
Fixes: #4062
See: #4104
Changelog
@metamask/network-controller
getSelectedNetworkClient()
method that returns the provider and blockTracker for the currently selected network@metamask/selected-network-controller
SelectedNetworkController
now requires theNetworkController:getSelectedNetworkClient
messenger actionSelectedNetworkController. getProviderAndBlockTracker()
no longer throws an error if theuseRequestQueue
flag is falseSelectedNetworkController. getProviderAndBlockTracker()
threw an error if there was no networkClientId set for the passed domain. Now it returns a proxy that follows the globally selected network instead.Checklist