From ce1e65083ea7fcbb744a3927de8be80f1f1bf77f Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Mon, 2 Aug 2021 10:16:23 +0200 Subject: [PATCH 1/6] adding checks to ensure denoms are sorted --- x/bank/types/balance.go | 13 ++++++++++--- x/bank/types/balance_test.go | 12 ++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/x/bank/types/balance.go b/x/bank/types/balance.go index b944fea27..2aa7967d5 100644 --- a/x/bank/types/balance.go +++ b/x/bank/types/balance.go @@ -34,6 +34,11 @@ func (b Balance) Validate() error { if err != nil { return err } + + var prevDenom string + if !b.Coins.Empty() { + prevDenom = b.Coins[0].Denom + } seenDenoms := make(map[string]bool) // NOTE: we perform a custom validation since the coins.Validate function @@ -47,16 +52,18 @@ func (b Balance) Validate() error { return err } + if coin.Denom < prevDenom { + return fmt.Errorf("denomination %s is not sorted", coin.Denom) + } + if coin.IsNegative() { return fmt.Errorf("coin %s amount is cannot be negative", coin.Denom) } seenDenoms[coin.Denom] = true + prevDenom = coin.Denom } - // sort the coins post validation - b.Coins = b.Coins.Sort() - return nil } diff --git a/x/bank/types/balance_test.go b/x/bank/types/balance_test.go index f32831491..1c7d4255c 100644 --- a/x/bank/types/balance_test.go +++ b/x/bank/types/balance_test.go @@ -64,6 +64,18 @@ func TestBalanceValidate(t *testing.T) { }, true, }, + { + "unsorted coins", + bank.Balance{ + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{ + sdk.NewInt64Coin("atom", 2), + sdk.NewInt64Coin("zatom", 2), + sdk.NewInt64Coin("batom", 12), + }, + }, + true, + }, } for _, tc := range testCases { From 2cea5e5750d220d476ddc3184d19cdf0a7de52c5 Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Mon, 2 Aug 2021 10:30:16 +0200 Subject: [PATCH 2/6] updating changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e4b82b50..8861b7145 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (server) [#9704](https://github.com/cosmos/cosmos-sdk/pull/9704) Start GRPCWebServer in goroutine, avoid blocking other services from starting. * [\#9762](https://github.com/cosmos/cosmos-sdk/pull/9762) The init command uses the chain-id from the client config if --chain-id is not provided * [\#9793](https://github.com/cosmos/cosmos-sdk/pull/9793) Fixed ECDSA/secp256r1 transaction malleability. +* [\#9829](https://github.com/cosmos/cosmos-sdk/pull/9829) Fixed Coin denom sorting not being checked during `Balance.Validate` check ### State Machine Breaking From 40d22c7ab3a385bde480ce0b10d9920d8646d67e Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Tue, 3 Aug 2021 19:43:38 +0200 Subject: [PATCH 3/6] refactoring balance.coin validation --- x/bank/types/balance.go | 32 +++----------------------------- 1 file changed, 3 insertions(+), 29 deletions(-) diff --git a/x/bank/types/balance.go b/x/bank/types/balance.go index 2aa7967d5..78539ace6 100644 --- a/x/bank/types/balance.go +++ b/x/bank/types/balance.go @@ -30,38 +30,12 @@ func (b Balance) GetCoins() sdk.Coins { // Validate checks for address and coins correctness. func (b Balance) Validate() error { - _, err := sdk.AccAddressFromBech32(b.Address) - if err != nil { + if _, err := sdk.AccAddressFromBech32(b.Address); err != nil { return err } - var prevDenom string - if !b.Coins.Empty() { - prevDenom = b.Coins[0].Denom - } - 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.Denom < prevDenom { - return fmt.Errorf("denomination %s is not sorted", coin.Denom) - } - - if coin.IsNegative() { - return fmt.Errorf("coin %s amount is cannot be negative", coin.Denom) - } - - seenDenoms[coin.Denom] = true - prevDenom = coin.Denom + if err := b.Coins.Validate(); err != nil { + return err } return nil From 2468b64cffd889e626ec6b7e047459ed39cafd58 Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Tue, 3 Aug 2021 19:44:03 +0200 Subject: [PATCH 4/6] adding 0 value coin test case --- x/bank/types/balance_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/x/bank/types/balance_test.go b/x/bank/types/balance_test.go index 1c7d4255c..fd6b72a3a 100644 --- a/x/bank/types/balance_test.go +++ b/x/bank/types/balance_test.go @@ -64,6 +64,17 @@ func TestBalanceValidate(t *testing.T) { }, true, }, + { + "0 value coin", + bank.Balance{ + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{ + sdk.NewInt64Coin("atom", 0), + sdk.NewInt64Coin("zatom", 2), + }, + }, + true, + }, { "unsorted coins", bank.Balance{ From 66b91ee7c7c9f916282d6cd1fffdfebf4cdf74ab Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Wed, 4 Aug 2021 14:54:01 +0200 Subject: [PATCH 5/6] added valid sorted coin testcase --- x/bank/types/balance_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/x/bank/types/balance_test.go b/x/bank/types/balance_test.go index fd6b72a3a..10ee2a74b 100644 --- a/x/bank/types/balance_test.go +++ b/x/bank/types/balance_test.go @@ -87,6 +87,18 @@ func TestBalanceValidate(t *testing.T) { }, true, }, + { + "valid sorted coins", + bank.Balance{ + Address: "cosmos1yq8lgssgxlx9smjhes6ryjasmqmd3ts2559g0t", + Coins: sdk.Coins{ + sdk.NewInt64Coin("atom", 2), + sdk.NewInt64Coin("batom", 12), + sdk.NewInt64Coin("zatom", 2), + }, + }, + false, + }, } for _, tc := range testCases { From 59b788a3bb4ecf7fc95c062551898e4db505765b Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Wed, 4 Aug 2021 14:56:02 +0200 Subject: [PATCH 6/6] updating changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8861b7145..f9937a297 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,7 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (server) [#9704](https://github.com/cosmos/cosmos-sdk/pull/9704) Start GRPCWebServer in goroutine, avoid blocking other services from starting. * [\#9762](https://github.com/cosmos/cosmos-sdk/pull/9762) The init command uses the chain-id from the client config if --chain-id is not provided * [\#9793](https://github.com/cosmos/cosmos-sdk/pull/9793) Fixed ECDSA/secp256r1 transaction malleability. -* [\#9829](https://github.com/cosmos/cosmos-sdk/pull/9829) Fixed Coin denom sorting not being checked during `Balance.Validate` check +* [\#9829](https://github.com/cosmos/cosmos-sdk/pull/9829) Fixed Coin denom sorting not being checked during `Balance.Validate` check. Refactored the Validation logic to use `Coins.Validate` for `Balance.Coins` ### State Machine Breaking