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

stargate: balance and metadata validation #8421

Merged
merged 2 commits into from
Jan 26, 2021
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ Ref: https://keepachangelog.com/en/1.0.0/

# Changelog

## [Unreleased]

### Bug Fixes

* (x/bank) [\#8417](https://github.com/cosmos/cosmos-sdk/pull/8417) Validate balances and coin denom metadata on genesis

## [v0.40.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.1) - 2021-01-19

### Improvements
Expand Down
2 changes: 1 addition & 1 deletion x/bank/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
93 changes: 93 additions & 0 deletions x/bank/types/balance.go
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we ignoring the error from here? Feels like we should be panicking instead.

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)
}
Comment on lines +47 to +53
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should put the cheaper call before to avoid wasted CPU cycles and from look at the code flow, most likely coins will be negative than to be within this code path and have invalid denominations.


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
}
}
}
80 changes: 80 additions & 0 deletions x/bank/types/balance_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}
74 changes: 28 additions & 46 deletions x/bank/types/genesis.go
Original file line number Diff line number Diff line change
@@ -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)
// Validate performs basic validation of supply genesis data returning an
// error for any failed validation criteria.
func (gs GenesisState) Validate() error {
if err := gs.Params.Validate(); err != nil {
return err
}

// GetAddress returns the account address of the Balance object.
func (b Balance) GetAddress() sdk.AccAddress {
addr1, _ := sdk.AccAddressFromBech32(b.Address)
return addr1
}
seenBalances := make(map[string]bool)
seenMetadatas := make(map[string]bool)

// GetAddress returns the account coins of the Balance object.
func (b Balance) GetCoins() sdk.Coins {
return b.Coins
}
for _, balance := range gs.Balances {
if seenBalances[balance.Address] {
return fmt.Errorf("duplicate balance for address %s", balance.Address)
}

// 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
})
if err := balance.Validate(); err != nil {
return err
}

for _, balance := range balances {
balance.Coins = balance.Coins.Sort()
seenBalances[balance.Address] = true
}

return balances
}
for _, metadata := range gs.DenomMetadata {
if seenMetadatas[metadata.Base] {
return fmt.Errorf("duplicate client metadata for denom %s", metadata.Base)
}

// ValidateGenesis 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 {
return err
if err := metadata.Validate(); err != nil {
return err
}

seenMetadatas[metadata.Base] = true
}

return NewSupply(data.Supply).ValidateBasic()
// NOTE: this errors if supply for any given coin is zero
return NewSupply(gs.Supply).ValidateBasic()
}

// NewGenesisState creates a new genesis state.
Expand Down Expand Up @@ -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
}
}
}
Loading