From d3bdb09ffce1a6cb2ed941f3d614b8999e5c423b Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Fri, 25 May 2018 20:29:40 -0700 Subject: [PATCH 1/4] passes, needs tests --- cmd/gaia/app/app.go | 11 +++--- examples/basecoin/app/app.go | 11 +++--- examples/democoin/app/app.go | 13 ++++---- x/auth/ante.go | 8 +++-- x/auth/ante_test.go | 25 ++++++++------ x/auth/context_test.go | 2 +- x/auth/feekeeper.go | 65 ++++++++++++++++++++++++++++++++++++ x/auth/mapper_test.go | 8 +++-- 8 files changed, 111 insertions(+), 32 deletions(-) create mode 100644 x/auth/feekeeper.go diff --git a/cmd/gaia/app/app.go b/cmd/gaia/app/app.go index b2f51498ba..dbecada004 100644 --- a/cmd/gaia/app/app.go +++ b/cmd/gaia/app/app.go @@ -40,10 +40,11 @@ type GaiaApp struct { keyStake *sdk.KVStoreKey // Manage getting and setting accounts - accountMapper auth.AccountMapper - coinKeeper bank.Keeper - ibcMapper ibc.Mapper - stakeKeeper stake.Keeper + accountMapper auth.AccountMapper + feeCollectionKeeper auth.FeeCollectionKeeper + coinKeeper bank.Keeper + ibcMapper ibc.Mapper + stakeKeeper stake.Keeper } func NewGaiaApp(logger log.Logger, db dbm.DB) *GaiaApp { @@ -81,7 +82,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB) *GaiaApp { app.SetInitChainer(app.initChainer) app.SetEndBlocker(stake.NewEndBlocker(app.stakeKeeper)) app.MountStoresIAVL(app.keyMain, app.keyAccount, app.keyIBC, app.keyStake) - app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper)) + app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper, app.feeCollectionKeeper)) err := app.LoadLatestVersion(app.keyMain) if err != nil { cmn.Exit(err.Error()) diff --git a/examples/basecoin/app/app.go b/examples/basecoin/app/app.go index 610a9e5525..086fa32b36 100644 --- a/examples/basecoin/app/app.go +++ b/examples/basecoin/app/app.go @@ -35,10 +35,11 @@ type BasecoinApp struct { keyStake *sdk.KVStoreKey // Manage getting and setting accounts - accountMapper auth.AccountMapper - coinKeeper bank.Keeper - ibcMapper ibc.Mapper - stakeKeeper stake.Keeper + accountMapper auth.AccountMapper + feeCollectionKeeper auth.FeeCollectionKeeper + coinKeeper bank.Keeper + ibcMapper ibc.Mapper + stakeKeeper stake.Keeper } func NewBasecoinApp(logger log.Logger, db dbm.DB) *BasecoinApp { @@ -78,7 +79,7 @@ func NewBasecoinApp(logger log.Logger, db dbm.DB) *BasecoinApp { // Initialize BaseApp. app.SetInitChainer(app.initChainer) app.MountStoresIAVL(app.keyMain, app.keyAccount, app.keyIBC, app.keyStake) - app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper)) + app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper, app.feeCollectionKeeper)) err := app.LoadLatestVersion(app.keyMain) if err != nil { cmn.Exit(err.Error()) diff --git a/examples/democoin/app/app.go b/examples/democoin/app/app.go index 9696630b6e..2075a64da0 100644 --- a/examples/democoin/app/app.go +++ b/examples/democoin/app/app.go @@ -39,11 +39,12 @@ type DemocoinApp struct { capKeyStakingStore *sdk.KVStoreKey // keepers - coinKeeper bank.Keeper - coolKeeper cool.Keeper - powKeeper pow.Keeper - ibcMapper ibc.Mapper - stakeKeeper simplestake.Keeper + feeCollectionKeeper auth.FeeCollectionKeeper + coinKeeper bank.Keeper + coolKeeper cool.Keeper + powKeeper pow.Keeper + ibcMapper ibc.Mapper + stakeKeeper simplestake.Keeper // Manage getting and setting accounts accountMapper auth.AccountMapper @@ -89,7 +90,7 @@ func NewDemocoinApp(logger log.Logger, db dbm.DB) *DemocoinApp { // Initialize BaseApp. app.SetInitChainer(app.initChainerFn(app.coolKeeper, app.powKeeper)) app.MountStoresIAVL(app.capKeyMainStore, app.capKeyAccountStore, app.capKeyPowStore, app.capKeyIBCStore, app.capKeyStakingStore) - app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper)) + app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper, app.feeCollectionKeeper)) err := app.LoadLatestVersion(app.capKeyMainStore) if err != nil { cmn.Exit(err.Error()) diff --git a/x/auth/ante.go b/x/auth/ante.go index c92a87641a..21f8df0fbd 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -9,13 +9,14 @@ import ( ) const ( - verifyCost = 100 + deductFeesCost sdk.Gas = 10 + verifyCost = 100 ) // NewAnteHandler returns an AnteHandler that checks // and increments sequence numbers, checks signatures, // and deducts fees from the first signer. -func NewAnteHandler(am AccountMapper) sdk.AnteHandler { +func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler { return func( ctx sdk.Context, tx sdk.Tx, @@ -77,7 +78,10 @@ func NewAnteHandler(am AccountMapper) sdk.AnteHandler { if i == 0 { // TODO: min fee if !fee.Amount.IsZero() { + ctx.GasMeter().ConsumeGas(deductFeesCost, "deductFees") signerAcc, res = deductFees(signerAcc, fee) + fck.addCollectedFees(ctx, fee.Amount) + if !res.IsOK() { return ctx, res, true } diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index fcde2b464b..e21be8f16f 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -69,11 +69,12 @@ func newTestTxWithSignBytes(msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, f // Test various error cases in the AnteHandler control flow. func TestAnteHandlerSigErrors(t *testing.T) { // setup - ms, capKey := setupMultiStore() + ms, capKey, capKey2 := setupMultiStore() cdc := wire.NewCodec() RegisterBaseAccount(cdc) mapper := NewAccountMapper(cdc, capKey, &BaseAccount{}) - anteHandler := NewAnteHandler(mapper) + feeCollector := NewFeeCollectionKeeper(cdc, capKey2) + anteHandler := NewAnteHandler(mapper, feeCollector) ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil, log.NewNopLogger()) // keys and addresses @@ -110,11 +111,12 @@ func TestAnteHandlerSigErrors(t *testing.T) { // Test logic around sequence checking with one signer and many signers. func TestAnteHandlerSequences(t *testing.T) { // setup - ms, capKey := setupMultiStore() + ms, capKey, capKey2 := setupMultiStore() cdc := wire.NewCodec() RegisterBaseAccount(cdc) mapper := NewAccountMapper(cdc, capKey, &BaseAccount{}) - anteHandler := NewAnteHandler(mapper) + feeCollector := NewFeeCollectionKeeper(cdc, capKey2) + anteHandler := NewAnteHandler(mapper, feeCollector) ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil, log.NewNopLogger()) // keys and addresses @@ -176,11 +178,12 @@ func TestAnteHandlerSequences(t *testing.T) { // Test logic around fee deduction. func TestAnteHandlerFees(t *testing.T) { // setup - ms, capKey := setupMultiStore() + ms, capKey, capKey2 := setupMultiStore() cdc := wire.NewCodec() RegisterBaseAccount(cdc) mapper := NewAccountMapper(cdc, capKey, &BaseAccount{}) - anteHandler := NewAnteHandler(mapper) + feeCollector := NewFeeCollectionKeeper(cdc, capKey2) + anteHandler := NewAnteHandler(mapper, feeCollector) ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil, log.NewNopLogger()) // keys and addresses @@ -213,11 +216,12 @@ func TestAnteHandlerFees(t *testing.T) { func TestAnteHandlerBadSignBytes(t *testing.T) { // setup - ms, capKey := setupMultiStore() + ms, capKey, capKey2 := setupMultiStore() cdc := wire.NewCodec() RegisterBaseAccount(cdc) mapper := NewAccountMapper(cdc, capKey, &BaseAccount{}) - anteHandler := NewAnteHandler(mapper) + feeCollector := NewFeeCollectionKeeper(cdc, capKey2) + anteHandler := NewAnteHandler(mapper, feeCollector) ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil, log.NewNopLogger()) // keys and addresses @@ -288,11 +292,12 @@ func TestAnteHandlerBadSignBytes(t *testing.T) { func TestAnteHandlerSetPubKey(t *testing.T) { // setup - ms, capKey := setupMultiStore() + ms, capKey, capKey2 := setupMultiStore() cdc := wire.NewCodec() RegisterBaseAccount(cdc) mapper := NewAccountMapper(cdc, capKey, &BaseAccount{}) - anteHandler := NewAnteHandler(mapper) + feeCollector := NewFeeCollectionKeeper(cdc, capKey2) + anteHandler := NewAnteHandler(mapper, feeCollector) ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil, log.NewNopLogger()) // keys and addresses diff --git a/x/auth/context_test.go b/x/auth/context_test.go index e1db131679..a93de44d0c 100644 --- a/x/auth/context_test.go +++ b/x/auth/context_test.go @@ -12,7 +12,7 @@ import ( ) func TestContextWithSigners(t *testing.T) { - ms, _ := setupMultiStore() + ms, _, _ := setupMultiStore() ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil, log.NewNopLogger()) _, _, addr1 := keyPubAddr() diff --git a/x/auth/feekeeper.go b/x/auth/feekeeper.go new file mode 100644 index 0000000000..0828fb3652 --- /dev/null +++ b/x/auth/feekeeper.go @@ -0,0 +1,65 @@ +package auth + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + wire "github.com/cosmos/cosmos-sdk/wire" +) + +// This FeeCollectionKeeper handles collection of fees in the anteHandler +// and setting of MinFees for different fee tokens +type FeeCollectionKeeper struct { + + // The (unexposed) key used to access the fee store from the Context. + key sdk.StoreKey + + // The wire codec for binary encoding/decoding of accounts. + cdc *wire.Codec +} + +// NewFeeKeeper returns a new FeeKeeper +func NewFeeCollectionKeeper(cdc *wire.Codec, key sdk.StoreKey) FeeCollectionKeeper { + return FeeCollectionKeeper{ + key: key, + cdc: cdc, + } +} + +// Adds to Collected Fee Pool +func (fck FeeCollectionKeeper) GetCollectedFees(ctx sdk.Context) sdk.Coins { + store := ctx.KVStore(fck.key) + bz := store.Get([]byte("collectedFees")) + if bz == nil { + return sdk.Coins{} + } + + feePool := &(sdk.Coins{}) + err := fck.cdc.UnmarshalBinary(bz, feePool) + if err != nil { + panic("should not happen") + } + return *feePool +} + +// Sets to Collected Fee Pool +func (fck FeeCollectionKeeper) setCollectedFees(ctx sdk.Context, coins sdk.Coins) { + bz, err := fck.cdc.MarshalBinary(coins) + if err != nil { + panic("should not happen") + } + + store := ctx.KVStore(fck.key) + store.Set([]byte("collectedFees"), bz) +} + +// Adds to Collected Fee Pool +func (fck FeeCollectionKeeper) addCollectedFees(ctx sdk.Context, coins sdk.Coins) sdk.Coins { + newCoins := fck.GetCollectedFees(ctx).Plus(coins) + fck.setCollectedFees(ctx, newCoins) + + return newCoins +} + +// Clears the collected Fee Pool +func (fck FeeCollectionKeeper) ClearCollectedFees(ctx sdk.Context) { + fck.setCollectedFees(ctx, sdk.Coins{}) +} diff --git a/x/auth/mapper_test.go b/x/auth/mapper_test.go index cdd418990a..7f6397069a 100644 --- a/x/auth/mapper_test.go +++ b/x/auth/mapper_test.go @@ -14,17 +14,19 @@ import ( wire "github.com/cosmos/cosmos-sdk/wire" ) -func setupMultiStore() (sdk.MultiStore, *sdk.KVStoreKey) { +func setupMultiStore() (sdk.MultiStore, *sdk.KVStoreKey, *sdk.KVStoreKey) { db := dbm.NewMemDB() capKey := sdk.NewKVStoreKey("capkey") + capKey2 := sdk.NewKVStoreKey("capkey2") ms := store.NewCommitMultiStore(db) ms.MountStoreWithDB(capKey, sdk.StoreTypeIAVL, db) + ms.MountStoreWithDB(capKey2, sdk.StoreTypeIAVL, db) ms.LoadLatestVersion() - return ms, capKey + return ms, capKey, capKey2 } func TestAccountMapperGetSet(t *testing.T) { - ms, capKey := setupMultiStore() + ms, capKey, _ := setupMultiStore() cdc := wire.NewCodec() RegisterBaseAccount(cdc) From f81a70b3150606c2d24ed028f2605990500eb208 Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Fri, 25 May 2018 20:48:27 -0700 Subject: [PATCH 2/4] added keeper tests --- x/auth/feekeeper_test.go | 75 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 x/auth/feekeeper_test.go diff --git a/x/auth/feekeeper_test.go b/x/auth/feekeeper_test.go new file mode 100644 index 0000000000..2f1ffc59bc --- /dev/null +++ b/x/auth/feekeeper_test.go @@ -0,0 +1,75 @@ +package auth + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + abci "github.com/tendermint/abci/types" + "github.com/tendermint/tmlibs/log" + + sdk "github.com/cosmos/cosmos-sdk/types" + wire "github.com/cosmos/cosmos-sdk/wire" +) + +var ( + emptyCoins = sdk.Coins{} + oneCoin = sdk.Coins{{"foocoin", 1}} + twoCoins = sdk.Coins{{"foocoin", 2}} +) + +func TestFeeCollectionKeeperGetSet(t *testing.T) { + ms, _, capKey2 := setupMultiStore() + cdc := wire.NewCodec() + + // make context and keeper + ctx := sdk.NewContext(ms, abci.Header{}, false, nil, log.NewNopLogger()) + fck := NewFeeCollectionKeeper(cdc, capKey2) + + // no coins initially + currFees := fck.GetCollectedFees(ctx) + assert.True(t, currFees.IsEqual(emptyCoins)) + + // set feeCollection to oneCoin + fck.setCollectedFees(ctx, oneCoin) + + // check that it is equal to oneCoin + assert.True(t, fck.GetCollectedFees(ctx).IsEqual(oneCoin)) +} + +func TestFeeCollectionKeeperAdd(t *testing.T) { + ms, _, capKey2 := setupMultiStore() + cdc := wire.NewCodec() + + // make context and keeper + ctx := sdk.NewContext(ms, abci.Header{}, false, nil, log.NewNopLogger()) + fck := NewFeeCollectionKeeper(cdc, capKey2) + + // no coins initially + assert.True(t, fck.GetCollectedFees(ctx).IsEqual(emptyCoins)) + + // add oneCoin and check that pool is now oneCoin + fck.addCollectedFees(ctx, oneCoin) + assert.True(t, fck.GetCollectedFees(ctx).IsEqual(oneCoin)) + + // add oneCoin again and check that pool is now twoCoins + fck.addCollectedFees(ctx, oneCoin) + assert.True(t, fck.GetCollectedFees(ctx).IsEqual(twoCoins)) +} + +func TestFeeCollectionKeeperClear(t *testing.T) { + ms, _, capKey2 := setupMultiStore() + cdc := wire.NewCodec() + + // make context and keeper + ctx := sdk.NewContext(ms, abci.Header{}, false, nil, log.NewNopLogger()) + fck := NewFeeCollectionKeeper(cdc, capKey2) + + // set coins initially + fck.setCollectedFees(ctx, twoCoins) + assert.True(t, fck.GetCollectedFees(ctx).IsEqual(twoCoins)) + + // clear fees and see that pool is now empty + fck.ClearCollectedFees(ctx) + assert.True(t, fck.GetCollectedFees(ctx).IsEqual(emptyCoins)) +} From 4f6c77d8cba765f8f31125c3c753ed86efc88d5d Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Fri, 25 May 2018 21:10:09 -0700 Subject: [PATCH 3/4] antehandler tests --- x/auth/ante.go | 3 +-- x/auth/ante_test.go | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/x/auth/ante.go b/x/auth/ante.go index 21f8df0fbd..9663bcfe45 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -80,11 +80,10 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler { if !fee.Amount.IsZero() { ctx.GasMeter().ConsumeGas(deductFeesCost, "deductFees") signerAcc, res = deductFees(signerAcc, fee) - fck.addCollectedFees(ctx, fee.Amount) - if !res.IsOK() { return ctx, res, true } + fck.addCollectedFees(ctx, fee.Amount) } } diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index e21be8f16f..b7f22e5d54 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -209,9 +209,13 @@ func TestAnteHandlerFees(t *testing.T) { mapper.SetAccount(ctx, acc1) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInsufficientFunds) + assert.True(t, feeCollector.GetCollectedFees(ctx).IsEqual(emptyCoins)) + acc1.SetCoins(sdk.Coins{{"atom", 150}}) mapper.SetAccount(ctx, acc1) checkValidTx(t, anteHandler, ctx, tx) + + assert.True(t, feeCollector.GetCollectedFees(ctx).IsEqual(sdk.Coins{{"atom", 150}})) } func TestAnteHandlerBadSignBytes(t *testing.T) { From bf02cdcf974c2f22b7b61e18483a48ea63a6d43f Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Fri, 25 May 2018 21:14:49 -0700 Subject: [PATCH 4/4] address Chris review --- x/auth/feekeeper.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/x/auth/feekeeper.go b/x/auth/feekeeper.go index 0828fb3652..3e03a81aa2 100644 --- a/x/auth/feekeeper.go +++ b/x/auth/feekeeper.go @@ -5,6 +5,10 @@ import ( wire "github.com/cosmos/cosmos-sdk/wire" ) +var ( + collectedFeesKey = []byte("collectedFees") +) + // This FeeCollectionKeeper handles collection of fees in the anteHandler // and setting of MinFees for different fee tokens type FeeCollectionKeeper struct { @@ -27,28 +31,21 @@ func NewFeeCollectionKeeper(cdc *wire.Codec, key sdk.StoreKey) FeeCollectionKeep // Adds to Collected Fee Pool func (fck FeeCollectionKeeper) GetCollectedFees(ctx sdk.Context) sdk.Coins { store := ctx.KVStore(fck.key) - bz := store.Get([]byte("collectedFees")) + bz := store.Get(collectedFeesKey) if bz == nil { return sdk.Coins{} } feePool := &(sdk.Coins{}) - err := fck.cdc.UnmarshalBinary(bz, feePool) - if err != nil { - panic("should not happen") - } + fck.cdc.MustUnmarshalBinary(bz, feePool) return *feePool } // Sets to Collected Fee Pool func (fck FeeCollectionKeeper) setCollectedFees(ctx sdk.Context, coins sdk.Coins) { - bz, err := fck.cdc.MarshalBinary(coins) - if err != nil { - panic("should not happen") - } - + bz := fck.cdc.MustMarshalBinary(coins) store := ctx.KVStore(fck.key) - store.Set([]byte("collectedFees"), bz) + store.Set(collectedFeesKey, bz) } // Adds to Collected Fee Pool