Fix: use estimate gas instead of fixed gas by vinistevam · Pull Request #22374 · MetaMask/metamask-extension

vinistevam · 2023-12-21T13:12:22Z

Description

Update the transaction controller to:

Fix to properly estimate the gas instead of using a fixed gas when deploying a contract without specifying the gas.

See related core PR.

Related issues

Fixes: #22359

Manual testing steps

Transaction regression

Scenario of the error

Go to https://stackblitz.com/github/wevm/viem/tree/main/examples/contracts_deploying-contracts
Wait for the example to build
Connect MM
Click deploy
Submit tx
Contract should deploy successfully

Screenshots/Recordings

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.

github-actions · 2023-12-21T13:12:34Z

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.

sleepytanya · 2023-12-21T16:08:03Z

Contract successfully deployed:

gas.mov

## **Description** OpenSea renamed the field `supply`, which used to be required, to `total_supply`. Fixes the error: `undefined is not an object (evaluating 'contract.supply.toString')` Which prevented some NFTs from loading. A more accurate `total_supply` was also added to the collection object, which is now preferred to the one on the contract. ## **Related issues** ## **Manual testing steps** 1. Import a wallet containing NFTs on mainnet 2. Enable NFT autodetection in settings, verify the NFTs appear 3. Verify descriptions, collection/token images are correct ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [ ] I've linked related issues - [x] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] 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. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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.

## **Description** This is an automation test for toggling the hex data. The user should be able to input text via the send transaction feature.  ## **Related issues** Fixes: #22271 ## **Manual testing steps** 1. Go to User settings page 2. In the advanced tab, there should be toggle for "Show Hex Data" 3. In the send transaction, verify that the HexData text box appears. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [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/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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.

## **Description** Adds the ledger-iframe to the offscreen document and establishes MV3 support for ledger. This is done by the following method: 1. ledger-iframe.ts interfaces with the ledger keyring iframe, working as a middle layer between the two. Messages received by the offscreen document that target the ledger iframe will be forwarded to the ledger keyring iframe (the github pages generated content from @MetaMask/eth-ledger-bridge-keyring. A callback manager uses an incremental id to assign an id to each message which is returned in the response such that a callback can be declared only for a specific id. 2. ledger-offscreen-bridge.ts is a bridge for the LedgerKeyring that interfaces with the offscreen document such that when the keyring calls out for specific methods, it calls into the offscreen bridge and then the communication is sent to the offscreen document and forwarded to the keyring iframe. In addition some more cleanup of the #18794 deprecation of u2f and ledger live. We no longer support setting any other method than the browsers lowest level method for connecting. For chrome webhid is the gold standard, for firefox which doesn't support webhid we use u2f. There were still a number of places in the code that reference the transportType from preferences and migrations were done to change the value for many users when we only need to know what the support for those two protocols are. So firstly it changes the setLedgerTransportPreference to not take an input, but rather just uses the browsers support of navigator.hid to determine whether to use webhid or u2f. When #18794 landed service workers did not have access to webhid, now they do, so this is a viable approach. I also added deprecation messages where appropriate. In a future PR we should go through and remove any usages of the preference. Ideally we'd also set the transport type for the ledger device closer to the bridge, however i'm not familiar with the unlock mechanism of the keyring and trying to do this was causing issues with the mv3 connectivity. ## **Related issues** Fixes #17724 - No longer relevant because background/service workers have access to window.navigator.webhid Fixes #21624 - Connects ledger to offscreen doc ## **Screenshots/Recordings** ## **Manual testing steps** 1. run `yarn start:mv3` 2. reload the extension in your browser. 3. try to add a ledger device. (Remember you must have your device unlocked and the ethereum app open on your ledger 4. Try to use the test dapp to send tx, sign messages, etc. ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [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/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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.

## **Description** Move `FormTextField` js version into deprecated folder with deprecation notice Update import of any production uses for `FormTextField` to the js version Update/create TS version of FormTextField  ## **Related issues** Fixes: #19120 ## **Manual testing steps** 1. Run storybook 2. [Visit FormTextField](http://localhost:6006/?path=/docs/components-componentlibrary-formtextfield--docs) 3. Test inputs and props ## **Screenshots/Recordings**  ### **Before**  ### **After** https://github.com/MetaMask/metamask-extension/assets/26469696/94f335e5-aa48-49e1-88f7-08d5660a309a  ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [X] I've clearly explained what problem this PR is solving and how it is solved. - [X] I've linked related issues - [X] I've included manual testing steps - [X] I've included screenshots/recordings if applicable - [X] I’ve included tests if applicable - [ ] 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/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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. --------- Co-authored-by: George Marshall <george.marshall@consensys.net>

@Gudahtt

## **Description** This PR addresses some issues that were identified by @Gudahtt when reviewing the diff for v11.7.3 (#22354), although the comments were put on the v11.7.2 PR, because what is currently 11.7.3 was once 11.7.2: #22336 ![Screenshot from 2023-12-21 08-34-37](https://github.com/MetaMask/metamask-extension/assets/7499938/fbbcdaef-d3a6-4dc2-bf50-75ad0ab2452a) ![Screenshot from 2023-12-21 08-34-23](https://github.com/MetaMask/metamask-extension/assets/7499938/36ec46da-7626-4f47-82d0-348d3478d141) ![Screenshot from 2023-12-21 08-33-58](https://github.com/MetaMask/metamask-extension/assets/7499938/acf805ad-a014-4e37-bf4c-adefc664251f) ## **Manual testing steps** 1. The token testing scenarios should all pass https://github.com/MetaMask/metamask-extension/tree/develop/test/scenarios/4.%20tokens 2. Fiat values for erc20 tokens should be correctly displayed 3. For the scenarios in 1 and 2 related to autodetection and token display, the tests should also pass after switching networks and switching networks multiple times in short succession ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've clearly explained what problem this PR is solving and how it is solved. - [ ] I've linked related issues - [ ] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] 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. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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.

socket-security · 2023-12-26T10:01:28Z

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/transaction-controller	18.3.1...19.0.1	None	`+8/-7`	3.17 MB	metamaskbot

VersoriumX · 2023-12-30T23:33:37Z

Upon review all checks pass

sleepytanya · 2024-01-04T14:59:49Z

No issues found with the transaction regression.

socket-security · 2024-01-04T15:21:21Z

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue	Package	Version	Note	Source
New author	eth-method-registry	3.0.0	New Author: lgbot Previous Author: rekmarks	@metamask/transaction-controller@19.0.1 via `package.json`

Next steps

What is new author?

A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package.

Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore eth-method-registry@3.0.0

codecov · 2024-01-05T06:38:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3902e7) 68.10% compared to head (8dc06d7) 68.15%.

❗ Current head 8dc06d7 differs from pull request most recent head 5c68509. Consider uploading reports for the commit 5c68509 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #22374      +/-   ##
===========================================
+ Coverage    68.10%   68.15%   +0.06%     
===========================================
  Files         1083     1081       -2     
  Lines        42477    42295     -182     
  Branches     11357    11302      -55     
===========================================
- Hits         28925    28826      -99     
+ Misses       13552    13469      -83

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

vinistevam · 2024-01-05T08:53:12Z

@metamaskbot update-policies

metamaskbot · 2024-01-05T09:03:56Z

No policy changes

add transaction controller patch estimation gas fix

9dddc2e

Merge branch 'develop' into fix-tx-controller-estimate-gas

78cbe66

sleepytanya added the QA Passed label Dec 21, 2023

vinistevam changed the title ~~WIP: fix gas estimation~~ WIP: Fix: use estimate gas instead of fixed gas Dec 21, 2023

bergeron and others added 8 commits December 26, 2023 09:46

fix: JSDoc in preferences-controller (#21890)

b4d6ff3

bump transaction controller and remove patch

23a89da

Merge branch 'develop' into fix-tx-controller-estimate-gas

e417b33

vinistevam changed the title ~~WIP: Fix: use estimate gas instead of fixed gas~~ Fix: use estimate gas instead of fixed gas Dec 26, 2023

vinistevam marked this pull request as ready for review December 26, 2023 09:50

vinistevam requested a review from a team as a code owner December 26, 2023 09:50

vinistevam added the team-confirmations-system-deprecated DEPRECATED: please use "team-confirmations" instead label Dec 26, 2023

dbrans previously approved these changes Jan 3, 2024

View reviewed changes

cryptotavares previously approved these changes Jan 3, 2024

View reviewed changes

Merge branch 'develop' into fix-tx-controller-estimate-gas

5089265

vinistevam dismissed stale reviews from cryptotavares and dbrans via 5089265 January 5, 2024 10:16

vinistevam force-pushed the fix-tx-controller-estimate-gas branch from ea8bc7f to 5089265 Compare January 5, 2024 10:16

cryptotavares previously approved these changes Jan 5, 2024

View reviewed changes

OGPoyraz previously approved these changes Jan 8, 2024

View reviewed changes

vinistevam added 2 commits January 10, 2024 08:48

Merge branch 'develop' into fix-tx-controller-estimate-gas

b663266

fix conflict yarn.lock

5c68509

vinistevam dismissed stale reviews from OGPoyraz and cryptotavares via 5c68509 January 10, 2024 08:50

vinistevam force-pushed the fix-tx-controller-estimate-gas branch from 8dc06d7 to 5c68509 Compare January 10, 2024 08:50

matthewwalsh0 approved these changes Jan 10, 2024

View reviewed changes

OGPoyraz approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'develop' into fix-tx-controller-estimate-gas

2cee21f

vinistevam merged commit 2f894db into develop Jan 10, 2024

vinistevam deleted the fix-tx-controller-estimate-gas branch January 10, 2024 12:58

github-actions bot locked and limited conversation to collaborators Jan 10, 2024

metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: use estimate gas instead of fixed gas#22374

Fix: use estimate gas instead of fixed gas#22374
vinistevam merged 14 commits intodevelopfrom
fix-tx-controller-estimate-gas

vinistevam commented Dec 21, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 21, 2023

Uh oh!

sleepytanya commented Dec 21, 2023

Uh oh!

socket-security bot commented Dec 26, 2023 •

edited

Loading

Uh oh!

VersoriumX commented Dec 30, 2023

Uh oh!

sleepytanya commented Jan 4, 2024

Uh oh!

socket-security bot commented Jan 4, 2024 •

edited

Loading

Uh oh!

codecov bot commented Jan 5, 2024 •

edited

Loading

Uh oh!

vinistevam commented Jan 5, 2024

Uh oh!

metamaskbot commented Jan 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

Conversation

vinistevam commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Dec 21, 2023

Uh oh!

sleepytanya commented Dec 21, 2023

Uh oh!

socket-security bot commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VersoriumX commented Dec 30, 2023

Uh oh!

sleepytanya commented Jan 4, 2024

Uh oh!

socket-security bot commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Next steps

Uh oh!

codecov bot commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vinistevam commented Jan 5, 2024

Uh oh!

metamaskbot commented Jan 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

vinistevam commented Dec 21, 2023 •

edited

Loading

socket-security bot commented Dec 26, 2023 •

edited

Loading

socket-security bot commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading