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.

Sign up for GitHub

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

Jump to bottom

custom-diff: x/bank #5

Closed
wants to merge 1 commit into from
Closed

custom-diff: x/bank #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 docs/core/proto-docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1695,6 +1695,7 @@ GenesisState defines the bank module's genesis state.
| `balances` | [Balance](#cosmos.bank.v1beta1.Balance) | repeated | balances is an array containing the balances of all the accounts. |
| `supply` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) | repeated | supply represents the total supply. If it is left empty, then supply will be calculated based on the provided balances. Otherwise, it will be used to validate that the sum of the balances equals this amount. |
| `denom_metadata` | [Metadata](#cosmos.bank.v1beta1.Metadata) | repeated | denom_metadata defines the metadata of the differents coins. |
| `blocked_addrs` | [string](#string) | repeated | blocked_addrs is an array containing the blocked addresses |
Copy link

Choose a reason for hiding this comment

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

Suggested change
| `blocked_addrs` | [string](#string) | repeated | blocked_addrs is an array containing the blocked addresses |
| `blocked_addrs` | [string](#string) | repeated | blocked_addrs is an array containing the blocked addresses. |




Expand Down
3 changes: 3 additions & 0 deletions proto/cosmos/bank/v1beta1/genesis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ message GenesisState {

// denom_metadata defines the metadata of the differents coins.
repeated Metadata denom_metadata = 4 [(gogoproto.moretags) = "yaml:\"denom_metadata\"", (gogoproto.nullable) = false];

// blocked_addrs is an array containing the blocked addresses
repeated string blocked_addrs = 5 [(gogoproto.moretags) = "yaml:\"blocked_addrs\"", (gogoproto.nullable) = false];
Copy link

Choose a reason for hiding this comment

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

I suggest another name: blocked_addresses, since using full Address instead of abbreviated Addr seems the sdk's convention.

}

// Balance defines an account address and balance pair used in the bank module's
Expand Down
18 changes: 16 additions & 2 deletions simapp/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,29 @@ func TestSimAppExportAndBlockedAddrs(t *testing.T) {
encCfg := MakeTestEncodingConfig()
db := dbm.NewMemDB()
app := NewSimApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, map[int64]bool{}, DefaultNodeHome, 0, encCfg, EmptyAppOptions{})

ctx := app.NewContext(true, tmproto.Header{Height: app.LastBlockHeight()})
for acc := range maccPerms {
require.True(
t,
app.BankKeeper.BlockedAddr(app.AccountKeeper.GetModuleAddress(acc)),
app.BankKeeper.BlockedAddr(ctx, app.AccountKeeper.GetModuleAddress(acc)),
"ensure that blocked addresses are properly set in bank keeper",
)
}

sampleAddr := "cosmos1qf3v4kns89qg42xwqhek5cmjw9fsr0ssy7z0jwcjy2dgz6pvjnyq0xf9dk"
dynamicBlockedAddr, err := sdk.AccAddressFromBech32(sampleAddr)
require.NoError(t, err)
res := app.BankKeeper.BlockedAddr(ctx, dynamicBlockedAddr)
require.False(t, res)
app.BankKeeper.AddBlockedAddr(ctx, dynamicBlockedAddr)
res = app.BankKeeper.BlockedAddr(ctx, dynamicBlockedAddr)
require.True(t, res)
blockedAddrs := app.BankKeeper.GetAllBlockedAddrs(ctx)
require.Equal(t, blockedAddrs, []string{sampleAddr})
app.BankKeeper.RemoveBlockedAddr(ctx, dynamicBlockedAddr)
res = app.BankKeeper.BlockedAddr(ctx, dynamicBlockedAddr)
require.False(t, res)

genesisState := NewDefaultGenesisState(encCfg.Marshaler)
stateBytes, err := json.MarshalIndent(genesisState, "", " ")
require.NoError(t, err)
Expand Down
4 changes: 2 additions & 2 deletions simapp/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func SetupWithGenesisValSet(t *testing.T, valSet *tmtypes.ValidatorSet, genAccs
})

// update total supply
bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, totalSupply, []banktypes.Metadata{})
bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, totalSupply, []banktypes.Metadata{}, []string{})
genesisState[banktypes.ModuleName] = app.AppCodec().MustMarshalJSON(bankGenesis)

stateBytes, err := json.MarshalIndent(genesisState, "", " ")
Expand Down Expand Up @@ -177,7 +177,7 @@ func SetupWithGenesisAccounts(genAccs []authtypes.GenesisAccount, balances ...ba
totalSupply = totalSupply.Add(b.Coins...)
}

bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, totalSupply, []banktypes.Metadata{})
bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, totalSupply, []banktypes.Metadata{}, []string{})
genesisState[banktypes.ModuleName] = app.AppCodec().MustMarshalJSON(bankGenesis)

stateBytes, err := json.MarshalIndent(genesisState, "", " ")
Expand Down
2 changes: 1 addition & 1 deletion x/auth/vesting/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (s msgServer) CreateVestingAccount(goCtx context.Context, msg *types.MsgCre
return nil, err
}

if bk.BlockedAddr(to) {
if bk.BlockedAddr(ctx, to) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", msg.ToAddress)
}

Expand Down
2 changes: 1 addition & 1 deletion x/auth/vesting/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ import (
type BankKeeper interface {
IsSendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error
SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
BlockedAddr(addr sdk.AccAddress) bool
BlockedAddr(ctx sdk.Context, addr sdk.AccAddress) bool
}
9 changes: 9 additions & 0 deletions x/bank/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ func (k BaseKeeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
for _, meta := range genState.DenomMetadata {
k.SetDenomMetaData(ctx, meta)
}

for _, addr := range genState.BlockedAddrs {
acc, err := sdk.AccAddressFromBech32(addr)
if err != nil {
panic(err)
}
k.AddBlockedAddr(ctx, acc)
}
}

// ExportGenesis returns the bank module's genesis state.
Expand All @@ -53,5 +61,6 @@ func (k BaseKeeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
k.GetAccountsBalances(ctx),
totalSupply,
k.GetAllDenomMetaData(ctx),
k.GetAllBlockedAddrs(ctx),
)
}
6 changes: 3 additions & 3 deletions x/bank/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,17 +83,17 @@ func (suite *IntegrationTestSuite) TestTotalSupply() {
}{
{
"calculation NOT matching genesis Supply field",
types.NewGenesisState(defaultGenesis.Params, balances, sdk.NewCoins(sdk.NewCoin("wrongcoin", sdk.NewInt(1))), defaultGenesis.DenomMetadata),
types.NewGenesisState(defaultGenesis.Params, balances, sdk.NewCoins(sdk.NewCoin("wrongcoin", sdk.NewInt(1))), defaultGenesis.DenomMetadata, defaultGenesis.BlockedAddrs),
nil, true, "genesis supply is incorrect, expected 1wrongcoin, got 21barcoin,11foocoin",
},
{
"calculation matches genesis Supply field",
types.NewGenesisState(defaultGenesis.Params, balances, totalSupply, defaultGenesis.DenomMetadata),
types.NewGenesisState(defaultGenesis.Params, balances, totalSupply, defaultGenesis.DenomMetadata, defaultGenesis.BlockedAddrs),
totalSupply, false, "",
},
{
"calculation is correct, empty genesis Supply field",
types.NewGenesisState(defaultGenesis.Params, balances, nil, defaultGenesis.DenomMetadata),
types.NewGenesisState(defaultGenesis.Params, balances, nil, defaultGenesis.DenomMetadata, defaultGenesis.BlockedAddrs),
totalSupply, false, "",
},
}
Expand Down
2 changes: 1 addition & 1 deletion x/bank/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func (k BaseKeeper) SendCoinsFromModuleToAccount(
panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
}

if k.BlockedAddr(recipientAddr) {
if k.BlockedAddr(ctx, recipientAddr) {
return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", recipientAddr)
}

Expand Down
4 changes: 2 additions & 2 deletions x/bank/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (k msgServer) Send(goCtx context.Context, msg *types.MsgSend) (*types.MsgSe
return nil, err
}

if k.BlockedAddr(to) {
if k.BlockedAddr(ctx, to) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", msg.ToAddress)
}

Expand Down Expand Up @@ -85,7 +85,7 @@ func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*t
if err != nil {
panic(err)
}
if k.BlockedAddr(accAddr) {
if k.BlockedAddr(ctx, accAddr) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive transactions", out.Address)
}
}
Expand Down
33 changes: 30 additions & 3 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ type SendKeeper interface {
IsSendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool
IsSendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error

BlockedAddr(addr sdk.AccAddress) bool
BlockedAddr(ctx sdk.Context, addr sdk.AccAddress) bool
GetAllBlockedAddrs(ctx sdk.Context) (blockedAddrs []string)
AddBlockedAddr(ctx sdk.Context, addr sdk.AccAddress)
RemoveBlockedAddr(ctx sdk.Context, addr sdk.AccAddress)
}

var _ SendKeeper = (*BaseSendKeeper)(nil)
Expand Down Expand Up @@ -285,6 +288,30 @@ func (k BaseSendKeeper) IsSendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool {

// BlockedAddr checks if a given address is restricted from
// receiving funds.
func (k BaseSendKeeper) BlockedAddr(addr sdk.AccAddress) bool {
return k.blockedAddrs[addr.String()]
func (k BaseSendKeeper) BlockedAddr(ctx sdk.Context, addr sdk.AccAddress) bool {
return k.blockedAddrs[addr.String()] || ctx.KVStore(k.storeKey).Has(types.BlockedAddrKey(addr))
}

// GetAllBlockedAddrs returns a list of blocked addresses.
func (k BaseSendKeeper) GetAllBlockedAddrs(ctx sdk.Context) (blockedAddrs []string) {
Copy link

Choose a reason for hiding this comment

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

I think returning []sdk.AccAddress is more intuitive.

store := ctx.KVStore(k.storeKey)
iter := sdk.KVStorePrefixIterator(store, types.BlockedAddrsPrefix)
defer iter.Close()
for ; iter.Valid(); iter.Next() {
blockedAddrs = append(blockedAddrs, types.ParseBlockedAddrKey(iter.Key()).String())
}
return
}

// AddBlockedAddr add a given address to blocked address map to be restricted from receiving funds.
func (k BaseSendKeeper) AddBlockedAddr(ctx sdk.Context, addr sdk.AccAddress) {
ctx.KVStore(k.storeKey).Set(types.BlockedAddrKey(addr), []byte{})
}

// RemoveBlockedAddr delete a given address to blocked address map.
func (k BaseSendKeeper) RemoveBlockedAddr(ctx sdk.Context, addr sdk.AccAddress) {
store := ctx.KVStore(k.storeKey)
if store.Has(types.BlockedAddrKey(addr)) {
Copy link

Choose a reason for hiding this comment

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

No need to check if the key presents, since store.Delete will do nothing if the key doesn't exist.

store.Delete(types.BlockedAddrKey(addr))
}
}
2 changes: 1 addition & 1 deletion x/bank/legacy/v040/migrate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestMigrate(t *testing.T) {
}

migrated := v040bank.Migrate(bankGenState, authGenState, supplyGenState)
expected := `{"params":{"send_enabled":[],"default_send_enabled":true},"balances":[{"address":"cosmos1xxkueklal9vejv9unqu80w9vptyepfa95pd53u","coins":[{"denom":"stake","amount":"50"}]},{"address":"cosmos15v50ymp6n5dn73erkqtmq0u8adpl8d3ujv2e74","coins":[{"denom":"stake","amount":"50"}]}],"supply":[{"denom":"stake","amount":"1000"}],"denom_metadata":[]}`
expected := `{"params":{"send_enabled":[],"default_send_enabled":true},"balances":[{"address":"cosmos1xxkueklal9vejv9unqu80w9vptyepfa95pd53u","coins":[{"denom":"stake","amount":"50"}]},{"address":"cosmos15v50ymp6n5dn73erkqtmq0u8adpl8d3ujv2e74","coins":[{"denom":"stake","amount":"50"}]}],"supply":[{"denom":"stake","amount":"1000"}],"denom_metadata":[],"blocked_addrs":[]}`

bz, err := clientCtx.Codec.MarshalJSON(migrated)
require.NoError(t, err)
Expand Down
1 change: 1 addition & 0 deletions x/bank/legacy/v043/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func TestMigrateJSON(t *testing.T) {
]
}
],
"blocked_addrs": [],
"denom_metadata": [],
"params": {
"default_send_enabled": false,
Expand Down
7 changes: 5 additions & 2 deletions x/bank/spec/02_keepers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Typically, these addresses are module accounts. If these addresses receive funds
outside the expected rules of the state machine, invariants are likely to be
broken and could result in a halted network.

By providing the `x/bank` module with a blocklisted set of addresses, an error occurs for the operation if a user or client attempts to directly or indirectly send funds to a blocklisted account, for example, by using [IBC](http://docs.cosmos.network/master/ibc/).
By providing the `x/bank` module with a blocklisted set of addresses or calling `AddBlockedAddr`, `RemoveBlockedAddr` to update blocked addresses, an error occurs for the operation if a user or client attempts to directly or indirectly send funds to a blocklisted account, for example, by using [IBC](http://docs.cosmos.network/master/ibc/).

## Common Types

Expand Down Expand Up @@ -105,7 +105,10 @@ type SendKeeper interface {
IsSendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool
IsSendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error

BlockedAddr(addr sdk.AccAddress) bool
BlockedAddr(ctx sdk.Context, addr sdk.AccAddress) bool
GetAllBlockedAddrs(ctx sdk.Context) (blockedAddrs []string)
AddBlockedAddr(ctx sdk.Context, addr sdk.AccAddress)
RemoveBlockedAddr(ctx sdk.Context, addr sdk.AccAddress)
}
```

Expand Down
10 changes: 8 additions & 2 deletions x/bank/types/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,28 @@ func (gs GenesisState) Validate() error {
}
}

for _, addr := range gs.BlockedAddrs {
if _, err := sdk.AccAddressFromBech32(addr); err != nil {
return err
}
}
return nil
}

// NewGenesisState creates a new genesis state.
func NewGenesisState(params Params, balances []Balance, supply sdk.Coins, denomMetaData []Metadata) *GenesisState {
func NewGenesisState(params Params, balances []Balance, supply sdk.Coins, denomMetaData []Metadata, blockedAddrs []string) *GenesisState {
Copy link

Choose a reason for hiding this comment

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

How about accepting blockedAddrs []sdk.AccAddress?

Copy link

Choose a reason for hiding this comment

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

Ref: https://github.com/cosmos/cosmos-sdk/blob/4081bc3d0080644541ba3de34fbe1bde27779565/x/distribution/types/genesis.go#L9

Copy link
Author

Choose a reason for hiding this comment

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

BankKeeper and others are receiving BlockedAddr as string, so string was used to follow this format as much as possible.

Ref. https://github.com/cosmos/cosmos-sdk/blob/4081bc3d0080644541ba3de34fbe1bde27779565/x/bank/keeper/keeper.go%23L96-L110

Copy link

Choose a reason for hiding this comment

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

I think the original version used string, not sdk.AccAddress because it is used as key of a map.
Since sdk.AccAddress is []byte, the original author might thought that it's not suitable for map keys.
After changing the parameter type from map to slice, we can safely use []sdk.AccAddress again.
But as you said, to keep consistency between the original one, it's still possible to use []string as well.

return &GenesisState{
Params: params,
Balances: balances,
Supply: supply,
DenomMetadata: denomMetaData,
BlockedAddrs: blockedAddrs,
}
}

// DefaultGenesisState returns a default bank module genesis state.
func DefaultGenesisState() *GenesisState {
return NewGenesisState(DefaultParams(), []Balance{}, sdk.Coins{}, []Metadata{})
return NewGenesisState(DefaultParams(), []Balance{}, sdk.Coins{}, []Metadata{}, []string{})
}

// GetGenesisStateFromAppState returns x/bank GenesisState given raw application
Expand Down
Loading