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

fix: get supportedChains to avoid blocking the confirmation process #28313

Merged
merged 15 commits into from
Nov 12, 2024
Merged

fix: get supportedChains to avoid blocking the confirmation process #28313

merged 15 commits into from
Nov 12, 2024
+271 −293

Conversation

vinistevam
Copy link
Contributor

@vinistevam vinistevam commented Nov 6, 2024 •
edited
Loading

Description

This PR addresses the blocking behaviour in the confirmation process caused by the synchronous supportedChains check. Previously, the Send flow was halted until a response from the security-alerts.api.cx.metamask.io API was received. This created delays and negatively impacted the user experience, especially when the API response was slow.

Key Changes:
Asynchronous Chain Validation: The supportedChains check is now performed asynchronously within the non-awaited validateRequestWithPPOM function. This ensures that the Send flow is not blocked while waiting for the API response.
Introduction of CheckingChain Result Type: A new result type, CheckingChain, is introduced to represent the intermediate state before a definitive result is obtained. This state is used before the loading status in both middleware and transaction utility functions.

Open in GitHub Codespaces

Related issues

Fixes: #28255 #28257

Manual testing steps

  1. Use a proxy to intercept requests
  2. Start a Send flow
  3. Intercept the request to security-alerts.api.cx.metamask.io
  4. See send flow cannot be initiated until we have a response to this request

Screenshots/Recordings

supported-chains-.webm
unsupported-chains.webm

Before

After

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.

@vinistevam vinistevam added the team-confirmations Push issues to confirmations team label Nov 6, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

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.

@vinistevam vinistevam force-pushed the fix/supported-chains-blocks-sendflow branch from 8e1be2a to 01585d0 Compare November 6, 2024 10:13
@vinistevam vinistevam marked this pull request as ready for review November 6, 2024 11:04
@vinistevam vinistevam requested review from a team as code owners November 6, 2024 11:04
@vinistevam vinistevam force-pushed the fix/supported-chains-blocks-sendflow branch from 29aa3cb to df1e50c Compare November 6, 2024 11:13
@vinistevam vinistevam force-pushed the fix/supported-chains-blocks-sendflow branch from 505d22f to 071863b Compare November 6, 2024 14:30
@metamaskbot
Copy link
Collaborator

Builds ready [48d0796]
Page Load Metrics (1891 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29321691813374180
domContentLoaded16072110184613565
load16322172189113263
domInteractive2510647189
backgroundConnect9150453617
firstReactRender492941197637
getState499252512
initialActions01000
loadScripts11581659136312962
setupStore1399362813
uiStartup180729732176261126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 198 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 342 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [6d98182]
Page Load Metrics (2126 ± 97 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint18402609213020297
domContentLoaded17882601209220197
load18432609212620297
domInteractive17116592411
backgroundConnect998372713
firstReactRender842041273416
getState55921199
initialActions01000
loadScripts12851979155117484
setupStore117023199
uiStartup206029442395245118
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 198 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 342 Bytes (0.00%)

@seaona
Copy link
Contributor

seaona commented Nov 8, 2024 •
edited
Loading

Overall looks good 🙌 In summary:

  • The send flow is no longer blocked waiting for that request
  • No banner loading indenfinetly with 500 response
  • ❓ I've seen that if I block all the requests to Blockaid API, a transaction is still flagged as malicious. I'm wondering how this is possible :thinking_face: does the offline module still works too? This can also be seen in your video, or also in this one I share (you can test it faster, just by using a regular browser and blocking all requests to blockaid API) --> this was replied by @vinistevam , so it's expected

We kept the local PPOM as a fallback mechanism until we have the API battle tested

fix-blocking-reqs-send.mp4
blocking-all-api-reqs-malicious.mp4

matthewwalsh0
matthewwalsh0 requested changes Nov 8, 2024
View reviewed changes
app/scripts/lib/ppom/ppom-util.ts Outdated Show resolved Hide resolved
app/scripts/lib/ppom/ppom-util.ts Outdated Show resolved Hide resolved
shared/constants/security-provider.ts Show resolved Hide resolved
shared/constants/security-provider.ts Show resolved Hide resolved
test/e2e/tests/metrics/signature-approved.spec.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [f0ad166]
Page Load Metrics (1892 ± 94 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17172607190519393
domContentLoaded16802592186219895
load16912610189219694
domInteractive26222554120
backgroundConnect106737209
firstReactRender551961033316
getState676312211
initialActions01000
loadScripts11801976137616981
setupStore65813157
uiStartup188828412125222107
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 238 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 342 Bytes (0.00%)

@vinistevam vinistevam force-pushed the fix/supported-chains-blocks-sendflow branch from f0ad166 to bcca3d4 Compare November 9, 2024 13:09
@metamaskbot
Copy link
Collaborator

Builds ready [bcca3d4]
Page Load Metrics (1863 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32522091708472227
domContentLoaded15822187183317785
load15902224186318689
domInteractive269541157
backgroundConnect9120272613
firstReactRender45204983215
getState462202010
initialActions0482105
loadScripts11371685133215876
setupStore55214157
uiStartup17692566207320699
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 238 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 342 Bytes (0.00%)

@vinistevam vinistevam force-pushed the fix/supported-chains-blocks-sendflow branch from 3697061 to 391feea Compare November 12, 2024 00:16
@metamaskbot
Copy link
Collaborator

Builds ready [391feea]
Page Load Metrics (1899 ± 115 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint156224171908233112
domContentLoaded154924041873235113
load155724201899240115
domInteractive16175523818
backgroundConnect96825188
firstReactRender522951085124
getState495272814
initialActions01000
loadScripts110018711372211101
setupStore65114157
uiStartup174926392127263126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 238 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 343 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [3fe3280]
Page Load Metrics (2028 ± 87 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint40124901949399192
domContentLoaded16852414200016579
load16952495202818187
domInteractive2910356189
backgroundConnect881292311
firstReactRender713811286531
getState592162110
initialActions01000
loadScripts12071880147515775
setupStore682212311
uiStartup191028412295237114
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 238 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 343 Bytes (0.00%)

@vinistevam
fix: use updateSecurityResponse (#28418)
d850354 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28418?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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 Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] 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.

## **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.
@metamaskbot
Copy link
Collaborator

Builds ready [d850354]
Page Load Metrics (1717 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15642125171912158
domContentLoaded1525198016939948
load15642147171712560
domInteractive19172483216
backgroundConnect10164283416
firstReactRender4611695136
getState46111157
initialActions01000
loadScripts1108144812467938
setupStore546994
uiStartup17062670193419292
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 176 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 343 Bytes (0.00%)

@vinistevam vinistevam added this pull request to the merge queue Nov 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@vinistevam vinistevam added this pull request to the merge queue Nov 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@vinistevam vinistevam added this pull request to the merge queue Nov 12, 2024
Merged via the queue into develop with commit e54f555 Nov 12, 2024
76 checks passed
@vinistevam vinistevam deleted the fix/supported-chains-blocks-sendflow branch November 12, 2024 12:21
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jpuri jpuri jpuri left review comments

@matthewwalsh0 matthewwalsh0 matthewwalsh0 approved these changes

@pedronfigueiredo pedronfigueiredo pedronfigueiredo approved these changes

Assignees
No one assigned
Labels
release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-confirmations Push issues to confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Security Alerts API - GET request to get supportedChains blocks the Send Flow until resolved
6 participants
@vinistevam @metamaskbot @seaona @jpuri @pedronfigueiredo @matthewwalsh0