From 5b45deb9aee14a91a85db4ada6856587fe3c2b21 Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Tue, 26 Jan 2021 10:34:02 -0300 Subject: [PATCH 1/5] stargate: balance and metadata validation (#8421) From: #8417 --- CHANGELOG.md | 6 ++ x/bank/module.go | 2 +- x/bank/types/balance.go | 93 ++++++++++++++++++++ x/bank/types/balance_test.go | 80 ++++++++++++++++++ x/bank/types/genesis.go | 74 ++++++---------- x/bank/types/genesis_test.go | 149 ++++++++++++++++++++++++++------ x/bank/types/metadata.go | 56 ++++++++++++ x/bank/types/metadata_test.go | 155 ++++++++++++++++++++++++++++++++++ x/bank/types/supply.go | 4 +- 9 files changed, 545 insertions(+), 74 deletions(-) create mode 100644 x/bank/types/balance.go create mode 100644 x/bank/types/balance_test.go create mode 100644 x/bank/types/metadata.go create mode 100644 x/bank/types/metadata_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index de85d1dc787e..91ca28b567eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/x/bank/module.go b/x/bank/module.go index bc998634c12e..f271fa21975c 100644 --- a/x/bank/module.go +++ b/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. diff --git a/x/bank/types/balance.go b/x/bank/types/balance.go new file mode 100644 index 000000000000..93517a1d925b --- /dev/null +++ b/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 + } + } +} diff --git a/x/bank/types/balance_test.go b/x/bank/types/balance_test.go new file mode 100644 index 000000000000..73e4d8eff387 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/x/bank/types/genesis.go b/x/bank/types/genesis.go index ccf35be86231..4adc758f3bd9 100644 --- a/x/bank/types/genesis.go +++ b/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) +// 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. @@ -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 - } - } -} diff --git a/x/bank/types/genesis_test.go b/x/bank/types/genesis_test.go index 5f612aed726b..6751e9721f31 100644 --- a/x/bank/types/genesis_test.go +++ b/x/bank/types/genesis_test.go @@ -5,48 +5,147 @@ import ( "github.com/stretchr/testify/require" - "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/bank/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 types.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", + types.GenesisState{ + Params: types.DefaultParams(), + Balances: []types.Balance{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.NewInt64Coin("uatom", 1)}, + }, + }, + Supply: sdk.Coins{sdk.NewInt64Coin("uatom", 1)}, + DenomMetadata: []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", + }, + }, }, - Base: "uatom", - Display: "atom", + false, }, + {"empty genesisState", types.GenesisState{}, false}, + { + "invalid params ", + types.GenesisState{ + Params: types.Params{ + SendEnabled: []*types.SendEnabled{ + {"", true}, + }, + }, + }, + true, + }, + { + "dup balances", + types.GenesisState{ + Balances: []types.Balance{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.NewInt64Coin("uatom", 1)}, + }, + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{sdk.NewInt64Coin("uatom", 1)}, + }, + }, + }, + true, + }, + { + "invalid balance", + types.GenesisState{ + Balances: []types.Balance{ + { + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + }, + }, + }, + true, + }, + { + "dup Metadata", + types.GenesisState{ + DenomMetadata: []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", + }, + { + 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", + }, + }, + }, + true, + }, + { + "invalid Metadata", + types.GenesisState{ + DenomMetadata: []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: "", + Display: "", + }, + }, + }, + true, + }, + { + "invalid supply", + types.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) } }) } diff --git a/x/bank/types/metadata.go b/x/bank/types/metadata.go new file mode 100644 index 000000000000..5106964e994e --- /dev/null +++ b/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 +} diff --git a/x/bank/types/metadata_test.go b/x/bank/types/metadata_test.go new file mode 100644 index 000000000000..29f4d220f95f --- /dev/null +++ b/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) + } + }) + } +} diff --git a/x/bank/types/supply.go b/x/bank/types/supply.go index 558c852c2b83..c5c14b15bc2c 100644 --- a/x/bank/types/supply.go +++ b/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 From 5179e7e604b923108086e9f9ad98df7476243bc7 Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Tue, 26 Jan 2021 10:55:58 -0300 Subject: [PATCH 2/5] backport #8418 (#8424) Co-authored-by: Alessio Treglia --- CHANGELOG.md | 1 + simapp/simd/cmd/genaccounts.go | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91ca28b567eb..87a485254050 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (simapp) [\#8418](https://github.com/cosmos/cosmos-sdk/pull/8418) Add balance coin to supply when adding a new genesis account * (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 diff --git a/simapp/simd/cmd/genaccounts.go b/simapp/simd/cmd/genaccounts.go index dc6925120e8d..57de144c36b4 100644 --- a/simapp/simd/cmd/genaccounts.go +++ b/simapp/simd/cmd/genaccounts.go @@ -151,6 +151,7 @@ contain valid denominations. Accounts may optionally be supplied with vesting pa bankGenState := banktypes.GetGenesisStateFromAppState(depCdc, appState) bankGenState.Balances = append(bankGenState.Balances, balances) bankGenState.Balances = banktypes.SanitizeGenesisBalances(bankGenState.Balances) + bankGenState.Supply = bankGenState.Supply.Add(balances.Coins...) bankGenStateBz, err := cdc.MarshalJSON(bankGenState) if err != nil { From f67c4f9e99cf9cff936b4a617657faaec78dea9b Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 26 Jan 2021 14:42:45 +0000 Subject: [PATCH 3/5] Reorder IBC channel callbacks (#8441) From: #8404 Thanks: @fedekunze @colin-axner --- CHANGELOG.md | 4 ++++ x/ibc/core/keeper/msg_server.go | 16 ++++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87a485254050..c598bbb43bc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +### 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. + ### Bug Fixes * (simapp) [\#8418](https://github.com/cosmos/cosmos-sdk/pull/8418) Add balance coin to supply when adding a new genesis account diff --git a/x/ibc/core/keeper/msg_server.go b/x/ibc/core/keeper/msg_server.go index 5ea590fea93a..dcddcaed16d4 100644 --- a/x/ibc/core/keeper/msg_server.go +++ b/x/ibc/core/keeper/msg_server.go @@ -326,15 +326,15 @@ func (k Keeper) ChannelOpenAck(goCtx context.Context, msg *channeltypes.MsgChann return nil, sdkerrors.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) } - if err = cbs.OnChanOpenAck(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyVersion); err != nil { - return nil, sdkerrors.Wrap(err, "channel open ack callback failed") - } - _, err = channel.HandleMsgChannelOpenAck(ctx, k.ChannelKeeper, cap, msg) if err != nil { return nil, err } + if err = cbs.OnChanOpenAck(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyVersion); err != nil { + return nil, sdkerrors.Wrap(err, "channel open ack callback failed") + } + return &channeltypes.MsgChannelOpenAckResponse{}, nil } @@ -354,15 +354,15 @@ func (k Keeper) ChannelOpenConfirm(goCtx context.Context, msg *channeltypes.MsgC return nil, sdkerrors.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) } - if err = cbs.OnChanOpenConfirm(ctx, msg.PortId, msg.ChannelId); err != nil { - return nil, sdkerrors.Wrap(err, "channel open confirm callback failed") - } - _, err = channel.HandleMsgChannelOpenConfirm(ctx, k.ChannelKeeper, cap, msg) if err != nil { return nil, err } + if err = cbs.OnChanOpenConfirm(ctx, msg.PortId, msg.ChannelId); err != nil { + return nil, sdkerrors.Wrap(err, "channel open confirm callback failed") + } + return &channeltypes.MsgChannelOpenConfirmResponse{}, nil } From e6dac1da3d8669166c60887148adcd500a5b0b97 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 26 Jan 2021 15:03:22 +0000 Subject: [PATCH 4/5] ibc: MsgTransfer amino JSON, commits from @fedekunze (#8440) From: #8437 Closes: #8266 Thanks: @fedekunze --- CHANGELOG.md | 4 ++++ x/auth/client/rest/rest_test.go | 3 +-- x/ibc/applications/transfer/module.go | 4 +++- x/ibc/applications/transfer/types/codec.go | 18 +++++++++++++++++- x/ibc/applications/transfer/types/msgs.go | 5 ++--- x/ibc/applications/transfer/types/msgs_test.go | 10 ++++++++++ 6 files changed, 37 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c598bbb43bc5..c20d838b323d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +### State Machine Breaking + +* (x/ibc) [\#8266](https://github.com/cosmos/cosmos-sdk/issues/8266) Add amino JSON for IBC messages in order to support Ledger text signing. + ### 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. diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index e20874f6e716..0ba55cea67df 100644 --- a/x/auth/client/rest/rest_test.go +++ b/x/auth/client/rest/rest_test.go @@ -186,8 +186,7 @@ func (s *IntegrationTestSuite) TestBroadcastIBCTxRequest() { res, err := rest.PostRequest(fmt.Sprintf("%s/txs", val.APIAddress), "application/json", []byte(req)) s.Require().NoError(err) - // Make sure the error message is correct. - s.Require().Contains(string(res), "this transaction cannot be broadcasted via legacy REST endpoints") + s.Require().NotContains(string(res), "this transaction cannot be broadcasted via legacy REST endpoints", string(res)) } // Helper function to test querying txs. We will use it to query StdTx and service `Msg`s. diff --git a/x/ibc/applications/transfer/module.go b/x/ibc/applications/transfer/module.go index bf324e2c9b36..039597afe5e4 100644 --- a/x/ibc/applications/transfer/module.go +++ b/x/ibc/applications/transfer/module.go @@ -46,7 +46,9 @@ func (AppModuleBasic) Name() string { } // RegisterLegacyAminoCodec implements AppModuleBasic interface -func (AppModuleBasic) RegisterLegacyAminoCodec(*codec.LegacyAmino) {} +func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { + types.RegisterLegacyAminoCodec(cdc) +} // RegisterInterfaces registers module concrete types into protobuf Any. func (AppModuleBasic) RegisterInterfaces(registry codectypes.InterfaceRegistry) { diff --git a/x/ibc/applications/transfer/types/codec.go b/x/ibc/applications/transfer/types/codec.go index 18b594f8a77a..24ad7e5a9023 100644 --- a/x/ibc/applications/transfer/types/codec.go +++ b/x/ibc/applications/transfer/types/codec.go @@ -7,6 +7,12 @@ import ( "github.com/cosmos/cosmos-sdk/types/msgservice" ) +// RegisterLegacyAminoCodec registers the necessary x/ibc transfer interfaces and concrete types +// on the provided LegacyAmino codec. These types are used for Amino JSON serialization. +func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { + cdc.RegisterConcrete(&MsgTransfer{}, "cosmos-sdk/MsgTransfer", nil) +} + // RegisterInterfaces register the ibc transfer module interfaces to protobuf // Any. func RegisterInterfaces(registry codectypes.InterfaceRegistry) { @@ -16,10 +22,20 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) { } var ( + amino = codec.NewLegacyAmino() + // ModuleCdc references the global x/ibc-transfer module codec. Note, the codec // should ONLY be used in certain instances of tests and for JSON encoding. // - // The actual codec used for serialization should be provided to x/ibc-transfer and + // The actual codec used for serialization should be provided to x/ibc transfer and // defined at the application level. ModuleCdc = codec.NewProtoCodec(codectypes.NewInterfaceRegistry()) + + // AminoCdc is a amino codec created to support amino json compatible msgs. + AminoCdc = codec.NewAminoCodec(amino) ) + +func init() { + RegisterLegacyAminoCodec(amino) + amino.Seal() +} diff --git a/x/ibc/applications/transfer/types/msgs.go b/x/ibc/applications/transfer/types/msgs.go index 065482671375..cf2293213a82 100644 --- a/x/ibc/applications/transfer/types/msgs.go +++ b/x/ibc/applications/transfer/types/msgs.go @@ -70,10 +70,9 @@ func (msg MsgTransfer) ValidateBasic() error { return ValidateIBCDenom(msg.Token.Denom) } -// GetSignBytes implements sdk.Msg. The function will panic since it is used -// for amino transaction verification which IBC does not support. +// GetSignBytes implements sdk.Msg. func (msg MsgTransfer) GetSignBytes() []byte { - panic("IBC messages do not support amino") + return sdk.MustSortJSON(AminoCdc.MustMarshalJSON(&msg)) } // GetSigners implements sdk.Msg diff --git a/x/ibc/applications/transfer/types/msgs_test.go b/x/ibc/applications/transfer/types/msgs_test.go index 89bd39524a51..1fc70c543bbe 100644 --- a/x/ibc/applications/transfer/types/msgs_test.go +++ b/x/ibc/applications/transfer/types/msgs_test.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -51,6 +52,15 @@ func TestMsgTransferType(t *testing.T) { require.Equal(t, "transfer", msg.Type()) } +func TestMsgTransferGetSignBytes(t *testing.T) { + msg := NewMsgTransfer(validPort, validChannel, coin, addr1, addr2, timeoutHeight, 0) + expected := fmt.Sprintf(`{"type":"cosmos-sdk/MsgTransfer","value":{"receiver":"%s","sender":"%s","source_channel":"testchannel","source_port":"testportid","timeout_height":{"revision_height":"10"},"token":{"amount":"100","denom":"atom"}}}`, addr2, addr1) + require.NotPanics(t, func() { + res := msg.GetSignBytes() + require.Equal(t, expected, string(res)) + }) +} + // TestMsgTransferValidation tests ValidateBasic for MsgTransfer func TestMsgTransferValidation(t *testing.T) { testCases := []struct { From c54025dac01ba36224b0eaff1a0030afe939b22c Mon Sep 17 00:00:00 2001 From: Frojdi Dymylja <33157909+fdymylja@users.noreply.github.com> Date: Tue, 26 Jan 2021 17:21:59 +0100 Subject: [PATCH 5/5] chore: update RELEASE_NOTES.md (#8443) Co-authored-by: Alessio Treglia --- CHANGELOG.md | 5 +---- RELEASE_NOTES.md | 45 +++++++++++++++------------------------------ 2 files changed, 16 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c20d838b323d..973ea6e054ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,14 +34,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ # Changelog -## [Unreleased] +## [v0.41.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.41.0) - 2021-01-26 ### State Machine Breaking * (x/ibc) [\#8266](https://github.com/cosmos/cosmos-sdk/issues/8266) Add amino JSON for IBC messages in order to support Ledger text signing. - -### 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. ### Bug Fixes diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index b097a34513b8..57a705af9b17 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,45 +1,30 @@ -# Cosmos SDK v0.40.1 "Stargate" Release Notes +# Cosmos SDK v0.41.0 "Stargate" Release Notes -This is a bug fix release to the *Cosmos SDK 0.40* "Stargate" release series. No breaking changes are introduced, thus no migration should be needed. -Among the various bugfixes, this release introduces important security patches. +This release includes two breaking changes, and a few minor bugfixes. -See the [Cosmos SDK v0.40.1 milestone](https://github.com/cosmos/cosmos-sdk/milestone/36?closed=1) on our issue tracker for details. +See the [Cosmos SDK v0.41.0 milestone](https://github.com/cosmos/cosmos-sdk/milestone/37?closed=1) on our issue tracker for details. -### Gogo protobuf security release +### Support Amino JSON for IBC MsgTransfer -[Gogoprotobuf](https://github.com/gogo/protobuf) released a bugfix addressing [CVE-2021-3121](https://nvd.nist.gov/vuln/detail/CVE-2021-3121). Cosmos SDK respective dependency has been updated and protobuf generated files were regenerated. +This change **breaks state backward compatibility**. -### Tendermint security patches +At the moment hardware wallets are [unable to sign messages using `SIGN_MODE_DIRECT` because the cosmos ledger app does not support proto encoding and`SIGN_MODE_TEXTUAL` is not available yet](https://https://github.com/cosmos/cosmos-sdk/issues/8266). -This release comes with a newer release of Tendermint that, other than fixing various bugs it also addresses a high-severity security vulnerability. -For the comprehensive list of changes introduced by Tendermint since Cosmos SDK v0.40.0, please refer to [Tendermint's v0.34.3 release notes](https://github.com/tendermint/tendermint/blob/v0.34.3/CHANGELOG.md#v0.34.3). +In order to enable hardware wallets users to interact with IBC, amino JSON support was added to `MsgTransfer` only. -### Fix zero time checks +### Counterparty.ChannelID not available in OnChanOpenAck callback implementation. -In Cosmos SDK applications, it is recommended to use `Time.Unix() <= 0` instead of `Time.IsZero()`, which may lead to unexpected results. -See [\#8085](https://github.com/cosmos/cosmos-sdk/pull/8058) for more information. +This change **breaks state backward compatibility**. -### Querying upgrade plans panics when no plan exists +In a previous version the `Counterparty.ChannelID` was available for an `OnChanOpenAck` callback implementation (read via `channelKeeper.GetChannel()`. Due to a regression, the channelID is currently empty. -The `x/upgrade` module command and REST endpoints for querying upgrade plans would panic when no plan existed. This is now resolved. +The issue has been fixed by reordering IBC `ChanOpenAck` and `ChanOpenConfirm` to execute the core handlers logic first, followed by application callbacks. -### Fix account sequence +It breaks state backward compatibility because the current change consumes more gas, which means that in an updated node a TX might fail because it ran out of gas whilst in older versions it would be successful. -In Cosmos SDK v0.40.0 a new structure (`SignatureV2`) for handling message signatures was introduced. -Although the `tx sign --signature-only` command was still capable of generating correct signatures, it was not returning the account's correct sequence number. +### Bug Fixes -### Reproducible builds +Now `x/bank` correctly verifies balances and metadata at init genesis stage. -Our automatic builds were not working correctly due to small gaps in file paths. Fixed in [\8300](https://github.com/cosmos/cosmos-sdk/pull/8300) and [\8301](https://github.com/cosmos/cosmos-sdk/pull/8301). +`simapp` correctly adds the coins of genesis accounts to supply. -### Wrapper errors were not reflective - -Cosmos SDK errors typically support the `Is()` method. The `Go` `errors.Is()` function compares an error to a value and should always return `true` when the receiver is passed as an argument to its own method, e.g. `err.Is(err)`. This was not a case for the error types provided by the `types/errors` package. - -### Fix latest consensus state - -Errors occur when using the client to send IBC transactions without flag `--absolute-timeouts`, e.g: - - gaiad tx ibc-transfer transfer - -The issue was caused by incorrect interface decoding and unpacking of `Any` values and is now fixed.