feat(txfees): added charge fees method #76
Conversation
x/txfees/keeper/fees.go
Outdated
| // If the fee token is unknown, it is burned directly. | ||
| func (k Keeper) ChargeFees( |
There was a problem hiding this comment.
2 things:
- should be swapped before sending to beneficiary (i.e send it DYM)
- shouldnt be burned
x/txfees/keeper/fees.go
Outdated
| feetoken, err := k.GetFeeToken(ctx, takerFeeCoin.Denom) | ||
| if err != nil { | ||
| // This should never happen in practice | ||
| k.Logger(ctx).Error("Unknown fee token", "denom", takerFeeCoin.Denom, "error", err) |
There was a problem hiding this comment.
I blv this can happen. imagine the following:
- create rollapp without IRO
- no pool exists
- send ibc transfer an charge bridging fee
x/txfees/keeper/fees.go
Outdated
| k.Logger(ctx).Error("Unknown fee token", "denom", takerFeeCoin.Denom, "error", err) | ||
|
|
||
| // Burn unknown fee tokens | ||
| err = k.bankKeeper.BurnCoins(ctx, types.ModuleName, sdk.NewCoins(takerFeeCoin)) |
There was a problem hiding this comment.
we shouldn't burn unknown fee tokens but send them to the community pool.
expanded more about that on later comment.
x/txfees/keeper/fees.go
Outdated
| k.Logger(ctx).Error("Failed to swap fee token to base token. Trying to burn the tokens", "denom", takerFeeCoin.Denom, "error", err) | ||
|
|
||
| // Burn unknown fee tokens | ||
| err = k.bankKeeper.BurnCoins(ctx, types.ModuleName, sdk.NewCoins(takerFeeCoin)) |
There was a problem hiding this comment.
same here. shouldn't burn them but send them to the community pool.
| return nil | ||
| } | ||
| // Charge the fee from the payer to x/txfees | ||
| err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, payer, types.ModuleName, sdk.NewCoins(takerFeeCoin)) |
There was a problem hiding this comment.
It's not clear on the PRD but the intent was to send tokens after swapping to DYM.
since currently we only give the owner fees on:
- IRO (he gets dym directly cause taker fee is only in DYM)
- SWAP (we assume at max 1 hop)
than we should always have a way to swap it for the beneficiary.
| @@ -39,7 +38,7 @@ func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumb | |||
| return nil | |||
| } | |||
There was a problem hiding this comment.
do we need still need the epochs?
|
what about |
x/txfees/keeper/fees.go
Outdated
| return fmt.Errorf("swap fee to base denom: %w", err) | ||
| } | ||
| if baseDenomFee.IsNil() || baseDenomFee.IsZero() { | ||
| // Fee is unknown token, thus sent to the community pool |
There was a problem hiding this comment.
might be clearer to call FundCommunityPool here
x/txfees/keeper/fees.go
Outdated
| return err | ||
| }) | ||
| if err != nil { | ||
| k.Logger(ctx).Error("Failed to swap fee token to base token. Trying to burn the tokens", "denom", takerFeeCoin.Denom, "error", err) |
|
@omritoptix maybe we can move the txfees module to dymension repo on this opportunity? |
|
we discusses not to handle non-DYM gas fees for now |
|
@mtsitrin lets see if will have time for it. I'd rather not do it now. opened an issue: dymensionxyz/dymension#1388 |
Closes: dymensionxyz/dymension#1246
What is the purpose of the change
(E.g.: This pull request improves documation of area A by adding ....
Brief Changelog
(for example:)
Testing and Verifying
(Please pick one of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? (yes / no)x/<module>/spec/) / Osmosis docs repo / not documented)