Skip to content

Commit

Permalink
fix: nil check in post (backport #118) (#119)
Browse files Browse the repository at this point in the history 
* fix: nil check in post (#118)

* fix

* fix with check

* add early exit

* fix

(cherry picked from commit f0997fb)

# Conflicts:
#	x/feemarket/post/fee.go
#	x/feemarket/post/fee_test.go

* ok

* ok

* fix

---------

Co-authored-by: Alex Johnson <[email protected]>
  • Loading branch information
@mergify
mergify[bot] and Alex Johnson authored Jun 13, 2024
1 parent 8f88057 commit dbc959b
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 212 deletions.
2 changes: 1 addition & 1 deletion docs/INTEGRATIONS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ There are two ways to construct a transaction with `gasPrice`:

### Understanding Fee Deducted

The actual amount of fee deducted from the fee payer is based on gas consumed, not `gasLimit`.
The actual amount of fee deducted from the fee payer is based on gas consumed, not `gasLimit`. The total amount deducted (`fee + tip`) will be equal to the amount of fee specified on your transaction.

The amount consumed is equal to the `inferredTip + gasPrice * gasConsumed`, where `inferredTip = feeAmount - gasLimit * gasPrice` (This may be different than the tip you specified when building the transaction because the `gasPrice` on chain may have changed since when you queried it.)

Expand Down
10 changes: 0 additions & 10 deletions tests/e2e/e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,10 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth"
"github.com/cosmos/cosmos-sdk/x/bank"
"github.com/cosmos/cosmos-sdk/x/gov"
<<<<<<< HEAD
interchaintest "github.com/strangelove-ventures/interchaintest/v7"
"github.com/strangelove-ventures/interchaintest/v7/chain/cosmos"
"github.com/strangelove-ventures/interchaintest/v7/ibc"
ictestutil "github.com/strangelove-ventures/interchaintest/v7/testutil"
=======
interchaintest "github.com/strangelove-ventures/interchaintest/v8"
"github.com/strangelove-ventures/interchaintest/v8/chain/cosmos"
"github.com/strangelove-ventures/interchaintest/v8/ibc"
>>>>>>> daca8c8 (test: make extendable (#112))
"github.com/stretchr/testify/suite"

"github.com/skip-mev/feemarket/tests/e2e"
Expand Down Expand Up @@ -121,11 +115,7 @@ var (
func TestE2ETestSuite(t *testing.T) {
s := e2e.NewIntegrationSuite(
spec,
<<<<<<< HEAD
=======
oracleImage,
txCfg,
>>>>>>> daca8c8 (test: make extendable (#112))
)

suite.Run(t, s)
Expand Down
162 changes: 29 additions & 133 deletions tests/e2e/suite.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,83 +37,6 @@ func init() {
r = rand.New(s)
}

<<<<<<< HEAD
=======
func DefaultOracleSidecar(image ibc.DockerImage) ibc.SidecarConfig {
return ibc.SidecarConfig{
ProcessName: "oracle",
Image: image,
HomeDir: "/oracle",
Ports: []string{"8080", "8081"},
StartCmd: []string{
"slinky",
"--oracle-config", "/oracle/oracle.json",
},
ValidatorProcess: true,
PreStart: true,
}
}

func DefaultOracleConfig(url string) oracleconfig.OracleConfig {
cfg := marketmap.DefaultAPIConfig
cfg.Endpoints = []oracleconfig.Endpoint{
{
URL: url,
},
}

// Create the oracle config
oracleConfig := oracleconfig.OracleConfig{
UpdateInterval: 500 * time.Millisecond,
MaxPriceAge: 1 * time.Minute,
Host: "0.0.0.0",
Port: "8080",
Providers: map[string]oracleconfig.ProviderConfig{
marketmap.Name: {
Name: marketmap.Name,
API: cfg,
Type: "market_map_provider",
},
},
}

return oracleConfig
}

func DefaultMarketMap() mmtypes.MarketMap {
return mmtypes.MarketMap{}
}

func GetOracleSideCar(node *cosmos.ChainNode) *cosmos.SidecarProcess {
if len(node.Sidecars) == 0 {
panic("no sidecars found")
}
return node.Sidecars[0]
}

type TestTxConfig struct {
SmallSendsNum int
LargeSendsNum int
TargetIncreaseGasPrice math.LegacyDec
}

func (tx *TestTxConfig) Validate() error {
if tx.SmallSendsNum < 1 || tx.LargeSendsNum < 1 {
return fmt.Errorf("sends num should be greater than 1")
}

if tx.TargetIncreaseGasPrice.IsNil() {
return fmt.Errorf("target increase gas price is nil")
}

if tx.TargetIncreaseGasPrice.LTE(math.LegacyZeroDec()) {
return fmt.Errorf("target increase gas price is less than or equal to 0")
}

return nil
}

>>>>>>> daca8c8 (test: make extendable (#112))
// TestSuite runs the feemarket e2e test-suite against a given interchaintest specification
type TestSuite struct {
suite.Suite
Expand Down Expand Up @@ -191,30 +114,18 @@ func WithChainConstructor(cc ChainConstructor) Option {
}
}

<<<<<<< HEAD
func NewIntegrationSuite(spec *interchaintest.ChainSpec, opts ...Option) *TestSuite {
func NewIntegrationSuite(spec *interchaintest.ChainSpec, txCfg TestTxConfig, opts ...Option) *TestSuite {
if err := txCfg.Validate(); err != nil {
panic(err)
}

suite := &TestSuite{
spec: spec,
denom: defaultDenom,
authority: authtypes.NewModuleAddress(govtypes.ModuleName),
icc: DefaultInterchainConstructor,
cc: DefaultChainConstructor,
=======
func NewIntegrationSuite(spec *interchaintest.ChainSpec, oracleImage ibc.DockerImage, txCfg TestTxConfig, opts ...Option) *TestSuite {
if err := txCfg.Validate(); err != nil {
panic(err)
}

suite := &TestSuite{
spec: spec,
oracleConfig: DefaultOracleSidecar(oracleImage),
denom: defaultDenom,
gasPrices: "",
authority: authtypes.NewModuleAddress(govtypes.ModuleName),
icc: DefaultInterchainConstructor,
cc: DefaultChainConstructor,
txConfig: txCfg,
>>>>>>> daca8c8 (test: make extendable (#112))
txConfig: txCfg,
}

for _, opt := range opts {
Expand All @@ -224,6 +135,28 @@ func NewIntegrationSuite(spec *interchaintest.ChainSpec, oracleImage ibc.DockerI
return suite
}

type TestTxConfig struct {
SmallSendsNum int
LargeSendsNum int
TargetIncreaseGasPrice math.LegacyDec
}

func (tx *TestTxConfig) Validate() error {
if tx.SmallSendsNum < 1 || tx.LargeSendsNum < 1 {
return fmt.Errorf("sends num should be greater than 1")
}

if tx.TargetIncreaseGasPrice.IsNil() {
return fmt.Errorf("target increase gas price is nil")
}

if tx.TargetIncreaseGasPrice.LTE(math.LegacyZeroDec()) {
return fmt.Errorf("target increase gas price is less than or equal to 0")
}

return nil
}

func (s *TestSuite) WithKeyringOptions(cdc codec.Codec, opts keyring.Option) {
s.broadcasterOverrides = &KeyringOverride{
cdc: cdc,
Expand Down Expand Up @@ -353,13 +286,6 @@ func (s *TestSuite) TestSendTxDecrease() {

s.Run("expect fee market state to decrease", func() {
s.T().Log("performing sends...")
<<<<<<< HEAD
for {
// send with the exact expected fee
height, err := s.chain.(*cosmos.CosmosChain).Height(context.Background())
s.Require().NoError(err)
// send with the exact expected defaultGasPrice
=======
sig := make(chan struct{})
quit := make(chan struct{})
defer close(quit)
Expand All @@ -384,7 +310,6 @@ func (s *TestSuite) TestSendTxDecrease() {
break

case <-time.After(100 * time.Millisecond):
>>>>>>> daca8c8 (test: make extendable (#112))
wg := sync.WaitGroup{}
wg.Add(3)

Expand Down Expand Up @@ -436,17 +361,6 @@ func (s *TestSuite) TestSendTxDecrease() {
s.Require().Equal(uint32(0), txResp.DeliverTx.Code, txResp.DeliverTx)
}()
wg.Wait()
<<<<<<< HEAD
s.WaitForHeight(s.chain.(*cosmos.CosmosChain), height+1)

gasPrice := s.QueryDefaultGasPrice()
s.T().Log("base defaultGasPrice", gasPrice.String())

if gasPrice.Amount.Equal(params.MinBaseGasPrice) {
break
}
=======
>>>>>>> daca8c8 (test: make extendable (#112))
}

// wait for 5 blocks
Expand Down Expand Up @@ -475,13 +389,6 @@ func (s *TestSuite) TestSendTxIncrease() {
nodes := cosmosChain.Nodes()
s.Require().True(len(nodes) > 0)

<<<<<<< HEAD
baseGasPrice := s.QueryDefaultGasPrice()
gas := int64(20000100)
sendAmt := int64(100)

=======
>>>>>>> daca8c8 (test: make extendable (#112))
params := s.QueryParams()

gas := int64(params.MaxBlockUtilization)
Expand Down Expand Up @@ -519,7 +426,7 @@ func (s *TestSuite) TestSendTxIncrease() {
// add headroom
minBaseFeeCoins := sdk.NewCoins(sdk.NewCoin(minBaseFee.Denom, minBaseFee.Amount.Add(math.LegacyNewDec(10)).TruncateInt()))

height, err := s.chain.(*cosmos.CosmosChain).Height(context.Background())
_, err := s.chain.(*cosmos.CosmosChain).Height(context.Background())
s.Require().NoError(err)
wg := sync.WaitGroup{}
wg.Add(3)
Expand Down Expand Up @@ -572,17 +479,6 @@ func (s *TestSuite) TestSendTxIncrease() {
s.Require().Equal(uint32(0), txResp.DeliverTx.Code, txResp.DeliverTx)
}()
wg.Wait()
<<<<<<< HEAD
s.WaitForHeight(s.chain.(*cosmos.CosmosChain), height+1)

baseGasPrice = s.QueryDefaultGasPrice()
s.T().Log("gas price", baseGasPrice.String())

if baseGasPrice.Amount.GT(params.MinBaseGasPrice.Mul(math.LegacyNewDec(10))) {
break
}
=======
>>>>>>> daca8c8 (test: make extendable (#112))
}

// wait for 5 blocks
Expand Down
1 change: 0 additions & 1 deletion x/feemarket/ante/fee.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula
}

ctx = ctx.WithPriority(GetTxPriority(priorityFee, int64(gas), baseGasPrice))
return next(ctx, tx, simulate)
}
return next(ctx, tx, simulate)
}
Expand Down
34 changes: 14 additions & 20 deletions x/feemarket/post/fee.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"fmt"

errorsmod "cosmossdk.io/errors"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Expand Down Expand Up @@ -53,11 +53,6 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul
return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidGasLimit, "must provide positive gas")
}

var (
tip sdk.Coin
payCoin sdk.Coin
)

// update fee market params
params, err := dfd.feemarketKeeper.GetParams(ctx)
if err != nil {
Expand Down Expand Up @@ -88,6 +83,11 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul
feeCoin := feeCoins[0]
feeGas := int64(feeTx.GetGas())

var (
tip = sdk.NewCoin(feeCoin.Denom, math.ZeroInt())
payCoin = feeCoin
)

minGasPrice, err := dfd.feemarketKeeper.GetMinGasPrice(ctx, feeCoin.GetDenom())
if err != nil {
return ctx, errorsmod.Wrapf(err, "unable to get min gas price for denom %s", feeCoins[0].GetDenom())
Expand All @@ -106,7 +106,7 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul
}

ctx.Logger().Info("fee deduct post handle",
"fee", feeCoins,
"fee", payCoin,
"tip", tip,
)

Expand Down Expand Up @@ -159,9 +159,11 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
if dfd.feegrantKeeper == nil {
return sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled")
} else if !feeGranter.Equals(feePayer) {
err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, sdk.NewCoins(fee), sdkTx.GetMsgs())
if err != nil {
return errorsmod.Wrapf(err, "%s does not allow to pay fees for %s", feeGranter, feePayer)
if !fee.IsNil() {
err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, sdk.NewCoins(fee), sdkTx.GetMsgs())
if err != nil {
return errorsmod.Wrapf(err, "%s does not allow to pay fees for %s", feeGranter, feePayer)
}
}
}

Expand All @@ -176,7 +178,7 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
var events sdk.Events

// deduct the fees and tip
if !fee.Amount.IsNil() && !fee.IsZero() {
if !fee.IsNil() {
err := DeductCoins(dfd.bankKeeper, ctx, deductFeesFromAcc, sdk.NewCoins(fee), distributeFees)
if err != nil {
return err
Expand All @@ -190,7 +192,7 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
}

proposer := sdk.AccAddress(ctx.BlockHeader().ProposerAddress)
if !tip.Amount.IsNil() && !tip.IsZero() {
if !tip.IsNil() {
err := SendTip(dfd.bankKeeper, ctx, deductFeesFromAcc.GetAddress(), proposer, sdk.NewCoins(tip))
if err != nil {
return err
Expand All @@ -211,10 +213,6 @@ func (dfd FeeMarketDeductDecorator) DeductFeeAndTip(ctx sdk.Context, sdkTx sdk.T
// DeductCoins deducts coins from the given account.
// Coins can be sent to the default fee collector (causes coins to be distributed to stakers) or sent to the feemarket fee collector account (causes coins to be burned).
func DeductCoins(bankKeeper BankKeeper, ctx sdk.Context, acc authtypes.AccountI, coins sdk.Coins, distributeFees bool) error {
if !coins.IsValid() {
return errorsmod.Wrapf(sdkerrors.ErrInsufficientFee, "invalid coin amount: %s", coins)
}

targetModuleAcc := feemarkettypes.FeeCollectorName
if distributeFees {
targetModuleAcc = authtypes.FeeCollectorName
Expand All @@ -230,10 +228,6 @@ func DeductCoins(bankKeeper BankKeeper, ctx sdk.Context, acc authtypes.AccountI,

// SendTip sends a tip to the current block proposer.
func SendTip(bankKeeper BankKeeper, ctx sdk.Context, acc, proposer sdk.AccAddress, coins sdk.Coins) error {
if !coins.IsValid() {
return errorsmod.Wrapf(sdkerrors.ErrInsufficientFee, "invalid coin amount: %s", coins)
}

err := bankKeeper.SendCoins(ctx, acc, proposer, coins)
if err != nil {
return err
Expand Down
Loading

0 comments on commit dbc959b

Please sign in to comment.