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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 20 commits into from

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Nov 21, 2024 •
edited
Loading

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:

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:

Screen.Recording.2024-11-22.at.12.25.33.PM.mov

Pre-merge author checklist

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.

@github-actions GitHub Actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@mcmire mcmire force-pushed the bump-swaps-controller branch from fa4f400 to f0c6e60 Compare November 21, 2024 20:28
@socket-security Socket Security
Copy link

socket-security bot commented Nov 21, 2024 •
edited
Loading

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

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None 0 303 kB metamaskbot

🚮 Removed packages: npm/@metamask/[email protected]

View full report↗︎

@mcmire mcmire added the No QA Needed Apply this label when your PR does not need any QA effort. label Nov 21, 2024
@mcmire mcmire force-pushed the bump-swaps-controller branch 3 times, most recently from 58fe119 to eb6f6e9 Compare November 21, 2024 21:15
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 22, 2024
@mcmire mcmire changed the title [DON'T MERGE] Bump swaps-controller to test removal of global provider chore: [DON'T MERGE] Bump swaps-controller to test removal of global provider Nov 22, 2024
@mcmire
Copy link
Contributor Author

mcmire commented Nov 22, 2024 •
edited
Loading

Need to add missing unit tests in order to make SonarCloud pass on this, but all of the existing unit tests should pass and you should be able to follow the manual testing steps listed in the PR description. SonarCloud is passing now.

@mcmire mcmire added DO-NOT-MERGE Pull requests that should not be merged and removed DO-NOT-MERGE Pull requests that should not be merged No QA Needed Apply this label when your PR does not need any QA effort. labels Nov 22, 2024
@mcmire mcmire changed the title chore: [DON'T MERGE] Bump swaps-controller to test removal of global provider chore: Bump swaps-controller to test removal of global provider Nov 26, 2024
@mcmire mcmire changed the title chore: Bump swaps-controller to test removal of global provider chore: Bump @metamask/swaps-controller to 12.0.0 Nov 26, 2024
@mcmire mcmire force-pushed the bump-swaps-controller branch from 6d28eb6 to 5f16ffe Compare December 9, 2024 19:05
@mcmire mcmire added the Run Smoke E2E Triggers smoke e2e on Bitrise label Dec 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 5f16ffe
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/737e986b-4e57-49c5-972e-356f8643d7ca

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@mcmire
Upgrade @metamask/swaps-controller to 12.0.0
5f144ec 
This version updates SwapsController 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.

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.
@mcmire mcmire force-pushed the bump-swaps-controller branch from 5f16ffe to 5f144ec Compare December 9, 2024 19:10
@mcmire mcmire added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 657e5a9
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/65c2f70d-2331-4f30-a3f8-6d063ace3fa0

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@mcmire mcmire marked this pull request as ready for review December 9, 2024 20:36
@mcmire mcmire requested review from a team as code owners December 9, 2024 20:36
@mcmire mcmire added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 11, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 11, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: f3c4cc2
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8b808ae6-0e0d-4fcc-95fd-5f8b0a774a27

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

cryptodev-2s
cryptodev-2s previously approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!, I tested locally swaps flow seems to work correctly

sethkfman
sethkfman previously approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - reviewed: package.json, yarn.lock & Engine.ts

@mcmire mcmire added the QA in Progress QA has started on the feature. label Dec 12, 2024
@mcmire mcmire dismissed stale reviews from sethkfman and cryptodev-2s via 7c32a0e December 12, 2024 20:57
@mcmire mcmire added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: d235bd1
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/075b7d45-5566-4c10-a061-56683188c392

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 12, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
84.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@mcmire mcmire added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jclancy93 jclancy93 jclancy93 approved these changes

@bergeron bergeron bergeron approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s left review comments

@sethkfman sethkfman sethkfman left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@mcmire mcmire

Labels
INVALID-PR-TEMPLATE PR's body doesn't match template QA in Progress QA has started on the feature. Run Smoke E2E Triggers smoke e2e on Bitrise team-wallet-framework
Projects
PR review queue
Status: Needs dev review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade @metamask/swaps-controller to 12.0.0
6 participants
@mcmire @cryptodev-2s @bergeron @jclancy93 @sethkfman @metamaskbot