Skip to content
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

Merge the two network controllers #753

Closed
Gudahtt opened this issue Mar 24, 2022 · 1 comment
Closed

Merge the two network controllers #753

Gudahtt opened this issue Mar 24, 2022 · 1 comment
Labels

Comments

@Gudahtt
Copy link
Member

Gudahtt commented Mar 24, 2022

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.
@mcmire
Copy link
Contributor

mcmire commented Apr 4, 2022

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 added a commit to MetaMask/metamask-extension that referenced this issue Aug 11, 2022
We are working on migrating the extension to a unified network
controller, but before we do so we want to extract some of the existing
pieces, specifically `createInfuraClient` and `createJsonRpcClient`,
which provide the majority of the behavior exhibited within the provider
API that the existing NetworkController exposes. This necessitates that
we understand and test that behavior as a whole.

With that in mind, this commit starts with the Infura-specific network
client and adds some initial functional tests for `createInfuraClient`,
specifically covering three pieces of middleware provided by
`eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`,
`createBlockCacheMiddleware`, and `createBlockRefMiddleware`.

One note about these tests is that in order to aid in debugging, I've
added some logging to the underlying packages used when a request passes
through the middleware stack. This greatly helps to demonstrate what's
going on beneath the hood and understand what requests are *actually*
being made, as failures from Nock are not sufficient to do so.
This logging is disabled by default but can be activated by setting
`DEBUG_PROVIDER_TESTS=1` alongside the `jest` command.

[1]: MetaMask/core#753
mcmire added a commit to MetaMask/metamask-extension that referenced this issue Aug 11, 2022
We are working on migrating the extension to a unified network
controller, but before we do so we want to extract some of the existing
pieces, specifically `createInfuraClient` and `createJsonRpcClient`,
which provide the majority of the behavior exhibited within the provider
API that the existing NetworkController exposes. This necessitates that
we understand and test that behavior as a whole.

With that in mind, this commit starts with the Infura-specific network
client and adds some initial functional tests for `createInfuraClient`,
specifically covering three pieces of middleware provided by
`eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`,
`createBlockCacheMiddleware`, and `createBlockRefMiddleware`.

These tests exercise logic that originate from multiple different places
and combine in sometimes surprising ways, and as a result, understanding
the nature of the tests can be tricky. I've tried to explain the logic
(both of the implementation and the tests) via comments. Additionally,
debugging why a certain test is failing is not the most fun thing in the
world, so to aid with this, I've added some logging to the underlying
packages used when a request passes through the middleware stack.
Because some middleware change the request being made, or make new
requests altogether, this greatly helps to peel back the curtain, as
failures from Nock do not supply much meaningful information on their
own. This logging is disabled by default, but can be activated by
setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command.

[1]: MetaMask/core#753
mcmire added a commit to MetaMask/metamask-extension that referenced this issue Aug 11, 2022
We are working on migrating the extension to a unified network
controller, but before we do so we want to extract some of the existing
pieces, specifically `createInfuraClient` and `createJsonRpcClient`,
which provide the majority of the behavior exhibited within the provider
API that the existing NetworkController exposes. This necessitates that
we understand and test that behavior as a whole.

With that in mind, this commit starts with the Infura-specific network
client and adds some initial functional tests for `createInfuraClient`,
specifically covering three pieces of middleware provided by
`eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`,
`createBlockCacheMiddleware`, and `createBlockRefMiddleware`.

These tests exercise logic that originate from multiple different places
and combine in sometimes surprising ways, and as a result, understanding
the nature of the tests can be tricky. I've tried to explain the logic
(both of the implementation and the tests) via comments. Additionally,
debugging why a certain test is failing is not the most fun thing in the
world, so to aid with this, I've added some logging to the underlying
packages used when a request passes through the middleware stack.
Because some middleware change the request being made, or make new
requests altogether, this greatly helps to peel back the curtain, as
failures from Nock do not supply much meaningful information on their
own. This logging is disabled by default, but can be activated by
setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command.

[1]: MetaMask/core#753
mcmire added a commit to MetaMask/metamask-extension that referenced this issue Aug 11, 2022
We are working on migrating the extension to a unified network
controller, but before we do so we want to extract some of the existing
pieces, specifically `createInfuraClient` and `createJsonRpcClient`,
which provide the majority of the behavior exhibited within the provider
API that the existing NetworkController exposes. This necessitates that
we understand and test that behavior as a whole.

With that in mind, this commit starts with the Infura-specific network
client and adds some initial functional tests for `createInfuraClient`,
specifically covering three pieces of middleware provided by
`eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`,
`createBlockCacheMiddleware`, and `createBlockRefMiddleware`.

These tests exercise logic that originate from multiple different places
and combine in sometimes surprising ways, and as a result, understanding
the nature of the tests can be tricky. I've tried to explain the logic
(both of the implementation and the tests) via comments. Additionally,
debugging why a certain test is failing is not the most fun thing in the
world, so to aid with this, I've added some logging to the underlying
packages used when a request passes through the middleware stack.
Because some middleware change the request being made, or make new
requests altogether, this greatly helps to peel back the curtain, as
failures from Nock do not supply much meaningful information on their
own. This logging is disabled by default, but can be activated by
setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command.

[1]: MetaMask/core#753
mcmire added a commit to MetaMask/metamask-extension that referenced this issue Aug 12, 2022
We are working on migrating the extension to a unified network
controller, but before we do so we want to extract some of the existing
pieces, specifically `createInfuraClient` and `createJsonRpcClient`,
which provide the majority of the behavior exhibited within the provider
API that the existing NetworkController exposes. This necessitates that
we understand and test that behavior as a whole.

With that in mind, this commit starts with the Infura-specific network
client and adds some initial functional tests for `createInfuraClient`,
specifically covering three pieces of middleware provided by
`eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`,
`createBlockCacheMiddleware`, and `createBlockRefMiddleware`.

These tests exercise logic that originate from multiple different places
and combine in sometimes surprising ways, and as a result, understanding
the nature of the tests can be tricky. I've tried to explain the logic
(both of the implementation and the tests) via comments. Additionally,
debugging why a certain test is failing is not the most fun thing in the
world, so to aid with this, I've added some logging to the underlying
packages used when a request passes through the middleware stack.
Because some middleware change the request being made, or make new
requests altogether, this greatly helps to peel back the curtain, as
failures from Nock do not supply much meaningful information on their
own. This logging is disabled by default, but can be activated by
setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command.

[1]: MetaMask/core#753
mcmire added a commit to MetaMask/metamask-extension that referenced this issue Aug 12, 2022
We are working on migrating the extension to a unified network
controller, but before we do so we want to extract some of the existing
pieces, specifically `createInfuraClient` and `createJsonRpcClient`,
which provide the majority of the behavior exhibited within the provider
API that the existing NetworkController exposes. This necessitates that
we understand and test that behavior as a whole.

With that in mind, this commit starts with the Infura-specific network
client and adds some initial functional tests for `createInfuraClient`,
specifically covering three pieces of middleware provided by
`eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`,
`createBlockCacheMiddleware`, and `createBlockRefMiddleware`.

These tests exercise logic that originate from multiple different places
and combine in sometimes surprising ways, and as a result, understanding
the nature of the tests can be tricky. I've tried to explain the logic
(both of the implementation and the tests) via comments. Additionally,
debugging why a certain test is failing is not the most fun thing in the
world, so to aid with this, I've added some logging to the underlying
packages used when a request passes through the middleware stack.
Because some middleware change the request being made, or make new
requests altogether, this greatly helps to peel back the curtain, as
failures from Nock do not supply much meaningful information on their
own. This logging is disabled by default, but can be activated by
setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command.

Also note that we are using a custom version of `eth-block-tracker`
which provides a `destroy` method, which we use in tests to properly
ensure that the block tracker is stopped before moving on to the next
step. This change comes from [this PR][2] which has yet to be merged.

[1]: MetaMask/core#753
[2]: MetaMask/eth-block-tracker#106
@Gudahtt Gudahtt added the Epic label Nov 17, 2022
@mcmire mcmire removed their assignment Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants