x/bank: balance and metadata validation (#8417)

* x/bank: balance validation

* metadata validation

* minor change

* balance and metadata tests

* changelog

* check for empty coins

* genesis test

CHANGELOG.md

@@ -38,10 +38,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
-* (x/ibc) [\#8404](https://github.com/cosmos/cosmos-sdk/pull/8404) Reorder IBC `ChanOpenAck` and `ChanOpenConfirm` handler execution to perform core handler first, followed by application callbacks. 
+* (x/ibc) [\#8404](https://github.com/cosmos/cosmos-sdk/pull/8404) Reorder IBC `ChanOpenAck` and `ChanOpenConfirm` handler execution to perform core handler first, followed by application callbacks.
 
 ### Bug Fixes
 
+* (x/bank) [\#8417](https://github.com/cosmos/cosmos-sdk/pull/8417) Validate balances and coin denom metadata on genesis
 * (server) [\#8399](https://github.com/cosmos/cosmos-sdk/pull/8399) fix gRPC-web flag default value
 
 ## [v0.40.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.1) - 2021-01-19

x/bank/module.go

@@ -58,7 +58,7 @@ func (AppModuleBasic) ValidateGenesis(cdc codec.JSONMarshaler, _ client.TxEncodi
 		return fmt.Errorf("failed to unmarshal %s genesis state: %w", types.ModuleName, err)
 	}
 
-	return types.ValidateGenesis(data)
+	return data.Validate()
 }
 
 // RegisterRESTRoutes registers the REST routes for the bank module.

x/bank/types/balance.go

@@ -0,0 +1,93 @@
+package types
+
+import (
+	"bytes"
+	"encoding/json"
+	fmt "fmt"
+	"sort"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/bank/exported"
+)
+
+var _ exported.GenesisBalance = (*Balance)(nil)
+
+// GetAddress returns the account address of the Balance object.
+func (b Balance) GetAddress() sdk.AccAddress {
+	addr, _ := sdk.AccAddressFromBech32(b.Address)
+	return addr
+}
+
+// GetCoins returns the account coins of the Balance object.
+func (b Balance) GetCoins() sdk.Coins {
+	return b.Coins
+}
+
+// Validate checks for address and coins correctness.
+func (b Balance) Validate() error {
+	_, err := sdk.AccAddressFromBech32(b.Address)
+	if err != nil {
+		return err
+	}
+
+	if len(b.Coins) == 0 {
+		return fmt.Errorf("empty or nil coins for address %s", b.Address)
+	}
+
+	seenDenoms := make(map[string]bool)
+
+	// NOTE: we perform a custom validation since the coins.Validate function
+	// errors on zero balance coins
+	for _, coin := range b.Coins {
+		if seenDenoms[coin.Denom] {
+			return fmt.Errorf("duplicate denomination %s", coin.Denom)
+		}
+
+		if err := sdk.ValidateDenom(coin.Denom); err != nil {
+			return err
+		}
+
+		if coin.IsNegative() {
+			return fmt.Errorf("coin %s amount is cannot be negative", coin.Denom)
+		}
+
+		seenDenoms[coin.Denom] = true
+	}
+
+	// sort the coins post validation
+	b.Coins = b.Coins.Sort()
+
+	return nil
+}
+
+// SanitizeGenesisBalances sorts addresses and coin sets.
+func SanitizeGenesisBalances(balances []Balance) []Balance {
+	sort.Slice(balances, func(i, j int) bool {
+		addr1, _ := sdk.AccAddressFromBech32(balances[i].Address)
+		addr2, _ := sdk.AccAddressFromBech32(balances[j].Address)
+		return bytes.Compare(addr1.Bytes(), addr2.Bytes()) < 0
+	})
+
+	for _, balance := range balances {
+		balance.Coins = balance.Coins.Sort()
+	}
+
+	return balances
+}
+
+// GenesisBalancesIterator implements genesis account iteration.
+type GenesisBalancesIterator struct{}
+
+// IterateGenesisBalances iterates over all the genesis balances found in
+// appGenesis and invokes a callback on each genesis account. If any call
+// returns true, iteration stops.
+func (GenesisBalancesIterator) IterateGenesisBalances(
+	cdc codec.JSONMarshaler, appState map[string]json.RawMessage, cb func(exported.GenesisBalance) (stop bool),
+) {
+	for _, balance := range GetGenesisStateFromAppState(cdc, appState).Balances {
+		if cb(balance) {
+			break
+		}
+	}
+}

x/bank/types/balance_test.go

@@ -0,0 +1,80 @@
+package types
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+func TestBalanceValidate(t *testing.T) {
+
+	testCases := []struct {
+		name    string
+		balance Balance
+		expErr  bool
+	}{
+		{
+			"valid balance",
+			Balance{
+				Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+				Coins:   sdk.Coins{sdk.NewInt64Coin("uatom", 1)},
+			},
+			false,
+		},
+		{"empty balance", Balance{}, true},
+		{
+			"nil balance coins",
+			Balance{
+				Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+			},
+			true,
+		},
+		{
+			"dup coins",
+			Balance{
+				Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+				Coins: sdk.Coins{
+					sdk.NewInt64Coin("uatom", 1),
+					sdk.NewInt64Coin("uatom", 1),
+				},
+			},
+			true,
+		},
+		{
+			"invalid coin denom",
+			Balance{
+				Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+				Coins: sdk.Coins{
+					sdk.Coin{Denom: "", Amount: sdk.OneInt()},
+				},
+			},
+			true,
+		},
+		{
+			"negative coin",
+			Balance{
+				Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+				Coins: sdk.Coins{
+					sdk.Coin{Denom: "uatom", Amount: sdk.NewInt(-1)},
+				},
+			},
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+
+			err := tc.balance.Validate()
+
+			if tc.expErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}

x/bank/types/genesis.go

@@ -1,51 +1,49 @@
 package types
 
 import (
-	"bytes"
 	"encoding/json"
-	"sort"
+	"fmt"
 
 	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/cosmos/cosmos-sdk/x/bank/exported"
 )
 
-var _ exported.GenesisBalance = (*Balance)(nil)
-
-// GetAddress returns the account address of the Balance object.
-func (b Balance) GetAddress() sdk.AccAddress {
-	addr1, _ := sdk.AccAddressFromBech32(b.Address)
-	return addr1
-}
-
-// GetAddress returns the account coins of the Balance object.
-func (b Balance) GetCoins() sdk.Coins {
-	return b.Coins
-}
-
-// SanitizeGenesisAccounts sorts addresses and coin sets.
-func SanitizeGenesisBalances(balances []Balance) []Balance {
-	sort.Slice(balances, func(i, j int) bool {
-		addr1, _ := sdk.AccAddressFromBech32(balances[i].Address)
-		addr2, _ := sdk.AccAddressFromBech32(balances[j].Address)
-		return bytes.Compare(addr1.Bytes(), addr2.Bytes()) < 0
-	})
-
-	for _, balance := range balances {
-		balance.Coins = balance.Coins.Sort()
-	}
-
-	return balances
-}
-
-// ValidateGenesis performs basic validation of supply genesis data returning an
+// Validate performs basic validation of supply genesis data returning an
 // error for any failed validation criteria.
-func ValidateGenesis(data GenesisState) error {
-	if err := data.Params.Validate(); err != nil {
+func (gs GenesisState) Validate() error {
+	if err := gs.Params.Validate(); err != nil {
 		return err
 	}
 
-	return NewSupply(data.Supply).ValidateBasic()
+	seenBalances := make(map[string]bool)
+	seenMetadatas := make(map[string]bool)
+
+	for _, balance := range gs.Balances {
+		if seenBalances[balance.Address] {
+			return fmt.Errorf("duplicate balance for address %s", balance.Address)
+		}
+
+		if err := balance.Validate(); err != nil {
+			return err
+		}
+
+		seenBalances[balance.Address] = true
+	}
+
+	for _, metadata := range gs.DenomMetadata {
+		if seenMetadatas[metadata.Base] {
+			return fmt.Errorf("duplicate client metadata for denom %s", metadata.Base)
+		}
+
+		if err := metadata.Validate(); err != nil {
+			return err
+		}
+
+		seenMetadatas[metadata.Base] = true
+	}
+
+	// NOTE: this errors if supply for any given coin is zero
+	return NewSupply(gs.Supply).ValidateBasic()
 }
 
 // NewGenesisState creates a new genesis state.
@@ -74,19 +72,3 @@ func GetGenesisStateFromAppState(cdc codec.JSONMarshaler, appState map[string]js
 
 	return &genesisState
 }
-
-// GenesisAccountIterator implements genesis account iteration.
-type GenesisBalancesIterator struct{}
-
-// IterateGenesisAccounts iterates over all the genesis accounts found in
-// appGenesis and invokes a callback on each genesis account. If any call
-// returns true, iteration stops.
-func (GenesisBalancesIterator) IterateGenesisBalances(
-	cdc codec.JSONMarshaler, appState map[string]json.RawMessage, cb func(exported.GenesisBalance) (stop bool),
-) {
-	for _, balance := range GetGenesisStateFromAppState(cdc, appState).Balances {
-		if cb(balance) {
-			break
-		}
-	}
-}

x/bank/types/genesis_test.go

@@ -1,52 +1,150 @@
-package types_test
+package types
 
 import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
 
-	"github.com/cosmos/cosmos-sdk/codec"
-	"github.com/cosmos/cosmos-sdk/x/bank/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
-func TestMarshalJSONMetaData(t *testing.T) {
-	cdc := codec.NewLegacyAmino()
+func TestGenesisStateValidate(t *testing.T) {
 
 	testCases := []struct {
-		name      string
-		input     []types.Metadata
-		strOutput string
+		name         string
+		genesisState GenesisState
+		expErr       bool
 	}{
-		{"nil metadata", nil, `null`},
-		{"empty metadata", []types.Metadata{}, `[]`},
-		{"non-empty coins", []types.Metadata{{
-			Description: "The native staking token of the Cosmos Hub.",
-			DenomUnits: []*types.DenomUnit{
-				{"uatom", uint32(0), []string{"microatom"}}, // The default exponent value 0 is omitted in the json
-				{"matom", uint32(3), []string{"milliatom"}},
-				{"atom", uint32(6), nil},
+		{
+			"valid genesisState",
+			GenesisState{
+				Params: DefaultParams(),
+				Balances: []Balance{
+					{
+						Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+						Coins:   sdk.Coins{sdk.NewInt64Coin("uatom", 1)},
+					},
+				},
+				Supply: sdk.Coins{sdk.NewInt64Coin("uatom", 1)},
+				DenomMetadata: []Metadata{
+					{
+						Description: "The native staking token of the Cosmos Hub.",
+						DenomUnits: []*DenomUnit{
+							{"uatom", uint32(0), []string{"microatom"}},
+							{"matom", uint32(3), []string{"milliatom"}},
+							{"atom", uint32(6), nil},
+						},
+						Base:    "uatom",
+						Display: "atom",
+					},
+				},
 			},
-			Base:    "uatom",
-			Display: "atom",
+			false,
 		},
+		{"empty genesisState", GenesisState{}, false},
+		{
+			"invalid params ",
+			GenesisState{
+				Params: Params{
+					SendEnabled: []*SendEnabled{
+						{"", true},
+					},
+				},
+			},
+			true,
+		},
+		{
+			"dup balances",
+			GenesisState{
+				Balances: []Balance{
+					{
+						Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+						Coins:   sdk.Coins{sdk.NewInt64Coin("uatom", 1)},
+					},
+					{
+						Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+						Coins:   sdk.Coins{sdk.NewInt64Coin("uatom", 1)},
+					},
+				},
+			},
+			true,
+		},
+		{
+			"invalid balance",
+			GenesisState{
+				Balances: []Balance{
+					{
+						Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t",
+					},
+				},
+			},
+			true,
+		},
+		{
+			"dup Metadata",
+			GenesisState{
+				DenomMetadata: []Metadata{
+					{
+						Description: "The native staking token of the Cosmos Hub.",
+						DenomUnits: []*DenomUnit{
+							{"uatom", uint32(0), []string{"microatom"}},
+							{"matom", uint32(3), []string{"milliatom"}},
+							{"atom", uint32(6), nil},
+						},
+						Base:    "uatom",
+						Display: "atom",
+					},
+					{
+						Description: "The native staking token of the Cosmos Hub.",
+						DenomUnits: []*DenomUnit{
+							{"uatom", uint32(0), []string{"microatom"}},
+							{"matom", uint32(3), []string{"milliatom"}},
+							{"atom", uint32(6), nil},
+						},
+						Base:    "uatom",
+						Display: "atom",
+					},
+				},
+			},
+			true,
+		},
+		{
+			"invalid Metadata",
+			GenesisState{
+				DenomMetadata: []Metadata{
+					{
+						Description: "The native staking token of the Cosmos Hub.",
+						DenomUnits: []*DenomUnit{
+							{"uatom", uint32(0), []string{"microatom"}},
+							{"matom", uint32(3), []string{"milliatom"}},
+							{"atom", uint32(6), nil},
+						},
+						Base:    "",
+						Display: "",
+					},
+				},
+			},
+			true,
+		},
+		{
+			"invalid supply",
+			GenesisState{
+				Supply: sdk.Coins{sdk.Coin{Denom: "", Amount: sdk.OneInt()}},
+			},
+			true,
 		},
-			`[{"description":"The native staking token of the Cosmos Hub.","denom_units":[{"denom":"uatom","aliases":["microatom"]},{"denom":"matom","exponent":3,"aliases":["milliatom"]},{"denom":"atom","exponent":6}],"base":"uatom","display":"atom"}]`},
 	}
 
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			bz, err := cdc.MarshalJSON(tc.input)
-			require.NoError(t, err)
-			require.Equal(t, tc.strOutput, string(bz))
 
-			var newMetadata []types.Metadata
-			require.NoError(t, cdc.UnmarshalJSON(bz, &newMetadata))
+			err := tc.genesisState.Validate()
 
-			if len(tc.input) == 0 {
-				require.Nil(t, newMetadata)
+			if tc.expErr {
+				require.Error(t, err)
 			} else {
-				require.Equal(t, tc.input, newMetadata)
+				require.NoError(t, err)
 			}
 		})
 	}

x/bank/types/metadata.go

@@ -0,0 +1,56 @@
+package types
+
+import (
+	"fmt"
+	"strings"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+// Validate performs a basic validation of the coin metadata fields
+func (m Metadata) Validate() error {
+	if err := sdk.ValidateDenom(m.Base); err != nil {
+		return fmt.Errorf("invalid metadata base denom: %w", err)
+	}
+
+	if err := sdk.ValidateDenom(m.Display); err != nil {
+		return fmt.Errorf("invalid metadata display denom: %w", err)
+	}
+
+	seenUnits := make(map[string]bool)
+	for _, denomUnit := range m.DenomUnits {
+		if seenUnits[denomUnit.Denom] {
+			return fmt.Errorf("duplicate denomination unit %s", denomUnit.Denom)
+		}
+
+		if err := denomUnit.Validate(); err != nil {
+			return err
+		}
+
+		seenUnits[denomUnit.Denom] = true
+	}
+
+	return nil
+}
+
+// Validate performs a basic validation of the denomination unit fields
+func (du DenomUnit) Validate() error {
+	if err := sdk.ValidateDenom(du.Denom); err != nil {
+		return fmt.Errorf("invalid denom unit: %w", err)
+	}
+
+	seenAliases := make(map[string]bool)
+	for _, alias := range du.Aliases {
+		if seenAliases[alias] {
+			return fmt.Errorf("duplicate denomination unit alias %s", alias)
+		}
+
+		if strings.TrimSpace(alias) == "" {
+			return fmt.Errorf("alias for denom unit %s cannot be blank", du.Denom)
+		}
+
+		seenAliases[alias] = true
+	}
+
+	return nil
+}

x/bank/types/metadata_test.go

@@ -0,0 +1,155 @@
+package types_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/x/bank/types"
+)
+
+func TestMetadataValidate(t *testing.T) {
+	testCases := []struct {
+		name     string
+		metadata types.Metadata
+		expErr   bool
+	}{
+		{
+			"non-empty coins",
+			types.Metadata{
+				Description: "The native staking token of the Cosmos Hub.",
+				DenomUnits: []*types.DenomUnit{
+					{"uatom", uint32(0), []string{"microatom"}},
+					{"matom", uint32(3), []string{"milliatom"}},
+					{"atom", uint32(6), nil},
+				},
+				Base:    "uatom",
+				Display: "atom",
+			},
+			false,
+		},
+		{"empty metadata", types.Metadata{}, true},
+		{
+			"invalid base denom",
+			types.Metadata{
+				Base: "",
+			},
+			true,
+		},
+		{
+			"invalid display denom",
+			types.Metadata{
+				Base:    "uatom",
+				Display: "",
+			},
+			true,
+		},
+		{
+			"duplicate denom unit",
+			types.Metadata{
+				Description: "The native staking token of the Cosmos Hub.",
+				DenomUnits: []*types.DenomUnit{
+					{"uatom", uint32(0), []string{"microatom"}},
+					{"uatom", uint32(0), []string{"microatom"}},
+				},
+				Base:    "uatom",
+				Display: "atom",
+			},
+			true,
+		},
+		{
+			"invalid denom unit",
+			types.Metadata{
+				Description: "The native staking token of the Cosmos Hub.",
+				DenomUnits: []*types.DenomUnit{
+					{"", uint32(0), []string{"microatom"}},
+				},
+				Base:    "uatom",
+				Display: "atom",
+			},
+			true,
+		},
+		{
+			"invalid denom unit alias",
+			types.Metadata{
+				Description: "The native staking token of the Cosmos Hub.",
+				DenomUnits: []*types.DenomUnit{
+					{"uatom", uint32(0), []string{""}},
+				},
+				Base:    "uatom",
+				Display: "atom",
+			},
+			true,
+		},
+		{
+			"duplicate denom unit alias",
+			types.Metadata{
+				Description: "The native staking token of the Cosmos Hub.",
+				DenomUnits: []*types.DenomUnit{
+					{"uatom", uint32(0), []string{"microatom", "microatom"}},
+				},
+				Base:    "uatom",
+				Display: "atom",
+			},
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+
+			err := tc.metadata.Validate()
+
+			if tc.expErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestMarshalJSONMetaData(t *testing.T) {
+	cdc := codec.NewLegacyAmino()
+
+	testCases := []struct {
+		name      string
+		input     []types.Metadata
+		strOutput string
+	}{
+		{"nil metadata", nil, `null`},
+		{"empty metadata", []types.Metadata{}, `[]`},
+		{"non-empty coins", []types.Metadata{{
+			Description: "The native staking token of the Cosmos Hub.",
+			DenomUnits: []*types.DenomUnit{
+				{"uatom", uint32(0), []string{"microatom"}}, // The default exponent value 0 is omitted in the json
+				{"matom", uint32(3), []string{"milliatom"}},
+				{"atom", uint32(6), nil},
+			},
+			Base:    "uatom",
+			Display: "atom",
+		},
+		},
+			`[{"description":"The native staking token of the Cosmos Hub.","denom_units":[{"denom":"uatom","aliases":["microatom"]},{"denom":"matom","exponent":3,"aliases":["milliatom"]},{"denom":"atom","exponent":6}],"base":"uatom","display":"atom"}]`},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			bz, err := cdc.MarshalJSON(tc.input)
+			require.NoError(t, err)
+			require.Equal(t, tc.strOutput, string(bz))
+
+			var newMetadata []types.Metadata
+			require.NoError(t, cdc.UnmarshalJSON(bz, &newMetadata))
+
+			if len(tc.input) == 0 {
+				require.Nil(t, newMetadata)
+			} else {
+				require.Equal(t, tc.input, newMetadata)
+			}
+		})
+	}
+}

x/bank/types/supply.go

@@ -50,8 +50,8 @@ func (supply Supply) String() string {
 
 // ValidateBasic validates the Supply coins and returns error if invalid
 func (supply Supply) ValidateBasic() error {
-	if !supply.Total.IsValid() {
-		return fmt.Errorf("invalid total supply: %s", supply.Total.String())
+	if err := supply.Total.Validate(); err != nil {
+		return fmt.Errorf("invalid total supply: %w", err)
 	}
 
 	return nil