From a74335d457a2b50380bf2e100af61fa3a0688366 Mon Sep 17 00:00:00 2001 From: freeelancer Date: Thu, 17 Oct 2024 15:00:52 +0800 Subject: [PATCH 1/2] added tests for stuck escrow fee --- tests/integration/integration_test.go | 2 +- testutils/keeper/keeper.go | 6 +- x/feemarket/ante/fee_test.go | 4 +- x/feemarket/ante/feegrant_test.go | 2 +- x/feemarket/ante/suite/suite.go | 11 ++- x/feemarket/keeper/keeper_test.go | 2 +- x/feemarket/post/fee_distribute_test.go | 121 ++++++++++++++++++++++++ x/feemarket/post/fee_test.go | 10 +- 8 files changed, 144 insertions(+), 14 deletions(-) create mode 100644 x/feemarket/post/fee_distribute_test.go diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index 16358c5..8eb84b4 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -37,7 +37,7 @@ func TestIntegrationTestSuite(t *testing.T) { func (s *IntegrationTestSuite) SetupTest() { s.encCfg = MakeTestEncodingConfig() - s.ctx, s.TestKeepers, s.TestMsgServers = testkeeper.NewTestSetup(s.T()) + s.ctx, s.TestKeepers, s.TestMsgServers = testkeeper.NewTestSetup(s.T(), false) } func (s *IntegrationTestSuite) TestState() { diff --git a/testutils/keeper/keeper.go b/testutils/keeper/keeper.go index 6f15b49..f1f4bdb 100644 --- a/testutils/keeper/keeper.go +++ b/testutils/keeper/keeper.go @@ -36,7 +36,7 @@ var additionalMaccPerms = map[string][]string{ } // NewTestSetup returns initialized instances of all the keepers and message servers of the modules -func NewTestSetup(t testing.TB, options ...testkeeper.SetupOption) (sdk.Context, TestKeepers, TestMsgServers) { +func NewTestSetup(t testing.TB, distributeFees bool, options ...testkeeper.SetupOption) (sdk.Context, TestKeepers, TestMsgServers) { options = append(options, testkeeper.WithAdditionalModuleAccounts(additionalMaccPerms)) _, tk, tms := testkeeper.NewTestSetup(t, options...) @@ -55,7 +55,9 @@ func NewTestSetup(t testing.TB, options ...testkeeper.SetupOption) (sdk.Context, err := feeMarketKeeper.SetState(ctx, feemarkettypes.DefaultState()) require.NoError(t, err) - err = feeMarketKeeper.SetParams(ctx, feemarkettypes.DefaultParams()) + params := feemarkettypes.DefaultParams() + params.DistributeFees = distributeFees + err = feeMarketKeeper.SetParams(ctx, params) require.NoError(t, err) testKeepers := TestKeepers{ diff --git a/x/feemarket/ante/fee_test.go b/x/feemarket/ante/fee_test.go index b90971a..4f6224b 100644 --- a/x/feemarket/ante/fee_test.go +++ b/x/feemarket/ante/fee_test.go @@ -197,7 +197,7 @@ func TestAnteHandleMock(t *testing.T) { for _, tc := range testCases { t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, tc.Mock) + s := antesuite.SetupTestSuite(t, tc.Mock, false) s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder() args := tc.Malleate(s) @@ -418,7 +418,7 @@ func TestAnteHandle(t *testing.T) { for _, tc := range testCases { t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, tc.Mock) + s := antesuite.SetupTestSuite(t, tc.Mock, false) s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder() args := tc.Malleate(s) diff --git a/x/feemarket/ante/feegrant_test.go b/x/feemarket/ante/feegrant_test.go index 6456416..7106ccf 100644 --- a/x/feemarket/ante/feegrant_test.go +++ b/x/feemarket/ante/feegrant_test.go @@ -126,7 +126,7 @@ func TestEscrowFunds(t *testing.T) { for name, stc := range cases { tc := stc // to make scopelint happy t.Run(name, func(t *testing.T) { - s := antesuite.SetupTestSuite(t, true) + s := antesuite.SetupTestSuite(t, true, false) protoTxCfg := tx.NewTxConfig(codec.NewProtoCodec(s.EncCfg.InterfaceRegistry), tx.DefaultSignModes) // this just tests our handler dfd := feemarketante.NewFeeMarketCheckDecorator(s.AccountKeeper, s.MockBankKeeper, s.MockFeeGrantKeeper, diff --git a/x/feemarket/ante/suite/suite.go b/x/feemarket/ante/suite/suite.go index 5113539..0a77ff1 100644 --- a/x/feemarket/ante/suite/suite.go +++ b/x/feemarket/ante/suite/suite.go @@ -3,6 +3,7 @@ package suite import ( "testing" + "cosmossdk.io/math" storetypes "cosmossdk.io/store/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" @@ -106,11 +107,11 @@ func (s *TestSuite) SetAccountBalances(accounts []TestAccountBalance) { } // SetupTestSuite setups a new test, with new app, context, and anteHandler. -func SetupTestSuite(t *testing.T, mock bool) *TestSuite { +func SetupTestSuite(t *testing.T, mock, distributeFees bool) *TestSuite { s := &TestSuite{} s.EncCfg = MakeTestEncodingConfig() - ctx, testKeepers, _ := testkeeper.NewTestSetup(t) + ctx, testKeepers, _ := testkeeper.NewTestSetup(t, distributeFees) s.Ctx = ctx s.AccountKeeper = testKeepers.AccountKeeper @@ -188,6 +189,7 @@ type TestCase struct { ExpErr error ExpectConsumedGas uint64 Mock bool + DistributeFees bool } type TestCaseArgs struct { @@ -244,6 +246,11 @@ func (s *TestSuite) RunTestCase(t *testing.T, tc TestCase, args TestCaseArgs) { newCtx, postErr = s.PostHandler(s.Ctx, tx, tc.Simulate, true) } + if tc.DistributeFees && !tc.Simulate && args.FeeAmount != nil { + postFeeBalance := s.BankKeeper.GetBalance(s.Ctx, s.AccountKeeper.GetModuleAddress(feemarkettypes.FeeCollectorName), args.FeeAmount.GetDenomByIndex(0)) + require.Equal(t, postFeeBalance.Amount, math.ZeroInt()) + } + if tc.ExpPass { require.NoError(t, txErr) require.NoError(t, anteErr) diff --git a/x/feemarket/keeper/keeper_test.go b/x/feemarket/keeper/keeper_test.go index e9b9b5e..dbfdacd 100644 --- a/x/feemarket/keeper/keeper_test.go +++ b/x/feemarket/keeper/keeper_test.go @@ -48,7 +48,7 @@ func (s *KeeperTestSuite) SetupTest() { s.encCfg = MakeTestEncodingConfig() s.authorityAccount = authtypes.NewModuleAddress(govtypes.ModuleName) s.accountKeeper = mocks.NewAccountKeeper(s.T()) - ctx, tk, tm := testkeeper.NewTestSetup(s.T()) + ctx, tk, tm := testkeeper.NewTestSetup(s.T(), false) s.ctx = ctx s.feeMarketKeeper = tk.FeeMarketKeeper diff --git a/x/feemarket/post/fee_distribute_test.go b/x/feemarket/post/fee_distribute_test.go new file mode 100644 index 0000000..206daa4 --- /dev/null +++ b/x/feemarket/post/fee_distribute_test.go @@ -0,0 +1,121 @@ +package post_test + +import ( + "fmt" + "testing" + + "cosmossdk.io/math" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/mock" + + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + antesuite "github.com/skip-mev/feemarket/x/feemarket/ante/suite" + "github.com/skip-mev/feemarket/x/feemarket/post" + "github.com/skip-mev/feemarket/x/feemarket/types" +) + +func TestPostHandleDistributeFeesMock(t *testing.T) { + // Same data for every test case + const ( + baseDenom = "stake" + resolvableDenom = "atom" + expectedConsumedGas = 11730 + expectedConsumedSimGas = expectedConsumedGas + post.BankSendGasConsumption + gasLimit = expectedConsumedSimGas + ) + + validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit)) + validFeeAmountWithTip := validFeeAmount.Add(math.LegacyNewDec(100)) + validFeeWithTip := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmountWithTip.TruncateInt())) + + testCases := []antesuite.TestCase{ + { + Name: "signer has enough funds, should pass with tip", + Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs { + accs := s.CreateTestAccounts(1) + s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), + types.FeeCollectorName, mock.Anything).Return(nil).Once() + s.MockBankKeeper.On("SendCoinsFromModuleToModule", mock.Anything, types.FeeCollectorName, authtypes.FeeCollectorName, mock.Anything).Return(nil).Once() + s.MockBankKeeper.On("SendCoinsFromModuleToAccount", mock.Anything, types.FeeCollectorName, mock.Anything, mock.Anything).Return(nil).Once() + return antesuite.TestCaseArgs{ + Msgs: []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())}, + GasLimit: gasLimit, + FeeAmount: validFeeWithTip, + } + }, + RunAnte: true, + RunPost: true, + Simulate: false, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, + Mock: true, + DistributeFees: true, + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) { + s := antesuite.SetupTestSuite(t, tc.Mock, true) + s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder() + args := tc.Malleate(s) + + s.RunTestCase(t, tc, args) + }) + } +} + +func TestPostHandleDistributeFees(t *testing.T) { + // Same data for every test case + const ( + baseDenom = "stake" + resolvableDenom = "atom" + expectedConsumedGas = 54500 + + gasLimit = 100000 + ) + + validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit)) + validFeeAmountWithTip := validFeeAmount.Add(math.LegacyNewDec(100)) + validFeeWithTip := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmountWithTip.TruncateInt())) + + testCases := []antesuite.TestCase{ + { + Name: "signer has enough funds, gaslimit is not enough to complete entire transaction, should pass", + Malleate: func(s *antesuite.TestSuite) antesuite.TestCaseArgs { + accs := s.CreateTestAccounts(1) + + balance := antesuite.TestAccountBalance{ + TestAccount: accs[0], + Coins: validFeeWithTip, + } + s.SetAccountBalances([]antesuite.TestAccountBalance{balance}) + + return antesuite.TestCaseArgs{ + Msgs: []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())}, + GasLimit: 35188, + FeeAmount: validFeeWithTip, + } + }, + RunAnte: true, + RunPost: true, + Simulate: false, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: 65558, + Mock: false, + DistributeFees: true, + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) { + s := antesuite.SetupTestSuite(t, tc.Mock, true) + s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder() + args := tc.Malleate(s) + + s.RunTestCase(t, tc, args) + }) + } +} diff --git a/x/feemarket/post/fee_test.go b/x/feemarket/post/fee_test.go index af67585..e682e55 100644 --- a/x/feemarket/post/fee_test.go +++ b/x/feemarket/post/fee_test.go @@ -63,7 +63,7 @@ func TestDeductCoins(t *testing.T) { } for _, tc := range tests { t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, true) + s := antesuite.SetupTestSuite(t, true, tc.distributeFees) if tc.distributeFees { s.MockBankKeeper.On("SendCoinsFromModuleToModule", s.Ctx, types.FeeCollectorName, authtypes.FeeCollectorName, @@ -101,7 +101,7 @@ func TestDeductCoinsAndDistribute(t *testing.T) { } for _, tc := range tests { t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, true) + s := antesuite.SetupTestSuite(t, true, false) s.MockBankKeeper.On("SendCoinsFromModuleToModule", s.Ctx, types.FeeCollectorName, authtypes.FeeCollectorName, tc.coins).Return(nil).Once() @@ -136,7 +136,7 @@ func TestSendTip(t *testing.T) { } for _, tc := range tests { t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, true) + s := antesuite.SetupTestSuite(t, true, false) accs := s.CreateTestAccounts(2) s.MockBankKeeper.On("SendCoinsFromModuleToAccount", s.Ctx, types.FeeCollectorName, mock.Anything, tc.coins).Return(nil).Once() @@ -525,7 +525,7 @@ func TestPostHandleMock(t *testing.T) { for _, tc := range testCases { t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, tc.Mock) + s := antesuite.SetupTestSuite(t, tc.Mock, false) s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder() args := tc.Malleate(s) @@ -983,7 +983,7 @@ func TestPostHandle(t *testing.T) { for _, tc := range testCases { t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) { - s := antesuite.SetupTestSuite(t, tc.Mock) + s := antesuite.SetupTestSuite(t, tc.Mock, false) s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder() args := tc.Malleate(s) From 9b9134e7811fb01da13af8f0c23db85699c5a3e8 Mon Sep 17 00:00:00 2001 From: freeelancer Date: Thu, 17 Oct 2024 15:17:50 +0800 Subject: [PATCH 2/2] fix to distribute stuck escrowed fees --- x/feemarket/post/expected_keeper.go | 1 + x/feemarket/post/fee.go | 38 ++++++++++++++++++++--------- x/feemarket/types/keys.go | 11 +++++---- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/x/feemarket/post/expected_keeper.go b/x/feemarket/post/expected_keeper.go index a759656..f71ca3c 100644 --- a/x/feemarket/post/expected_keeper.go +++ b/x/feemarket/post/expected_keeper.go @@ -31,6 +31,7 @@ type BankKeeper interface { SendCoinsFromAccountToModule(ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error SendCoinsFromModuleToModule(ctx context.Context, senderModule, recipientModule string, amt sdk.Coins) error SendCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error + GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins } // FeeMarketKeeper defines the expected feemarket keeper. diff --git a/x/feemarket/post/fee.go b/x/feemarket/post/fee.go index 108af3e..c80e19f 100644 --- a/x/feemarket/post/fee.go +++ b/x/feemarket/post/fee.go @@ -159,18 +159,8 @@ func (dfd FeeMarketDeductDecorator) PayOutFeeAndTip(ctx sdk.Context, fee, tip sd var events sdk.Events // deduct the fees and tip - if !fee.IsNil() { - err := DeductCoins(dfd.bankKeeper, ctx, sdk.NewCoins(fee), params.DistributeFees) - if err != nil { - return err - } - - events = append(events, sdk.NewEvent( - feemarkettypes.EventTypeFeePay, - sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), - )) - } - + // fees from previous transactions may be stuck in escrow if transaction is out of gas + // distribute the stuck escrow fees based on the distribute fees parameter proposer := sdk.AccAddress(ctx.BlockHeader().ProposerAddress) if !tip.IsNil() { err := SendTip(dfd.bankKeeper, ctx, proposer, sdk.NewCoins(tip)) @@ -185,6 +175,30 @@ func (dfd FeeMarketDeductDecorator) PayOutFeeAndTip(ctx sdk.Context, fee, tip sd )) } + if params.DistributeFees { + feemarketCollector := dfd.accountKeeper.GetModuleAccount(ctx, feemarkettypes.FeeCollectorName) + feesToDistribute := dfd.bankKeeper.GetAllBalances(ctx, feemarketCollector.GetAddress()) + if !feesToDistribute.IsZero() { + err := DeductCoins(dfd.bankKeeper, ctx, feesToDistribute, params.DistributeFees) + if err != nil { + return err + } + if feesToDistribute.AmountOf(fee.Denom).GT(fee.Amount) { + events = append(events, sdk.NewEvent( + feemarkettypes.EventTypeDistributeStuckEscrowFees, + sdk.NewAttribute(sdk.AttributeKeyFee, feesToDistribute.Sub(fee).String()), + )) + } + } + } + + if !fee.IsNil() { + events = append(events, sdk.NewEvent( + feemarkettypes.EventTypeFeePay, + sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), + )) + } + ctx.EventManager().EmitEvents(events) return nil } diff --git a/x/feemarket/types/keys.go b/x/feemarket/types/keys.go index 8bbd54c..62d9387 100644 --- a/x/feemarket/types/keys.go +++ b/x/feemarket/types/keys.go @@ -26,9 +26,10 @@ var ( // KeyEnabledHeight is the store key for the feemarket module's enabled height. KeyEnabledHeight = []byte{prefixEnableHeight} - EventTypeFeePay = "fee_pay" - EventTypeTipPay = "tip_pay" - AttributeKeyTip = "tip" - AttributeKeyTipPayer = "tip_payer" - AttributeKeyTipPayee = "tip_payee" + EventTypeFeePay = "fee_pay" + EventTypeTipPay = "tip_pay" + EventTypeDistributeStuckEscrowFees = "distribute_stuck_escrow_fees" + AttributeKeyTip = "tip" + AttributeKeyTipPayer = "tip_payer" + AttributeKeyTipPayee = "tip_payee" )