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

Consensus min gas fee of .0025 uosmo #4244

Merged
merged 25 commits into from
Feb 16, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
dfe43ca
Refactor tests a bit
ValarDragon Feb 7, 2023
38946b7
Add consensus min gas fee
ValarDragon Feb 7, 2023
c676d48
Fix tests
ValarDragon Feb 7, 2023
923b45e
Delete more crud
ValarDragon Feb 7, 2023
7a82e14
Cleanup more duplicate test cases
ValarDragon Feb 7, 2023
125d59d
flip isCheckTx sign
ValarDragon Feb 7, 2023
13679ce
One more test needed
ValarDragon Feb 7, 2023
295aebb
Run invalid fee denom test across both check & deliver
ValarDragon Feb 7, 2023
a374529
Remove extra line
ValarDragon Feb 7, 2023
8c7f27d
Merge branch 'main' into dev/consensus_min_gas_fee
ValarDragon Feb 7, 2023
e42e675
Add hack to get around txfee problem in ibctesting
ValarDragon Feb 8, 2023
f40b0c7
Handle genesis
ValarDragon Feb 8, 2023
b278a48
Minor code cleanup
ValarDragon Feb 8, 2023
5db078c
tryfix simulation
ValarDragon Feb 8, 2023
a8b5c4c
Fix for legacy simulator
ValarDragon Feb 8, 2023
275cfb8
Update x/txfees/keeper/feedecorator_test.go
ValarDragon Feb 8, 2023
e7db7e1
Apply matt comments
ValarDragon Feb 8, 2023
1896d2e
See what happens by adding fees
ValarDragon Feb 8, 2023
f3ffdcb
Merge branch 'main' into dev/consensus_min_gas_fee
ValarDragon Feb 13, 2023
bf1b8bc
Add genesis logic update
ValarDragon Feb 13, 2023
81ea3fd
Try fixing e2e
ValarDragon Feb 13, 2023
efe1326
Add some better logging to find error
ValarDragon Feb 13, 2023
540eaff
remove stat, fix typo
ValarDragon Feb 13, 2023
63c07fa
fix(e2e): fix e2e test for consensus min gas fee #4244 (#4317)
p0mvn Feb 16, 2023
af6f4a0
Temp disable geo twap E2E
ValarDragon Feb 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions x/txfees/keeper/feedecorator.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,22 +71,32 @@ func (mfd MempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
}
}

// If we are in CheckTx, this function is ran locally to determine if these fees are sufficient
// to enter our mempool.
// So we ensure that the provided fees meet a minimum threshold for the validator,
// converting every non-osmo specified asset into an osmo-equivalent amount, to determine sufficiency.
// Determine if these fees are sufficient for the tx to pass.
// if we are in block execution, we use the governance set parameter in
// https://www.mintscan.io/osmosis/proposals/354 . (.0025 Uosmo / gas)
// Once ABCI++ Process Proposal lands, we can have block validity conditions enforce this.
minBaseGasPrice := sdk.NewDecWithPrec(25, 4)

// If we are in CheckTx, a separate function is ran locally to ensure sufficient fees for entering our mempool.
// So we ensure that the provided fees meet a minimum threshold for the validator
if (ctx.IsCheckTx() || ctx.IsReCheckTx()) && !simulate {
minBaseGasPrice := mfd.GetMinBaseGasPriceForTx(ctx, baseDenom, feeTx)
if !(minBaseGasPrice.IsZero()) {
// You should only be able to pay with one fee token in a single tx
if len(feeCoins) != 1 {
return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "no fee attached")
}
err = mfd.TxFeesKeeper.IsSufficientFee(ctx, minBaseGasPrice, feeTx.GetGas(), feeCoins[0])
if err != nil {
return ctx, err
}
}
minBaseGasPrice = sdk.MaxDec(minBaseGasPrice, mfd.GetMinBaseGasPriceForTx(ctx, baseDenom, feeTx))
}

// If minBaseGasPrice is zero, then we don't need to check the fee. Continue
if minBaseGasPrice.IsZero() {
return next(ctx, tx, simulate)
}
// You should only be able to pay with one fee token in a single tx
if len(feeCoins) != 1 {
return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee,
"Expected 1 fee denom attached, got %d", len(feeCoins))
}
// The minimum base gas price is in uosmo, convert the fee denom's worth to uosmo terms.
// Then compare if its sufficient for paying the tx fee.
err = mfd.TxFeesKeeper.IsSufficientFee(ctx, minBaseGasPrice, feeTx.GetGas(), feeCoins[0])
if err != nil {
return ctx, err
}

return next(ctx, tx, simulate)
Expand Down
193 changes: 79 additions & 114 deletions x/txfees/keeper/feedecorator_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper_test

import (
"fmt"

clienttx "github.com/cosmos/cosmos-sdk/client/tx"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/simapp"
Expand All @@ -19,137 +21,92 @@ func (suite *KeeperTestSuite) TestFeeDecorator() {
mempoolFeeOpts := types.NewDefaultMempoolFeeOptions()
mempoolFeeOpts.MinGasPriceForHighGasTx = sdk.MustNewDecFromStr("0.0025")
baseDenom, _ := suite.App.TxFeesKeeper.GetBaseDenom(suite.Ctx)
baseGas := uint64(10000)
consensusMinFeeAmt := int64(25)

// uion is setup with a relative price of 1:1
uion := "uion"

uionPoolId := suite.PrepareBalancerPoolWithCoins(
sdk.NewInt64Coin(sdk.DefaultBondDenom, 500),
sdk.NewInt64Coin(uion, 500),
)
suite.ExecuteUpgradeFeeTokenProposal(uion, uionPoolId)

tests := []struct {
type testcase struct {
name string
txFee sdk.Coins
minGasPrices sdk.DecCoins
gasRequested uint64
minGasPrices sdk.DecCoins // if blank, set to 0
gasRequested uint64 // if blank, set to base gas
isCheckTx bool
expectPass bool
baseDenomGas bool
}{
{
name: "no min gas price - checktx",
txFee: sdk.NewCoins(),
minGasPrices: sdk.NewDecCoins(),
gasRequested: 10000,
isCheckTx: true,
expectPass: true,
baseDenomGas: true,
},
{
name: "no min gas price - delivertx",
txFee: sdk.NewCoins(),
minGasPrices: sdk.NewDecCoins(),
gasRequested: 10000,
isCheckTx: false,
expectPass: true,
baseDenomGas: true,
},
Comment on lines -40 to -57
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to be consensus min gas failure tests.

}

tests := []testcase{}
txType := []string{"delivertx", "checktx"}
succesType := []string{"does", "doesn't"}
for isCheckTx := 0; isCheckTx <= 1; isCheckTx++ {
mattverse marked this conversation as resolved.
Show resolved Hide resolved
tests = append(tests, []testcase{
{
name: fmt.Sprintf("no min gas price - %s. Fails w/ consensus minimum", txType[isCheckTx]),
txFee: sdk.NewCoins(),
isCheckTx: isCheckTx == 1,
expectPass: false,
},
{
name: fmt.Sprintf("Consensus min gas price - %s", txType[isCheckTx]),
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, consensusMinFeeAmt)),
isCheckTx: isCheckTx == 1,
expectPass: true,
},
{
name: fmt.Sprintf("multiple fee coins - %s", txType[isCheckTx]),
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1), sdk.NewInt64Coin(uion, 1)),
isCheckTx: isCheckTx == 1,
expectPass: false,
},
{
name: fmt.Sprintf("%s work with insufficient mempool fee in %s", succesType[isCheckTx], txType[isCheckTx]),
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, consensusMinFeeAmt)), // consensus minimum
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
isCheckTx: isCheckTx == 1,
expectPass: isCheckTx != 1,
},
{
name: fmt.Sprintf("%s work with insufficient converted mempool fee in %s", succesType[isCheckTx], txType[isCheckTx]),
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 25)), // consensus minimum
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
isCheckTx: isCheckTx == 1,
expectPass: isCheckTx != 1,
},
}...)
}

custTests := []testcase{
{
name: "works with valid basedenom fee",
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1000)),
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
gasRequested: 10000,
isCheckTx: true,
expectPass: true,
baseDenomGas: true,
},
{
name: "doesn't work with not enough fee in checktx",
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1)),
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
gasRequested: 10000,
isCheckTx: true,
expectPass: false,
baseDenomGas: true,
},
{
name: "works with not enough fee in delivertx",
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1)),
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
gasRequested: 10000,
isCheckTx: false,
expectPass: true,
baseDenomGas: true,
isCheckTx: true,
expectPass: true,
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deduplicated above

{
name: "works with valid converted fee",
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1000)),
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
gasRequested: 10000,
isCheckTx: true,
expectPass: true,
baseDenomGas: false,
isCheckTx: true,
expectPass: true,
},
{
name: "doesn't work with not enough converted fee in checktx",
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1)),
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
gasRequested: 10000,
isCheckTx: true,
expectPass: false,
baseDenomGas: false,
},
{
name: "works with not enough converted fee in delivertx",
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1)),
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom,
sdk.MustNewDecFromStr("0.1"))),
gasRequested: 10000,
isCheckTx: false,
expectPass: true,
baseDenomGas: false,
},
{
name: "multiple fee coins - checktx",
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1), sdk.NewInt64Coin(uion, 1)),
minGasPrices: sdk.NewDecCoins(),
gasRequested: 10000,
isCheckTx: true,
expectPass: false,
baseDenomGas: false,
},
{
name: "multiple fee coins - delivertx",
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1), sdk.NewInt64Coin(uion, 1)),
minGasPrices: sdk.NewDecCoins(),
gasRequested: 10000,
isCheckTx: false,
expectPass: false,
baseDenomGas: false,
},
Comment on lines -99 to -135
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to be deduplicated across check/deliver

{
name: "invalid fee denom",
txFee: sdk.NewCoins(sdk.NewInt64Coin("moo", 1)),
minGasPrices: sdk.NewDecCoins(),
gasRequested: 10000,
isCheckTx: false,
expectPass: false,
baseDenomGas: false,
name: "invalid fee denom",
txFee: sdk.NewCoins(sdk.NewInt64Coin("moooooo", 1000)),
isCheckTx: false,
expectPass: false,
},
{
name: "mingasprice not containing basedenom gets treated as min gas price 0",
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 100000000)),
minGasPrices: sdk.NewDecCoins(sdk.NewInt64DecCoin(uion, 1)),
gasRequested: 10000,
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1000)),
minGasPrices: sdk.NewDecCoins(sdk.NewInt64DecCoin(uion, 1000000)),
isCheckTx: true,
expectPass: true,
baseDenomGas: false,
},
{
name: "tx with gas wanted more than allowed should not pass",
Expand All @@ -158,7 +115,6 @@ func (suite *KeeperTestSuite) TestFeeDecorator() {
gasRequested: mempoolFeeOpts.MaxGasWantedPerTx + 1,
isCheckTx: true,
expectPass: false,
baseDenomGas: false,
},
{
name: "tx with high gas and not enough fee should no pass",
Expand All @@ -167,7 +123,6 @@ func (suite *KeeperTestSuite) TestFeeDecorator() {
gasRequested: mempoolFeeOpts.HighGasTxThreshold,
isCheckTx: true,
expectPass: false,
baseDenomGas: false,
},
{
name: "tx with high gas and enough fee should pass",
Expand All @@ -176,23 +131,31 @@ func (suite *KeeperTestSuite) TestFeeDecorator() {
gasRequested: mempoolFeeOpts.HighGasTxThreshold,
isCheckTx: true,
expectPass: true,
baseDenomGas: false,
},
}
tests = append(tests, custTests...)

for _, tc := range tests {
// reset pool and accounts for each test
suite.SetupTest(false)
suite.Run(tc.name, func() {
// setup uion with 1:1 fee
uionPoolId := suite.PrepareBalancerPoolWithCoins(
sdk.NewInt64Coin(sdk.DefaultBondDenom, 500),
sdk.NewInt64Coin(uion, 500),
)
suite.ExecuteUpgradeFeeTokenProposal(uion, uionPoolId)

if tc.minGasPrices == nil {
tc.minGasPrices = sdk.NewDecCoins()
}
if tc.gasRequested == 0 {
tc.gasRequested = baseGas
}
suite.Ctx = suite.Ctx.WithIsCheckTx(tc.isCheckTx).WithMinGasPrices(tc.minGasPrices)
suite.Ctx = suite.Ctx.WithMinGasPrices(tc.minGasPrices)

// TODO: Cleanup this code.
// TxBuilder components reset for every test case
txBuilder := suite.clientCtx.TxConfig.NewTxBuilder()
priv0, _, addr0 := testdata.KeyTestPubAddr()
Expand Down Expand Up @@ -226,11 +189,13 @@ func (suite *KeeperTestSuite) TestFeeDecorator() {
_, err := antehandlerMFD(suite.Ctx, tx, false)

if tc.expectPass {
if tc.baseDenomGas && !tc.txFee.IsZero() {
moduleAddr := suite.App.AccountKeeper.GetModuleAddress(types.FeeCollectorName)
suite.Require().Equal(tc.txFee[0], suite.App.BankKeeper.GetBalance(suite.Ctx, moduleAddr, baseDenom), tc.name)
} else if !tc.txFee.IsZero() {
moduleAddr := suite.App.AccountKeeper.GetModuleAddress(types.NonNativeFeeCollectorName)
// ensure fee was collected
if !tc.txFee.IsZero() {
moduleName := types.FeeCollectorName
if tc.txFee[0].Denom != baseDenom {
moduleName = types.NonNativeFeeCollectorName
}
Comment on lines +198 to +200
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base denom gas was an unnecessary field, its role is automated with this if statement.

moduleAddr := suite.App.AccountKeeper.GetModuleAddress(moduleName)
suite.Require().Equal(tc.txFee[0], suite.App.BankKeeper.GetBalance(suite.Ctx, moduleAddr, tc.txFee[0].Denom), tc.name)
}
suite.Require().NoError(err, "test: %s", tc.name)
Expand Down