From c90a7fa76ab3ad271bf93a241991c0cc525790ef Mon Sep 17 00:00:00 2001 From: freeelancer Date: Wed, 16 Oct 2024 17:28:30 +0800 Subject: [PATCH 1/3] burn stucked escrow fees --- app/app.go | 1 + app/post_handler.go | 6 +++ x/xfeemarket/post/expected_keepers.go | 10 ++++ x/xfeemarket/post/fee.go | 43 ++++++++++------ .../post/mocks/mock_account_keeper.go | 50 +++++++++++++++++++ x/xfeemarket/post/suite/suite.go | 2 + x/xfeemarket/types/keys.go | 3 +- 7 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 x/xfeemarket/post/mocks/mock_account_keeper.go diff --git a/app/app.go b/app/app.go index 9feecfcc..a29bae8a 100644 --- a/app/app.go +++ b/app/app.go @@ -1036,6 +1036,7 @@ func (app *App) setAnteHandler(txConfig client.TxConfig, wasmConfig wasmtypes.Wa func (app *App) setPostHandler() { postHandler := PostHandlerOptions{ + AccountKeeper: app.AccountKeeper, BankKeeper: app.BankKeeper, FeeMarketKeeper: app.FeeMarketKeeper, } diff --git a/app/post_handler.go b/app/post_handler.go index a26a319d..366e4d9b 100644 --- a/app/post_handler.go +++ b/app/post_handler.go @@ -9,12 +9,17 @@ import ( // PostHandlerOptions are the options required for constructing a FeeMarket PostHandler. type PostHandlerOptions struct { + AccountKeeper xfeemarketpost.AccountKeeper BankKeeper xfeemarketpost.BankKeeper FeeMarketKeeper xfeemarketpost.FeeMarketKeeper } // NewPostHandler returns a PostHandler chain with the fee deduct decorator. func NewPostHandler(options PostHandlerOptions) (sdk.PostHandler, error) { + if options.AccountKeeper == nil { + return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "account keeper is required for post builder") + } + if options.BankKeeper == nil { return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "bank keeper is required for post builder") } @@ -25,6 +30,7 @@ func NewPostHandler(options PostHandlerOptions) (sdk.PostHandler, error) { postDecorators := []sdk.PostDecorator{ xfeemarketpost.NewFeeMarketDeductDecorator( + options.AccountKeeper, options.BankKeeper, options.FeeMarketKeeper, ), diff --git a/x/xfeemarket/post/expected_keepers.go b/x/xfeemarket/post/expected_keepers.go index a0a50e74..40a6ffef 100644 --- a/x/xfeemarket/post/expected_keepers.go +++ b/x/xfeemarket/post/expected_keepers.go @@ -1,11 +1,21 @@ package post import ( + "context" + sdk "github.com/cosmos/cosmos-sdk/types" bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" feemarkettypes "github.com/skip-mev/feemarket/x/feemarket/types" ) +// AccountKeeper defines the contract needed for AccountKeeper related APIs. +// Interface provides support to use non-sdk AccountKeeper for AnteHandler's decorators. +// +//go:generate mockery --name AccountKeeper --filename mock_account_keeper.go +type AccountKeeper interface { + GetModuleAccount(ctx context.Context, name string) sdk.ModuleAccountI +} + // BankKeeper defines the contract needed for supply related APIs. // //go:generate mockery --name BankKeeper --filename mock_bank_keeper.go diff --git a/x/xfeemarket/post/fee.go b/x/xfeemarket/post/fee.go index bbce9299..1cfc9728 100644 --- a/x/xfeemarket/post/fee.go +++ b/x/xfeemarket/post/fee.go @@ -21,12 +21,14 @@ const BankSendGasConsumption = 12490 + 37325 // Call next PostHandler if fees successfully deducted. // CONTRACT: Tx must implement FeeTx interface type FeeMarketDeductDecorator struct { + accountKeeper AccountKeeper bankKeeper BankKeeper feemarketKeeper FeeMarketKeeper } -func NewFeeMarketDeductDecorator(bk BankKeeper, fmk FeeMarketKeeper) FeeMarketDeductDecorator { +func NewFeeMarketDeductDecorator(ak AccountKeeper, bk BankKeeper, fmk FeeMarketKeeper) FeeMarketDeductDecorator { return FeeMarketDeductDecorator{ + accountKeeper: ak, bankKeeper: bk, feemarketKeeper: fmk, } @@ -148,19 +150,6 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul func (dfd FeeMarketDeductDecorator) BurnFeeAndRefund(ctx sdk.Context, fee, tip sdk.Coin, feePayer sdk.AccAddress, defaultFeeDenom string) error { var events sdk.Events - // burn the fees if it is the default fee denom - if !fee.IsNil() && !fee.IsZero() && fee.Denom == defaultFeeDenom { - err := BurnCoins(dfd.bankKeeper, ctx, sdk.NewCoins(fee)) - if err != nil { - return err - } - - events = append(events, sdk.NewEvent( - feemarkettypes.EventTypeFeePay, - sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), - )) - } - // refund the tip if it is not nil and non zero if !tip.IsNil() && !tip.IsZero() { err := RefundTip(dfd.bankKeeper, ctx, feePayer, sdk.NewCoins(tip)) @@ -175,6 +164,32 @@ func (dfd FeeMarketDeductDecorator) BurnFeeAndRefund(ctx sdk.Context, fee, tip s )) } + // fees from previous transactions may be stuck in escrow if transaction is out of gas + // burn both the fee and the stuck escrowed fees + feemarketCollector := dfd.accountKeeper.GetModuleAccount(ctx, feemarkettypes.FeeCollectorName) + burnCoin := dfd.bankKeeper.GetBalance(ctx, feemarketCollector.GetAddress(), defaultFeeDenom) + stuckEscrowedFee := burnCoin.Sub(fee) + if burnCoin.IsPositive() { + err := BurnCoins(dfd.bankKeeper, ctx, sdk.NewCoins(burnCoin)) + if err != nil { + return err + } + + if !fee.IsNil() && !fee.IsZero() && fee.Denom == defaultFeeDenom { + events = append(events, sdk.NewEvent( + feemarkettypes.EventTypeFeePay, + sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), + )) + } + + if stuckEscrowedFee.IsPositive() { + events = append(events, sdk.NewEvent( + xfeemarkettypes.EventTypeBurnStuckEscrowedFee, + sdk.NewAttribute(sdk.AttributeKeyFee, stuckEscrowedFee.String()), + )) + } + } + ctx.EventManager().EmitEvents(events) return nil } diff --git a/x/xfeemarket/post/mocks/mock_account_keeper.go b/x/xfeemarket/post/mocks/mock_account_keeper.go new file mode 100644 index 00000000..31d4dbbd --- /dev/null +++ b/x/xfeemarket/post/mocks/mock_account_keeper.go @@ -0,0 +1,50 @@ +// Code generated by mockery v2.45.1. DO NOT EDIT. + +package mocks + +import ( + context "context" + + mock "github.com/stretchr/testify/mock" + + types "github.com/cosmos/cosmos-sdk/types" +) + +// AccountKeeper is an autogenerated mock type for the AccountKeeper type +type AccountKeeper struct { + mock.Mock +} + +// GetModuleAccount provides a mock function with given fields: ctx, name +func (_m *AccountKeeper) GetModuleAccount(ctx context.Context, name string) types.ModuleAccountI { + ret := _m.Called(ctx, name) + + if len(ret) == 0 { + panic("no return value specified for GetModuleAccount") + } + + var r0 types.ModuleAccountI + if rf, ok := ret.Get(0).(func(context.Context, string) types.ModuleAccountI); ok { + r0 = rf(ctx, name) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.ModuleAccountI) + } + } + + return r0 +} + +// NewAccountKeeper creates a new instance of AccountKeeper. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewAccountKeeper(t interface { + mock.TestingT + Cleanup(func()) +}) *AccountKeeper { + mock := &AccountKeeper{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/x/xfeemarket/post/suite/suite.go b/x/xfeemarket/post/suite/suite.go index 628bdcd2..56b2c7e0 100644 --- a/x/xfeemarket/post/suite/suite.go +++ b/x/xfeemarket/post/suite/suite.go @@ -134,6 +134,7 @@ func SetupTestSuite(t *testing.T, mock bool) *TestSuite { } func (s *TestSuite) SetupHandlers(mock bool) { + accountkeeper := s.AccountKeeper bankKeeper := s.BankKeeper feeGrantKeeper := s.FeeGrantKeeper @@ -165,6 +166,7 @@ func (s *TestSuite) SetupHandlers(mock bool) { // create basic postHandler with the feemarket decorator postDecorators := []sdk.PostDecorator{ feemarketpost.NewFeeMarketDeductDecorator( + accountkeeper, bankKeeper, s.FeeMarketKeeper, ), diff --git a/x/xfeemarket/types/keys.go b/x/xfeemarket/types/keys.go index 3c8129c3..f2156bfa 100644 --- a/x/xfeemarket/types/keys.go +++ b/x/xfeemarket/types/keys.go @@ -17,7 +17,8 @@ var ( ParamsKey = collections.NewPrefix("p_xfeemarket") PrefixDenomMultiplier = []byte{0x01} - EventTypeTipRefund = "tip_refund" + EventTypeTipRefund = "tip_refund" + EventTypeBurnStuckEscrowedFee = "burn_stuck_escrowed_fee" ) func KeyPrefix(p string) []byte { From 9979d8d74659d6b45a6c7b033f9746cfee0ce07f Mon Sep 17 00:00:00 2001 From: freeelancer Date: Wed, 16 Oct 2024 19:29:39 +0800 Subject: [PATCH 2/3] fix tests --- x/xfeemarket/post/fee.go | 5 ++++- x/xfeemarket/post/fee_test.go | 14 +++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/x/xfeemarket/post/fee.go b/x/xfeemarket/post/fee.go index 1cfc9728..836328ea 100644 --- a/x/xfeemarket/post/fee.go +++ b/x/xfeemarket/post/fee.go @@ -168,7 +168,10 @@ func (dfd FeeMarketDeductDecorator) BurnFeeAndRefund(ctx sdk.Context, fee, tip s // burn both the fee and the stuck escrowed fees feemarketCollector := dfd.accountKeeper.GetModuleAccount(ctx, feemarkettypes.FeeCollectorName) burnCoin := dfd.bankKeeper.GetBalance(ctx, feemarketCollector.GetAddress(), defaultFeeDenom) - stuckEscrowedFee := burnCoin.Sub(fee) + stuckEscrowedFee := sdk.NewCoin(defaultFeeDenom, math.ZeroInt()) + if fee.Denom == defaultFeeDenom && burnCoin.Amount.GT(fee.Amount) { + stuckEscrowedFee = burnCoin.Sub(fee) + } if burnCoin.IsPositive() { err := BurnCoins(dfd.bankKeeper, ctx, sdk.NewCoins(burnCoin)) if err != nil { diff --git a/x/xfeemarket/post/fee_test.go b/x/xfeemarket/post/fee_test.go index f0497057..dcb3e976 100644 --- a/x/xfeemarket/post/fee_test.go +++ b/x/xfeemarket/post/fee_test.go @@ -134,11 +134,12 @@ func TestPostHandleMock(t *testing.T) { const ( baseDenom = "stake" resolvableDenom = "atom" - expectedConsumedGas = 9307 + expectedConsumedGas = 25250 expectedConsumedSimGas = expectedConsumedGas + post.BankSendGasConsumption gasLimit = expectedConsumedSimGas // 152800 12490 + 36385 ) + zeroInt := math.ZeroInt() validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit)) validFee := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmount.TruncateInt())) validResolvableFee := sdk.NewCoins(sdk.NewCoin(resolvableDenom, validFeeAmount.TruncateInt())) @@ -208,6 +209,7 @@ func TestPostHandleMock(t *testing.T) { accs := s.CreateTestAccounts(1) s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil).Once() + s.MockBankKeeper.On("GetBalance", mock.Anything, mock.Anything, mock.Anything).Return(sdk.NewCoin(baseDenom, zeroInt)).Once() return postsuite.TestCaseArgs{ Msgs: []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())}, @@ -229,7 +231,7 @@ func TestPostHandleMock(t *testing.T) { accs := s.CreateTestAccounts(1) s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil) - s.MockBankKeeper.On("BurnCoins", mock.Anything, types.FeeCollectorName, mock.Anything).Return(nil).Once() + s.MockBankKeeper.On("GetBalance", mock.Anything, mock.Anything, mock.Anything).Return(sdk.NewCoin(baseDenom, zeroInt)).Once() s.MockBankKeeper.On("SendCoinsFromModuleToAccount", mock.Anything, types.FeeCollectorName, accs[0].Account.GetAddress(), mock.Anything).Return(nil).Once() return postsuite.TestCaseArgs{ @@ -296,6 +298,7 @@ func TestPostHandleMock(t *testing.T) { accs := s.CreateTestAccounts(1) s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil).Once() + s.MockBankKeeper.On("GetBalance", mock.Anything, mock.Anything, mock.Anything).Return(sdk.NewCoin(baseDenom, zeroInt)).Once() s.MockBankKeeper.On("SendCoinsFromModuleToAccount", mock.Anything, types.FeeCollectorName, accs[0].Account.GetAddress(), mock.Anything).Return(nil).Once() @@ -319,6 +322,7 @@ func TestPostHandleMock(t *testing.T) { accs := s.CreateTestAccounts(1) s.MockBankKeeper.On("SendCoinsFromAccountToModule", mock.Anything, accs[0].Account.GetAddress(), types.FeeCollectorName, mock.Anything).Return(nil).Once() + s.MockBankKeeper.On("GetBalance", mock.Anything, mock.Anything, mock.Anything).Return(sdk.NewCoin(baseDenom, zeroInt)).Once() return postsuite.TestCaseArgs{ Msgs: []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())}, @@ -428,9 +432,9 @@ func TestPostHandle(t *testing.T) { const ( baseDenom = "stake" resolvableDenom = "atom" - expectedConsumedGas = 59122 + expectedConsumedGas = 61632 - expectedConsumedGasResolve = 25360 // slight difference due to denom resolver + expectedConsumedGasResolve = 27870 // slight difference due to denom resolver gasLimit = 100000 ) @@ -536,7 +540,7 @@ func TestPostHandle(t *testing.T) { Simulate: false, ExpPass: true, ExpErr: nil, - ExpectConsumedGas: 34836, + ExpectConsumedGas: 37325, Mock: false, }, { From 83026874d58badbd171361fe8463d49c7b518d98 Mon Sep 17 00:00:00 2001 From: freeelancer Date: Wed, 16 Oct 2024 19:31:26 +0800 Subject: [PATCH 3/3] fix lint --- app/app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/app.go b/app/app.go index a29bae8a..77b63658 100644 --- a/app/app.go +++ b/app/app.go @@ -1036,7 +1036,7 @@ func (app *App) setAnteHandler(txConfig client.TxConfig, wasmConfig wasmtypes.Wa func (app *App) setPostHandler() { postHandler := PostHandlerOptions{ - AccountKeeper: app.AccountKeeper, + AccountKeeper: app.AccountKeeper, BankKeeper: app.BankKeeper, FeeMarketKeeper: app.FeeMarketKeeper, }