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

Version v11.5.2 #21949

Merged
merged 5 commits into from
Nov 23, 2023
Merged

Version v11.5.2 #21949

merged 5 commits into from
Nov 23, 2023

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Nov 22, 2023

[11.5.2]

Fixed

  • Fix bug that could cause the fetching quotes step of Swaps to fail (#21923)

jiexi and others added 2 commits November 22, 2023 17:08
## **Description**

Fixes bug with the Web3Provider being reinstantiated with the provider
proxy before it was updated to point to the new network. This is
achieved by changing the listened event from `stateChange` to
`networkDidChange`

## **Related issues**

Fixes: #21904

## **Manual testing steps**

1. Change your network to Linea
2. Restart extension
3. Open extension
4. Change network to Mainnet
5. Open swaps
6. Swap 1 WETH for ETH 
7. Should no longer error

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] 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-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.
@danjm danjm requested a review from a team as a code owner November 22, 2023 20:45
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.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 22, 2023
@legobeat
Copy link
Contributor

@danjm Should this also include #21928 ? The broken release [email protected] is still used here.

`@metamask/snaps-ui` is indirectly used by `@metamask/keyring-api` but
is deprecated, so we are getting an audit failure. Replacing
`@metamask/snaps-ui` with `@metamask/snaps-sdk` will solve this but in
the meantime we are ignoring this deprecation to unblock further merges.
@danjm
Copy link
Contributor Author

danjm commented Nov 22, 2023

@legobeat yes, let's include it. I just approved your PR to do that. Thank-you

@metamaskbot
Copy link
Collaborator

Builds ready [044a68b]
Page Load Metrics (1347 ± 359 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint84138102188
domContentLoaded6813595209
load7922011347747359
domInteractive6813595209

…work events (#21958)

While QAing v11.5.2, this issue was discovered:

1. Add a network, through the popular network list, that you have tokens
on.
2. use "+ Import tokens" form to import a token that that you have on
this network
3. try to swap that token for the network's native currency, fetching
quotes will fail and an "Error fetching quotes" message will be shown

We thought we fixed this with
#21923, but the case
of adding a network and then immediately trying to swap an erc20 token
on that network, was not covered by that solution.

The reasons for that are detailed extensively here
https://consensys.slack.com/archives/GTQAGKY5V/p1700700399190349

tldr: When the `NetworkDidChange` event occurs during the network adding
flow, the network's status is not yet "Available" and so the swaps
controller subscriber for network changes does allow the provider to
update

The fix in this PR, which also covers the problem addressed by the
aforementioned PR, is to only update the swaps controller's
ethersProvider at the time it is needed, within the `fetchAndSetQuotes`,
and to stop subscribing to network controller changes.

1. Add a network, through the popular network list, that you have tokens
on.
2. use "+ Import tokens" form to import a token that that you have on
this network
3. try to swap that token for the network's native currency, fetching
quotes should succeed

https://github.com/MetaMask/metamask-extension/assets/7499938/acc62c6c-ac67-41be-a4a9-78e7d9941043

https://github.com/MetaMask/metamask-extension/assets/7499938/3eeea1c2-9d14-4f56-9a66-4669af9ae0e1

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

- [ ] 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.
@metamaskbot
Copy link
Collaborator

Builds ready [ec61967]
Page Load Metrics (1276 ± 381 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint841189884
domContentLoaded7011291126
load8121021276793381
domInteractive7011291126

@danjm
Copy link
Contributor Author

danjm commented Nov 23, 2023

I just did a final QA and it passed

@danjm danjm merged commit 1503dee into master Nov 23, 2023
67 of 68 checks passed
@danjm danjm deleted the Version-v11.5.2 branch November 23, 2023 22:31
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants