-
-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: add removeNetwork
to multichain-network-controller
#5516
Conversation
removeNetwork
to multichain-network-controller
@@ -53,6 +66,23 @@ export function checkIfSupportedCaipChainId( | |||
export const toEvmCaipChainId = (chainId: Hex): CaipChainId => | |||
toCaipChainId(KnownCaipNamespace.Eip155, hexToNumber(chainId).toString()); | |||
|
|||
/** | |||
* Convert an eip155 CAIP chain ID to a hex 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.
Question: I wonder if we should check for the special case for EOA? cause eip155:0
would give a chain ID of 0, which is not really valid for that context. (It is valid for us regarding the scopes
and CAIP, but not here IMO). Having said that, we should never end up using eip155:0
either (not in the "network management" context at least)
WDYT?
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 agree with both points, guarding against eip155:0
would add a layer of error management and it should apply to multiple methods on this class but as you said it shouldn't be part of this context. Will think about it in the meanwhile
packages/multichain-network-controller/src/MultichainNetworkController.ts
Show resolved
Hide resolved
packages/multichain-network-controller/src/MultichainNetworkController.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Charly Chevalier <[email protected]>
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!
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
References
Changelog
@metamask/multichain-network-controller
removeNetwork
method. The new added method handles two cases,network-controller
if certain conditions are met@metamask/network-controller
Checklist