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 gas estimation for ETHSend transactions #11414

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Conversation

onyb
Copy link
Member

@onyb onyb commented Dec 3, 2021

There were two main issues with gas estimation for ETHSend:

  1. We skipped querying eth_estimateGas for ETHSend transactions, and hardcoded the value 21000. This doesn't work for simple transfers to smart contracts which are slightly heavier in gas. We still fallback to 21000 if the call to eth_estimateGas fails in the RPC controller.
  2. We were using 0x0 for the data field while querying eth_estimateGas, which always returned the following error response:
    {
      "jsonrpc": "2.0",
      "id": 1,
      "error": {
        "code": -32602,
        "message": "invalid argument 0: json: cannot unmarshal hex string of odd length into Go struct field TransactionArgs.data of type hexutil.Bytes"
      }
    }
    We never noticed this because we had a fallback to 21000 in place in case of an error. Turns out that geth wants us to either use "0x" or "" or completely omit the data field from the request for ETH transfers.

Resolves brave/brave-browser#19835.

Uplifts 1.34.x 1.33.x 1.32.x

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Demo and test plan

👉 Attn QA: The test plan consists of the following cases:

  1. Create an ETH transfer. Verify that the gas limit is 21000.
  2. Create an ETH transfer to the address 0x92cd4bd69e305634a72fc95992ecfa0699c784e7. Verify that the gas limit is 23276. The recipient address came from a user report (see Don't hardcode 21000 for gas limit for ETH transfers brave-browser#19835).
  3. Create an ERC-20 token transfer. Verify that the gas limit is something large (typically more than 50000).
  4. Create a swap transaction. Verify that the gas limit is something close to 200000.
  5. Create a MATIC transfer on Polygon chain. Verify that the gas limit is 21000.
  6. Create a token transfer (ex, PoS USDC) on Polygon chain. Verify that the gas limit is something greater than 21000.
Screen.Recording.2021-12-03.at.11.08.23.AM.mov

@onyb onyb requested review from bbondy, yrliou and a team December 3, 2021 05:44
@onyb onyb self-assigned this Dec 3, 2021
@onyb onyb force-pushed the h/wallet/gas-estimation-fix branch from ee648d8 to 1958e3d Compare December 3, 2021 06:24
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

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

Just a few nits left.

components/brave_wallet/browser/eth_tx_controller.cc Outdated Show resolved Hide resolved
components/brave_wallet/browser/eth_tx_controller.cc Outdated Show resolved Hide resolved
@onyb onyb force-pushed the h/wallet/gas-estimation-fix branch from 1958e3d to 06d9c0a Compare December 3, 2021 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't hardcode 21000 for gas limit for ETH transfers
3 participants