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: Allow to restrict MintCoins from app.go #10771

Merged
merged 13 commits into from
Feb 18, 2022
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,14 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#10748](https://github.com/cosmos/cosmos-sdk/pull/10748) Move legacy `x/gov` api to `v1beta1` directory.
* [\#10816](https://github.com/cosmos/cosmos-sdk/pull/10816) Reuse blocked addresses from the bank module. No need to pass them to distribution.
* [\#10852](https://github.com/cosmos/cosmos-sdk/pull/10852) Move `x/gov/types` to `x/gov/types/v1beta2`.
* [\#10922](https://github.com/cosmos/cosmos-sdk/pull/10922), [/#10957](https://github.com/cosmos/cosmos-sdk/pull/10957) Move key `server.Generate*` functions to testutil and support custom mnemonics in in-process testing network. Moved `TestMnemonic` from `testutil` package to `testdata`.
* (x/bank) [\#10771](https://github.com/cosmos/cosmos-sdk/pull/10771) Add safety check on bank module perms to allow module-specific mint restrictions (e.g. only minting a certain denom).* (x/bank) [\#10771](https://github.com/cosmos/cosmos-sdk/pull/10771) Add `bank.BaseKeeper.WithMintCoinsRestriction` function to restrict use of bank `MintCoins` usage.
* [\#10868](https://github.com/cosmos/cosmos-sdk/pull/10868), [\#10989](https://github.com/cosmos/cosmos-sdk/pull/10989) The Gov keeper accepts now 2 more mandatory arguments, the ServiceMsgRouter and a maximum proposal metadata length.
* [\#10868](https://github.com/cosmos/cosmos-sdk/pull/10868), [\#10989](https://github.com/cosmos/cosmos-sdk/pull/10989), [\#11093](https://github.com/cosmos/cosmos-sdk/pull/11093) The Gov keeper accepts now 2 more mandatory arguments, the ServiceMsgRouter and a gov Config including the max metadata length.
* [\#11124](https://github.com/cosmos/cosmos-sdk/pull/11124) Add `GetAllVersions` to application store
* (x/authz) [\#10447](https://github.com/cosmos/cosmos-sdk/pull/10447) authz `NewGrant` takes a new argument: block time, to correctly validate expire time.


### Client Breaking Changes

* [\#11089](https://github.com/cosmos/cosmos-sdk/pull/11089]) interacting with the node through `grpc.Dial` requires clients to pass a codec refer to [doc](docs/run-node/interact-node.md).
Expand Down
50 changes: 40 additions & 10 deletions x/bank/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ var _ Keeper = (*BaseKeeper)(nil)
// between accounts.
type Keeper interface {
SendKeeper
WithMintCoinsRestriction(MintingRestrictionFn) BaseKeeper
Copy link
Contributor

@amaury1093 amaury1093 Jan 26, 2022

Choose a reason for hiding this comment

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

We're changing a public API so this is API changing. When resolving conflicts in Changelog, could you move the Add bank.BaseKeeper.WithMintCoinsRestrictionfunction to restrict use of bankMintCoins usage. part into the API-breaking section?

Edit: when backporting, let's remove this line from the backport. There might be some manual casting to access this method on the bank keeper, but at least it allows backporting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha!


InitGenesis(sdk.Context, *types.GenesisState)
ExportGenesis(sdk.Context) *types.GenesisState
Expand Down Expand Up @@ -52,12 +53,15 @@ type Keeper interface {
type BaseKeeper struct {
BaseSendKeeper

ak types.AccountKeeper
cdc codec.BinaryCodec
storeKey storetypes.StoreKey
paramSpace paramtypes.Subspace
ak types.AccountKeeper
cdc codec.BinaryCodec
storeKey storetypes.StoreKey
paramSpace paramtypes.Subspace
mintCoinsRestrictionFn MintingRestrictionFn
}

type MintingRestrictionFn func(ctx sdk.Context, coins sdk.Coins) error

// GetPaginatedTotalSupply queries for the supply, ignoring 0 coins, with a given pagination
func (k BaseKeeper) GetPaginatedTotalSupply(ctx sdk.Context, pagination *query.PageRequest) (sdk.Coins, *query.PageResponse, error) {
store := ctx.KVStore(k.storeKey)
Expand Down Expand Up @@ -103,12 +107,33 @@ func NewBaseKeeper(
}

return BaseKeeper{
BaseSendKeeper: NewBaseSendKeeper(cdc, storeKey, ak, paramSpace, blockedAddrs),
ak: ak,
cdc: cdc,
storeKey: storeKey,
paramSpace: paramSpace,
BaseSendKeeper: NewBaseSendKeeper(cdc, storeKey, ak, paramSpace, blockedAddrs),
ak: ak,
cdc: cdc,
storeKey: storeKey,
paramSpace: paramSpace,
mintCoinsRestrictionFn: func(ctx sdk.Context, coins sdk.Coins) error { return nil },
}
}

// WithMintCoinsRestriction restricts the bank Keeper used within a specific module to
// have restricted permissions on minting via function passed in parameter.
// Previous restriction functions can be nested as such:
// bankKeeper.WithMintCoinsRestriction(restriction1).WithMintCoinsRestriction(restriction2)
func (k BaseKeeper) WithMintCoinsRestriction(check MintingRestrictionFn) BaseKeeper {
oldRestrictionFn := k.mintCoinsRestrictionFn
k.mintCoinsRestrictionFn = func(ctx sdk.Context, coins sdk.Coins) error {
err := check(ctx, coins)
if err != nil {
return err
}
err = oldRestrictionFn(ctx, coins)
if err != nil {
return err
}
return nil
}
return k
}

// DelegateCoins performs delegation by deducting amt coins from an account with
Expand Down Expand Up @@ -380,6 +405,11 @@ func (k BaseKeeper) UndelegateCoinsFromModuleToAccount(
// MintCoins creates new coins from thin air and adds it to the module account.
// It will panic if the module account does not exist or is unauthorized.
func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amounts sdk.Coins) error {
err := k.mintCoinsRestrictionFn(ctx, amounts)
if err != nil {
ctx.Logger().Error(fmt.Sprintf("Module %q attempted to mint coins %s it doesn't have permission for, error %v", moduleName, amounts, err))
return err
}
acc := k.ak.GetModuleAccount(ctx, moduleName)
if acc == nil {
panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", moduleName))
Expand All @@ -389,7 +419,7 @@ func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amounts sdk.Co
panic(sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to mint tokens", moduleName))
}

err := k.addCoins(ctx, acc.GetAddress(), amounts)
err = k.addCoins(ctx, acc.GetAddress(), amounts)
if err != nil {
return err
}
Expand Down
71 changes: 71 additions & 0 deletions x/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -1167,6 +1168,76 @@ func (suite *IntegrationTestSuite) getTestMetadata() []types.Metadata {
}
}

func (suite *IntegrationTestSuite) TestMintCoinRestrictions() {
type BankMintingRestrictionFn func(ctx sdk.Context, coins sdk.Coins) error

maccPerms := simapp.GetMaccPerms()
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}

suite.app.AccountKeeper = authkeeper.NewAccountKeeper(
suite.app.AppCodec(), suite.app.GetKey(authtypes.StoreKey), suite.app.GetSubspace(authtypes.ModuleName),
authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix,
)
suite.app.AccountKeeper.SetModuleAccount(suite.ctx, multiPermAcc)

type testCase struct {
coinsToTry sdk.Coin
expectPass bool
}

tests := []struct {
name string
restrictionFn BankMintingRestrictionFn
testCases []testCase
}{
{
"restriction",
func(ctx sdk.Context, coins sdk.Coins) error {
for _, coin := range coins {
if coin.Denom != fooDenom {
return fmt.Errorf("Module %s only has perms for minting %s coins, tried minting %s coins", types.ModuleName, fooDenom, coin.Denom)
}
}
return nil
},
[]testCase{
{
coinsToTry: newFooCoin(100),
expectPass: true,
},
{
coinsToTry: newBarCoin(100),
expectPass: false,
},
},
},
}

for _, test := range tests {
suite.app.BankKeeper = keeper.NewBaseKeeper(suite.app.AppCodec(), suite.app.GetKey(types.StoreKey),
suite.app.AccountKeeper, suite.app.GetSubspace(types.ModuleName), nil).WithMintCoinsRestriction(keeper.MintingRestrictionFn(test.restrictionFn))
for _, testCase := range test.testCases {
if testCase.expectPass {
suite.Require().NoError(
suite.app.BankKeeper.MintCoins(
suite.ctx,
multiPermAcc.Name,
sdk.NewCoins(testCase.coinsToTry),
),
)
} else {
suite.Require().Error(
suite.app.BankKeeper.MintCoins(
suite.ctx,
multiPermAcc.Name,
sdk.NewCoins(testCase.coinsToTry),
),
)
}
}
}
}

func TestKeeperTestSuite(t *testing.T) {
suite.Run(t, new(IntegrationTestSuite))
}
3 changes: 3 additions & 0 deletions x/bank/spec/02_keepers.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,14 @@ message Output {

The base keeper provides full-permission access: the ability to arbitrary modify any account's balance and mint or burn coins.

Restricted permission to mint per module could be achieved by using baseKeeper with `WithMintCoinsRestriction` to give specific restrictions to mint (e.g. only minting certain denom).

```go
// Keeper defines a module interface that facilitates the transfer of coins
// between accounts.
type Keeper interface {
SendKeeper
WithMintCoinsRestriction(NewRestrictionFn BankMintingRestrictionFn) BaseKeeper
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved

InitGenesis(sdk.Context, *types.GenesisState)
ExportGenesis(sdk.Context) *types.GenesisState
Expand Down