refactor: use decimal as min_base_fee

[MSalopek]

This PR changes the feemarket implementation to use a decimal instead of an int for the min_base_fee field.

Not using a decimal could cause an inadvertant and unexpeced rise in the gas fee costs.

In some calculations I was not sure about the correctnes of the operation. Search for // Note: not sure about across the repo.

Feedback is appreciated.

[aljo242]

@MSalopek I'm not so sure that moving to use a decimal value is going to be a good idea here. Or at least this implementation of it.

In the event where the decimal value goes below 1, anytime the TruncateInt is called, the value is rounded down, meaning the resulting fee will be 0 right?

I don't thing there should ever be a case where the on-chain fee is 0. Is this behavior you are intending?

[aljo242]

There should at least be logic that ensures that the resulting fee is non-zero

[MSalopek]

@MSalopek I'm not so sure that moving to use a decimal value is going to be a good idea here. Or at least this implementation of it.

In the event where the decimal value goes below 1, anytime the TruncateInt is called, the value is rounded down, meaning the resulting fee will be 0 right?

I don't thing there should ever be a case where the on-chain fee is 0. Is this behavior you are intending?

I see your point, we can add changes that mitigate that issue.

We only truncate because sdk.Coin requires an integer value.

I was checking why sdk.Coins is used in GetMinPrices - it was not clear to me why that was chosen. It seems that we can return sdk.DecCoins in GetMinGasPrices to avoid truncation and change the logic to always round up to the nearest integer value.

Rounding up can be seen in the gaia x/globalfee.

The WithGasPrices context func also handles sdk.DecCoins and not sdk.Coins and there are some Coins -> DecCoins conversions already happening in the feemarket antehandler. Those can be refactored:

• https://github.com/cosmos/cosmos-sdk/blob/v0.47.10/types/context.go#L235
• https://github.com/skip-mev/feemarket/blob/main/x/feemarket/ante/fee.go#L65

That could solve the issue of inadvertant zero fee transactions.

The changes are outlined below (+ some changes in protos):

// FILE: x/feemarket/keeper/feemarket.go

func (k *Keeper) GetMinGasPrices(ctx sdk.Context) (sdk.DecCoins, error) {
	baseFee, err := k.GetBaseFee(ctx)
	if err != nil {
		return sdk.NewDecCoins(), err
	}

	params, err := k.GetParams(ctx)
	if err != nil {
		return sdk.NewDecCoins(), err
	}

	// NOTE: no truncation
	fee := sdk.NewDecCoinFromDec(params.FeeDenom, baseFee)
	minGasPrices := sdk.NewDecCoins(fee)

	return minGasPrices, nil
}

// FILE: x/feemarket/ante/fee.go

- func CheckTxFees(ctx sdk.Context, minFees sdk.Coins, feeTx sdk.FeeTx, isCheck bool) (feeCoins sdk.Coins, tip sdk.Coins, err error) {
- 	minFeesDecCoins := sdk.NewDecCoinsFromCoins(minFees...)
+ func CheckTxFees(ctx sdk.Context, minFeesDecCoins sdk.DecCoins, feeTx sdk.FeeTx, isCheck bool) (feeCoins sdk.Coins, tip sdk.Coins, err error) {
	feeCoins = feeTx.GetFee()

// FILE: x/feemarket/types/state.go
func (s *State) UpdateBaseFee(params Params) (fee math.LegacyDec) {
       ...

	// Update the base fee.
-	fee = s.BaseFee.Mul(learningRateAdjustment).Add(net).TruncateDec()
+	fee = s.BaseFee.Mul(learningRateAdjustment).Add(net)
       ...
}

What do you think?

[aljo242]

Approving - but there are some caveats here:

This code must be tested much further before it is brought into production. I would recommend expanding on the tests/e2e directory we have significantly before bringing to prod. Load tests as well