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

Refund fees #13135

Closed
robert-zaremba opened this issue Sep 2, 2022 · 8 comments
Closed

Refund fees #13135

robert-zaremba opened this issue Sep 2, 2022 · 8 comments
Labels
S:needs architecture review To discuss on the next architecture review call to come to alignment

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Sep 2, 2022

Problem Definition

Setting tx fees is pain in the ass from the user perspective:

  • He doesn't really know the gas amount he need for a tx. If he will set too high, then he will be charged more than needed
  • It's confusing: fee vs gas price & gas-limit (that's more for tooling - we don't have much narrative to improve that flows)
  • Tx will fail if user will use fee that it is too small.

Proposal

Now, with 1) Tx prioritizaiton and 2) post ante handlers we can refund fees. Proposed flow
The gas charged should be max(gas_limit / 2, gas_used) * gas_price (same as proposed in evmos/ethermint#1085)

  1. change defaults to use gas-price and gas-limit in tx (CLI) instead of total fee.
  2. in pre-ante handler, do as today:
    • verify gas-price against validator min-gas-price (in check-tx)
    • charge gas_limit * gas_price (in deliver-tx)
  3. in post-ante handler
    • check the gas spent, and send back to the user the unused refund: gas_limit * gas_price - max(gas_limit / 2, gas_used) * gas_price
  4. make min-gas-price setting query-able

Let's talk about viability of this solution.

Related: #2150, evmos/ethermint#1085

@robert-zaremba robert-zaremba added T: Feature S:needs architecture review To discuss on the next architecture review call to come to alignment labels Sep 2, 2022
@yihuang
Copy link
Collaborator

yihuang commented Sep 2, 2022

There was a proposal in another issue that only refund min(gas_limit - gas_used, gas_limit / 2) * gas_price
Because our block proposal is based on gas limit, otherwise it's vulnerable to DoS by setting a big gas limit.

@robert-zaremba
Copy link
Collaborator Author

good point! To be able to account in block we would need Tendermint update :/

@alexanderbez
Copy link
Contributor

Personally, I do not find fees or the UX confusing at all. It's all quite simple and everything is documented. Yes, we need gas-adjustment, which is annoying I know, but that's just an artifact of how BaseApp is designed. Apart from that, it's all very simple.

The two big UX pain points are:

  1. The need for gas-adjustment
  2. Min gas prices not being public

So it's not completely fair to say "pain in the ass".

Anyway. I do agree that a refund mechanism is wanted. Also, we already have an issue for this if you searched: #2150

So we should close this one :)

@robert-zaremba
Copy link
Collaborator Author

I think the right UX is following:

  • user sets maximum gas (gas-limit) and gas-price
  • user will be refunded as much as possible for not used gas (I understand the issue with the block size, hence refunding half is still better than refunding nothing)
  • having stats about validators min gas prices (good point @alexanderbez ).

Instead, currently user has to think in terms of total fees (user has no idea what the total fee of a tx is) and might be afraid of setting higher gas limits. I consider this bad UX. Sometimes I need to manually increase gas limit for some transactions in Kepler to make them pass - that is super annoying.

@robert-zaremba
Copy link
Collaborator Author

I've updated the proposal, and suggest to adopt the solution proposed by @yihuang : evmos/ethermint#1085

@yihuang
Copy link
Collaborator

yihuang commented Sep 3, 2022

I've updated the proposal, and suggest to adopt the solution proposed by @yihuang : evmos/ethermint#1085

It's actually originally proposed by @ValarDragon, #2150

@alexanderbez
Copy link
Contributor

Yeah, we don't need to two issues. We can close this @robert-zaremba :)

@tac0turtle
Copy link
Member

Closing in favor of #2150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:needs architecture review To discuss on the next architecture review call to come to alignment
Projects
None yet
Development

No branches or pull requests

4 participants