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

Prevent setting gas price below minimum #10519

Open
leafcutterant opened this issue Feb 25, 2021 · 6 comments
Open

Prevent setting gas price below minimum #10519

leafcutterant opened this issue Feb 25, 2021 · 6 comments
Labels
area-gas team-confirmations-planning (only for internal use within Confirmations team) type-enhancement

Comments

@leafcutterant
Copy link

leafcutterant commented Feb 25, 2021

We should query txpool.pricebump from the current network and disallow setting a new gas price that falls below that. This way users wouldn't even get to the point of trying to submit a transaction with a gas price below the minimum (which will be certainly rejected).

Original issue description (see discussion below for edit reason):

Somewhat related to #9317, in that the way to reproduce is the same, only the error message is now different:

Transaction N failed! Error: [ethjs-rpc] while formatting outputs from RPC '{"value":{"code":-32000, "message":"replacement transaction underpriced"}}'

Stop making decisions instead of the user on what gas price is allowed. If I want to speed up by only adding 10 Gwei to the original, and I'm fine with my transaction confirming within 48 hours, then let me do it.

  • MetaMask Version: 9.0.5
@leafcutterant leafcutterant changed the title Stop prohibiting speedups whch are "underpriced" Stop prohibiting speedups that are "underpriced" Feb 25, 2021
@Gudahtt
Copy link
Member

Gudahtt commented Feb 25, 2021

Thanks for the report! Unfortunately, there is little we can do about this. We don't impose any such restriction. This error comes from Infura, which requires that replacement transactions have a gas price at least 10% higher than the last gas price.

See the txpool.pricebump option here for more information. The default is 10 (i.e. 10%), which is what Infura uses.

@Gudahtt Gudahtt closed this as completed Feb 25, 2021
@leafcutterant
Copy link
Author

Oh. This is annoying. However, what you could do is display a useful error message instead of this garbled text, so people can act on it. How about:

Sorry, to speed up this transaction, please increase the gas price by at least X%, to Y Gwei.

where X% is queried from Infura and Y is calculated from X% and the previous gas price.

@Gudahtt
Copy link
Member

Gudahtt commented Mar 1, 2021

Yes, that's a good point! This error isn't very informative.

How did you encounter this error by the way? Did you see this error in a dapp after suggesting a transaction with too low of a gas price? Or was this seen on the confirmation UI, or in the send flow?

@leafcutterant
Copy link
Author

The error was displayed as a Metamask notification pop-up after I approved the transaction from MM. I got it after manually setting a new, higher gas price for a transaction I previously sent.

@leafcutterant
Copy link
Author

Hey, how about querying txpool.pricebump from the endpoint and not even allowing setting a new gas price that falls below that? This way users wouldn't even get to the point of trying to submit the transaction (which will be certainly rejected).

Would you consider renaming the issue accordingly and reopen it?

@Gudahtt
Copy link
Member

Gudahtt commented Mar 11, 2021

Certainly! That sounds reasonable

@Gudahtt Gudahtt changed the title Stop prohibiting speedups that are "underpriced" Prevent setting gas price below minimum Mar 11, 2021
@Gudahtt Gudahtt reopened this Mar 11, 2021
@bschorchit bschorchit added area-transactions team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels May 25, 2022
@hilvmason hilvmason 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 5, 2023
brad-decker added a commit that referenced this issue Sep 26, 2023
## **Description**
Our fork of ethjs-query has been namespaced to the `@metamask/`
namespace, and updated with mostly development only fixes. There is one
exception which is the reason for this pull request which is the removal
of a try/catch that was catching too broadly and wrapping legitimate
errors in a new Error object that claimed the issue was with the
formatting of the output. In most cases this is incorrect and results in
a wide swath of errors being lumped together inside of sentry. This
change will result in the real errors being surfaced, after which we can
decide where to prioritize efforts to resolve RPC issues.

This PR progresses, and is expected to change the stack trace for the
following issues:
#9317 
#10519 
#10619
#11488 
#11974 
#13395 
#14298 
#14365 
#15250 
#17073 
#17803 
#19697 
#20699 

We are closing these issues opened automatically by sentry-io which are
not fully resolved but should result in better errors and stack traces:

fixes #10552 
fixes #14660 
fixes #14676 
fixes #14730 
fixes #14801
fixes #15065 

## **Manual testing steps**
1. Attempt to reproduce any of the bugs listed, 17073 was the easiest to
reproduce for me. This involves getting test currency from the wemix
faucet as listed in the issue and initiating a transaction between two
accounts you own.
2. On develop you'll see an 'error formatting outputs' error text
similar to what is reported in the issue.
3. On this branch you'll get the original error, without the wrapped
'formatting' error. including a different stack trace.

## **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:
  - [x] What problem this PR is solving.
  - [x] How this problem was solved.
  - [x] How reviewers can test my changes.
- [x] I’ve indicated what issue this PR is linked to: Fixes #???
- [x] I’ve included tests if applicable.
- [x] I’ve documented any added code.
- [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)).
- [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.

---------

Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-gas team-confirmations-planning (only for internal use within Confirmations team) type-enhancement
Projects
None yet
Development

No branches or pull requests

4 participants