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: increase gas limit validation threshold #29264

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

vinistevam
Copy link
Contributor

@vinistevam vinistevam commented Dec 17, 2024

Description

This PR aims to increase the gas limit validation threshold to 30M.

Open in GitHub Codespaces

Related issues

Fixes: #21927

Manual testing steps

  1. send a transaction
  2. edit gaslimit on advance tab

Screenshots/Recordings

Screencast.from.17-12-2024.08.51.19.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 Dec 17, 2024
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.

@vinistevam vinistevam marked this pull request as ready for review December 17, 2024 08:52
@vinistevam vinistevam requested a review from a team as a code owner December 17, 2024 08:52
@metamaskbot
Copy link
Collaborator

Builds ready [5a837f0]
Page Load Metrics (1666 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1533185816668541
domContentLoaded1517183916368742
load1532186316668641
domInteractive25164423215
backgroundConnect10139323115
firstReactRender1698483014
getState56913168
initialActions01000
loadScripts1132134012286029
setupStore677182010
uiStartup17792370199420197
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 1 Bytes (0.00%)

@vinistevam vinistevam added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@@ -154,7 +154,7 @@ describe('AdvancedGasFeeGasLimit', () => {
);
expect(
screen.queryByText(
`Gas limit must be greater than 29999 and less than 7920050`,
`Gas limit must be greater than 29999 and less than 30000000`,
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't the lower limit 20999?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I also tried to fix that and got stuck until I realised that we have an error logic where we use editGasLimitOutOfBoundsV2 which accepts two values so in the mocking data we are sending originalGasEstimate: '0x78D9B2' (30000) and there is this logic here changing the min to 29999.

@metamaskbot
Copy link
Collaborator

Builds ready [ecca93f]
Page Load Metrics (1796 ± 160 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint146529371800338162
domContentLoaded141229091770342164
load146729191796333160
domInteractive23189564421
backgroundConnect85724136
firstReactRender16103332713
getState585292512
initialActions01000
loadScripts102623761337304146
setupStore76119189
uiStartup167132962063402193
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 1 Bytes (0.00%)

@vinistevam vinistevam enabled auto-merge December 17, 2024 14:11
@metamaskbot
Copy link
Collaborator

Builds ready [1480430]
Page Load Metrics (1689 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint21919301555454218
domContentLoaded14171921167114670
load14301981168915173
domInteractive2410838199
backgroundConnect76620178
firstReactRender15102442814
getState596192512
initialActions01000
loadScripts10171432122012861
setupStore686202512
uiStartup166926251999248119
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 1 Bytes (0.00%)

@vinistevam vinistevam added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@vinistevam vinistevam added this pull request to the merge queue Dec 17, 2024
Merged via the queue into main with commit 0ef0d54 Dec 17, 2024
77 checks passed
@vinistevam vinistevam deleted the fix/increase-gas-limit-allowance branch December 17, 2024 15:29
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2024
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Dec 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Should allow users to use gasLimit above 7920027
5 participants