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

[Bug]: Gas Limit value suggested from a Dapp is not respected, defaulting always to 21000 #18417

Closed
seaona opened this issue Apr 3, 2023 · 6 comments · Fixed by #29352
Closed

[Bug]: Gas Limit value suggested from a Dapp is not respected, defaulting always to 21000 #18417

seaona opened this issue Apr 3, 2023 · 6 comments · Fixed by #29352
Assignees
@jpuri
Labels
needs-triage release-12.11.0 Issue or pull request that will be included in release 12.11.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug

Comments

@seaona
Copy link
Contributor

seaona commented Apr 3, 2023

Describe the bug

Problem: when I start a transaction from a Dapp, if I've set a gasLimit value, I can see how it is not respected and it always defaults to 21000.

This happens both to Legacy and EIP1159 transactions.

gaslimit-not-respecteddapp.mp4

Steps to reproduce

  1. Clone test dapp
  2. Change line 865 and 882 gasLimit to another value (i.e. 0x5277 which is 21111)
  3. Start test dapp yarn start
  4. Connect MM
  5. Click Send Legacy // Send EIP1159
  6. Click Edit Fees
  7. Click Edit --- see how gasLimit is the same as always 21000

Error messages or log output

No response

Version

10.27.0

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response
@seaona seaona added type-bug team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Apr 3, 2023
@bschorchit
Copy link

Possibly related with #16593

And possibly caused by this: #6625

@bschorchit bschorchit mentioned this issue Apr 11, 2023
Merged
@bschorchit bschorchit added team-confirmations-planning (only for internal use within Confirmations team) and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Jun 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Sep 5, 2023
@github-actions
Copy link
Contributor

This issue was closed because there has been no follow up activity in the last 45 days. If you feel this was closed in error, please reopen and provide evidence on the latest release of the extension. Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2023
@seaona
Copy link
Contributor Author

seaona commented Mar 6, 2024

this issue is still present

@seaona seaona reopened this Mar 6, 2024
@seaona seaona removed the stale issues and PRs marked as stale label Mar 6, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Mar 6, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Apr 9, 2024
@gauthierpetetin gauthierpetetin added Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Apr 12, 2024
@cryptotavares cryptotavares added the team-confirmations Push issues to confirmations team label Apr 24, 2024
@cryptotavares cryptotavares added team-transactions Transactions team and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead team-confirmations-planning (only for internal use within Confirmations team) team-confirmations Push issues to confirmations team labels May 3, 2024
@dbrans dbrans added team-confirmations Push issues to confirmations team and removed team-transactions Transactions team labels Jul 30, 2024
@bschorchit
Copy link

@seaona do you know if this (overriding the dapp proposed value for gas limit) also happens for other transaction types?

@bschorchit
Copy link

bschorchit commented Sep 18, 2024 •
edited
Loading

Potential solution: we should not use the dapp gas limit if it's below 21000, but use it if it's higher than that.

@jpuri jpuri self-assigned this Dec 19, 2024
@jpuri jpuri mentioned this issue Dec 19, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Dec 20, 2024
@jpuri
fix: UI is not displaying gas limit set by dapp (#29352)
a33f526 
## **Description**

UI is not displaying correct gas limit set by dapp. It always displays
`21000`.

## **Related issues**

Fixes: #18417

## **Manual testing steps**

1. Submit confirmation request with gas limit not `21000`
2. Check gas limit in gas editing popup

## **Screenshots/Recordings**
<img width="354" alt="Screenshot 2024-12-19 at 7 20 45 PM"
src="https://github.com/user-attachments/assets/db53d4f0-bf3b-46cf-970c-8c167ebcf6c4"
/>

## **Pre-merge author checklist**

- [X] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [X] I've completed the PR template to the best of my ability
- [X] I’ve included tests if applicable
- [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [X] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.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.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Dec 20, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Dec 20, 2024
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jpuri jpuri

Labels
needs-triage release-12.11.0 Issue or pull request that will be included in release 12.11.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug
Projects
Bugs by severity
Archived in project
Bugs by team
Status: Fixed
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: UI is not displaying gas limit set by dapp MetaMask/metamask-extension
7 participants
@dbrans @jpuri @gauthierpetetin @cryptotavares @bschorchit @metamaskbot @seaona