-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Initialize asset controllers with the current network #1361
Initialize asset controllers with the current network #1361
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
1e825b7
to
be06843
Compare
04b418e
to
76a0b37
Compare
@@ -122,7 +122,7 @@ export interface ApiNftCreator { | |||
*/ | |||
export interface NftDetectionConfig extends BaseConfig { | |||
interval: number; | |||
chainId: `0x${string}` | `${number}` | number; | |||
chainId: string; |
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 is only ever set via the network controller provider config, so it has been updated to be just string
, which is what the type is at the moment in the network controller. This will get more specific in an upcoming PR.
76a0b37
to
f9683e5
Compare
Thank you for doing this, well-needed! I note that each controller still inlines the default chainid. Do you think it would make sense to introduce a constant |
Hmm, I'm not aware of any examples of this. Maybe I missed some cases. They are intended to have no default chain ID. |
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.
Makes sense!
Certain controllers in `@metamask/assets-controllers` would default to assuming the current selected network was Ethereum Mainnet, or they'd default with an empty string as the `chainId` (causing errors if any methods were called). Instead they now all get initialized with the current chain ID, passed in as a new constructor parameter. Affected controllers are: * `AssetsContractController` * `NftController` * `NftDetectionController` * `TokenRatesController` * `TokensController` Relates to #1209
9820305
to
d11e953
Compare
* Initialize asset controllers with the current network Certain controllers in `@metamask/assets-controllers` would default to assuming the current selected network was Ethereum Mainnet, or they'd default with an empty string as the `chainId` (causing errors if any methods were called). Instead they now all get initialized with the current chain ID, passed in as a new constructor parameter. Affected controllers are: * `AssetsContractController` * `NftController` * `NftDetectionController` * `TokenRatesController` * `TokensController` Relates to #1209 * Fix more tests
* Initialize asset controllers with the current network Certain controllers in `@metamask/assets-controllers` would default to assuming the current selected network was Ethereum Mainnet, or they'd default with an empty string as the `chainId` (causing errors if any methods were called). Instead they now all get initialized with the current chain ID, passed in as a new constructor parameter. Affected controllers are: * `AssetsContractController` * `NftController` * `NftDetectionController` * `TokenRatesController` * `TokensController` Relates to #1209 * Fix more tests
Description
Certain controllers in
@metamask/assets-controllers
would default to assuming the current selected network was Ethereum Mainnet, or they'd default with an empty string as thechainId
(causing errors if any methods were called). Instead they now all get initialized with the current chain ID, passed in as a new constructor parameter.Affected controllers are:
AssetsContractController
NftController
NftDetectionController
TokenRatesController
TokensController
Changes
chainId
required constructor parameter:AssetsContractController
NftController
NftDetectionController
TokenRatesController
TokensController
References
Relates to #1209
Checklist