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

fix: Display alerts on add network request if there are pending confirmations #30634

Merged
merged 16 commits into from
Mar 7, 2025

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Feb 28, 2025

Description

Add confirmation reject warning when user adds a new network. As user confirms the warning pending confirmations are deleted.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4292

Manual testing steps

  1. Enable EVM multichain locally using env variable EVM_MULTICHAIN_ENABLED
  2. Submit a confirmation
  3. Add new network - ensure a warning appears and pending confirmations are deleted as user confirms the warning

Screenshots/Recordings

Uploading Screen Recording 2025-02-28 at 4.30.58 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.

@jpuri jpuri added the team-confirmations Push issues to confirmations team label Feb 28, 2025
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.

@jpuri jpuri marked this pull request as ready for review February 28, 2025 13:29
@jpuri jpuri requested a review from a team as a code owner February 28, 2025 13:29
@metamaskbot
Copy link
Collaborator

Builds ready [1730a64]
Page Load Metrics (1750 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30521311610458220
domContentLoaded15062105172614871
load15322134175015273
domInteractive259638168
backgroundConnect106728178
firstReactRender147726189
getState56617199
initialActions01000
loadScripts10671526128811656
setupStore864242211
uiStartup17362362200016177
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 840 Bytes (0.01%)
  • ui: 3.55 KiB (0.05%)
  • common: 227 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [bb8e799]
Page Load Metrics (1838 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27123071769387186
domContentLoaded15362255180717283
load15452311183817986
domInteractive23114412512
backgroundConnect10110322512
firstReactRender1681392311
getState560252110
initialActions01000
loadScripts11261670136313666
setupStore86916178
uiStartup18042592210218790
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 840 Bytes (0.01%)
  • ui: 3.79 KiB (0.05%)
  • common: 227 Bytes (0.00%)

@jpuri jpuri requested a review from matthewwalsh0 March 5, 2025 13:51
@metamaskbot
Copy link
Collaborator

Builds ready [6bd09fb]
Page Load Metrics (1721 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30419981629323155
domContentLoaded15062155170315072
load15502169172115172
domInteractive14117422412
backgroundConnect115224157
firstReactRender1574432612
getState55011136
initialActions00000
loadScripts11041660126713464
setupStore75714147
uiStartup17652470196416479
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 846 Bytes (0.01%)
  • ui: 3.8 KiB (0.05%)
  • common: 362 Bytes (0.00%)

* @param origin - Origin to ger approvals from.
* @returns array of approvals from an origin
*/
export const getApprovalsByOrigin = (state, origin) => {
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but we try and name selectors using selectX to make it explicit they are selectors and not utils.

Also, should we avoid adding to the legacy selectors.js and maybe use ui/selectors/approvals.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update this in following PR

@@ -6035,7 +6035,7 @@ export default class MetamaskController extends EventEmitter {
engine.push(createSelectedNetworkMiddleware(this.controllerMessenger));

// Add a middleware that will switch chain on each request (as needed)
if (!process.env.EVM_MULTICHAIN_ENABLED) {
if (process.env.EVM_MULTICHAIN_ENABLED !== true) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor, does this mean we can also remove the ENV for each build type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember it created trouble with build. Let me experiment in next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried it in next pr @matthewwalsh0 and it caused CI to break:

Screenshot 2025-03-07 at 8 11 36 PM

@jpuri jpuri added this pull request to the merge queue Mar 7, 2025
Merged via the queue into main with commit 135d0d2 Mar 7, 2025
74 checks passed
@jpuri jpuri deleted the add_network_alert branch March 7, 2025 13:30
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2025
@metamaskbot metamaskbot added the release-12.15.0 Issue or pull request that will be included in release 12.15.0 label Mar 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.15.0 Issue or pull request that will be included in release 12.15.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants