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

Enable/disable coin transfers by denom #6527

Merged
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
3c3a232
initial implementation of per denom sendenabled
Jun 26, 2020
5cb0357
Fix for accidentally removed keyword
Jun 27, 2020
b1ea0e2
Validate individual param in param array
Jun 27, 2020
03ae118
Lint fix
Jun 27, 2020
fba1a1a
Refactor bank params to use protobuf
Jun 29, 2020
fe4844b
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
iramiller Jun 30, 2020
dbf96c3
Refactor simulation genesis for clarity
Jun 30, 2020
d05321f
update changelog for bank sendenable per denom
Jun 30, 2020
8568819
fix NoOpMsg type in multisend test
Jun 30, 2020
9308435
Add a coin denom send check utility function
Jun 30, 2020
a76d751
Additional godoc comments and clarification
Jun 30, 2020
42d8c08
Add default send enabled parameter to bank.
Jun 30, 2020
fc3bd0e
Minor suggested improvements.
Jun 30, 2020
ba1853c
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
fedekunze Jun 30, 2020
db4646b
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
Jul 2, 2020
0ff3263
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
Jul 3, 2020
7ede620
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
Jul 4, 2020
1f753e6
simulation fix
Jul 6, 2020
f06e854
bank proto sendenabled package name removed
Jul 6, 2020
e11d1d1
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
iramiller Jul 6, 2020
7d63967
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
fedekunze Jul 7, 2020
23494e8
Remove extra gogo proto yaml tags
Jul 7, 2020
d25c8f9
Params rename IsSendEnabled to SendEnabledDenom
Jul 7, 2020
9c8918d
Refactor to SendEnabledCoin(s)
Jul 7, 2020
c7f1902
Merge branch '6518-enable-disable-coin-transfers-by-denom' of github.…
Jul 7, 2020
78b521b
update slashing test to use bank params
Jul 7, 2020
9458c82
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
alexanderbez Jul 8, 2020
8778d26
Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom
fedekunze Jul 8, 2020
eda215b
Clean up change log entry for feature.
Jul 8, 2020
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ be used to retrieve the actual proposal `Content`. Also the `NewMsgSubmitProposa
* (modules) [\#6311](https://github.com/cosmos/cosmos-sdk/issues/6311) Remove `alias.go` usage
* (x/auth) [\#6443](https://github.com/cosmos/cosmos-sdk/issues/6443) Move `FeeTx` and `TxWithMemo` interfaces from `x/auth/ante` to `types`.
* (modules) [\#6447](https://github.com/cosmos/cosmos-sdk/issues/6447) Rename `blacklistedAddrs` to `blockedAddrs`.
* (x/bank) [\6518](https://github.com/cosmos/cosmos-sdk/pull/6518) Support for per-denomination send_enabled flags. Existing send_enabled flag has been moved into a Params structure and transformed into an array of: `{denom: string, enabled: bool}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you move this to State Machine Breaking? 🙏


Migration guide:

Expand Down
19 changes: 19 additions & 0 deletions proto/cosmos/bank/bank.proto
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,25 @@ import "cosmos/cosmos.proto";

option go_package = "github.com/cosmos/cosmos-sdk/x/bank/types";

// Params defines the set of bank parameters.
message Params {
option (gogoproto.goproto_stringer) = false;
repeated SendEnabled send_enabled = 1[
(gogoproto.moretags) = "yaml:\"send_enabled,omitempty\""
];
bool default_send_enabled = 2[
(gogoproto.moretags) = "yaml:\"default_send_enabled,omitempty\""
];
}

// Send enabled configuration properties for each denomination
message SendEnabled {
option (gogoproto.equal) = true;
option (gogoproto.goproto_stringer) = false;
string denom = 1;
bool enabled = 2;
}

// MsgSend - high level transaction of the coin module
message MsgSend {
option (gogoproto.equal) = true;
Expand Down
4 changes: 2 additions & 2 deletions simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func SetupWithGenesisValSet(t *testing.T, valSet *tmtypes.ValidatorSet, genAccs
}

// update total supply
bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().SendEnabled, balances, totalSupply)
bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, totalSupply)
genesisState[banktypes.ModuleName] = app.Codec().MustMarshalJSON(bankGenesis)

stateBytes, err := codec.MarshalJSONIndent(app.Codec(), genesisState)
Expand Down Expand Up @@ -157,7 +157,7 @@ func SetupWithGenesisAccounts(genAccs []authtypes.GenesisAccount, balances ...ba
totalSupply = totalSupply.Add(b.Coins...)
}

bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().SendEnabled, balances, totalSupply)
bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, totalSupply)
genesisState[banktypes.ModuleName] = app.Codec().MustMarshalJSON(bankGenesis)

stateBytes, err := codec.MarshalJSONIndent(app.Codec(), genesisState)
Expand Down
7 changes: 5 additions & 2 deletions x/bank/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

// InitGenesis initializes the bank module's state from a given genesis state.
func InitGenesis(ctx sdk.Context, keeper keeper.Keeper, genState types.GenesisState) {
keeper.SetSendEnabled(ctx, genState.SendEnabled)
keeper.SetParams(ctx, genState.Params)

var totalSupply sdk.Coins

Expand Down Expand Up @@ -57,11 +57,14 @@ func ExportGenesis(ctx sdk.Context, keeper keeper.Keeper) types.GenesisState {
})
}

return types.NewGenesisState(keeper.GetSendEnabled(ctx), balances, keeper.GetSupply(ctx).GetTotal())
return types.NewGenesisState(keeper.GetParams(ctx), balances, keeper.GetSupply(ctx).GetTotal())
}

// ValidateGenesis performs basic validation of supply genesis data returning an
// error for any failed validation criteria.
func ValidateGenesis(data types.GenesisState) error {
if err := data.Params.Validate(); err != nil {
return err
}
return types.NewSupply(data.Supply).ValidateBasic()
}
10 changes: 6 additions & 4 deletions x/bank/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ func NewHandler(k keeper.Keeper) sdk.Handler {

// Handle MsgSend.
func handleMsgSend(ctx sdk.Context, k keeper.Keeper, msg *types.MsgSend) (*sdk.Result, error) {
if !k.GetSendEnabled(ctx) {
return nil, types.ErrSendDisabled
if err := k.SendEnabledCoins(ctx, msg.Amount...); err != nil {
return nil, err
}

if k.BlockedAddr(msg.ToAddress) {
Expand Down Expand Up @@ -66,8 +66,10 @@ func handleMsgSend(ctx sdk.Context, k keeper.Keeper, msg *types.MsgSend) (*sdk.R
// Handle MsgMultiSend.
func handleMsgMultiSend(ctx sdk.Context, k keeper.Keeper, msg *types.MsgMultiSend) (*sdk.Result, error) {
// NOTE: totalIn == totalOut should already have been checked
if !k.GetSendEnabled(ctx) {
return nil, types.ErrSendDisabled
for _, in := range msg.Inputs {
iramiller marked this conversation as resolved.
Show resolved Hide resolved
if err := k.SendEnabledCoins(ctx, in.Coins...); err != nil {
return nil, err
}
}

for _, out := range msg.Outputs {
Expand Down
46 changes: 41 additions & 5 deletions x/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (suite *IntegrationTestSuite) SetupTest() {
ctx := app.BaseApp.NewContext(false, abci.Header{})

app.AccountKeeper.SetParams(ctx, authtypes.DefaultParams())
app.BankKeeper.SetSendEnabled(ctx, true)
app.BankKeeper.SetParams(ctx, types.DefaultParams())

suite.app = app
suite.ctx = ctx
Expand Down Expand Up @@ -454,9 +454,45 @@ func (suite *IntegrationTestSuite) TestBalance() {

func (suite *IntegrationTestSuite) TestSendEnabled() {
app, ctx := suite.app, suite.ctx
enabled := false
app.BankKeeper.SetSendEnabled(ctx, enabled)
suite.Require().Equal(enabled, app.BankKeeper.GetSendEnabled(ctx))
enabled := true
params := types.DefaultParams()
suite.Require().Equal(enabled, params.DefaultSendEnabled)

app.BankKeeper.SetParams(ctx, params)

bondCoin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.OneInt())
fooCoin := sdk.NewCoin("foocoin", sdk.OneInt())
barCoin := sdk.NewCoin("barcoin", sdk.OneInt())

// assert with default (all denom) send enabled both Bar and Bond Denom are enabled
suite.Require().Equal(enabled, app.BankKeeper.SendEnabledCoin(ctx, barCoin))
suite.Require().Equal(enabled, app.BankKeeper.SendEnabledCoin(ctx, bondCoin))

// Both coins should be send enabled.
err := app.BankKeeper.SendEnabledCoins(ctx, fooCoin, bondCoin)
suite.Require().NoError(err)

// Set default send_enabled to !enabled, add a foodenom that overrides default as enabled
params.DefaultSendEnabled = !enabled
params = params.SetSendEnabledParam(fooCoin.Denom, enabled)
app.BankKeeper.SetParams(ctx, params)

// Expect our specific override to be enabled, others to be !enabled.
suite.Require().Equal(enabled, app.BankKeeper.SendEnabledCoin(ctx, fooCoin))
suite.Require().Equal(!enabled, app.BankKeeper.SendEnabledCoin(ctx, barCoin))
suite.Require().Equal(!enabled, app.BankKeeper.SendEnabledCoin(ctx, bondCoin))

// Foo coin should be send enabled.
err = app.BankKeeper.SendEnabledCoins(ctx, fooCoin)
suite.Require().NoError(err)

// Expect an error when one coin is not send enabled.
err = app.BankKeeper.SendEnabledCoins(ctx, fooCoin, bondCoin)
suite.Require().Error(err)

// Expect an error when all coins are not send enabled.
err = app.BankKeeper.SendEnabledCoins(ctx, bondCoin, barCoin)
suite.Require().Error(err)
}

func (suite *IntegrationTestSuite) TestHasBalance() {
Expand Down Expand Up @@ -531,7 +567,7 @@ func (suite *IntegrationTestSuite) TestMsgSendEvents() {
func (suite *IntegrationTestSuite) TestMsgMultiSendEvents() {
app, ctx := suite.app, suite.ctx

app.BankKeeper.SetSendEnabled(ctx, true)
app.BankKeeper.SetParams(ctx, types.DefaultParams())

addr := sdk.AccAddress([]byte("addr1"))
addr2 := sdk.AccAddress([]byte("addr2"))
Expand Down
39 changes: 29 additions & 10 deletions x/bank/keeper/send.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ type SendKeeper interface {
SetBalance(ctx sdk.Context, addr sdk.AccAddress, balance sdk.Coin) error
SetBalances(ctx sdk.Context, addr sdk.AccAddress, balances sdk.Coins) error

GetSendEnabled(ctx sdk.Context) bool
SetSendEnabled(ctx sdk.Context, enabled bool)
GetParams(ctx sdk.Context) types.Params
SetParams(ctx sdk.Context, params types.Params)
Copy link
Member

Choose a reason for hiding this comment

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

Why did we add SetParams to SendKeeper? This breaks ocaps even more. Just noting that this should be placed elsewhere at some point in the future.


SendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool
SendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error

BlockedAddr(addr sdk.AccAddress) bool
}
Expand Down Expand Up @@ -60,6 +63,17 @@ func NewBaseSendKeeper(
}
}

// GetParams returns the total set of bank parameters.
func (k BaseSendKeeper) GetParams(ctx sdk.Context) (params types.Params) {
k.paramSpace.GetParamSet(ctx, &params)
return params
}

// SetParams sets the total set of bank parameters.
func (k BaseSendKeeper) SetParams(ctx sdk.Context, params types.Params) {
k.paramSpace.SetParamSet(ctx, &params)
}

// InputOutputCoins performs multi-send functionality. It accepts a series of
// inputs that correspond to a series of outputs. It returns an error if the
// inputs and outputs don't lineup or if any single transfer of tokens fails.
Expand Down Expand Up @@ -256,16 +270,21 @@ func (k BaseSendKeeper) SetBalance(ctx sdk.Context, addr sdk.AccAddress, balance
return nil
}

// GetSendEnabled returns the current SendEnabled
func (k BaseSendKeeper) GetSendEnabled(ctx sdk.Context) bool {
var enabled bool
k.paramSpace.Get(ctx, types.ParamStoreKeySendEnabled, &enabled)
return enabled
// SendEnabledCoins checks the coins provide and returns an ErrSendDisabled if
// any of the coins are not configured for sending. Returns nil if sending is enabled
// for all provided coin
func (k BaseSendKeeper) SendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error {
for _, coin := range coins {
if !k.SendEnabledCoin(ctx, coin) {
return sdkerrors.Wrapf(types.ErrSendDisabled, "%s transfers are currently disabled", coin.Denom)
}
}
return nil
}

// SetSendEnabled sets the send enabled
func (k BaseSendKeeper) SetSendEnabled(ctx sdk.Context, enabled bool) {
k.paramSpace.Set(ctx, types.ParamStoreKeySendEnabled, &enabled)
// SendEnabledCoin returns the current SendEnabled status of the provided coin's denom
func (k BaseSendKeeper) SendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool {
return k.GetParams(ctx).SendEnabledDenom(coin.Denom)
}

// BlockedAddr checks if a given address is restricted from
Expand Down
52 changes: 41 additions & 11 deletions x/bank/simulation/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,36 @@ package simulation
// DONTCOVER

import (
"fmt"
"math/rand"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/bank/types"
)

// Simulation parameter constants
const (
SendEnabled = "send_enabled"
)
// RandomGenesisDefaultSendParam computes randomized allow all send transfers param for the bank module
func RandomGenesisDefaultSendParam(r *rand.Rand) bool {
// 90% chance of transfers being enable or P(a) = 0.9 for success
return r.Int63n(101) <= 90
}

// GenSendEnabled randomized SendEnabled
func GenSendEnabled(r *rand.Rand) bool {
return r.Int63n(101) <= 95 // 95% chance of transfers being enabled
// RandomGenesisSendParams randomized Parameters for the bank module
func RandomGenesisSendParams(r *rand.Rand) types.SendEnabledParams {
params := types.DefaultParams()
// 90% chance of transfers being DefaultSendEnabled=true or P(a) = 0.9 for success
// 50% of the time add an additional denom specific record (P(b) = 0.475 = 0.5 * 0.95)
if r.Int63n(101) <= 50 {
// set send enabled 95% of the time
bondEnabled := r.Int63n(101) <= 95
params = params.SetSendEnabledParam(
sdk.DefaultBondDenom,
bondEnabled)
}

// overall probability of enabled for bond denom is 94.75% (P(a)+P(b) - P(a)*P(b))
return params.SendEnabled
}

// RandomGenesisAccounts returns a slice of account balances. Each account has
Expand All @@ -37,16 +52,31 @@ func RandomGenesisBalances(simState *module.SimulationState) []types.Balance {

// RandomizedGenState generates a random GenesisState for bank
func RandomizedGenState(simState *module.SimulationState) {
var sendEnabled bool
var sendEnabledParams types.SendEnabledParams
simState.AppParams.GetOrGenerate(
simState.Cdc, SendEnabled, &sendEnabled, simState.Rand,
func(r *rand.Rand) { sendEnabled = GenSendEnabled(r) },
simState.Cdc, string(types.KeySendEnabled), &sendEnabledParams, simState.Rand,
func(r *rand.Rand) { sendEnabledParams = RandomGenesisSendParams(r) },
)

var defaultSendEnabledParam bool
simState.AppParams.GetOrGenerate(
simState.Cdc, string(types.KeyDefaultSendEnabled), &defaultSendEnabledParam, simState.Rand,
func(r *rand.Rand) { defaultSendEnabledParam = RandomGenesisDefaultSendParam(r) },
)

numAccs := int64(len(simState.Accounts))
totalSupply := sdk.NewInt(simState.InitialStake * (numAccs + simState.NumBonded))
supply := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, totalSupply))

bankGenesis := types.NewGenesisState(sendEnabled, RandomGenesisBalances(simState), supply)
bankGenesis := types.GenesisState{
Params: types.Params{
SendEnabled: sendEnabledParams,
DefaultSendEnabled: defaultSendEnabledParam,
},
Balances: RandomGenesisBalances(simState),
Supply: supply,
}

fmt.Printf("Selected randomly generated bank parameters:\n%s\n", codec.MustMarshalJSONIndent(simState.Cdc, bankGenesis.Params))
simState.GenState[types.ModuleName] = simState.Cdc.MustMarshalJSON(bankGenesis)
}
17 changes: 9 additions & 8 deletions x/bank/simulation/operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ func SimulateMsgSend(ak types.AccountKeeper, bk keeper.Keeper) simtypes.Operatio
r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context,
accs []simtypes.Account, chainID string,
) (simtypes.OperationMsg, []simtypes.FutureOperation, error) {
simAccount, toSimAcc, coins, skip := randomSendFields(r, ctx, accs, bk, ak)

if !bk.GetSendEnabled(ctx) {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgSend, "transfers are not enabled"), nil, nil
// Check send_enabled status of each coin denom
if err := bk.SendEnabledCoins(ctx, coins...); err != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgSend, err.Error()), nil, nil
}

simAccount, toSimAcc, coins, skip := randomSendFields(r, ctx, accs, bk, ak)

if skip {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgSend, "skip all transfers"), nil, nil
}
Expand Down Expand Up @@ -130,10 +130,6 @@ func SimulateMsgMultiSend(ak types.AccountKeeper, bk keeper.Keeper) simtypes.Ope
accs []simtypes.Account, chainID string,
) (simtypes.OperationMsg, []simtypes.FutureOperation, error) {

if !bk.GetSendEnabled(ctx) {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgMultiSend, "transfers are not enabled"), nil, nil
}

// random number of inputs/outputs between [1, 3]
inputs := make([]types.Input, r.Intn(3)+1)
outputs := make([]types.Output, r.Intn(3)+1)
Expand Down Expand Up @@ -169,6 +165,11 @@ func SimulateMsgMultiSend(ak types.AccountKeeper, bk keeper.Keeper) simtypes.Ope
totalSentCoins = totalSentCoins.Add(coins...)
}

// Check send_enabled status of each sent coin denom
if err := bk.SendEnabledCoins(ctx, totalSentCoins...); err != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgMultiSend, err.Error()), nil, nil
}

for o := range outputs {
outAddr, _ := simtypes.RandomAcc(r, accs)

Expand Down
11 changes: 7 additions & 4 deletions x/bank/simulation/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,18 @@ import (
"github.com/cosmos/cosmos-sdk/x/bank/types"
)

const keySendEnabled = "sendenabled"

// ParamChanges defines the parameters that can be modified by param change proposals
// on the simulation
func ParamChanges(r *rand.Rand) []simtypes.ParamChange {
return []simtypes.ParamChange{
simulation.NewSimParamChange(types.ModuleName, keySendEnabled,
simulation.NewSimParamChange(types.ModuleName, string(types.KeySendEnabled),
func(r *rand.Rand) string {
return fmt.Sprintf("%s", types.ModuleCdc.MustMarshalJSON(RandomGenesisSendParams(r)))
},
),
simulation.NewSimParamChange(types.ModuleName, string(types.KeyDefaultSendEnabled),
func(r *rand.Rand) string {
return fmt.Sprintf("%v", GenSendEnabled(r))
return fmt.Sprintf("%v", RandomGenesisDefaultSendParam(r))
},
),
}
Expand Down
19 changes: 16 additions & 3 deletions x/bank/spec/05_params.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,20 @@ order: 5

The bank module contains the following parameters:

| Key | Type | Example |
|-------------|------|---------|
| sendenabled | bool | true |
| Key | Type | Example |
|--------------------|---------------|------------------------------------|
| SendEnabled | []SendEnabled | [{denom: "stake", enabled: true }] |
| DefaultSendEnabled | bool | true |


## SendEnabled

The send enabled parameter is an array of SendEnabled entries mapping coin
denominations to their send_enabled status. Entries in this list take
precedence over the `DefaultSendEnabled` setting.

## DefaultSendEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really convinced this is necessary or provides a cleaner UX. What is the drawback to just using SendEnabled? I can see people getting confused over this IMHO.


The default send enabled value controls send transfer capability for all
coin denominations unless specifically included in the array of `SendEnabled`
parameters.
Loading