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

feat: configurable fee collector for DeductFeeDecorator #407

Merged
merged 3 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion CHANGELOG-Agoric.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ Ref: https://keepachangelog.com/en/1.0.0/

## Unreleased

### API Breaking Changes
### Improvements

* (auth) [#407](https://github.com/agoric-labs/cosmos-sdk/pull/407) Configurable fee collector module account in DeductFeeDecorator.

### API Breaking

* (auth, bank) Agoric/agoric-sdk#8989 Remove deprecated lien support

Expand Down
34 changes: 21 additions & 13 deletions x/auth/ante/fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,30 @@ type TxFeeChecker func(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error)
// Call next AnteHandler if fees successfully deducted
// CONTRACT: Tx must implement FeeTx interface to use DeductFeeDecorator
type DeductFeeDecorator struct {
accountKeeper AccountKeeper
bankKeeper types.BankKeeper
feegrantKeeper FeegrantKeeper
txFeeChecker TxFeeChecker
accountKeeper AccountKeeper
bankKeeper types.BankKeeper
feegrantKeeper FeegrantKeeper
txFeeChecker TxFeeChecker
feeCollectorName string
}

func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper, tfc TxFeeChecker) DeductFeeDecorator {
return NewDeductFeeDecoratorWithName(ak, bk, fk, tfc, types.FeeCollectorName)
}

// NewDeductFeeDecoratorWithName returns a DeductFeeDecorator using a custom fee collector module account.
// Agoric note: for collecting fees in the reserve account.
func NewDeductFeeDecoratorWithName(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper, tfc TxFeeChecker, feeCollectorName string) DeductFeeDecorator {
if tfc == nil {
tfc = checkTxFeeWithValidatorMinGasPrices
}

return DeductFeeDecorator{
accountKeeper: ak,
bankKeeper: bk,
feegrantKeeper: fk,
txFeeChecker: tfc,
accountKeeper: ak,
bankKeeper: bk,
feegrantKeeper: fk,
txFeeChecker: tfc,
feeCollectorName: feeCollectorName,
}
}

Expand Down Expand Up @@ -73,8 +81,8 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee
return sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")
}

if addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName); addr == nil {
return fmt.Errorf("fee collector module account (%s) has not been set", types.FeeCollectorName)
if addr := dfd.accountKeeper.GetModuleAddress(dfd.feeCollectorName); addr == nil {
return fmt.Errorf("fee collector module account (%s) has not been set", dfd.feeCollectorName)
}

feePayer := feeTx.FeePayer()
Expand Down Expand Up @@ -103,7 +111,7 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee

// deduct the fees
if !fee.IsZero() {
err := DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee)
err := DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee, dfd.feeCollectorName)
if err != nil {
return err
}
Expand All @@ -122,12 +130,12 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee
}

// DeductFees deducts fees from the given account.
func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc types.AccountI, fees sdk.Coins) error {
func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc types.AccountI, fees sdk.Coins, feeCollectorName string) error {
if !fees.IsValid() {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "invalid fee amount: %s", fees)
}

err := bankKeeper.SendCoinsFromAccountToModule(ctx, acc.GetAddress(), types.FeeCollectorName, fees)
err := bankKeeper.SendCoinsFromAccountToModule(ctx, acc.GetAddress(), feeCollectorName, fees)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())
}
Expand Down
46 changes: 46 additions & 0 deletions x/auth/ante/fee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/ante"
"github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/cosmos/cosmos-sdk/x/bank/testutil"
)

Expand Down Expand Up @@ -146,3 +147,48 @@ func (s *AnteTestSuite) TestDeductFees() {

s.Require().Nil(err, "Tx errored after account has been set with sufficient funds")
}

func (s *AnteTestSuite) TestDeductFees_WithName() {
s.SetupTest(false) // setup
s.txBuilder = s.clientCtx.TxConfig.NewTxBuilder()

// keys and addresses
priv1, _, addr1 := testdata.KeyTestPubAddr()

// msg and signatures
msg := testdata.NewTestMsg(addr1)
feeAmount := testdata.NewTestFeeAmount()
gasLimit := testdata.NewTestGasLimit()
s.Require().NoError(s.txBuilder.SetMsgs(msg))
s.txBuilder.SetFeeAmount(feeAmount)
s.txBuilder.SetGasLimit(gasLimit)

privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0}
tx, err := s.CreateTestTx(privs, accNums, accSeqs, s.ctx.ChainID())
s.Require().NoError(err)

// Set transacting account with sufficient funds
acc := s.app.AccountKeeper.NewAccountWithAddress(s.ctx, addr1)
s.app.AccountKeeper.SetAccount(s.ctx, acc)
coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(200)))
err = testutil.FundAccount(s.app.BankKeeper, s.ctx, addr1, coins)
s.Require().NoError(err)

feeCollectorAcc := s.app.AccountKeeper.GetModuleAccount(s.ctx, types.FeeCollectorName)
// pick a simapp module account
altCollectorName := "distribution"
altCollectorAcc := s.app.AccountKeeper.GetModuleAccount(s.ctx, altCollectorName)
s.Require().True(s.app.BankKeeper.GetAllBalances(s.ctx, feeCollectorAcc.GetAddress()).Empty())
altBalance := s.app.BankKeeper.GetAllBalances(s.ctx, altCollectorAcc.GetAddress())

// Run the transaction through a handler chain that deducts fees into altCollectorAcc.
dfd := ante.NewDeductFeeDecoratorWithName(s.app.AccountKeeper, s.app.BankKeeper, nil, nil, altCollectorName)
antehandler := sdk.ChainAnteDecorators(dfd)
_, err = antehandler(s.ctx, tx, false)
s.Require().NoError(err)

s.Require().True(s.app.BankKeeper.GetAllBalances(s.ctx, feeCollectorAcc.GetAddress()).Empty())
newAltBalance := s.app.BankKeeper.GetAllBalances(s.ctx, altCollectorAcc.GetAddress())
s.Require().True(newAltBalance.IsAllGTE(altBalance))
Comment on lines +190 to +192

Choose a reason for hiding this comment

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

Suggested change
s.Require().True(s.app.BankKeeper.GetAllBalances(s.ctx, feeCollectorAcc.GetAddress()).Empty())
newAltBalance := s.app.BankKeeper.GetAllBalances(s.ctx, altCollectorAcc.GetAddress())
s.Require().True(newAltBalance.IsAllGTE(altBalance))
// Assert that feeCollectorAcc is still empty but the altCollectorAcc balance has increased.
newFeeCollectorBalance := s.app.BankKeeper.GetAllBalances(s.ctx, feeCollectorAcc.GetAddress())
s.Require().True(newFeeCollectorBalance.Empty())
newAltBalance := s.app.BankKeeper.GetAllBalances(s.ctx, altCollectorAcc.GetAddress())
s.Require().True(newAltBalance.IsAllGTE(altBalance))
s.Require().False(newAltBalance.IsEqual(altBalance))

Copy link
Author

Choose a reason for hiding this comment

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

Done.

s.Require().False(newAltBalance.IsEqual(altBalance))
}
Loading