-
-
Notifications
You must be signed in to change notification settings - Fork 268
NftDetectionController: providerConfig -> selectedNetworkClientId #4307
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
NftDetectionController: providerConfig -> selectedNetworkClientId #4307
Conversation
81c0ef0 to
b0facae
Compare
| }); | ||
| }); | ||
|
|
||
| it('should respond to chain ID changing when using legacy polling', async () => { |
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 was a missing test.
| async ({ controller, controllerEvents }) => { | ||
| const selectedAddress = '0x1'; | ||
| triggerPreferencesStateChange({ | ||
| controllerEvents.preferencesStateChange({ |
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.
Note that there are a lot of changes in this PR that are unrelated to the change from providerConfig to selectedNetworkClientId. I took this opportunity to align the tests in this file to use our latest convention for mocking events couched as callbacks.
| preferencesStateChangeListeners.push(listener); | ||
| controllerEvents.preferencesStateChange = listener; | ||
| }, | ||
| onNetworkStateChange: (listener) => { |
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.
Previously we were just passing jest.fn() as the value for this callback, but now we are capturing the listener so that we can call it in tests, effectively firing the event.
| }, | ||
| ], | ||
| }); | ||
| console.log(nock.activeMocks()); |
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.
I am not sure why we were doing this; this was just adding to the noise when running these tests.
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.
I guess someone forgot to remove it while debugging
The `providerConfig` state property is being removed from NetworkController. Currently this property is used in NftDetectionController to get the currently selected chain, but `selectedNetworkClientId` can be used instead. This commit makes that transition so that we can fully drop `providerConfig`.
b0facae to
23a21ec
Compare
cryptodev-2s
left a comment
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
providerConfigstate property is being removed from NetworkController. Currently this property is used in NftDetectionController to get the currently selected chain, butselectedNetworkClientIdcan be used instead. This commit makes that transition so that we can fully dropproviderConfig.References
Progresses #4185.
Changelog
(Updates to changelog in PR.)
Checklist