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

eth: sending full ETH balance isn't resilient to small gas price changes #2186

Closed
norwnd opened this issue Feb 28, 2023 · 10 comments · Fixed by #2234
Closed

eth: sending full ETH balance isn't resilient to small gas price changes #2186

norwnd opened this issue Feb 28, 2023 · 10 comments · Fixed by #2234
Milestone

Comments

@norwnd
Copy link
Contributor

norwnd commented Feb 28, 2023

Noticed that trying to send out all ETH funds often errors like this for me on testnet, latest master (either send attempt itself is rejected, or a warning about fee estimation pops up):

sending-eth-issues.webm

a couple of related points/questions on this to discuss:

  • I observed this issue today quite consistently, that's probably not a behavior users are expected to hit often or maybe ever; as you can see on the vid ^ the difference in gas price (when estimates taken 1st to decide how much to send, and when 2nd verification check happens) isn't huge (it isn't small there either, but I saw sub 3% difference to prevent send attempt), should it handle small differences without error (what the threshold/wiggle-room should be, something on the order of 10-20% maybe) ?
  • I expect this issue might happen for ERC20s too when user has barely enough ETH to cover token transfer fees, such that slight gas price increase could render his balance lacking when 2nd check performed
  • on the vid above I was sending ETH (which has a fixed 21 000 gas limit, and it is spent 100% unlike with ERC20s where it gets much worse) and it seems the dust left after successful attempt in ETH account wasn't an insignificant amount; the main reason seems to be maxFee set to 2x of baseFee - which effectively (very often) means we are leaving roughly same amount of ETH as we spend on transaction in the wallet as dust for the use-case when user just wants his funds out (maybe never touching wallet again); any reason to use maxFee that high here instead of letting user to rebroadcast instead (if necessary) for sending out of his wallet (when trades are processing setting maxFee high is probably a warranted safety margin, no questions there) ?
@norwnd
Copy link
Contributor Author

norwnd commented Feb 28, 2023

And just saw another "flavor" of same error, where formula instead of exact values is displayed, not sure whether values should be there (why it's different from similar error message from above), leaving here as a reminder to maybe check that:

image

@chappjc
Copy link
Member

chappjc commented Feb 28, 2023

  • the dust left after successful attempt in ETH account wasn't an insignificant amount; the main reason seems to be maxFee set to 2x of baseFee - which effectively (very often) means we are leaving roughly same amount of ETH as we spend on transaction in the wallet as dust for the use-case when user just wants his funds out (maybe never touching wallet again); any reason to use maxFee that high here instead of letting user to rebroadcast instead (if necessary) for sending out of his wallet (when trades are processing setting maxFee high is probably a warranted safety margin, no questions there) ?

This is a default recommended max fee in various eth wallet software (actually 2*currentBaseFee + tip) so that your tx can get mined if the base fee goes up, and it fluctuates a good deal. We can use a lower multiplier perhaps, but transactions will end up getting stuck.

And the "dust" is unavoidable regardless of how lucky we get with the current base fee. Just try to "sweep" a metamask wallet. I always end up with lots of dust trying to send the max amount. If the eth community has solved this, I'm not aware of the solution.

@chappjc
Copy link
Member

chappjc commented Feb 28, 2023

  • I observed this issue today quite consistently, that's probably not a behavior users are expected to hit often or maybe ever; as you can see on the vid ^ the difference in gas price (when estimates taken 1st to decide how much to send, and when 2nd verification check happens) isn't huge (it isn't small there either, but I saw sub 3% difference to prevent send attempt), should it handle small differences without error (what the threshold/wiggle-room should be, something on the order of 10-20% maybe) ?
  • I expect this issue might happen for ERC20s too when user has barely enough ETH to cover token transfer fees, such that slight gas price increase could render his balance lacking when 2nd check performed

We have some tough problems with ETH and tokens to solve since we've decided to present the user with these "max send amounts". Personally I think it's a shame we have to get distracted by this when we want to focus on the swap aspects, but I'll try to be constructive... we should spend some time to make sure these errors are not so common, I agree. Not sure what numbers we need to tweak to get estimates that don't go stale quite so easily.

@chappjc
Copy link
Member

chappjc commented Feb 28, 2023

Probably for ETH we just need to use a larger gas price for the max send estimate than we use for the actual send. Yeah, that will lead to dust, but same issue we will always have with dynamic/eip1559 txns.

@norwnd
Copy link
Contributor Author

norwnd commented Mar 1, 2023

We have some tough problems with ETH and tokens to solve since we've decided to present the user with these "max send amounts". Personally I think it's a shame we have to get distracted by this when we want to focus on the swap aspects

True, that's certainly not DEX value proposition, still ... it kinda feeds into it (at least until Ethereum alternatives are present on DEX; high ETH fees are a pain and keeping many retail users out of Ethereum, I personnally use it only rarely for that reason and prefer to take higher risk of Binance Smart Chain or other L1s / ETH L2s)

just to document the shortcomings of this 2*baseFee approach (in case we'd wanna get back to it at some point):

  • for ETH, it results in suboptimal (compared to what's possible to implement) amount of ETH left as dust when user just wants to empty his ETH account
  • for ERC20s (and also ETH to much lesser extent), to send his funds user will need more ETH to cover fees than he might expect (I encountered this issue personally with Trust Wallet in the past when trying to pre-seed it with enough ETH with just 1 transaction, had to make a 2nd one ... and it still doesn't have a knob to let me configure the desired gas price myself ...)

If the eth community has solved this, I'm not aware of the solution.

I don't think it's fully solvable, but 1) broadcasting txn with conservative fee (say, baseFee * 1.1) and 2) rebroadcasting txn with higher fee if it didn't get mined in a while (say, 5 mins) would be a typical way to get those transaction margins as tight as they can be. Dust will result from txn not spending gas limit in full, but at least not from overlocking on generous gas price.

  • for rebroadcasting we'd need to track nonce, but I think I've seen this implemented already somewhere (it should probably be reusable for this also)
  • as for signalling what happens with transaction to the user, we could just display Etherscan link in UI so he can track transaction status there + have a rebroadcast with higher fee button he can iteratively click until eventually miners accept his rebroadcast attempt (that's pretty much how it works in Metamask, the "Low" fee setting they have roughly equals baseFee, the default "Market" one is more like baseFee*1.4)

Anyway, that's how I see it, it's certainly not a priority any time soon, just merely a possible way to improve what's currently there.

@chappjc
Copy link
Member

chappjc commented Mar 1, 2023

  • have a rebroadcast with higher fee button he can iteratively click until eventually miners accept his rebroadcast attempt

Meanwhile urgent swap transactions with higher nonces cannot execute. No. We've lost the plot if we think this kind of footgun is ok. UX considerations in DEX must consider the swaps before all else.

The weakest link in a sequence of transactions holds them all up. This is how the ETH wizards have decided it should work.

Let's address this max send estimate issue, but we're not making it even easier to clobber their active swaps... especially not if our best mitigation is replacement txns.

@JoeGruffins
Copy link
Member

The max send logic here has been changing a bit recently and maybe isn't optimised for eth. It failing like that can probably be fixed.

But having funds left over is unavoidable currently unless eth implements a way to pay fee from transaction.

If you are comparing the experience to a traditional exchange it's different of course because they have all the funds in one account and show you numbers from a database.

@chappjc
Copy link
Member

chappjc commented Mar 20, 2023

This ETH send issue is pretty bad. Unless gas is presently trending down, and monotonically, you have to lower the send amount to actually send. @JoeGruffins can you remind what recent max send logic would be messing this up?

For 0.6 I think we should just have the max send estimate use a buffered max fee cap so that the max fee cap that actually gets used in the transaction is adequate.

On a related note, one of the most fundamental functions of almost all gui wallets is the ability to specify fee rates. The problem exists because the backend figures this value on submission of the send request.

@chappjc chappjc added this to the 0.6 milestone Mar 20, 2023
@martonp
Copy link
Contributor

martonp commented Mar 21, 2023

I will fix this.

@JoeGruffins
Copy link
Member

JoeGruffins commented Mar 22, 2023

@chappjc I was thinking of #1888 but it looks like that was never merged, so nevermind

Oh, it became #1967, so that one

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 a pull request may close this issue.

4 participants