Ad/fix/update get provider and block tracker for snaps #4259
Conversation
…proxied NetworkController selected network client"
…ly selected proxy if requesting domain is "metamask"
packages/selected-network-controller/tests/SelectedNetworkController.test.ts
Show resolved
Hide resolved
| const snapDomain = 'npm:@metamask/bip32-example-snap'; | ||
| const result = controller.getProviderAndBlockTracker(snapDomain); | ||
| expect(domainProxyMap.get(snapDomain)).toBeUndefined(); | ||
| expect(messenger.call).toHaveBeenCalledWith( |
There was a problem hiding this comment.
Nit: Hmm, this feels like testing internal implementation details, rather than testing the behavior we want. In theory we might refactor this to use a new equivalent action, or we might end up calling this in other code paths, or we might rename this action, etc. which could break the test despite the behaviour the test is meant to test being unchanged.
To make this test less fragile, we could try switching the globally selected network and test that the provider we got has the correct chain before and after the switch. Maybe that would work, that's just the first suggestion that comes to mind.
There was a problem hiding this comment.
I totally agree that this is not a great way to test it. Testing it the right way involves pulling in the actual NetworkController which I was hoping to do at a later time 😅 . I put this TODO in there about this same point a few weeks ago:
There was a problem hiding this comment.
added the TODO in another spot here: 0fd8b54
There was a problem hiding this comment.
Hmm, we've been trying to move away from using actual full controllers as unit test fixtures as that tends to be fragile as well for different reasons. e.g. any breaking change to controller construction, initialization, etc. breaks unrelated tests, and sometimes they have internals that aren't well suited to unit tests that are difficult to mock out (e.g. network requests).
Leaving this improvement as a TODO sounds good to me though!
packages/selected-network-controller/tests/SelectedNetworkController.test.ts
Show resolved
Hide resolved
…roller.test.ts Co-authored-by: Mark Stacey <markjstacey@gmail.com>
# @metamask/selected-network-controller ## [13.0.0] ### Changed - `getProviderAndBlockTracker` now returns the `NetworkController`'s globally selected network client proxy if the `domain` arg is either `metamask` or a snap (identified as starting with `npm:` or `local:`) ([#4259](#4259)) - **BREAKING:** Now when `setNetworkClientIdForDomain` is called with a snap's domain (identified as starting with `npm:` or `local:`), the `domain` will not be added to state and no proxy will be created for this domain in the `domainProxyMap` ([#4258](#4258)) - In order to remove snaps that made it into `domains` state prior to this change, consumers will need to run a migration. --------- Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Explanation
Modifies
SelectedNetworkController's,getProviderAndBlockTrackermethod to return theNetworkController's globally selected network client proxy if the domain arg is eithermetamaskor a snap (identified by if prefixed bynpm:orlocal:)References
see this thread for further information
Changelog
@metamask/selected-network-controllerSelectedNetworkController's,getProviderAndBlockTrackermethod to return theNetworkController's globally selected network client proxy if thedomainarg is eithermetamaskor a snap (identified as starting withnpm:orlocal:)Checklist