Standard minimum fee implementation

[leobragaz]

Description

This PR introduces a custom AnteHandler that checks minimum standard fee at 0.01 daric.
Closes #230.

Checklist

• Targeted PR against correct branch.
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Wrote unit tests.
• Wrote integration tests (simulation & CLI).
• Updated the documentation.
• Added an entry to the CHANGELOG.md file.
• Re-reviewed Files changed in the Github PR explorer.

[RiccardoM]

The implementation seems good, but I think there is one key point that could be improved.

Right now the FeeDenom and fixedRequiredFee values are all hard coded. This makes it nearly impossible to change them without creating a new version of the application and then performing a hard fork of the chain.

Instead, what I think could be done is create a new x/fee module inside which we can create some Params holding the data we want; eg:

type Params struct { 
  MinFee sdk.Coin `json:"min_fee",yaml:"min_fee"`
}

Then, inside the newly created AnteDecorator what we could do is the following:

minFee := feekeper.GetMinFee(ctx)
if !stableRequiredQty.IsZero() && stableRequiredQty.GT(minFee) {
  ...
}

By doing so we could easily change the minimum fee amount or denom by simply using governance proposals, which is very nice to have.

[RiccardoM]

While taking a look at the current implementation, I came up with some improvements that could be applied in order to make this module more generic.

File organization

I would move the ante.go and ante_test.go files from x/ante to x/fees. This would make it easier to comprehend that it's tied to that module instead of being part of another module.

Improving the fee parameters

Currently, the fee parameter is formed as follows:

type Params struct { 
  MinFee sdk.Coin `json:"min_fee",yaml:"min_fee"`
}

While this might work in some cases, it surely does not for everyone. What if we want to have a minimum fee only for some specific messages? How can we achieve this?

In order to make the module more generic and usable by other projects as well, I think what we can do is change the parameters to be as follows:

// MinFee contains the minimum amount of coins that should be paid as a fee for each message of the specified type sent
type MinFee struct {
  MessageType string   `json:"message", yaml:"message"`
  Amount      sdk.Coin `json:"amount", yaml:"amount"`
}

type Params struct { 
  MinFees []MinFee `json:"min_fees", yaml:"min_fees"`
}

By doing so, we could then have the following parameters:

{
  "min_fees": [
    {
      "message": "desmos/MsgCreatePost", 
      "amount": {
        "amount": "10000",
        "denom": "udaric"
      }
    },
    {
      "message": "cosmos-sdk/MsgCreateValidator",
      "amount": {
        "amount": "2000",
        "denom": "stake"
      }
    }
  ]
}

Improving the minimum fee check

currently, the minimum fee check is all performed inside the checkMinimumFees method of the ante handler. Instead, what I think should be done, is move this check inside the x/fee Keeper:

// CheckFees checks whether the given fees are sufficient to pay for all the given messages. 
// The check is performed considering the minimum fee amounts specified inside the module parameters.
func (k Keeper) CheckFees(ctx sdk.Context, fees sdk.Coins, msgs []sdk.Msg) error

This method should:

1. Fetch the current parameters.
2. For each message type included inside the parameters, compute how many fees should be paid
3. Check whether the given fees are enough to pay for all the ones that should be paid

This would simplify a lot the working of the AnteHandle that could then result in

// ...

// Check the minimum fees
if err := mfd.feesKeeper.CheckFees(ctx, stdTx.Fee.Amount, stdTx.Msgs); err != nil {
	return ctx, err
}

// ...

[leobragaz]

@RiccardoM Finally fixed everything! 🎉
You can take a look now

[RiccardoM]

The implementation looks mostly OK. There are some things I recommend changing

[RiccardoM]

We should also validate the Amount field by using the IsValid method of sdk.Coins here probably

[leobragaz]

By default when creating a Coin or Coins type that check is performed automatically

[RiccardoM]

I think that check is performed only when creating the Coin or Coins using the NewCoin and NewCoins methods respectively. I don't think it's performed when the Coins are parsed from a JSON file. I think it might be better to add it for this reason anyway

[leobragaz]

@RiccardoM I had a look at the nil parameters problem and I found out two things:

1. The code take care of checking if they're nil or not before returning them, with a check that, if fails, will cause a panic error.
2. When creating the different keepers inside the app.go file, the ones which have some parameters, have a check inside the NewKeeper constructor which do the following:

if !paramSpace.HasKeyTable() {
         paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable())
}

where the function types.ParamKeyTable() is declared as follow:

// ParamKeyTable Key declaration for parameters
func ParamKeyTable() paramsModule.KeyTable {
	return paramsModule.NewKeyTable().RegisterParamSet(&Params{})
}

So basically we will never be in the case of having nil parameters.

[RiccardoM]

@bragaz You're right. I've also checked if there would be some cases in which the params could be set as empty (types.Params{}) but it appear to be impossible. If such value is passed to the SetParams method it would panic since the validity is always checked first.