Carve path to removing reliance on global network #347

mcmire · 2024-11-20T19:26:55Z

Currently, SwapsController relies on the global network. It uses the global provider from the NetworkController and uses it to get the allowance for a user's token on a chain, and it uses the chain ID of the global network (again, coming from NetworkController) to retrieve parameters that will ultimately be used to control when and how swap quotes are requested and how to transform the resulting data. Finally, it also expects setProvider and/or setChainId to get called when the global network is switched, when it will restore previously cached quote data from state so that it can be presumably reshown in the UI.

Long-term, we want to remove the concept of the global network from the extension, so we need to prepare this controller accordingly. Fundamentally, we need to have the parts of the API which rely on the global provider to take a network client ID, which will be used to fetch a provider and chain ID.

A couple of things to note about this controller which prohibits us from completely removing the reliance on the global network:

Besides setChainId and setProvider, there is another entry point to the API via startFetchAndSetQuotes. This starts a loop to fetch quotes on a particular chain. Crucially, this loop does not support multiple simultaneous network access. If the loop is already active for a network and startFetchAndSetQuotes is called for another, the loop will be reset and all quotes captured for the previous network will be overwritten. We plan on keeping this behavior.
As alluded to above, the controller caches swaps quotes it fetches for a chain. To retain the previous behavior of restoring from cache when the network is switched, the controller now listens for the switch via NetworkController:networkDidChange.

Ultimately both of these things may need to be changed when the concept of the global network is completely removed, but we can revisit this at a later time.

See the changelog for the full list of changes.

Fixes #204.

Also see PR in mobile repo to upgrade to these changes: MetaMask/metamask-mobile#12378. This PR should pass CI and the manual testing steps.

socket-security · 2024-11-20T20:12:58Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/jest@29.5.14	None	`0`	78.8 kB	types

🚮 Removed packages: npm/@jest/types@26.6.2, npm/@types/jest@26.0.24, npm/@types/yargs@15.0.19, npm/diff-sequences@26.6.2, npm/jest-diff@26.6.2, npm/jest-get-type@26.3.0, npm/react-is@17.0.2

View full report↗︎

Currently, SwapsController relies on the global network. It uses the global provider from the NetworkController and uses it to get the allowance for a user's token on a chain, and it uses the chain ID of the global network (again, coming from NetworkController) to retrieve parameters that will ultimately be used to control when and how swap quotes are requested and how to transform the resulting data. Finally, it also expects `setProvider` and/or `setChainId` to get called when the global network is switched, when it will restore previously cached quote data from state so that it can be presumably reshown in the UI. Long-term, we want to remove the concept of the global network from the extension, so we need to prepare this controller accordingly. Fundamentally, we need to have the parts of the API which rely on the global provider to take a network client ID, which will be used to fetch a provider and chain ID. A couple of things to note about this controller which prohibits us from completely removing the reliance on the global network: - Besides `setChainId` and `setProvider`, there is another entry point to the API via `startFetchAndSetQuotes`. This starts a loop to fetch quotes on a particular chain. Crucially, this loop does not support multiple simultaneous network access. If the loop is already active for a network and `startFetchAndSetQuotes` is called for another, the loop will be reset and all quotes captured for the previous network will be overwritten. We plan on keeping this behavior. - As alluded to above, the controller caches swaps quotes it fetches for a chain. To retain the previous behavior of restoring from cache when the network is switched, the controller now listens for the switch via `NetworkController:networkDidChange`. Ultimately both of these things may need to be changed when the concept of the global network is completely removed, but we can revisit this at a later time. See the changelog for the full list of changes.

mcmire · 2024-11-20T20:15:53Z

src/SwapsController.test.ts

@@ -216,529 +370,3098 @@ describe('SwapsController', () => {
    swapsUtilEstimateGas.mockRestore();
  });

-  it('should set default options', () => {


You may want to view this diff in side-by-side mode, as it's a bit hard to read.

I wanted to verify that networkClientId was being added to the fetch params metadata correctly, but these tests did not seem right, and so I fixed them.

mcmire · 2024-11-20T20:17:13Z

src/SwapsController.test.ts


-    swapsController.__test__updatePrivate('#supportedChainIds', [chainId]);
+      // @ts-expect-error Intentionally not providing full NetworkState object.
+      rootMessenger.publish('NetworkController:networkDidChange', {


I am not using the messengerMock set up in the beforeEach because I want to actually publish the event, and publish is mocked there.

mcmire · 2024-11-20T20:19:41Z

src/SwapsController.test.ts

-        chainId: '0x1',
-        rpcUrl: 'test',
-      } as unknown as Provider;
+  describe('fetchTokenWithCache', () => {


These tests previously existed under the "tokens cache" describe. They did not seem right as they were using a "private" method to set the state necessary to set up the test, when we can just pass that state to the SwapsController constructor. They also did not test the use of the mutex in fetchTokenWithCache. I rewrote them to address these two problems and made sure that we also confirm that the given networkClientId is used correctly.

mcmire · 2024-11-20T20:20:35Z

src/SwapsController.test.ts

-        topAssetsLastFetched: 2912,
-        aggregatorMetadataLastFetched: 2913,
-      };
+  describe('fetchTopAssetsWithCache', () => {


These tests previously existed under the "top assets cache" describe. I rewrote them in a similar manner as the fetchTokenWithCache tests.

The tests for fetchTopAssetsWithCache are similar to the ones for fetchTokenWithCache since the implementation is similar.

mcmire · 2024-11-20T20:21:04Z

src/SwapsController.test.ts

-      });
-      await swapsController.fetchTopAssetsWithCache();
-      expect(swapsUtilFetchTopAssets).toHaveBeenCalled();
+  describe('fetchAggregatorMetadataWithCache', () => {


These tests previously existed under the "aggregator metadata cache" describe. I rewrote them in a similar manner as the fetchTokenWithCache tests.

The tests for fetchTopAssetsWithCache are similar to the ones for fetchTokenWithCache since the implementation is similar.

mcmire · 2024-11-20T20:52:30Z

src/SwapsController.ts

      this.stopPollingAndResetState.bind(this),
    );
+
+    this.messagingSystem.subscribe(


As mentioned in the PR description, I added this because we have this concept of a "chain cache" in state and it seemed important to preserve the existing behavior around that. Listening to networkDidChange seemed like the best way to accomplish this, but happy to hear other thoughts on what we could do here.

mcmire · 2024-11-20T20:55:40Z

package.json

  },
  "devDependencies": {
    "@babel/runtime": "^7.0.0",
+    "@ethersproject/abi": "^5.7.0",


Useful to encode the data and result for the ERC20 contract in tests.

mcmire · 2024-11-20T20:55:50Z

package.json

    "@metamask/eslint-config-jest": "^12.1.0",
    "@metamask/eslint-config-nodejs": "^12.1.0",
    "@metamask/eslint-config-typescript": "^12.1.0",
+    "@metamask/eth-json-rpc-provider": "^4.1.6",


Used by fake-provider.ts.

mcmire · 2024-11-20T20:55:52Z

package.json

+    "@metamask/eth-json-rpc-provider": "^4.1.6",
    "@metamask/gas-fee-controller": "^21.0.0",
    "@metamask/keyring-controller": "^17.0.0",
+    "@metamask/json-rpc-engine": "^10.0.1",


Used by fake-provider.ts.

mcmire · 2024-11-20T20:57:20Z

package.json

    "@metamask/network-controller": "^21.1.0",
    "@metamask/transaction-controller": "^37.3.0",
-    "@types/jest": "^26.0.22",
+    "@types/jest": "^29.5.14",


Useful to gain access to the types for the runOnlyPendingTimersAsync method available in Jest 29 (which we're already using).

mcmire · 2024-11-20T21:56:05Z

I have published @metamask-previews/swaps-controller@11.0.0-preview-8176e4502716cfe6450c2b73be6ee403b0cea072. I am working on getting this updated in Mobile.

mcmire · 2024-11-21T16:42:15Z

tests/fake-provider.ts

Source for this file: https://github.com/MetaMask/core/blob/73b9eb92666917b7358e9e7066684b7cf4f745a0/tests/fake-provider.ts.

mcmire · 2024-11-22T19:30:50Z

New preview build: @metamask-previews/swaps-controller@11.0.0-preview-40be04506cf92f5e6a271eb90c96b4ddcf57dd5d. MetaMask/metamask-mobile#12378 has been updated with this build.

package.json

Gudahtt · 2024-11-27T21:00:08Z

.eslintrc

    "@metamask/eslint-config"
-  ]
+  ],
+  "ignorePatterns": ["tests/fake-provider.ts"]


Nit: Could this be removed? ESlint is useful for mocks, even if we need to disable some rules. I see there are a bunch of inline disable comments already

This is happening because we don't have two tsconfig files in this project; we just have the one and it excludes anything outside of src by default. As a result, typescript-eslint fails when attempting to parse this file.

I have pushed 8e847c0 which moves this file to src and renames it to fake-provider.test.ts so that it will continue to be excluded from the build. I have also instructed Jest not to run this file as a test file.

In a later PR I will split the tsconfig files as per other projects and clean this up.

src/SwapsController.ts

mikesposito

Looks great! I have also tested this on mobile and the swap went through without issues.

Though, on mobile we can't switch chain while we are fetching quotes, so there are parts of code that I'm not sure how to test (or even if that's possible)

swap_test.webm

mikesposito · 2024-12-04T11:02:19Z

This is probably unrelated to this work, but I noticed that after swapping a token for some ETH (or vice-versa) token balances are not updated until I switch network (and switch back) or restart the app. This is uncomfortable because if I want to use the token that I just received from the swap (e.g. swapping again) the UI tells me that I can't because I have 0 balance

mcmire · 2024-12-04T16:26:08Z

This is probably unrelated to this work, but I noticed that after swapping a token for some ETH (or vice-versa) token balances are not updated until I switch network (and switch back) or restart the app.

Strange! I would agree that this is probably unrelated, since SwapsController should only be responsible for fetching quotes. This sounds more like a TokenBalancesController issue. Still, good catch. I'll see if there's a ticket for it and open one if not.

## Description This version upgrades `@metamask/swaps-controller` so that it is less reliant on the global network. Specifically: - The `setChainId` and `setProvider` methods have been removed from SwapsController. - The `fetchGasFeeEstimates` and `fetchEstimatedMultiLayerL1Fee` SwapsController constructor options are now expected to take a `networkClientId`. - The SwapsController constructor no longer takes a `chainId` option. - `startFetchAndSetQuotes`, `fetchTokenWithCache`, `fetchTopAssetsWithCache`, and `fetchAggregatorMetadataWithCache` now take a `networkClientId`. - The `fetchParamsMetaData` SwapsController state property now includes a `networkClientId`. - The chain cache in SwapsController state will now automatically be updated whenever the network has changed. See full changelog here: https://github.com/MetaMask/swaps-controller/blob/main/CHANGELOG.md#1200 At the moment, the global network client ID is still passed into SwapsController whenever it is used, but now that can be changed to use a dapp-level network client ID without needing to update SwapsController. ## Related issues Fixes #12470. Also see: - MetaMask/swaps-controller#347 - MetaMask/swaps-controller#204 ## Manual testing steps - Tap on the Swap button - Select a destination token and proceed - You should see new quotes - Ideally, you should be able to complete the swaps flow and create a transaction ## Screenshots/Recordings The Swaps flow should work exactly like it does now. Here is a video demonstrating fetching Swaps quotes: https://github.com/user-attachments/assets/62a490c9-32c4-49a2-886d-136328761658 ## Pre-merge author checklist - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## Pre-merge reviewer checklist - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

mcmire force-pushed the remove-reliance-on-global-network branch 2 times, most recently from df05d2b to 5f3d2e3 Compare November 20, 2024 20:12

mcmire force-pushed the remove-reliance-on-global-network branch from 5f3d2e3 to c771181 Compare November 20, 2024 20:28

mcmire force-pushed the remove-reliance-on-global-network branch from c771181 to ed63b7a Compare November 20, 2024 20:55

mcmire commented Nov 20, 2024

View reviewed changes

mcmire added 2 commits November 20, 2024 14:01

Exclude fake-provider.ts from lint

8176e45

Generate a preview build

d0def9c

mcmire commented Nov 21, 2024

View reviewed changes

mcmire mentioned this pull request Nov 21, 2024

chore: Bump @metamask/swaps-controller to 12.0.0 MetaMask/metamask-mobile#12378

Merged

7 tasks

mcmire added 2 commits November 22, 2024 11:06

Use ?. correctly

40be045

Generate another preview build

01956aa

mcmire marked this pull request as ready for review November 22, 2024 21:16

mcmire requested a review from a team November 22, 2024 21:16

mcmire added the team-wallet-framework label Nov 22, 2024

mcmire self-assigned this Nov 22, 2024

mcmire requested a review from a team November 22, 2024 21:24

mikesposito reviewed Nov 27, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

mcmire mentioned this pull request Nov 27, 2024

Upgrade @metamask/swaps-controller to 12.0.0 MetaMask/metamask-mobile#12470

Closed

9 tasks

Remove version bump / package name for preview build

7ef6be6

Gudahtt reviewed Nov 27, 2024

View reviewed changes

Don't exclude fake-provider.ts from linting

8e847c0

mikesposito reviewed Dec 3, 2024

View reviewed changes

src/SwapsController.ts Outdated Show resolved Hide resolved

mcmire added 2 commits December 3, 2024 13:31

Revert changes to .eslintrc

88c2c91

Allow the first argument of startFetchAndSetQuotes to be optional again

e0eb5ce

mikesposito previously approved these changes Dec 4, 2024

View reviewed changes

Merge branch 'main' into remove-reliance-on-global-network

fb2304a

mcmire dismissed mikesposito’s stale review via fb2304a December 4, 2024 16:23

Fix lint violation

15ccca3

mikesposito approved these changes Dec 4, 2024

View reviewed changes

mcmire merged commit 404a318 into main Dec 4, 2024
11 checks passed

mcmire deleted the remove-reliance-on-global-network branch December 4, 2024 16:34

Carve path to removing reliance on global network #347

Carve path to removing reliance on global network #347

Uh oh!

Conversation

mcmire commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcmire Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikesposito left a comment

Choose a reason for hiding this comment

Uh oh!

mikesposito commented Dec 4, 2024

Uh oh!

mcmire commented Dec 4, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mcmire commented Nov 20, 2024 •

edited

Loading

socket-security bot commented Nov 20, 2024 •

edited

Loading

mcmire Nov 20, 2024 •

edited

Loading

mcmire Nov 20, 2024 •

edited

Loading

mcmire commented Nov 20, 2024 •

edited

Loading

mcmire commented Nov 22, 2024 •

edited

Loading