metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

[Bug] Passing EIP-1559 gas params from a Dapp into MM while on non EIP-1559 network sets gas to `0`

Open seaona opened this issue 2 years ago • 11 comments

Describe the bug Whenever we are using a non EIP-1559 network, if a Dapp passes gas params according to EIP-1559 MetaMask accepts the transaction and sets Total gas value to 0.

Screenshots

https://user-images.githubusercontent.com/54408225/234829930-143d88a8-5484-454e-a2cf-aa2c4da930a3.mp4

To Reproduce Steps to reproduce the behavior

  1. Go to the test dapp https://metamask.github.io/test-dapp/
  2. Select a non EIP-1559 network (i.e. Polygon zkEVM testnet)
  3. Deploy failing contract
  4. Click Send failing transaction
  5. See how total amount of gas is set to 0

Expected behavior In this case, on Extension we do not let MM open and the test dapp displays the following error message Invalid transaction params: params specify an EIP-…but the current network does not support EIP-1559',

cc @bschorchit

Smartphone (please complete the following information):

  • Device: Pixel 6
  • OS: android
  • App Version: v6.5.0

to be added after bug submission by internal support / PM Severity

  • How critical is the impact of this bug on a user?
  • Add stats if available on % of customers impacted
  • Is this visible to all users?
  • Is this tech debt?

seaona avatar Apr 27 '23 10:04 seaona

I've added to this sprint backlog. Q: is the user able to confirm this transaction?

bschorchit avatar May 08 '23 13:05 bschorchit

@seaona Not sure if it's just me but the attached video is not playing...

segun avatar May 08 '23 16:05 segun

@bschorchit nope, as the gas params are not correct (see left side, where we are trying to pass mxPriorityFeeperGas, while we are on a non EIP1559 network)

image.png

@segun I believe the video is uploaded correctly. Are you still not able to see it? If not, ping me on slack and I can share it with you

seaona avatar May 16 '23 15:05 seaona

@bschorchit , @seaona : I would prefer to de-prioritise this issue reason being that change to throw error for such a transaction is required to be done in transaction controller. Transaction controller version used in mobile is not updated. Mobile is using "@metamask/transaction-controller": "4.0.0" while the latest version is 5.0.0. Thus it will be major version update which can be a breaking change.

Since the issue is not critical, I avoid prefer to do this change only after transaction controller is updated.

Since user is not not passing gasPrice metamask is setting it to 0. User is passing maxFee and priorityFee which are not used for legacy transactions.

Please suggest.

jpuri avatar May 18 '23 07:05 jpuri

For visibility, this is being deprioritized until mobile has updated transaction controller.

Is this https://github.com/MetaMask/metamask-mobile/pull/6291 the correct PR to track transaction controller update on mobile?

bschorchit avatar May 18 '23 11:05 bschorchit

@bschorchit : PR is actually only patching transaction controller not updating it. As I know mobile team is working towards un-blocking controller updates, its hard to do that as there are too many interdependencies between controllers and breaking changes to handle.

An option is that we make this fix in controller and wait for next update to include it.

jpuri avatar May 18 '23 11:05 jpuri

The behaviour of extension here has been reported as an issue: https://github.com/MetaMask/metamask-extension/issues/13862

I think correct approach in this case will be to use default network suggested gas price and not 0. This should also be done in controller here.

jpuri avatar May 18 '23 14:05 jpuri

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 7 days. Thank you for your contributions.

github-actions[bot] avatar Sep 03 '23 14:09 github-actions[bot]

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 7 days. Thank you for your contributions.

github-actions[bot] avatar Dec 04 '23 01:12 github-actions[bot]

This issue was closed because there has been no follow activity in 7 days. If you feel this was closed in error please provide evidence on the current production app in a new issue or comment in the existing issue to a maintainer. Thank you for your contributions.

github-actions[bot] avatar Dec 11 '23 01:12 github-actions[bot]

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 7 days. Thank you for your contributions.

github-actions[bot] avatar Jun 25 '24 16:06 github-actions[bot]

This issue was closed because there has been no follow activity in 7 days. If you feel this was closed in error please provide evidence on the current production app in a new issue or comment in the existing issue to a maintainer. Thank you for your contributions.

github-actions[bot] avatar Jul 02 '24 16:07 github-actions[bot]