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

Refactor: fee antehandler refactor #2351

Closed
5 tasks
yaruwangway opened this issue Apr 3, 2023 · 1 comment · Fixed by #2327
Closed
5 tasks

Refactor: fee antehandler refactor #2351

yaruwangway opened this issue Apr 3, 2023 · 1 comment · Fixed by #2327
Assignees

Comments

@yaruwangway
Copy link
Contributor

yaruwangway commented Apr 3, 2023

Summary

Gaia fee antehandler introduction

Gaia fee antehandler takes min_gas_price, Global fee into a combined fee requirement. The combined fee:

  1. takes the higher fees between min_gas_price and global fee if min_gas_price and global_fee have the same denom. Otherwise, take the global fee.
  2. consider the global fee denom as required denom even if global fee is zero coin.

For point 2, present gaia fee antehandler allows zero coins globalfee, for example, globalfee=[0stake] means the chain does not want to charge transaction fees, but if there is volunteer paid fee, it should be in the denom of stake.

Concerns

Globalfee is sdk.DecCoins type, but cosmos-sdk sdk.DecCoins are sanitized to remove zero coins in it, so several methods from sdk.DecCoins are over-written in gaia fee antehandler.
Even though present fee antehandler logic works, allowing zero coins in global fee makes the fee check logic in antehandler complicated and difficult to maintain.

Type

Code Debt
Design Debt

Impact

Simplify gaia fee antehandler, the present logic is complex and hard to maintain, need to maintain extra methods that overwritten sdk's methods.

### Potential state-breaking
For the hub, this refactor means users cannot propose zero coins in globalfee through gov proposal. If global fee is empty, the bond_denom will be used to check the paid fee denoms. This is potential state-breaking if in param store, globalfee params already contain zero coins (assume someone proposed zero coins as globalfee before this refactor). Please see below solutions, step4: upgrade handler, for how to deal with this scenarios.

Proposed Solution

Proposal:

## proposal 1:
Globalfee does not allow zero coins, this means globalfee=[0stake] is invalid setup.
- If the chain does not want to charge fees (above mentioned point 2), it can set globalfee=[], if globalfee empty, we can use bonding denom to check the denoms of the global fee.
- If the chain allows any other denoms as paid fee denom, it has to set a value in global fee, for example, globalfee=[0.0001stake]

This can simplify the logic, but disables one possibility:
bonding denom = uatom, but the chain want to set 0stake as globalfee (the chain does not charge tx fee, but tx still pays, it has to be stake (not a bonding denom of the chain.) )

So in summary:
after refactor, globalfee does not allow zero coins.
- if chain want to set globalfee=[0uatom] (uatom is bond-denom), it can set globalfee=[]
- if chain want to set globalfee=[0stake] (uatom is bond-denom), it has to setup as globalfee=[x stake], (x > 0)

Proposal 2:

globalfee can contain zero coins, in the fee check antehandler: split the combinedFeeRequirement (get a combined fee requirement from local fee(min-gas-prices in app.toml) and global fee) into zero and non-zero coins:
combinedFeeRequirement = nonZeroCoinFeesReq + zeroCoinFeesDenomReq

split the paidfee according to the nonZeroCoinFeesReq and zeroCoinFeesDenomReq denoms:
paidfee= feeCoinsNoZeroDenom + feeCoinsZeroDenom
since nonZeroCoinFeesReq does not contain zero coins, sdk DenomsSubsetOf and IsAnyGTE can be called.

example:
globalfee=[1photon, 0uatom, 1stake]
local min-gas-prices = [0.5stake]

get combinedFeeRequirement = [1photon, 0uatom, 1stake],
split the combinedFeeRequirement into [0uatom], and [1photon, 1stake]

paidFee =[1uatom, 0.5photon]
split the paidFee into [1uatom] (the same denom as zero coins in combinedFeeRequirement), and [0.5stake]

steps:

  1. check paidfee denoms
  2. if bypass-msg, tx pass
  3. if not bypass-msg, if paidfee contains feeCoinsZeroDenom, tx pass. feeCoinsZeroDenom means paidfees contain denoms from zero coins in combinedFeeRequirement.
  4. if not bypass-msg, if paidfee does not contain feeCoinsZeroDenom, check feeCoinsNoZeroDenom 's amount IsAnyGTE than nonZeroCoinFeesReq
  5. special case: when paidfee=[], and there is zero coins in combinedFeeRequirement, tx pass.

example:

globalfee=[0uatom,1stake,1photon], min-gas-prices=[] in app.toml.
combinedFeeRequirement=[0uatom,1stake,1photon].
nonZeroCoinFeesReq=[1stake,1photon], zeroCoinFeesReq=[0uatom].

  1. paidfee=[1uatom,3quark],
    fail in step 1.
  2. paidfee=[1uatom,2photon],
    pass denom checks, then split paid fee,feeCoinsZeroDenom=[1uatom], feeCoinsNoZeroDenom=[2photon],
    feeCoinsZeroDenom is not empty, direct pass tx.
  3. paidfee=[2photon]
    pass denom checks, then split paid fee,feeCoinsZeroDenom=[], feeCoinsNoZeroDenom=[2photon],
    feeCoinsZeroDenom empty, check feeCoinsNoZeroDenom IsAnyGTE than nonZeroCoinFeesReq, pass
  4. paidfee=[]
    pass denoms checks, then split paid fee,feeCoinsZeroDenom=[], feeCoinsNoZeroDenom=[], feeCoinsZeroDenom empty, if directly check feeCoinsNoZeroDenom IsAnyGTE than nonZeroCoinFeesReq, this tx will fail, so check if combinedFeeRequirement is also empty first before check IsAnyGTE
  5. paidfee=[0.5photon],
    pass denom checks, then split paid fee,feeCoinsZeroDenom=[], feeCoinsNoZeroDenom=[0.5photon],
    feeCoinsZeroDenom empty, check feeCoinsNoZeroDenom IsAnyGTE then nonZeroCoinFeesReq fail.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
  • Is a spike necessary to map out how the issue should be approached?
@yaruwangway yaruwangway self-assigned this Apr 3, 2023
@yaruwangway yaruwangway added this to the Gaia v10.0.0 milestone Apr 3, 2023
@yaruwangway yaruwangway changed the title fee antehandler refactor Refactor: fee antehandler refactor Apr 6, 2023
@mmulji-ic
Copy link
Contributor

Under proposal 2:

paidFee =[1uatom, 0.5photon]
split the paidFee into [1uatom] (the same denom as zero coins in combinedFeeRequirement), and [0.5stake]

The [0.5stake] should read [0.5photon]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants