From 4a65bda1f5c8e8e89405e1aa49707d20ba1108c6 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 2 Jan 2019 19:17:27 +0100 Subject: [PATCH 1/6] Merge PR #3207: Fix token printing bug * Add IsPositive, case check on coins[0] * Link to correct PR * Add testcase --- CHANGELOG.md | 8 ++++++++ types/coin.go | 7 ++++++- types/coin_test.go | 5 +++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b37dca1c9..7292702c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 0.29.1 + +BUG FIXES + +* SDK + * [\#3064](https://github.com/cosmos/cosmos-sdk/issues/3064) Sanitize `sdk.Coin` denom. Coins denoms are now case insensitive, i.e. 100fooToken equals to 100FOOTOKEN. + * [\#3207](https://github.com/cosmos/cosmos-sdk/issues/3207) - Fix token printing bug + ## 0.29.0 BREAKING CHANGES diff --git a/types/coin.go b/types/coin.go index 974254532..e56967402 100644 --- a/types/coin.go +++ b/types/coin.go @@ -140,8 +140,13 @@ func (coins Coins) IsValid() bool { case 1: return coins[0].IsPositive() default: + if strings.ToLower(coins[0].Denom) != coins[0].Denom { + return false + } + if !coins[0].IsPositive() { + return false + } lowDenom := coins[0].Denom - for _, coin := range coins[1:] { if coin.Denom <= lowDenom { return false diff --git a/types/coin_test.go b/types/coin_test.go index 774534860..92036adfc 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -281,6 +281,10 @@ func TestCoins(t *testing.T) { {"GAS", NewInt(1)}, {"MINERAL", NewInt(1)}, } + neg := Coins{ + {"gas", NewInt(-1)}, + {"mineral", NewInt(1)}, + } assert.True(t, good.IsValid(), "Coins are valid") assert.True(t, good.IsPositive(), "Expected coins to be positive: %v", good) @@ -292,6 +296,7 @@ func TestCoins(t *testing.T) { assert.False(t, badSort2.IsValid(), "Coins are not sorted") assert.False(t, badAmt.IsValid(), "Coins cannot include 0 amounts") assert.False(t, dup.IsValid(), "Duplicate coin") + assert.False(t, neg.IsValid(), "Negative first-denom coin") } func TestCoinsGT(t *testing.T) { From d3804774d4a3c06b83bbdd4737c840a58a0021ed Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Wed, 2 Jan 2019 13:14:12 -0600 Subject: [PATCH 2/6] Fix is_valid bug (#3211) --- types/coin.go | 6 ++---- types/coin_test.go | 10 ++++++++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/types/coin.go b/types/coin.go index e56967402..55a479748 100644 --- a/types/coin.go +++ b/types/coin.go @@ -140,10 +140,8 @@ func (coins Coins) IsValid() bool { case 1: return coins[0].IsPositive() default: - if strings.ToLower(coins[0].Denom) != coins[0].Denom { - return false - } - if !coins[0].IsPositive() { + // Check single coin case + if !(Coins{coins[0]}).IsValid() { return false } lowDenom := coins[0].Denom diff --git a/types/coin_test.go b/types/coin_test.go index 92036adfc..851d15515 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -251,8 +251,13 @@ func TestMinusCoins(t *testing.T) { func TestCoins(t *testing.T) { good := Coins{ - {"GAS", NewInt(1)}, - {"MINERAL", NewInt(1)}, + {"gas", NewInt(1)}, + {"mineral", NewInt(1)}, + {"tree", NewInt(1)}, + } + mixedCase1 := Coins{ + {"gAs", NewInt(1)}, + {"MineraL", NewInt(1)}, {"TREE", NewInt(1)}, } empty := Coins{ @@ -287,6 +292,7 @@ func TestCoins(t *testing.T) { } assert.True(t, good.IsValid(), "Coins are valid") + assert.False(t, mixedCase1.IsValid(), "Coins denoms contain upper case characters") assert.True(t, good.IsPositive(), "Expected coins to be positive: %v", good) assert.False(t, null.IsPositive(), "Expected coins to not be positive: %v", null) assert.True(t, good.IsAllGTE(empty), "Expected %v to be >= %v", good, empty) From 0be04aff86154259e10225fb319080f01a55e239 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Wed, 2 Jan 2019 16:19:48 -0500 Subject: [PATCH 3/6] Add IsValid check on sendCoins (#3212) --- types/coin.go | 3 ++- x/bank/keeper.go | 13 ++++++++++- x/bank/keeper_test.go | 23 ++++++++++++++++--- x/bank/msgs.go | 51 +++++++++++++++++++++++++++---------------- 4 files changed, 66 insertions(+), 24 deletions(-) diff --git a/types/coin.go b/types/coin.go index 55a479748..1b353e6b4 100644 --- a/types/coin.go +++ b/types/coin.go @@ -140,10 +140,11 @@ func (coins Coins) IsValid() bool { case 1: return coins[0].IsPositive() default: - // Check single coin case + // check single coin case if !(Coins{coins[0]}).IsValid() { return false } + lowDenom := coins[0].Denom for _, coin := range coins[1:] { if coin.Denom <= lowDenom { diff --git a/x/bank/keeper.go b/x/bank/keeper.go index 706f445eb..1a11631ca 100644 --- a/x/bank/keeper.go +++ b/x/bank/keeper.go @@ -194,8 +194,13 @@ func addCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt s } // SendCoins moves coins from one account to another -// NOTE: Make sure to revert state changes from tx on error func sendCoins(ctx sdk.Context, am auth.AccountKeeper, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) { + // Safety check ensuring that when sending coins the keeper must maintain the + // supply invariant. + if !amt.IsValid() { + return nil, sdk.ErrInvalidCoins(amt.String()) + } + _, subTags, err := subtractCoins(ctx, am, fromAddr, amt) if err != nil { return nil, err @@ -212,6 +217,12 @@ func sendCoins(ctx sdk.Context, am auth.AccountKeeper, fromAddr sdk.AccAddress, // InputOutputCoins handles a list of inputs and outputs // NOTE: Make sure to revert state changes from tx on error func inputOutputCoins(ctx sdk.Context, am auth.AccountKeeper, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error) { + // Safety check ensuring that when sending coins the keeper must maintain the + // supply invariant. + if err := ValidateInputsOutputs(inputs, outputs); err != nil { + return nil, err + } + allTags := sdk.EmptyTags() for _, in := range inputs { diff --git a/x/bank/keeper_test.go b/x/bank/keeper_test.go index 14f40f9c4..241bad20d 100644 --- a/x/bank/keeper_test.go +++ b/x/bank/keeper_test.go @@ -10,6 +10,19 @@ import ( dbm "github.com/tendermint/tendermint/libs/db" "github.com/tendermint/tendermint/libs/log" + codec "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/store" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/auth" + "github.com/cosmos/cosmos-sdk/x/params" +) + +type testInput struct { + cdc *codec.Codec + ctx sdk.Context + ak auth.AccountKeeper +} + codec "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/store" sdk "github.com/cosmos/cosmos-sdk/types" @@ -108,7 +121,6 @@ func TestKeeper(t *testing.T) { require.True(t, bankKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 21), sdk.NewInt64Coin("foocoin", 4)})) require.True(t, bankKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 7), sdk.NewInt64Coin("foocoin", 6)})) require.True(t, bankKeeper.GetCoins(ctx, addr3).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 2), sdk.NewInt64Coin("foocoin", 5)})) - } func TestSendKeeper(t *testing.T) { @@ -146,8 +158,8 @@ func TestSendKeeper(t *testing.T) { require.True(t, sendKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("foocoin", 10)})) require.True(t, sendKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("foocoin", 5)})) - _, err2 := sendKeeper.SendCoins(ctx, addr, addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 50)}) - assert.Implements(t, (*sdk.Error)(nil), err2) + _, err := sendKeeper.SendCoins(ctx, addr, addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 50)}) + require.Implements(t, (*sdk.Error)(nil), err) require.True(t, sendKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("foocoin", 10)})) require.True(t, sendKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("foocoin", 5)})) @@ -156,6 +168,11 @@ func TestSendKeeper(t *testing.T) { require.True(t, sendKeeper.GetCoins(ctx, addr).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 20), sdk.NewInt64Coin("foocoin", 5)})) require.True(t, sendKeeper.GetCoins(ctx, addr2).IsEqual(sdk.Coins{sdk.NewInt64Coin("barcoin", 10), sdk.NewInt64Coin("foocoin", 10)})) + // validate coins with invalid denoms or negative values cannot be sent + // NOTE: We must use the Coin literal as the constructor does not allow + // negative values. + _, err = sendKeeper.SendCoins(ctx, addr, addr2, sdk.Coins{sdk.Coin{"FOOCOIN", sdk.NewInt(-5)}}) + require.Error(t, err) } func TestViewKeeper(t *testing.T) { diff --git a/x/bank/msgs.go b/x/bank/msgs.go index 48d251a1f..74705ebc7 100644 --- a/x/bank/msgs.go +++ b/x/bank/msgs.go @@ -37,25 +37,8 @@ func (msg MsgSend) ValidateBasic() sdk.Error { if len(msg.Outputs) == 0 { return ErrNoOutputs(DefaultCodespace).TraceSDK("") } - // make sure all inputs and outputs are individually valid - var totalIn, totalOut sdk.Coins - for _, in := range msg.Inputs { - if err := in.ValidateBasic(); err != nil { - return err.TraceSDK("") - } - totalIn = totalIn.Plus(in.Coins) - } - for _, out := range msg.Outputs { - if err := out.ValidateBasic(); err != nil { - return err.TraceSDK("") - } - totalOut = totalOut.Plus(out.Coins) - } - // make sure inputs and outputs match - if !totalIn.IsEqual(totalOut) { - return sdk.ErrInvalidCoins(totalIn.String()).TraceSDK("inputs and outputs don't match") - } - return nil + + return ValidateInputsOutputs(msg.Inputs, msg.Outputs) } // Implements Msg. @@ -170,3 +153,33 @@ func NewOutput(addr sdk.AccAddress, coins sdk.Coins) Output { } return output } + +// ---------------------------------------------------------------------------- +// Auxiliary + +// ValidateInputsOutputs validates that each respective input and output is +// valid and that the sum of inputs is equal to the sum of outputs. +func ValidateInputsOutputs(inputs []Input, outputs []Output) sdk.Error { + var totalIn, totalOut sdk.Coins + + for _, in := range inputs { + if err := in.ValidateBasic(); err != nil { + return err.TraceSDK("") + } + totalIn = totalIn.Plus(in.Coins) + } + + for _, out := range outputs { + if err := out.ValidateBasic(); err != nil { + return err.TraceSDK("") + } + totalOut = totalOut.Plus(out.Coins) + } + + // make sure inputs and outputs match + if !totalIn.IsEqual(totalOut) { + return sdk.ErrInvalidCoins(totalIn.String()).TraceSDK("inputs and outputs don't match") + } + + return nil +} From 5e5ea27a0460fe9159fd0973f4487459e1091f26 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 3 Jan 2019 00:17:03 +0100 Subject: [PATCH 4/6] Fix errors from merge --- x/bank/keeper_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/x/bank/keeper_test.go b/x/bank/keeper_test.go index 241bad20d..142af7eb8 100644 --- a/x/bank/keeper_test.go +++ b/x/bank/keeper_test.go @@ -14,7 +14,6 @@ import ( "github.com/cosmos/cosmos-sdk/store" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/auth" - "github.com/cosmos/cosmos-sdk/x/params" ) type testInput struct { @@ -23,13 +22,6 @@ type testInput struct { ak auth.AccountKeeper } - codec "github.com/cosmos/cosmos-sdk/codec" - "github.com/cosmos/cosmos-sdk/store" - sdk "github.com/cosmos/cosmos-sdk/types" - - "github.com/cosmos/cosmos-sdk/x/auth" -) - func setupMultiStore() (sdk.MultiStore, *sdk.KVStoreKey) { db := dbm.NewMemDB() authKey := sdk.NewKVStoreKey("authkey") From a8685c58eee07b9ae6e824e6da8f432c4792b908 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 3 Jan 2019 00:21:20 +0100 Subject: [PATCH 5/6] Remove not-included CHANGELOG entry --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7292702c3..3662b7b19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,6 @@ BUG FIXES * SDK - * [\#3064](https://github.com/cosmos/cosmos-sdk/issues/3064) Sanitize `sdk.Coin` denom. Coins denoms are now case insensitive, i.e. 100fooToken equals to 100FOOTOKEN. * [\#3207](https://github.com/cosmos/cosmos-sdk/issues/3207) - Fix token printing bug ## 0.29.0 From 62d49ed1e22a77c568329d3036f9361fe6f11a8e Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 3 Jan 2019 00:22:16 +0100 Subject: [PATCH 6/6] Remove unused struct --- x/bank/keeper_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/x/bank/keeper_test.go b/x/bank/keeper_test.go index 142af7eb8..456303831 100644 --- a/x/bank/keeper_test.go +++ b/x/bank/keeper_test.go @@ -16,12 +16,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth" ) -type testInput struct { - cdc *codec.Codec - ctx sdk.Context - ak auth.AccountKeeper -} - func setupMultiStore() (sdk.MultiStore, *sdk.KVStoreKey) { db := dbm.NewMemDB() authKey := sdk.NewKVStoreKey("authkey")