From fb0a2c46d3bd99c0d7b66effcabbc4f247435a2a Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Mon, 16 Dec 2019 19:10:18 -0300 Subject: [PATCH] fix DecCoins constructor (#5410) * fix decCoins * minor changes for standardization * changelog * add test cases --- CHANGELOG.md | 1 + types/dec_coin.go | 58 ++++++++++++++--------- types/dec_coin_test.go | 105 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c59ec9a9..15b78b68b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -223,6 +223,7 @@ to detail this new feature and how state transitions occur. * (rest) [\#5212](https://github.com/cosmos/cosmos-sdk/issues/5212) Fix pagination in the `/gov/proposals` handler. * (baseapp) [\#5350](https://github.com/cosmos/cosmos-sdk/issues/5350) Allow a node to restart successfully after a `halt-height` or `halt-time` has been triggered. +* (types) [\#5408](https://github.com/cosmos/cosmos-sdk/issues/5408) `NewDecCoins` constructor now sorts the coins. ## [v0.37.4] - 2019-11-04 diff --git a/types/dec_coin.go b/types/dec_coin.go index 0c11d3135..e26a88f87 100644 --- a/types/dec_coin.go +++ b/types/dec_coin.go @@ -11,17 +11,16 @@ import ( // ---------------------------------------------------------------------------- // Decimal Coin -// Coins which can have additional decimal points +// DecCoin defines a coin which can have additional decimal points type DecCoin struct { Denom string `json:"denom"` Amount Dec `json:"amount"` } +// NewDecCoin creates a new DecCoin instance from an Int. func NewDecCoin(denom string, amount Int) DecCoin { - mustValidateDenom(denom) - - if amount.IsNegative() { - panic(fmt.Sprintf("negative coin amount: %v\n", amount)) + if err := validate(denom, amount); err != nil { + panic(err) } return DecCoin{ @@ -30,6 +29,7 @@ func NewDecCoin(denom string, amount Int) DecCoin { } } +// NewDecCoinFromDec creates a new DecCoin instance from a Dec. func NewDecCoinFromDec(denom string, amount Dec) DecCoin { mustValidateDenom(denom) @@ -43,12 +43,10 @@ func NewDecCoinFromDec(denom string, amount Dec) DecCoin { } } +// NewDecCoinFromCoin creates a new DecCoin from a Coin. func NewDecCoinFromCoin(coin Coin) DecCoin { - if coin.Amount.IsNegative() { - panic(fmt.Sprintf("negative decimal coin amount: %v\n", coin.Amount)) - } - if strings.ToLower(coin.Denom) != coin.Denom { - panic(fmt.Sprintf("denom cannot contain upper case characters: %s\n", coin.Denom)) + if err := validate(coin.Denom, coin.Amount); err != nil { + panic(err) } return DecCoin{ @@ -97,7 +95,7 @@ func (coin DecCoin) IsEqual(other DecCoin) bool { return coin.Amount.Equal(other.Amount) } -// Adds amounts of two coins with same denom +// Add adds amounts of two decimal coins with same denom. func (coin DecCoin) Add(coinB DecCoin) DecCoin { if coin.Denom != coinB.Denom { panic(fmt.Sprintf("coin denom different: %v %v\n", coin.Denom, coinB.Denom)) @@ -105,12 +103,16 @@ func (coin DecCoin) Add(coinB DecCoin) DecCoin { return DecCoin{coin.Denom, coin.Amount.Add(coinB.Amount)} } -// Subtracts amounts of two coins with same denom +// Sub subtracts amounts of two decimal coins with same denom. func (coin DecCoin) Sub(coinB DecCoin) DecCoin { if coin.Denom != coinB.Denom { panic(fmt.Sprintf("coin denom different: %v %v\n", coin.Denom, coinB.Denom)) } - return DecCoin{coin.Denom, coin.Amount.Sub(coinB.Amount)} + res := DecCoin{coin.Denom, coin.Amount.Sub(coinB.Amount)} + if res.IsNegative() { + panic("negative decimal coin amount") + } + return res } // TruncateDecimal returns a Coin with a truncated decimal and a DecCoin for the @@ -118,7 +120,7 @@ func (coin DecCoin) Sub(coinB DecCoin) DecCoin { func (coin DecCoin) TruncateDecimal() (Coin, DecCoin) { truncated := coin.Amount.TruncateInt() change := coin.Amount.Sub(truncated.ToDec()) - return NewCoin(coin.Denom, truncated), DecCoin{coin.Denom, change} + return NewCoin(coin.Denom, truncated), NewDecCoinFromDec(coin.Denom, change) } // IsPositive returns true if coin amount is positive. @@ -141,18 +143,30 @@ func (coin DecCoin) String() string { return fmt.Sprintf("%v%v", coin.Amount, coin.Denom) } +// IsValid returns true if the DecCoin has a non-negative amount and the denom is vaild. +func (coin DecCoin) IsValid() bool { + if err := ValidateDenom(coin.Denom); err != nil { + return false + } + return !coin.IsNegative() +} + // ---------------------------------------------------------------------------- // Decimal Coins -// coins with decimal +// DecCoins defines a slice of coins with decimal values type DecCoins []DecCoin +// NewDecCoins constructs a new coin set with decimal values +// from regular Coins. func NewDecCoins(coins Coins) DecCoins { - dcs := make(DecCoins, len(coins)) - for i, coin := range coins { - dcs[i] = NewDecCoinFromCoin(coin) + decCoins := make(DecCoins, len(coins)) + newCoins := NewCoins(coins...) + for i, coin := range newCoins { + decCoins[i] = NewDecCoinFromCoin(coin) } - return dcs + + return decCoins } // String implements the Stringer interface for DecCoins. It returns a @@ -177,7 +191,7 @@ func (coins DecCoins) TruncateDecimal() (truncatedCoins Coins, changeCoins DecCo for _, coin := range coins { truncated, change := coin.TruncateDecimal() if !truncated.IsZero() { - truncatedCoins = truncatedCoins.Add(Coins{truncated}) + truncatedCoins = truncatedCoins.Add(NewCoins(truncated)) } if !change.IsZero() { changeCoins = changeCoins.Add(DecCoins{change}) @@ -404,7 +418,7 @@ func (coins DecCoins) Empty() bool { return len(coins) == 0 } -// returns the amount of a denom from deccoins +// AmountOf returns the amount of a denom from deccoins func (coins DecCoins) AmountOf(denom string) Dec { mustValidateDenom(denom) @@ -452,7 +466,7 @@ func (coins DecCoins) IsEqual(coinsB DecCoins) bool { return true } -// return whether all coins are zero +// IsZero returns whether all coins are zero func (coins DecCoins) IsZero() bool { for _, coin := range coins { if !coin.Amount.IsZero() { diff --git a/types/dec_coin_test.go b/types/dec_coin_test.go index 3c8df6572..65408a0b3 100644 --- a/types/dec_coin_test.go +++ b/types/dec_coin_test.go @@ -96,6 +96,111 @@ func TestAddDecCoins(t *testing.T) { } } +func TestIsValid(t *testing.T) { + tests := []struct { + coin DecCoin + expectPass bool + msg string + }{ + { + NewDecCoin("mytoken", NewInt(10)), + true, + "valid coins should have passed", + }, + { + DecCoin{Denom: "BTC", Amount: NewDec(10)}, + false, + "invalid denoms", + }, + { + DecCoin{Denom: "BTC", Amount: NewDec(-10)}, + false, + "negative amount", + }, + } + + for _, tc := range tests { + tc := tc + if tc.expectPass { + require.True(t, tc.coin.IsValid(), tc.msg) + } else { + require.False(t, tc.coin.IsValid(), tc.msg) + } + } +} + +func TestSubDecCoin(t *testing.T) { + tests := []struct { + coin DecCoin + expectPass bool + msg string + }{ + { + NewDecCoin("mytoken", NewInt(20)), + true, + "valid coins should have passed", + }, + { + NewDecCoin("othertoken", NewInt(20)), + false, + "denom mismatch", + }, + { + NewDecCoin("mytoken", NewInt(9)), + false, + "negative amount", + }, + } + + decCoin := NewDecCoin("mytoken", NewInt(10)) + + for _, tc := range tests { + tc := tc + if tc.expectPass { + equal := tc.coin.Sub(decCoin) + require.Equal(t, equal, decCoin, tc.msg) + } else { + require.Panics(t, func() { tc.coin.Sub(decCoin) }, tc.msg) + } + } +} + +func TestSubDecCoins(t *testing.T) { + tests := []struct { + coins DecCoins + expectPass bool + msg string + }{ + { + NewDecCoins(Coins{NewCoin("mytoken", NewInt(10)), NewCoin("btc", NewInt(20)), NewCoin("eth", NewInt(30))}), + true, + "sorted coins should have passed", + }, + { + DecCoins{NewDecCoin("mytoken", NewInt(10)), NewDecCoin("btc", NewInt(20)), NewDecCoin("eth", NewInt(30))}, + false, + "unorted coins should panic", + }, + { + DecCoins{DecCoin{Denom: "BTC", Amount: NewDec(10)}, NewDecCoin("eth", NewInt(15)), NewDecCoin("mytoken", NewInt(5))}, + false, + "invalid denoms", + }, + } + + decCoins := NewDecCoins(Coins{NewCoin("btc", NewInt(10)), NewCoin("eth", NewInt(15)), NewCoin("mytoken", NewInt(5))}) + + for _, tc := range tests { + tc := tc + if tc.expectPass { + equal := tc.coins.Sub(decCoins) + require.Equal(t, equal, decCoins, tc.msg) + } else { + require.Panics(t, func() { tc.coins.Sub(decCoins) }, tc.msg) + } + } +} + func TestSortDecCoins(t *testing.T) { good := DecCoins{ NewInt64DecCoin("gas", 1),