core icon indicating copy to clipboard operation
core copied to clipboard

Merge the two network controllers

Open Gudahtt opened this issue 3 years ago • 1 comments

Merge the network controller in this repository with the extension network controller.

General guidelines:

  • Write comprehensive unit tests for both current network controllers
    • This is the first step so that we can be certain how each controller behaves before we merge them. This will help ensure nothing changes unintentionally, especially breaking changes.
    • We can test the "configuration management" and the "RPC message handling" layers separately. For both current controllers, most of the controller API is for configuration management. The RPC message handling is solely in the "network client" object, which we can test independently from the controller API.
  • Document all functional differences between each controller
  • Extract any API interactions or complex utility functions
    • We want to minimize the direct responsibilities that our controllers have, so that they are simpler and easier to maintain. They need to be responsible for ensuring the integrity of our state, so that the wallet is always in a "valid" state, and to accomplish this goal the controllers will include most of the "business logic" of the wallet. But we can extract parts of this logic to other modules/libraries/middleware/etc. as appropriate, to keep the controllers focused on high-level operations and state management.
    • At the moment, there are many RPC middleware steps setup by the extension network controller that have nothing to do with the network (see createMetamaskMiddleware.js). These can be extracted to be setup alongside the rest of our RPC pipeline.
    • The extension network controller is also responsible at the moment for creating the "provider proxy" that we use internally. This could be extracted from the controller, it doesn't need to have this responsibility.
  • Normalize functionality across both controllers
    • We can proceed one-at-a-time through the list of functional differences prepared earlier. For each difference, we can change one or both controllers to have the same behaviour. The fewer differences we have, the easier the final migration will be.
    • It may not be practical to remove 100% of differences at this stage. Use your best judgement to decide whether a difference can be eliminated before the merge or not.
  • Migrate the @metamask/controllers network controller to BaseControllerV2
    • This will serve as the basis for the merged controller.
  • Migrate the extension to use the @metamask/controllers network controller.

Gudahtt avatar Mar 24 '22 18:03 Gudahtt

Notes from standup today:

  • Currently we are using web3-provider-engine in mobile and json-rpc-engine in extension, which relies on web3. We should use json-rpc-engine in the shared version to remove that dependency.
  • Currently we use eth-query to talk to the network. We should consider using ethers instead, as it has a simpler, more piecemeal API and supports TypeScript. This requires that eth-json-rpc-middleware return a provider that is EIP-1194-compatible, so that it can be passed to ethers.

To do both of these, we should first write unit tests in the extension and in mobile for the JSON-RPC methods that are usable by the provider that NetworkController exposes. This way we can ensure that the behavior that our new version of NetworkController exposes supports all of the same functionality as the individual network controllers, and therefore it should be easier for the new version to be a drop-in replacement for the old.

mcmire avatar Apr 04 '22 18:04 mcmire