-
-
Notifications
You must be signed in to change notification settings - Fork 239
chore: create network order controller #6022
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
base: main
Are you sure you want to change the base?
Conversation
0f98de3
to
66114bc
Compare
445dd16
to
d2a8ef8
Compare
|
||
// Currently the tests for NetworkController have a race condition which | ||
// causes intermittent failures. This seems to fix it. | ||
testEnvironment: 'jsdom', |
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: do you think we can drop this as may not be related to this controller ?
@@ -0,0 +1,15 @@ | |||
# `@metamask/network-order-controller` | |||
|
|||
Provides an interface to the currently network order via a MetaMask-compatible provider object |
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: what do you think about rephrasing this to ?
Provides an interface to the currently network order via a MetaMask-compatible provider object | |
Provides an interface to the current network display order via a MetaMask-compatible provider object |
{ | ||
"name": "@metamask/network-order-controller", | ||
"version": "0.0.0", | ||
"description": "Provides an interface to the currently network order via a MetaMask-compatible provider object", |
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: to align with README, if you opt in for the suggestion
"description": "Provides an interface to the currently network order via a MetaMask-compatible provider object", | |
"description": "Provides an interface to the current network display order via a MetaMask-compatible provider object", |
export type NetworkOrderStateChange = { | ||
type: `${typeof controllerName}:stateChange`; | ||
payload: [NetworkOrderControllerState, Patch[]]; | ||
}; |
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.
export type NetworkOrderStateChange = { | |
type: `${typeof controllerName}:stateChange`; | |
payload: [NetworkOrderControllerState, Patch[]]; | |
}; | |
export type NetworkOrderStateControllerChangeEvent = ControllerStateChangeEvent< | |
typeof controllerName, | |
NetworkOrderControllerState | |
>; | |
export type NetworkOrderControllerEvents = NetworkOrderStateControllerChangeEvent; | |
/** | |
* All events that {@link NetworkOrderController} calls internally. | |
*/ | |
type AllowedEvents = NetworkControllerStateChangeEvent; |
}; | ||
|
||
// Describes the action for updating the networks list | ||
export type NetworkOrderControllerupdateNetworksListAction = { |
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.
export type NetworkOrderControllerupdateNetworksListAction = { | |
export type NetworkOrderControllerUpdateNetworksListAction = { |
* @param networkControllerState - The state of the network controller. | ||
* @param networkControllerState.networkConfigurationsByChainId - The network configurations by chain id. | ||
*/ | ||
onNetworkControllerStateChange({ |
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: this method can be private ?
onNetworkControllerStateChange({ | |
#onNetworkControllerStateChange({ |
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.
can we move this file under src directory ?
"references": [ | ||
{ | ||
"path": "../../packages/base-controller/tsconfig.build.json" | ||
}, | ||
{ | ||
"path": "../../packages/network-controller/tsconfig.build.json" | ||
}, | ||
{ | ||
"path": "../../packages/multichain-network-controller/tsconfig.build.json" | ||
} | ||
], |
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.
"references": [ | |
{ | |
"path": "../../packages/base-controller/tsconfig.build.json" | |
}, | |
{ | |
"path": "../../packages/network-controller/tsconfig.build.json" | |
}, | |
{ | |
"path": "../../packages/multichain-network-controller/tsconfig.build.json" | |
} | |
], | |
"references": [ | |
{ "path": "../base-controller/tsconfig.build.json" }, | |
{ "path": "../network-controller/tsconfig.build.json" }, | |
{ "path": "../multichain-network-controller/tsconfig.build.json" } | |
], |
"references": [ | ||
{ | ||
"path": "../../packages/base-controller" | ||
}, | ||
{ | ||
"path": "../../packages/network-controller" | ||
}, | ||
{ | ||
"path": "../../packages/multichain-network-controller" | ||
} | ||
], |
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.
"references": [ | |
{ | |
"path": "../../packages/base-controller" | |
}, | |
{ | |
"path": "../../packages/network-controller" | |
}, | |
{ | |
"path": "../../packages/multichain-network-controller" | |
} | |
], | |
"references": [ | |
{ "path": "../base-controller" }, | |
{ "path": "../network-controller" }, | |
{ "path": "../multichain-network-controller" } | |
], |
"path": "../../packages/multichain-network-controller" | ||
} | ||
], | ||
"include": ["../../types", "./src", "tests/NetworkOrderController.test.ts"] |
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: if you opt in for the suggestion to move NetworkOrderController.test.ts
to test folder this can be dropped
"include": ["../../types", "./src", "tests/NetworkOrderController.test.ts"] | |
"include": ["../../types", "./src"] |
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.
Looks good! As for the suggestions by @cryptodev-2s, I think they make sense, except perhaps we want to keep NetworkOrderController.ts
as-is? We can consider aligning this controller with other controllers in another PR.
'0xaa36a7', | ||
'0xe705', | ||
'0x539', | ||
'0x18c6', | ||
'0x279f', |
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.
Should we add comments to document what these are?
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.
Should this file be called constants.ts
?
"@metamask/keyring-api": "^18.0.0", | ||
"@metamask/multichain-network-controller": "^0.9.0", | ||
"@metamask/utils": "^11.2.0", | ||
"@metamask/network-controller": "^24.0.0" |
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.
It looks like this controller talks to NetworkController, should this be a peer dependency instead?
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.
Can you re-run yarn update-readme-content
? It looks like some dependencies are not reflected here.
Explanation
The
NetworkOrderController
is a core implementation of network ordering functionality that was previously only available in the extension. This PR moves and expands this functionality from the extension'sNetworkOrder
controller to core, enabling network ordering features across both extension and mobile platforms. It also renames it toNetworkOrderController
to more accurately represent what it does. Maybe in the future this can be expanded to include other UX enhancements that don't need to be tightly coupled with theNetworkController
Key motivations for this change:
Platform Parity: Network ordering was previously only implemented in the extension via
NetworkOrder
controller, leaving mobile without this functionality. Moving this to core will unlock the ability to have this feature built on mobile.Code Quality Improvements: The move to core has provided an opportunity to:
The controller now handles three key aspects:
References
Changelog
Checklist