Merged
Conversation
This button calls `eth_accounts`. This button is always enabled, even when not connected.
The buttons that require you to have first connected have been disabled by default. Previously they would be enabled until the JavaScript finished initializing the page, at which point they'd be disabled. This resulted in a distracting flash as the page loaded and the buttons changed. The `signTypedData` button was added to the accounts button set as well, rather than being left enabled regardless of connected status.
The Connect button was broken previously in that after being disabled, it would stay disabled even if the dapp lost access to MetaMask. The button will now be enabled whenever not connected.
e57a3a5 to
245b58f
Compare
Collaborator
Builds ready [245b58f]
|
bergeron
added a commit
that referenced
this pull request
Feb 13, 2024
## **Description** Fixes a race condition during token detection after switching networks. During the network switch, some controllers state are still on the old chain id, and others are on the new chain id. This can cause different issues depending which controllers win the race. The worst case is that detected tokens are added to the wrong network (see related issues):  In that ^ screenshot there are 2 mainnet tokens that we have a balance of, but they incorrectly appear under linea. There's a fix for each of the relevant controllers (`DetectTokensController`, `TokensController`, `AssetsContractController`) ensuring we use the chain ID being switched *to*. ## **Related issues** [Auto token detection list collision with other networks #22512](#22512) [Autodetect tokens display Mainnet tokens on another network #7587](MetaMask/metamask-mobile#7587) ## **Manual testing steps** It takes a few minutes of switching networks back and forth to reproduce the bug. But basically keep doing that and we should not see tokens hop from 1 network to the other. ## **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** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've clearly explained what problem this PR is solving and how it is solved. - [ ] I've linked related issues - [ ] 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 - [ ] 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. - [ ] 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". ## **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.
sahar-fehri
pushed a commit
that referenced
this pull request
Feb 14, 2024
## **Description** Fixes a race condition during token detection after switching networks. During the network switch, some controllers state are still on the old chain id, and others are on the new chain id. This can cause different issues depending which controllers win the race. The worst case is that detected tokens are added to the wrong network (see related issues):  In that ^ screenshot there are 2 mainnet tokens that we have a balance of, but they incorrectly appear under linea. There's a fix for each of the relevant controllers (`DetectTokensController`, `TokensController`, `AssetsContractController`) ensuring we use the chain ID being switched *to*. ## **Related issues** [Auto token detection list collision with other networks #22512](#22512) [Autodetect tokens display Mainnet tokens on another network #7587](MetaMask/metamask-mobile#7587) ## **Manual testing steps** It takes a few minutes of switching networks back and forth to reproduce the bug. But basically keep doing that and we should not see tokens hop from 1 network to the other. ## **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** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've clearly explained what problem this PR is solving and how it is solved. - [ ] I've linked related issues - [ ] 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 - [ ] 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. - [ ] 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". ## **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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add Get Accounts button
This button calls
eth_accounts. This button is always enabled, evenwhen not connected.
Disable account buttons by default
The buttons that require you to have first connected have been disabled
by default. Previously they would be enabled until the JavaScript
finished initializing the page, at which point they'd be disabled. This
resulted in a distracting flash as the page loaded and the buttons
changed.
The
signTypedDatabutton was added to the accounts button set aswell, rather than being left enabled regardless of connected status.
Allow connect button to become re-enabled
The Connect button was broken previously in that after being disabled,
it would stay disabled even if the dapp lost access to MetaMask. The
button will now be enabled whenever not connected.
Stringify signTypedData results
Defer metamask onboarding bundle to speed up page load