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

feat: use Infura gas API #23717

Merged
merged 20 commits into from
Apr 23, 2024
Merged

feat: use Infura gas API #23717

merged 20 commits into from
Apr 23, 2024

Conversation

OGPoyraz
Copy link
Member

@OGPoyraz OGPoyraz commented Mar 26, 2024

Description

This PR introduces an updated GasFeeController that transitions to the new Infura gas API.

Preview GasFeeController PR: MetaMask/core#4068

Important Note: With the introduction of these changes, developers must complete two critical steps within their Infura project:

  1. Activate the Infura gas API in the Expansion APIs section.

infura all-endpoints

Access this section via https://app.infura.io/key/<INFURA_PROJECT_ID>/all-endpoints. Please replace <INFURA_PROJECT_ID> with the project ID found in your .metamaskrc file.

  1. Ensure that the REQUIRE API KEY SECRET FOR ALL REQUESTS option is disabled.

infura settings

Access this section via https://app.infura.io/key/<INFURA_PROJECT_ID>/settings. Again, <INFURA_PROJECT_ID> with the project ID found in your .metamaskrc file.

Open in GitHub Codespaces

Related issues

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

Manual testing steps

Although there are no functional changes, the update to the gas API URL warrants manual testing of gas-related components to ensure they remain operational.

Screenshots/Recordings

N/A

Before

N/A

After

N/A

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • 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 format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

@OGPoyraz OGPoyraz added the team-confirmations-system DEPRECATED: please use "team-confirmations" label instead label Mar 26, 2024
@OGPoyraz
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@OGPoyraz OGPoyraz marked this pull request as ready for review March 28, 2024 08:42
@OGPoyraz OGPoyraz requested a review from a team as a code owner March 28, 2024 08:42
@OGPoyraz
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policy update failed. You can review the logs or retry the policy update here

@OGPoyraz
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@OGPoyraz OGPoyraz requested review from a team as code owners March 28, 2024 08:53
@OGPoyraz OGPoyraz requested a review from a team as a code owner March 28, 2024 09:46
@@ -190,6 +190,54 @@ async function setupMocking(server, testSpecificMock, { chainId }) {
};
});

// Both are added to support swaps e2e tests
Copy link
Member Author

@OGPoyraz OGPoyraz Mar 28, 2024

Choose a reason for hiding this comment

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

Gas prices are fetch within swaps UI

I am not sure why chainId is override in the tests

Basically if we don't put these mocks then swaps e2e tests will fail, because it's not being caught by XHR mocks and auth modal will appear (as expected in the screenshot)

This is a quick fix but just patching the current implementation, fyi

Screenshot 2024-03-28 at 17 47 19

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, are you saying you believe this wasn't being mocked previously and was hitting the real endpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this issue went unnoticed previously because the former endpoint shared the same domain, allowing it to bypass the privacy snapshot. However, the new endpoint is protected by authentication, which explains why it is prompting for credentials, as in the screenshot.

@metamaskbot
Copy link
Collaborator

Builds ready [2949a1b]
Page Load Metrics (980 ± 533 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint753471387536
domContentLoaded116724147
load6233379801110533
domInteractive116724147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -111 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 355 Bytes (0.01%)

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 69.17%. Comparing base (860c03f) to head (2949a1b).
Report is 121 commits behind head on develop.

❗ Current head 2949a1b differs from pull request most recent head a5df691. Consider uploading reports for the commit a5df691 to get more accurate results

Files Patch % Lines
app/scripts/metamask-controller.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #23717      +/-   ##
===========================================
+ Coverage    69.11%   69.17%   +0.05%     
===========================================
  Files         1160     1160              
  Lines        44296    44263      -33     
  Branches     11850    11832      -18     
===========================================
  Hits         30615    30615              
+ Misses       13681    13648      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shared/constants/swaps.ts Show resolved Hide resolved
@@ -190,6 +190,54 @@ async function setupMocking(server, testSpecificMock, { chainId }) {
};
});

// Both are added to support swaps e2e tests
Copy link
Member

Choose a reason for hiding this comment

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

To clarify, are you saying you believe this wasn't being mocked previously and was hitting the real endpoint?

test/e2e/mock-e2e.js Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [afbf079]
Page Load Metrics (1451 ± 599 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint853611537335
domContentLoaded109029199
load68320914511248599
domInteractive109029199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -111 Bytes (-0.00%)
  • ui: 333 Bytes (0.00%)
  • common: 355 Bytes (0.01%)

matthewwalsh0
matthewwalsh0 previously approved these changes Apr 18, 2024
@OGPoyraz
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [83216da]
Page Load Metrics (1221 ± 604 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint673551205929
domContentLoaded12532394
load55287512211258604
domInteractive12532394
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 115.55 KiB (3.33%)
  • ui: 0 Bytes (0.00%)
  • common: 1.01 KiB (0.02%)

@sleepytanya
Copy link
Contributor

The PR looks great to me. All basic gas fee flows as well as different gas settings work as expected.

@OGPoyraz OGPoyraz merged commit 3ed1ee1 into develop Apr 23, 2024
66 of 69 checks passed
@OGPoyraz OGPoyraz deleted the feat/use-infura-gas-api branch April 23, 2024 08:56
@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 2024
@metamaskbot metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 23, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [4281423]
Page Load Metrics (1799 ± 684 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint792031373818
domContentLoaded107830188
load65350917991424684
domInteractive107830188
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 115.55 KiB (3.30%)
  • ui: 0 Bytes (0.00%)
  • common: 1.01 KiB (0.02%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed release-11.16.0 Issue or pull request that will be included in release 11.16.0 team-confirmations-system DEPRECATED: please use "team-confirmations" label instead
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants