From 41fc538ac7d1d8587e6c983d677ca5f4309b129f Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Tue, 20 Nov 2018 04:22:35 -0500 Subject: [PATCH] Add Safety Measures to Coin/Coins (#2797) --- PENDING.md | 4 +- cmd/gaia/app/genesis.go | 6 +- cmd/gaia/app/sim_test.go | 2 +- docs/_attic/sdk/core/examples/app2_test.go | 2 +- docs/_attic/sdk/core/examples/app4_test.go | 10 +- docs/examples/democoin/x/cool/app_test.go | 10 +- docs/examples/democoin/x/pow/app_test.go | 6 +- docs/intro/ocap.md | 2 +- types/coin.go | 328 +++++++++++++------- types/coin_benchmark_test.go | 64 ++++ types/coin_test.go | 337 +++++++++------------ types/int.go | 42 ++- types/int_test.go | 25 ++ x/auth/ante.go | 20 +- x/auth/ante_test.go | 1 - x/bank/keeper.go | 6 +- x/bank/msgs_test.go | 8 - x/bank/simulation/msgs.go | 2 +- x/gov/msgs_test.go | 3 - x/slashing/handler_test.go | 6 +- x/slashing/keeper_test.go | 23 +- x/slashing/tick_test.go | 5 +- x/stake/types/msg_test.go | 6 - 23 files changed, 556 insertions(+), 362 deletions(-) create mode 100644 types/coin_benchmark_test.go diff --git a/PENDING.md b/PENDING.md index a0985593e..41e5c39c6 100644 --- a/PENDING.md +++ b/PENDING.md @@ -62,8 +62,10 @@ IMPROVEMENTS * SDK - [x/mock/simulation] [\#2720] major cleanup, introduction of helper objects, reorganization + - \#2821 Codespaces are now strings + - [types] #2776 Improve safety of `Coin` and `Coins` types. Various functions + and methods will panic when a negative amount is discovered. - #2815 Gas unit fields changed from `int64` to `uint64`. - - #2821 Codespaces are now strings * Tendermint - #2796 Update to go-amino 0.14.1 diff --git a/cmd/gaia/app/genesis.go b/cmd/gaia/app/genesis.go index 5ce9ab8b9..ab958e933 100644 --- a/cmd/gaia/app/genesis.go +++ b/cmd/gaia/app/genesis.go @@ -279,10 +279,12 @@ func CollectStdTxs(cdc *codec.Codec, moniker string, genTxsDir string, genDoc tm func NewDefaultGenesisAccount(addr sdk.AccAddress) GenesisAccount { accAuth := auth.NewBaseAccountWithAddress(addr) coins := sdk.Coins{ - {"fooToken", sdk.NewInt(1000)}, - {bondDenom, freeFermionsAcc}, + sdk.NewCoin("fooToken", sdk.NewInt(1000)), + sdk.NewCoin(bondDenom, freeFermionsAcc), } + coins.Sort() + accAuth.Coins = coins return NewGenesisAccount(&accAuth) } diff --git a/cmd/gaia/app/sim_test.go b/cmd/gaia/app/sim_test.go index e56344554..70ae8e12a 100644 --- a/cmd/gaia/app/sim_test.go +++ b/cmd/gaia/app/sim_test.go @@ -63,7 +63,7 @@ func appStateFn(r *rand.Rand, accs []simulation.Account) json.RawMessage { // Randomly generate some genesis accounts for _, acc := range accs { - coins := sdk.Coins{sdk.Coin{stakeTypes.DefaultBondDenom, sdk.NewInt(amount)}} + coins := sdk.Coins{sdk.NewCoin(stakeTypes.DefaultBondDenom, sdk.NewInt(amount))} genesisAccounts = append(genesisAccounts, GenesisAccount{ Address: acc.Address, Coins: coins, diff --git a/docs/_attic/sdk/core/examples/app2_test.go b/docs/_attic/sdk/core/examples/app2_test.go index c59dec264..b7c94bbcf 100644 --- a/docs/_attic/sdk/core/examples/app2_test.go +++ b/docs/_attic/sdk/core/examples/app2_test.go @@ -20,7 +20,7 @@ func TestEncoding(t *testing.T) { sendMsg := MsgSend{ From: addr1, To: addr2, - Amount: sdk.Coins{{"testCoins", sdk.NewInt(100)}}, + Amount: sdk.Coins{sdk.NewCoin("testCoins", sdk.NewInt(100))}, } // Construct transaction diff --git a/docs/_attic/sdk/core/examples/app4_test.go b/docs/_attic/sdk/core/examples/app4_test.go index 7d95d7b57..86be8ea12 100644 --- a/docs/_attic/sdk/core/examples/app4_test.go +++ b/docs/_attic/sdk/core/examples/app4_test.go @@ -30,7 +30,7 @@ func InitTestChain(bc *bapp.BaseApp, chainID string, addrs ...sdk.AccAddress) { for _, addr := range addrs { acc := GenesisAccount{ Address: addr, - Coins: sdk.Coins{{"testCoin", sdk.NewInt(100)}}, + Coins: sdk.Coins{sdk.NewCoin("testCoin", sdk.NewInt(100))}, } accounts = append(accounts, &acc) } @@ -61,12 +61,12 @@ func TestBadMsg(t *testing.T) { addr2 := priv2.PubKey().Address().Bytes() // Attempt to spend non-existent funds - msg := GenerateSpendMsg(addr1, addr2, sdk.Coins{{"testCoin", sdk.NewInt(100)}}) + msg := GenerateSpendMsg(addr1, addr2, sdk.Coins{sdk.NewCoin("testCoin", sdk.NewInt(100))}) // Construct transaction fee := auth.StdFee{ Gas: 1000000000000000, - Amount: sdk.Coins{{"testCoin", sdk.NewInt(0)}}, + Amount: sdk.Coins{sdk.NewCoin("testCoin", sdk.NewInt(0))}, } signBytes := auth.StdSignBytes("test-chain", 0, 0, fee, []sdk.Msg{msg}, "") sig, err := priv1.Sign(signBytes) @@ -108,11 +108,11 @@ func TestMsgSend(t *testing.T) { InitTestChain(bc, "test-chain", addr1) // Send funds to addr2 - msg := GenerateSpendMsg(addr1, addr2, sdk.Coins{{"testCoin", sdk.NewInt(100)}}) + msg := GenerateSpendMsg(addr1, addr2, sdk.Coins{sdk.NewCoin("testCoin", sdk.NewInt(100))}) fee := auth.StdFee{ Gas: 1000000000000000, - Amount: sdk.Coins{{"testCoin", sdk.NewInt(0)}}, + Amount: sdk.Coins{sdk.NewCoin("testCoin", sdk.NewInt(0))}, } signBytes := auth.StdSignBytes("test-chain", 0, 0, fee, []sdk.Msg{msg}, "") sig, err := priv1.Sign(signBytes) diff --git a/docs/examples/democoin/x/cool/app_test.go b/docs/examples/democoin/x/cool/app_test.go index 3f1dd6734..7bfc8b9cc 100644 --- a/docs/examples/democoin/x/cool/app_test.go +++ b/docs/examples/democoin/x/cool/app_test.go @@ -90,15 +90,15 @@ func TestMsgQuiz(t *testing.T) { // Set the trend, submit a really cool quiz and check for reward mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{setTrendMsg1}, []int64{0}, []int64{0}, true, true, priv1) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{quizMsg1}, []int64{0}, []int64{1}, true, true, priv1) - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"icecold", sdk.NewInt(69)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("icecold", sdk.NewInt(69))}) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{quizMsg2}, []int64{0}, []int64{2}, false, false, priv1) // result without reward - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"icecold", sdk.NewInt(69)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("icecold", sdk.NewInt(69))}) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{quizMsg1}, []int64{0}, []int64{3}, true, true, priv1) - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"icecold", sdk.NewInt(138)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("icecold", sdk.NewInt(138))}) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{setTrendMsg2}, []int64{0}, []int64{4}, true, true, priv1) // reset the trend mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{quizMsg1}, []int64{0}, []int64{5}, false, false, priv1) // the same answer will nolonger do! - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"icecold", sdk.NewInt(138)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("icecold", sdk.NewInt(138))}) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{quizMsg2}, []int64{0}, []int64{6}, true, true, priv1) // earlier answer now relevant again - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"badvibesonly", sdk.NewInt(69)}, {"icecold", sdk.NewInt(138)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("badvibesonly", sdk.NewInt(69)), sdk.NewCoin("icecold", sdk.NewInt(138))}) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{setTrendMsg3}, []int64{0}, []int64{7}, false, false, priv1) // expect to fail to set the trend to something which is not cool } diff --git a/docs/examples/democoin/x/pow/app_test.go b/docs/examples/democoin/x/pow/app_test.go index 41901f097..58a7d3538 100644 --- a/docs/examples/democoin/x/pow/app_test.go +++ b/docs/examples/democoin/x/pow/app_test.go @@ -75,12 +75,12 @@ func TestMsgMine(t *testing.T) { // Mine and check for reward mineMsg1 := GenerateMsgMine(addr1, 1, 2) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{mineMsg1}, []int64{0}, []int64{0}, true, true, priv1) - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"pow", sdk.NewInt(1)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("pow", sdk.NewInt(1))}) // Mine again and check for reward mineMsg2 := GenerateMsgMine(addr1, 2, 3) mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{mineMsg2}, []int64{0}, []int64{1}, true, true, priv1) - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"pow", sdk.NewInt(2)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("pow", sdk.NewInt(2))}) // Mine again - should be invalid mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{mineMsg2}, []int64{0}, []int64{1}, false, false, priv1) - mock.CheckBalance(t, mapp, addr1, sdk.Coins{{"pow", sdk.NewInt(2)}}) + mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewCoin("pow", sdk.NewInt(2))}) } diff --git a/docs/intro/ocap.md b/docs/intro/ocap.md index 7d57d0c34..cea7c108d 100644 --- a/docs/intro/ocap.md +++ b/docs/intro/ocap.md @@ -63,7 +63,7 @@ principle: type AppAccount struct {...} var account := &AppAccount{ Address: pub.Address(), - Coins: sdk.Coins{{"ATM", 100}}, + Coins: sdk.Coins{sdk.NewInt64Coin("ATM", 100)}, } var sumValue := externalModule.ComputeSumValue(account) ``` diff --git a/types/coin.go b/types/coin.go index 31f0e98a4..5f9020e84 100644 --- a/types/coin.go +++ b/types/coin.go @@ -4,23 +4,41 @@ import ( "fmt" "regexp" "sort" - "strconv" "strings" ) -// Coin hold some amount of one currency +//----------------------------------------------------------------------------- +// Coin + +// Coin hold some amount of one currency. +// +// CONTRACT: A coin will never hold a negative amount of any denomination. +// +// TODO: Make field members private for further safety. type Coin struct { - Denom string `json:"denom"` - Amount Int `json:"amount"` + Denom string `json:"denom"` + + // To allow the use of unsigned integers (see: #1273) a larger refactor will + // need to be made. So we use signed integers for now with safety measures in + // place preventing negative values being used. + Amount Int `json:"amount"` } +// NewCoin returns a new coin with a denomination and amount. It will panic if +// the amount is negative. func NewCoin(denom string, amount Int) Coin { + if amount.LT(ZeroInt()) { + panic("negative coin amount") + } + return Coin{ Denom: denom, Amount: amount, } } +// NewInt64Coin returns a new coin with a denomination and amount. It will panic +// if the amount is negative. func NewInt64Coin(denom string, amount int64) Coin { return NewCoin(denom, NewInt(amount)) } @@ -57,33 +75,46 @@ func (coin Coin) IsEqual(other Coin) bool { return coin.SameDenomAs(other) && (coin.Amount.Equal(other.Amount)) } -// IsPositive returns true if coin amount is positive +// Adds amounts of two coins with same denom. If the coins differ in denom then +// it panics. +func (coin Coin) Plus(coinB Coin) Coin { + if !coin.SameDenomAs(coinB) { + panic(fmt.Sprintf("invalid coin denominations; %s, %s", coin.Denom, coinB.Denom)) + } + + return Coin{coin.Denom, coin.Amount.Add(coinB.Amount)} +} + +// Subtracts amounts of two coins with same denom. If the coins differ in denom +// then it panics. +func (coin Coin) Minus(coinB Coin) Coin { + if !coin.SameDenomAs(coinB) { + panic(fmt.Sprintf("invalid coin denominations; %s, %s", coin.Denom, coinB.Denom)) + } + + res := Coin{coin.Denom, coin.Amount.Sub(coinB.Amount)} + if !res.IsNotNegative() { + panic("negative count amount") + } + + return res +} + +// IsPositive returns true if coin amount is positive. +// +// TODO: Remove once unsigned integers are used. func (coin Coin) IsPositive() bool { return (coin.Amount.Sign() == 1) } -// IsNotNegative returns true if coin amount is not negative +// IsNotNegative returns true if coin amount is not negative and false otherwise. +// +// TODO: Remove once unsigned integers are used. func (coin Coin) IsNotNegative() bool { return (coin.Amount.Sign() != -1) } -// Adds amounts of two coins with same denom -func (coin Coin) Plus(coinB Coin) Coin { - if !coin.SameDenomAs(coinB) { - return coin - } - return Coin{coin.Denom, coin.Amount.Add(coinB.Amount)} -} - -// Subtracts amounts of two coins with same denom -func (coin Coin) Minus(coinB Coin) Coin { - if !coin.SameDenomAs(coinB) { - return coin - } - return Coin{coin.Denom, coin.Amount.Sub(coinB.Amount)} -} - -//---------------------------------------- +//----------------------------------------------------------------------------- // Coins // Coins is a set of Coin, one per currency @@ -101,127 +132,157 @@ func (coins Coins) String() string { return out[:len(out)-1] } -// IsValid asserts the Coins are sorted, and don't have 0 amounts +// IsValid asserts the Coins are sorted and have positive amounts. func (coins Coins) IsValid() bool { switch len(coins) { case 0: return true case 1: - return !coins[0].IsZero() + return coins[0].IsPositive() default: lowDenom := coins[0].Denom + for _, coin := range coins[1:] { if coin.Denom <= lowDenom { return false } - if coin.IsZero() { + if !coin.IsPositive() { return false } + // we compare each coin against the last denom lowDenom = coin.Denom } + return true } } -// Plus combines two sets of coins -// CONTRACT: Plus will never return Coins where one Coin has a 0 amount. +// Plus adds two sets of coins. +// +// e.g. +// {2A} + {A, 2B} = {3A, 2B} +// {2A} + {0B} = {2A} +// +// NOTE: Plus operates under the invariant that coins are sorted by +// denominations. +// +// CONTRACT: Plus will never return Coins where one Coin has a non-positive +// amount. In otherwords, IsValid will always return true. func (coins Coins) Plus(coinsB Coins) Coins { + return coins.safePlus(coinsB) +} + +// safePlus will perform addition of two coins sets. If both coin sets are +// empty, then an empty set is returned. If only a single set is empty, the +// other set is returned. Otherwise, the coins are compared in order of their +// denomination and addition only occurs when the denominations match, otherwise +// the coin is simply added to the sum assuming it's not zero. +func (coins Coins) safePlus(coinsB Coins) Coins { sum := ([]Coin)(nil) indexA, indexB := 0, 0 lenA, lenB := len(coins), len(coinsB) + for { if indexA == lenA { if indexB == lenB { + // return nil coins if both sets are empty return sum } - return append(sum, coinsB[indexB:]...) + + // return set B (excluding zero coins) if set A is empty + return append(sum, removeZeroCoins(coinsB[indexB:])...) } else if indexB == lenB { - return append(sum, coins[indexA:]...) + // return set A (excluding zero coins) if set B is empty + return append(sum, removeZeroCoins(coins[indexA:])...) } + coinA, coinB := coins[indexA], coinsB[indexB] + switch strings.Compare(coinA.Denom, coinB.Denom) { - case -1: - if coinA.IsZero() { - // ignore 0 sum coin type - } else { + case -1: // coin A denom < coin B denom + if !coinA.IsZero() { sum = append(sum, coinA) } + indexA++ - case 0: - if coinA.Amount.Add(coinB.Amount).IsZero() { - // ignore 0 sum coin type - } else { - sum = append(sum, coinA.Plus(coinB)) + + case 0: // coin A denom == coin B denom + res := coinA.Plus(coinB) + if !res.IsZero() { + sum = append(sum, res) } + indexA++ indexB++ - case 1: - if coinB.IsZero() { - // ignore 0 sum coin type - } else { + + case 1: // coin A denom > coin B denom + if !coinB.IsZero() { sum = append(sum, coinB) } + indexB++ } } } -// Negative returns a set of coins with all amount negative -func (coins Coins) Negative() Coins { - res := make([]Coin, 0, len(coins)) - for _, coin := range coins { - res = append(res, Coin{ - Denom: coin.Denom, - Amount: coin.Amount.Neg(), - }) - } - return res -} - -// Minus subtracts a set of coins from another (adds the inverse) +// Minus subtracts a set of coins from another. +// +// e.g. +// {2A, 3B} - {A} = {A, 3B} +// {2A} - {0B} = {2A} +// {A, B} - {A} = {B} +// +// CONTRACT: Minus will never return Coins where one Coin has a non-positive +// amount. In otherwords, IsValid will always return true. func (coins Coins) Minus(coinsB Coins) Coins { - return coins.Plus(coinsB.Negative()) + diff, hasNeg := coins.SafeMinus(coinsB) + if hasNeg { + panic("negative coin amount") + } + + return diff } -// IsAllGT returns True iff for every denom in coins, the denom is present at a +// SafeMinus performs the same arithmetic as Minus but returns a boolean if any +// negative coin amount was returned. +func (coins Coins) SafeMinus(coinsB Coins) (Coins, bool) { + diff := coins.safePlus(coinsB.negative()) + return diff, !diff.IsNotNegative() +} + +// IsAllGT returns true iff for every denom in coins, the denom is present at a // greater amount in coinsB. func (coins Coins) IsAllGT(coinsB Coins) bool { - diff := coins.Minus(coinsB) + diff, _ := coins.SafeMinus(coinsB) if len(diff) == 0 { return false } + return diff.IsPositive() } -// IsAllGTE returns True iff for every denom in coins, the denom is present at an -// equal or greater amount in coinsB. +// IsAllGTE returns true iff for every denom in coins, the denom is present at +// an equal or greater amount in coinsB. func (coins Coins) IsAllGTE(coinsB Coins) bool { - diff := coins.Minus(coinsB) + diff, _ := coins.SafeMinus(coinsB) if len(diff) == 0 { return true } + return diff.IsNotNegative() } // IsAllLT returns True iff for every denom in coins, the denom is present at // a smaller amount in coinsB. func (coins Coins) IsAllLT(coinsB Coins) bool { - diff := coinsB.Minus(coins) - if len(diff) == 0 { - return false - } - return diff.IsPositive() + return coinsB.IsAllGT(coins) } -// IsAllLTE returns True iff for every denom in coins, the denom is present at +// IsAllLTE returns true iff for every denom in coins, the denom is present at // a smaller or equal amount in coinsB. func (coins Coins) IsAllLTE(coinsB Coins) bool { - diff := coinsB.Minus(coins) - if len(diff) == 0 { - return true - } - return diff.IsNotNegative() + return coinsB.IsAllGTE(coins) } // IsZero returns true if there are no coins or all coins are zero. @@ -239,40 +300,22 @@ func (coins Coins) IsEqual(coinsB Coins) bool { if len(coins) != len(coinsB) { return false } + + coins = coins.Sort() + coinsB = coinsB.Sort() + for i := 0; i < len(coins); i++ { if coins[i].Denom != coinsB[i].Denom || !coins[i].Amount.Equal(coinsB[i].Amount) { return false } } + return true } -// IsPositive returns true if there is at least one coin, and all -// currencies have a positive value -func (coins Coins) IsPositive() bool { - if len(coins) == 0 { - return false - } - for _, coin := range coins { - if !coin.IsPositive() { - return false - } - } - return true -} - -// IsNotNegative returns true if there is no currency with a negative value -// (even no coins is true here) -func (coins Coins) IsNotNegative() bool { - if len(coins) == 0 { - return true - } - for _, coin := range coins { - if !coin.IsNotNegative() { - return false - } - } - return true +// Empty returns true if there are no coins and false otherwise. +func (coins Coins) Empty() bool { + return len(coins) == 0 } // Returns the amount of a denom from coins @@ -280,15 +323,18 @@ func (coins Coins) AmountOf(denom string) Int { switch len(coins) { case 0: return ZeroInt() + case 1: coin := coins[0] if coin.Denom == denom { return coin.Amount } return ZeroInt() + default: midIdx := len(coins) / 2 // 2:1, 3:1, 4:2 coin := coins[midIdx] + if denom < coin.Denom { return coins[:midIdx].AmountOf(denom) } else if denom == coin.Denom { @@ -299,7 +345,75 @@ func (coins Coins) AmountOf(denom string) Int { } } -//---------------------------------------- +// IsPositive returns true if there is at least one coin and all currencies +// have a positive value. +// +// TODO: Remove once unsigned integers are used. +func (coins Coins) IsPositive() bool { + if len(coins) == 0 { + return false + } + + for _, coin := range coins { + if !coin.IsPositive() { + return false + } + } + + return true +} + +// IsNotNegative returns true if there is no coin amount with a negative value +// (even no coins is true here). +// +// TODO: Remove once unsigned integers are used. +func (coins Coins) IsNotNegative() bool { + if len(coins) == 0 { + return true + } + + for _, coin := range coins { + if !coin.IsNotNegative() { + return false + } + } + + return true +} + +// negative returns a set of coins with all amount negative. +// +// TODO: Remove once unsigned integers are used. +func (coins Coins) negative() Coins { + res := make([]Coin, 0, len(coins)) + + for _, coin := range coins { + res = append(res, Coin{ + Denom: coin.Denom, + Amount: coin.Amount.Neg(), + }) + } + + return res +} + +// removeZeroCoins removes all zero coins from the given coin set in-place. +func removeZeroCoins(coins Coins) Coins { + i, l := 0, len(coins) + for i < l { + if coins[i].IsZero() { + // remove coin + coins = append(coins[:i], coins[i+1:]...) + l-- + } else { + i++ + } + } + + return coins[:i] +} + +//----------------------------------------------------------------------------- // Sort interface //nolint @@ -315,7 +429,7 @@ func (coins Coins) Sort() Coins { return coins } -//---------------------------------------- +//----------------------------------------------------------------------------- // Parsing var ( @@ -333,17 +447,17 @@ func ParseCoin(coinStr string) (coin Coin, err error) { matches := reCoin.FindStringSubmatch(coinStr) if matches == nil { - err = fmt.Errorf("invalid coin expression: %s", coinStr) - return + return Coin{}, fmt.Errorf("invalid coin expression: %s", coinStr) } + denomStr, amountStr := matches[2], matches[1] - amount, err := strconv.Atoi(amountStr) - if err != nil { - return + amount, ok := NewIntFromString(amountStr) + if !ok { + return Coin{}, fmt.Errorf("failed to parse coin amount: %s", amountStr) } - return Coin{denomStr, NewInt(int64(amount))}, nil + return Coin{denomStr, amount}, nil } // ParseCoins will parse out a list of coins separated by commas. diff --git a/types/coin_benchmark_test.go b/types/coin_benchmark_test.go new file mode 100644 index 000000000..9c13bf772 --- /dev/null +++ b/types/coin_benchmark_test.go @@ -0,0 +1,64 @@ +package types + +import ( + "fmt" + "testing" +) + +func BenchmarkCoinsAdditionIntersect(b *testing.B) { + benchmarkingFunc := func(numCoinsA int, numCoinsB int) func(b *testing.B) { + return func(b *testing.B) { + coinsA := Coins(make([]Coin, numCoinsA)) + coinsB := Coins(make([]Coin, numCoinsB)) + + for i := 0; i < numCoinsA; i++ { + coinsA[i] = NewCoin("COINZ_"+string(i), NewInt(int64(i))) + } + for i := 0; i < numCoinsB; i++ { + coinsB[i] = NewCoin("COINZ_"+string(i), NewInt(int64(i))) + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + coinsA.Plus(coinsB) + } + } + } + + benchmarkSizes := [][]int{{1, 1}, {5, 5}, {5, 20}, {1, 1000}, {2, 1000}} + for i := 0; i < len(benchmarkSizes); i++ { + sizeA := benchmarkSizes[i][0] + sizeB := benchmarkSizes[i][1] + b.Run(fmt.Sprintf("sizes: A_%d, B_%d", sizeA, sizeB), benchmarkingFunc(sizeA, sizeB)) + } +} + +func BenchmarkCoinsAdditionNoIntersect(b *testing.B) { + benchmarkingFunc := func(numCoinsA int, numCoinsB int) func(b *testing.B) { + return func(b *testing.B) { + coinsA := Coins(make([]Coin, numCoinsA)) + coinsB := Coins(make([]Coin, numCoinsB)) + + for i := 0; i < numCoinsA; i++ { + coinsA[i] = NewCoin("COINZ_"+string(numCoinsB+i), NewInt(int64(i))) + } + for i := 0; i < numCoinsB; i++ { + coinsB[i] = NewCoin("COINZ_"+string(i), NewInt(int64(i))) + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + coinsA.Plus(coinsB) + } + } + } + + benchmarkSizes := [][]int{{1, 1}, {5, 5}, {5, 20}, {1, 1000}, {2, 1000}, {1000, 2}} + for i := 0; i < len(benchmarkSizes); i++ { + sizeA := benchmarkSizes[i][0] + sizeB := benchmarkSizes[i][1] + b.Run(fmt.Sprintf("sizes: A_%d, B_%d", sizeA, sizeB), benchmarkingFunc(sizeA, sizeB)) + } +} diff --git a/types/coin_test.go b/types/coin_test.go index 77307f22a..774534860 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -1,43 +1,20 @@ package types import ( - "fmt" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestIsPositiveCoin(t *testing.T) { - cases := []struct { - inputOne Coin - expected bool - }{ - {NewInt64Coin("A", 1), true}, - {NewInt64Coin("A", 0), false}, - {NewInt64Coin("a", -1), false}, - } +// ---------------------------------------------------------------------------- +// Coin tests - for tcIndex, tc := range cases { - res := tc.inputOne.IsPositive() - require.Equal(t, tc.expected, res, "%s positivity is incorrect, tc #%d", tc.inputOne.String(), tcIndex) - } -} - -func TestIsNotNegativeCoin(t *testing.T) { - cases := []struct { - inputOne Coin - expected bool - }{ - {NewInt64Coin("A", 1), true}, - {NewInt64Coin("A", 0), true}, - {NewInt64Coin("a", -1), false}, - } - - for tcIndex, tc := range cases { - res := tc.inputOne.IsNotNegative() - require.Equal(t, tc.expected, res, "%s not-negativity is incorrect, tc #%d", tc.inputOne.String(), tcIndex) - } +func TestCoin(t *testing.T) { + require.Panics(t, func() { NewInt64Coin("A", -1) }) + require.Panics(t, func() { NewCoin("A", NewInt(-1)) }) + require.Equal(t, NewInt(5), NewInt64Coin("A", 5).Amount) + require.Equal(t, NewInt(5), NewCoin("A", NewInt(5)).Amount) } func TestSameDenomAsCoin(t *testing.T) { @@ -49,8 +26,7 @@ func TestSameDenomAsCoin(t *testing.T) { {NewInt64Coin("A", 1), NewInt64Coin("A", 1), true}, {NewInt64Coin("A", 1), NewInt64Coin("a", 1), false}, {NewInt64Coin("a", 1), NewInt64Coin("b", 1), false}, - {NewInt64Coin("stake", 1), NewInt64Coin("stake", 10), true}, - {NewInt64Coin("stake", -11), NewInt64Coin("stake", 10), true}, + {NewInt64Coin("steak", 1), NewInt64Coin("steak", 10), true}, } for tcIndex, tc := range cases { @@ -59,6 +35,78 @@ func TestSameDenomAsCoin(t *testing.T) { } } +func TestIsEqualCoin(t *testing.T) { + cases := []struct { + inputOne Coin + inputTwo Coin + expected bool + }{ + {NewInt64Coin("A", 1), NewInt64Coin("A", 1), true}, + {NewInt64Coin("A", 1), NewInt64Coin("a", 1), false}, + {NewInt64Coin("a", 1), NewInt64Coin("b", 1), false}, + {NewInt64Coin("steak", 1), NewInt64Coin("steak", 10), false}, + } + + for tcIndex, tc := range cases { + res := tc.inputOne.IsEqual(tc.inputTwo) + require.Equal(t, tc.expected, res, "coin equality relation is incorrect, tc #%d", tcIndex) + } +} + +func TestPlusCoin(t *testing.T) { + cases := []struct { + inputOne Coin + inputTwo Coin + expected Coin + shouldPanic bool + }{ + {NewInt64Coin("A", 1), NewInt64Coin("A", 1), NewInt64Coin("A", 2), false}, + {NewInt64Coin("A", 1), NewInt64Coin("A", 0), NewInt64Coin("A", 1), false}, + {NewInt64Coin("A", 1), NewInt64Coin("B", 1), NewInt64Coin("A", 1), true}, + } + + for tcIndex, tc := range cases { + if tc.shouldPanic { + require.Panics(t, func() { tc.inputOne.Plus(tc.inputTwo) }) + } else { + res := tc.inputOne.Plus(tc.inputTwo) + require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex) + } + } +} + +func TestMinusCoin(t *testing.T) { + cases := []struct { + inputOne Coin + inputTwo Coin + expected Coin + shouldPanic bool + }{ + {NewInt64Coin("A", 1), NewInt64Coin("B", 1), NewInt64Coin("A", 1), true}, + {NewInt64Coin("A", 10), NewInt64Coin("A", 1), NewInt64Coin("A", 9), false}, + {NewInt64Coin("A", 5), NewInt64Coin("A", 3), NewInt64Coin("A", 2), false}, + {NewInt64Coin("A", 5), NewInt64Coin("A", 0), NewInt64Coin("A", 5), false}, + {NewInt64Coin("A", 1), NewInt64Coin("A", 5), Coin{}, true}, + } + + for tcIndex, tc := range cases { + if tc.shouldPanic { + require.Panics(t, func() { tc.inputOne.Minus(tc.inputTwo) }) + } else { + res := tc.inputOne.Minus(tc.inputTwo) + require.Equal(t, tc.expected, res, "difference of coins is incorrect, tc #%d", tcIndex) + } + } + + tc := struct { + inputOne Coin + inputTwo Coin + expected int64 + }{NewInt64Coin("A", 1), NewInt64Coin("A", 1), 0} + res := tc.inputOne.Minus(tc.inputTwo) + require.Equal(t, tc.expected, res.Amount.Int64()) +} + func TestIsGTECoin(t *testing.T) { cases := []struct { inputOne Coin @@ -67,8 +115,7 @@ func TestIsGTECoin(t *testing.T) { }{ {NewInt64Coin("A", 1), NewInt64Coin("A", 1), true}, {NewInt64Coin("A", 2), NewInt64Coin("A", 1), true}, - {NewInt64Coin("A", -1), NewInt64Coin("A", 5), false}, - {NewInt64Coin("a", 1), NewInt64Coin("b", 1), false}, + {NewInt64Coin("A", 1), NewInt64Coin("B", 1), false}, } for tcIndex, tc := range cases { @@ -85,7 +132,6 @@ func TestIsLTCoin(t *testing.T) { }{ {NewInt64Coin("A", 1), NewInt64Coin("A", 1), false}, {NewInt64Coin("A", 2), NewInt64Coin("A", 1), false}, - {NewInt64Coin("A", -1), NewInt64Coin("A", 5), true}, {NewInt64Coin("a", 0), NewInt64Coin("b", 1), false}, {NewInt64Coin("a", 1), NewInt64Coin("b", 1), false}, {NewInt64Coin("a", 1), NewInt64Coin("a", 1), false}, @@ -98,76 +144,18 @@ func TestIsLTCoin(t *testing.T) { } } -func TestIsEqualCoin(t *testing.T) { - cases := []struct { - inputOne Coin - inputTwo Coin - expected bool - }{ - {NewInt64Coin("A", 1), NewInt64Coin("A", 1), true}, - {NewInt64Coin("A", 1), NewInt64Coin("a", 1), false}, - {NewInt64Coin("a", 1), NewInt64Coin("b", 1), false}, - {NewInt64Coin("stake", 1), NewInt64Coin("stake", 10), false}, - {NewInt64Coin("stake", -11), NewInt64Coin("stake", 10), false}, - } +func TestCoinIsZero(t *testing.T) { + coin := NewInt64Coin("A", 0) + res := coin.IsZero() + require.True(t, res) - for tcIndex, tc := range cases { - res := tc.inputOne.IsEqual(tc.inputTwo) - require.Equal(t, tc.expected, res, "coin equality relation is incorrect, tc #%d", tcIndex) - } + coin = NewInt64Coin("A", 1) + res = coin.IsZero() + require.False(t, res) } -func TestPlusCoin(t *testing.T) { - cases := []struct { - inputOne Coin - inputTwo Coin - expected Coin - }{ - {NewInt64Coin("A", 1), NewInt64Coin("A", 1), NewInt64Coin("A", 2)}, - {NewInt64Coin("A", 1), NewInt64Coin("B", 1), NewInt64Coin("A", 1)}, - {NewInt64Coin("asdf", -4), NewInt64Coin("asdf", 5), NewInt64Coin("asdf", 1)}, - } - - for tcIndex, tc := range cases { - res := tc.inputOne.Plus(tc.inputTwo) - require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex) - } - - tc := struct { - inputOne Coin - inputTwo Coin - expected int64 - }{NewInt64Coin("asdf", -1), NewInt64Coin("asdf", 1), 0} - res := tc.inputOne.Plus(tc.inputTwo) - require.Equal(t, tc.expected, res.Amount.Int64()) -} - -func TestMinusCoin(t *testing.T) { - cases := []struct { - inputOne Coin - inputTwo Coin - expected Coin - }{ - - {NewInt64Coin("A", 1), NewInt64Coin("B", 1), NewInt64Coin("A", 1)}, - {NewInt64Coin("asdf", -4), NewInt64Coin("asdf", 5), NewInt64Coin("asdf", -9)}, - {NewInt64Coin("asdf", 10), NewInt64Coin("asdf", 1), NewInt64Coin("asdf", 9)}, - } - - for tcIndex, tc := range cases { - res := tc.inputOne.Minus(tc.inputTwo) - require.Equal(t, tc.expected, res, "difference of coins is incorrect, tc #%d", tcIndex) - } - - tc := struct { - inputOne Coin - inputTwo Coin - expected int64 - }{NewInt64Coin("A", 1), NewInt64Coin("A", 1), 0} - res := tc.inputOne.Minus(tc.inputTwo) - require.Equal(t, tc.expected, res.Amount.Int64()) - -} +// ---------------------------------------------------------------------------- +// Coins tests func TestIsZeroCoins(t *testing.T) { cases := []struct { @@ -199,8 +187,7 @@ func TestEqualCoins(t *testing.T) { {Coins{NewInt64Coin("A", 0)}, Coins{NewInt64Coin("B", 0)}, false}, {Coins{NewInt64Coin("A", 0)}, Coins{NewInt64Coin("A", 1)}, false}, {Coins{NewInt64Coin("A", 0)}, Coins{NewInt64Coin("A", 0), NewInt64Coin("B", 1)}, false}, - // TODO: is it expected behaviour? shouldn't we sort the coins before comparing them? - {Coins{NewInt64Coin("A", 0), NewInt64Coin("B", 1)}, Coins{NewInt64Coin("B", 1), NewInt64Coin("A", 0)}, false}, + {Coins{NewInt64Coin("A", 0), NewInt64Coin("B", 1)}, Coins{NewInt64Coin("B", 1), NewInt64Coin("A", 0)}, true}, } for tcnum, tc := range cases { @@ -209,16 +196,65 @@ func TestEqualCoins(t *testing.T) { } } -func TestCoins(t *testing.T) { +func TestPlusCoins(t *testing.T) { + zero := NewInt(0) + one := NewInt(1) + two := NewInt(2) - //Define the coins to be used in tests + cases := []struct { + inputOne Coins + inputTwo Coins + expected Coins + }{ + {Coins{{"A", one}, {"B", one}}, Coins{{"A", one}, {"B", one}}, Coins{{"A", two}, {"B", two}}}, + {Coins{{"A", zero}, {"B", one}}, Coins{{"A", zero}, {"B", zero}}, Coins{{"B", one}}}, + {Coins{{"A", two}}, Coins{{"B", zero}}, Coins{{"A", two}}}, + {Coins{{"A", one}}, Coins{{"A", one}, {"B", two}}, Coins{{"A", two}, {"B", two}}}, + {Coins{{"A", zero}, {"B", zero}}, Coins{{"A", zero}, {"B", zero}}, Coins(nil)}, + } + + for tcIndex, tc := range cases { + res := tc.inputOne.Plus(tc.inputTwo) + assert.True(t, res.IsValid()) + require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex) + } +} + +func TestMinusCoins(t *testing.T) { + zero := NewInt(0) + one := NewInt(1) + two := NewInt(2) + + testCases := []struct { + inputOne Coins + inputTwo Coins + expected Coins + shouldPanic bool + }{ + {Coins{{"A", two}}, Coins{{"A", one}, {"B", two}}, Coins{{"A", one}, {"B", two}}, true}, + {Coins{{"A", two}}, Coins{{"B", zero}}, Coins{{"A", two}}, false}, + {Coins{{"A", one}}, Coins{{"B", zero}}, Coins{{"A", one}}, false}, + {Coins{{"A", one}, {"B", one}}, Coins{{"A", one}}, Coins{{"B", one}}, false}, + {Coins{{"A", one}, {"B", one}}, Coins{{"A", two}}, Coins{}, true}, + } + + for i, tc := range testCases { + if tc.shouldPanic { + require.Panics(t, func() { tc.inputOne.Minus(tc.inputTwo) }) + } else { + res := tc.inputOne.Minus(tc.inputTwo) + assert.True(t, res.IsValid()) + require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", i) + } + } +} + +func TestCoins(t *testing.T) { good := Coins{ {"GAS", NewInt(1)}, {"MINERAL", NewInt(1)}, {"TREE", NewInt(1)}, } - neg := good.Negative() - sum := good.Plus(neg) empty := Coins{ {"GOLD", NewInt(0)}, } @@ -228,6 +264,7 @@ func TestCoins(t *testing.T) { {"GAS", NewInt(1)}, {"MINERAL", NewInt(1)}, } + // both are after the first one, but the second and third are in the wrong order badSort2 := Coins{ {"GAS", NewInt(1)}, @@ -251,13 +288,10 @@ func TestCoins(t *testing.T) { assert.True(t, good.IsAllGTE(empty), "Expected %v to be >= %v", good, empty) assert.False(t, good.IsAllLT(empty), "Expected %v to be < %v", good, empty) assert.True(t, empty.IsAllLT(good), "Expected %v to be < %v", empty, good) - assert.False(t, neg.IsPositive(), "Expected neg coins to not be positive: %v", neg) - assert.Zero(t, len(sum), "Expected 0 coins") assert.False(t, badSort1.IsValid(), "Coins are not sorted") 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") - } func TestCoinsGT(t *testing.T) { @@ -314,32 +348,6 @@ func TestCoinsLTE(t *testing.T) { assert.True(t, Coins{}.IsAllLTE(Coins{{"A", one}})) } -func TestPlusCoins(t *testing.T) { - one := NewInt(1) - zero := NewInt(0) - negone := NewInt(-1) - two := NewInt(2) - - cases := []struct { - inputOne Coins - inputTwo Coins - expected Coins - }{ - {Coins{{"A", one}, {"B", one}}, Coins{{"A", one}, {"B", one}}, Coins{{"A", two}, {"B", two}}}, - {Coins{{"A", zero}, {"B", one}}, Coins{{"A", zero}, {"B", zero}}, Coins{{"B", one}}}, - {Coins{{"A", zero}, {"B", zero}}, Coins{{"A", zero}, {"B", zero}}, Coins(nil)}, - {Coins{{"A", one}, {"B", zero}}, Coins{{"A", negone}, {"B", zero}}, Coins(nil)}, - {Coins{{"A", negone}, {"B", zero}}, Coins{{"A", zero}, {"B", zero}}, Coins{{"A", negone}}}, - } - - for tcIndex, tc := range cases { - res := tc.inputOne.Plus(tc.inputTwo) - assert.True(t, res.IsValid()) - require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex) - } -} - -//Test the parsing of Coin and Coins func TestParse(t *testing.T) { one := NewInt(1) @@ -370,11 +378,9 @@ func TestParse(t *testing.T) { require.Equal(t, tc.expected, res, "coin parsing was incorrect, tc #%d", tcIndex) } } - } func TestSortCoins(t *testing.T) { - good := Coins{ NewInt64Coin("GAS", 1), NewInt64Coin("MINERAL", 1), @@ -424,7 +430,6 @@ func TestSortCoins(t *testing.T) { } func TestAmountOf(t *testing.T) { - case0 := Coins{} case1 := Coins{ NewInt64Coin("", 0), @@ -481,55 +486,3 @@ func TestAmountOf(t *testing.T) { assert.Equal(t, NewInt(tc.amountOfTREE), tc.coins.AmountOf("TREE")) } } - -func BenchmarkCoinsAdditionIntersect(b *testing.B) { - benchmarkingFunc := func(numCoinsA int, numCoinsB int) func(b *testing.B) { - return func(b *testing.B) { - coinsA := Coins(make([]Coin, numCoinsA)) - coinsB := Coins(make([]Coin, numCoinsB)) - for i := 0; i < numCoinsA; i++ { - coinsA[i] = NewCoin("COINZ_"+string(i), NewInt(int64(i))) - } - for i := 0; i < numCoinsB; i++ { - coinsB[i] = NewCoin("COINZ_"+string(i), NewInt(int64(i))) - } - b.ResetTimer() - for i := 0; i < b.N; i++ { - coinsA.Plus(coinsB) - } - } - } - - benchmarkSizes := [][]int{{1, 1}, {5, 5}, {5, 20}, {1, 1000}, {2, 1000}} - for i := 0; i < len(benchmarkSizes); i++ { - sizeA := benchmarkSizes[i][0] - sizeB := benchmarkSizes[i][1] - b.Run(fmt.Sprintf("sizes: A_%d, B_%d", sizeA, sizeB), benchmarkingFunc(sizeA, sizeB)) - } -} - -func BenchmarkCoinsAdditionNoIntersect(b *testing.B) { - benchmarkingFunc := func(numCoinsA int, numCoinsB int) func(b *testing.B) { - return func(b *testing.B) { - coinsA := Coins(make([]Coin, numCoinsA)) - coinsB := Coins(make([]Coin, numCoinsB)) - for i := 0; i < numCoinsA; i++ { - coinsA[i] = NewCoin("COINZ_"+string(numCoinsB+i), NewInt(int64(i))) - } - for i := 0; i < numCoinsB; i++ { - coinsB[i] = NewCoin("COINZ_"+string(i), NewInt(int64(i))) - } - b.ResetTimer() - for i := 0; i < b.N; i++ { - coinsA.Plus(coinsB) - } - } - } - - benchmarkSizes := [][]int{{1, 1}, {5, 5}, {5, 20}, {1, 1000}, {2, 1000}, {1000, 2}} - for i := 0; i < len(benchmarkSizes); i++ { - sizeA := benchmarkSizes[i][0] - sizeB := benchmarkSizes[i][1] - b.Run(fmt.Sprintf("sizes: A_%d, B_%d", sizeA, sizeB), benchmarkingFunc(sizeA, sizeB)) - } -} diff --git a/types/int.go b/types/int.go index 2083168e9..417a1763d 100644 --- a/types/int.go +++ b/types/int.go @@ -322,11 +322,11 @@ func NewUint(n uint64) Uint { // NewUintFromBigUint constructs Uint from big.Uint func NewUintFromBigInt(i *big.Int) Uint { - // Check overflow - if i.Sign() == -1 || i.Sign() == 1 && i.BitLen() > 256 { + res := Uint{i} + if UintOverflow(res) { panic("Uint overflow") } - return Uint{i} + return res } // NewUintFromString constructs Uint from string @@ -353,11 +353,12 @@ func NewUintWithDecimal(n uint64, dec int) Uint { i := new(big.Int) i.Mul(new(big.Int).SetUint64(n), exp) - // Check overflow - if i.Sign() == -1 || i.Sign() == 1 && i.BitLen() > 256 { + res := Uint{i} + if UintOverflow(res) { panic("NewUintWithDecimal() out of bound") } - return Uint{i} + + return res } // ZeroUint returns Uint value with zero @@ -408,8 +409,7 @@ func (i Uint) LT(i2 Uint) bool { // Add adds Uint from another func (i Uint) Add(i2 Uint) (res Uint) { res = Uint{add(i.i, i2.i)} - // Check overflow - if res.Sign() == -1 || res.Sign() == 1 && res.i.BitLen() > 256 { + if UintOverflow(res) { panic("Uint overflow") } return @@ -423,13 +423,23 @@ func (i Uint) AddRaw(i2 uint64) Uint { // Sub subtracts Uint from another func (i Uint) Sub(i2 Uint) (res Uint) { res = Uint{sub(i.i, i2.i)} - // Check overflow - if res.Sign() == -1 || res.Sign() == 1 && res.i.BitLen() > 256 { + if UintOverflow(res) { panic("Uint overflow") } return } +// SafeSub attempts to subtract one Uint from another. A boolean is also returned +// indicating if the result contains integer overflow. +func (i Uint) SafeSub(i2 Uint) (Uint, bool) { + res := Uint{sub(i.i, i2.i)} + if UintOverflow(res) { + return res, true + } + + return res, false +} + // SubRaw subtracts uint64 from Uint func (i Uint) SubRaw(i2 uint64) Uint { return i.Sub(NewUint(i2)) @@ -437,15 +447,15 @@ func (i Uint) SubRaw(i2 uint64) Uint { // Mul multiples two Uints func (i Uint) Mul(i2 Uint) (res Uint) { - // Check overflow if i.i.BitLen()+i2.i.BitLen()-1 > 256 { panic("Uint overflow") } + res = Uint{mul(i.i, i2.i)} - // Check overflow - if res.Sign() == -1 || res.Sign() == 1 && res.i.BitLen() > 256 { + if UintOverflow(res) { panic("Uint overflow") } + return } @@ -530,6 +540,12 @@ func (i *Uint) UnmarshalJSON(bz []byte) error { //__________________________________________________________________________ +// UintOverflow returns true if a given unsigned integer overflows and false +// otherwise. +func UintOverflow(x Uint) bool { + return x.i.Sign() == -1 || x.i.Sign() == 1 && x.i.BitLen() > 256 +} + // AddUint64Overflow performs the addition operation on two uint64 integers and // returns a boolean on whether or not the result overflows. func AddUint64Overflow(a, b uint64) (uint64, bool) { diff --git a/types/int_test.go b/types/int_test.go index 78bee66bf..9e189858c 100644 --- a/types/int_test.go +++ b/types/int_test.go @@ -591,6 +591,31 @@ func TestEncodingTableUint(t *testing.T) { } } +func TestSafeSub(t *testing.T) { + testCases := []struct { + x, y Uint + expected uint64 + overflow bool + }{ + {NewUint(0), NewUint(0), 0, false}, + {NewUint(10), NewUint(5), 5, false}, + {NewUint(5), NewUint(10), 5, true}, + {NewUint(math.MaxUint64), NewUint(0), math.MaxUint64, false}, + } + + for i, tc := range testCases { + res, overflow := tc.x.SafeSub(tc.y) + require.Equal( + t, tc.overflow, overflow, + "invalid overflow result; x: %s, y: %s, tc: #%d", tc.x, tc.y, i, + ) + require.Equal( + t, tc.expected, res.BigInt().Uint64(), + "invalid subtraction result; x: %s, y: %s, tc: #%d", tc.x, tc.y, i, + ) + } +} + func TestAddUint64Overflow(t *testing.T) { testCases := []struct { a, b uint64 diff --git a/x/auth/ante.go b/x/auth/ante.go index 7beb643c5..9a7a15e3e 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -281,24 +281,36 @@ func deductFees(acc Account, fee StdFee) (Account, sdk.Result) { coins := acc.GetCoins() feeAmount := fee.Amount - newCoins := coins.Minus(feeAmount) - if !newCoins.IsNotNegative() { + if !feeAmount.IsValid() { + return nil, sdk.ErrInsufficientFee(fmt.Sprintf("invalid fee amount: %s", feeAmount)).Result() + } + + newCoins, ok := coins.SafeMinus(feeAmount) + if ok { errMsg := fmt.Sprintf("%s < %s", coins, feeAmount) return nil, sdk.ErrInsufficientFunds(errMsg).Result() } + err := acc.SetCoins(newCoins) if err != nil { // Handle w/ #870 panic(err) } + return acc, sdk.Result{} } func ensureSufficientMempoolFees(ctx sdk.Context, stdTx StdTx) sdk.Result { // currently we use a very primitive gas pricing model with a constant gasPrice. // adjustFeesByGas handles calculating the amount of fees required based on the provided gas. - // TODO: Make the gasPrice not a constant, and account for tx size. - requiredFees := adjustFeesByGas(ctx.MinimumFees(), stdTx.Fee.Gas) + // + // TODO: + // - Make the gasPrice not a constant, and account for tx size. + // - Make Gas an unsigned integer and use tx basic validation + if stdTx.Fee.Gas <= 0 { + return sdk.ErrInternal(fmt.Sprintf("invalid gas supplied: %d", stdTx.Fee.Gas)).Result() + } + requiredFees := adjustFeesByGas(ctx.MinimumFees(), uint64(stdTx.Fee.Gas)) // NOTE: !A.IsAllGTE(B) is not the same as A.IsAllLT(B). if !ctx.MinimumFees().IsZero() && !stdTx.Fee.Amount.IsAllGTE(requiredFees) { diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index 0e8d13957..566892e07 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -708,7 +708,6 @@ func TestAdjustFeesByGas(t *testing.T) { }{ {"nil coins", args{sdk.Coins{}, 10000}, sdk.Coins{}}, {"nil coins", args{sdk.Coins{sdk.NewInt64Coin("A", 10), sdk.NewInt64Coin("B", 0)}, 10000}, sdk.Coins{sdk.NewInt64Coin("A", 20), sdk.NewInt64Coin("B", 10)}}, - {"negative coins", args{sdk.Coins{sdk.NewInt64Coin("A", -10), sdk.NewInt64Coin("B", 10)}, 10000}, sdk.Coins{sdk.NewInt64Coin("B", 20)}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/x/bank/keeper.go b/x/bank/keeper.go index 3a33870e1..af930953f 100644 --- a/x/bank/keeper.go +++ b/x/bank/keeper.go @@ -182,11 +182,13 @@ func hasCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt s // SubtractCoins subtracts amt from the coins at the addr. func subtractCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error) { ctx.GasMeter().ConsumeGas(costSubtractCoins, "subtractCoins") + oldCoins := getCoins(ctx, am, addr) - newCoins := oldCoins.Minus(amt) - if !newCoins.IsNotNegative() { + newCoins, hasNeg := oldCoins.SafeMinus(amt) + if hasNeg { return amt, nil, sdk.ErrInsufficientCoins(fmt.Sprintf("%s < %s", oldCoins, amt)) } + err := setCoins(ctx, am, addr, newCoins) tags := sdk.NewTags("sender", []byte(addr.String())) return newCoins, tags, err diff --git a/x/bank/msgs_test.go b/x/bank/msgs_test.go index 14e9a4c70..040d12bda 100644 --- a/x/bank/msgs_test.go +++ b/x/bank/msgs_test.go @@ -35,8 +35,6 @@ func TestInputValidation(t *testing.T) { emptyCoins := sdk.Coins{} emptyCoins2 := sdk.Coins{sdk.NewInt64Coin("eth", 0)} someEmptyCoins := sdk.Coins{sdk.NewInt64Coin("eth", 10), sdk.NewInt64Coin("atom", 0)} - minusCoins := sdk.Coins{sdk.NewInt64Coin("eth", -34)} - someMinusCoins := sdk.Coins{sdk.NewInt64Coin("atom", 20), sdk.NewInt64Coin("eth", -34)} unsortedCoins := sdk.Coins{sdk.NewInt64Coin("eth", 1), sdk.NewInt64Coin("atom", 1)} cases := []struct { @@ -52,8 +50,6 @@ func TestInputValidation(t *testing.T) { {false, NewInput(addr1, emptyCoins)}, // invalid coins {false, NewInput(addr1, emptyCoins2)}, // invalid coins {false, NewInput(addr1, someEmptyCoins)}, // invalid coins - {false, NewInput(addr1, minusCoins)}, // negative coins - {false, NewInput(addr1, someMinusCoins)}, // negative coins {false, NewInput(addr1, unsortedCoins)}, // unsorted coins } @@ -77,8 +73,6 @@ func TestOutputValidation(t *testing.T) { emptyCoins := sdk.Coins{} emptyCoins2 := sdk.Coins{sdk.NewInt64Coin("eth", 0)} someEmptyCoins := sdk.Coins{sdk.NewInt64Coin("eth", 10), sdk.NewInt64Coin("atom", 0)} - minusCoins := sdk.Coins{sdk.NewInt64Coin("eth", -34)} - someMinusCoins := sdk.Coins{sdk.NewInt64Coin("atom", 20), sdk.NewInt64Coin("eth", -34)} unsortedCoins := sdk.Coins{sdk.NewInt64Coin("eth", 1), sdk.NewInt64Coin("atom", 1)} cases := []struct { @@ -94,8 +88,6 @@ func TestOutputValidation(t *testing.T) { {false, NewOutput(addr1, emptyCoins)}, // invalid coins {false, NewOutput(addr1, emptyCoins2)}, // invalid coins {false, NewOutput(addr1, someEmptyCoins)}, // invalid coins - {false, NewOutput(addr1, minusCoins)}, // negative coins - {false, NewOutput(addr1, someMinusCoins)}, // negative coins {false, NewOutput(addr1, unsortedCoins)}, // unsorted coins } diff --git a/x/bank/simulation/msgs.go b/x/bank/simulation/msgs.go index f33c5e0c9..f1fd866c1 100644 --- a/x/bank/simulation/msgs.go +++ b/x/bank/simulation/msgs.go @@ -82,7 +82,7 @@ func createSingleInputSendMsg(r *rand.Rand, ctx sdk.Context, accs []simulation.A toAddr.String(), ) - coins := sdk.Coins{{initFromCoins[denomIndex].Denom, amt}} + coins := sdk.Coins{sdk.NewCoin(initFromCoins[denomIndex].Denom, amt)} msg = bank.MsgSend{ Inputs: []bank.Input{bank.NewInput(fromAcc.Address, coins)}, Outputs: []bank.Output{bank.NewOutput(toAddr, coins)}, diff --git a/x/gov/msgs_test.go b/x/gov/msgs_test.go index e488d2aba..4a661985c 100644 --- a/x/gov/msgs_test.go +++ b/x/gov/msgs_test.go @@ -13,7 +13,6 @@ import ( var ( coinsPos = sdk.Coins{sdk.NewInt64Coin(stakeTypes.DefaultBondDenom, 1000)} coinsZero = sdk.Coins{} - coinsNeg = sdk.Coins{sdk.NewInt64Coin(stakeTypes.DefaultBondDenom, -10000)} coinsPosNotAtoms = sdk.Coins{sdk.NewInt64Coin("foo", 10000)} coinsMulti = sdk.Coins{sdk.NewInt64Coin(stakeTypes.DefaultBondDenom, 1000), sdk.NewInt64Coin("foo", 10000)} ) @@ -40,7 +39,6 @@ func TestMsgSubmitProposal(t *testing.T) { {"Test Proposal", "the purpose of this proposal is to test", 0x05, addrs[0], coinsPos, false}, {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, sdk.AccAddress{}, coinsPos, false}, {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsZero, true}, - {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsNeg, false}, {"Test Proposal", "the purpose of this proposal is to test", ProposalTypeText, addrs[0], coinsMulti, true}, } @@ -66,7 +64,6 @@ func TestMsgDeposit(t *testing.T) { {0, addrs[0], coinsPos, true}, {1, sdk.AccAddress{}, coinsPos, false}, {1, addrs[0], coinsZero, true}, - {1, addrs[0], coinsNeg, false}, {1, addrs[0], coinsMulti, true}, } diff --git a/x/slashing/handler_test.go b/x/slashing/handler_test.go index 9041bfe56..c77150535 100644 --- a/x/slashing/handler_test.go +++ b/x/slashing/handler_test.go @@ -20,7 +20,11 @@ func TestCannotUnjailUnlessJailed(t *testing.T) { got := stake.NewHandler(sk)(ctx, msg) require.True(t, got.IsOK()) stake.EndBlocker(ctx, sk) - require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}}) + + require.Equal( + t, ck.GetCoins(ctx, sdk.AccAddress(addr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.Sub(amt))}, + ) require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, addr).GetPower())) // assert non-jailed validator can't be unjailed diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 94251ded3..e5318f292 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -35,7 +35,10 @@ func TestHandleDoubleSign(t *testing.T) { got := stake.NewHandler(sk)(ctx, NewTestMsgCreateValidator(operatorAddr, val, amt)) require.True(t, got.IsOK()) stake.EndBlocker(ctx, sk) - require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(operatorAddr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}}) + require.Equal( + t, ck.GetCoins(ctx, sdk.AccAddress(operatorAddr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.Sub(amt))}, + ) require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, operatorAddr).GetPower())) // handle a signature to set signing info @@ -76,7 +79,10 @@ func TestSlashingPeriodCap(t *testing.T) { require.True(t, got.IsOK()) stake.EndBlocker(ctx, sk) ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) - require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(operatorAddr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}}) + require.Equal( + t, ck.GetCoins(ctx, sdk.AccAddress(operatorAddr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.Sub(amt))}, + ) require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, operatorAddr).GetPower())) // handle a signature to set signing info @@ -140,8 +146,13 @@ func TestHandleAbsentValidator(t *testing.T) { got := sh(ctx, NewTestMsgCreateValidator(addr, val, amt)) require.True(t, got.IsOK()) stake.EndBlocker(ctx, sk) - require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}}) + + require.Equal( + t, ck.GetCoins(ctx, sdk.AccAddress(addr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.Sub(amt))}, + ) require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, addr).GetPower())) + // will exist since the validator has been bonded info, found := keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) @@ -296,7 +307,11 @@ func TestHandleNewValidator(t *testing.T) { got := sh(ctx, NewTestMsgCreateValidator(addr, val, sdk.NewInt(amt))) require.True(t, got.IsOK()) stake.EndBlocker(ctx, sk) - require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.SubRaw(amt)}}) + + require.Equal( + t, ck.GetCoins(ctx, sdk.AccAddress(addr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.SubRaw(amt))}, + ) require.Equal(t, sdk.NewDec(amt), sk.Validator(ctx, addr).GetPower()) // Now a validator, for two blocks diff --git a/x/slashing/tick_test.go b/x/slashing/tick_test.go index c6590c94e..932ac51a9 100644 --- a/x/slashing/tick_test.go +++ b/x/slashing/tick_test.go @@ -20,7 +20,10 @@ func TestBeginBlocker(t *testing.T) { got := stake.NewHandler(sk)(ctx, NewTestMsgCreateValidator(addr, pk, amt)) require.True(t, got.IsOK()) stake.EndBlocker(ctx, sk) - require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}}) + require.Equal( + t, ck.GetCoins(ctx, sdk.AccAddress(addr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.Sub(amt))}, + ) require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, addr).GetPower())) val := abci.Validator{ diff --git a/x/stake/types/msg_test.go b/x/stake/types/msg_test.go index 7e719f3ee..4b4055fca 100644 --- a/x/stake/types/msg_test.go +++ b/x/stake/types/msg_test.go @@ -12,7 +12,6 @@ import ( var ( coinPos = sdk.NewInt64Coin(DefaultBondDenom, 1000) coinZero = sdk.NewInt64Coin(DefaultBondDenom, 0) - coinNeg = sdk.NewInt64Coin(DefaultBondDenom, -10000) ) // test ValidateBasic for MsgCreateValidator @@ -34,8 +33,6 @@ func TestMsgCreateValidator(t *testing.T) { {"empty address", "a", "b", "c", "d", commission2, emptyAddr, pk1, coinPos, false}, {"empty pubkey", "a", "b", "c", "d", commission1, addr1, emptyPubkey, coinPos, true}, {"empty bond", "a", "b", "c", "d", commission2, addr1, pk1, coinZero, false}, - {"negative bond", "a", "b", "c", "d", commission2, addr1, pk1, coinNeg, false}, - {"negative bond", "a", "b", "c", "d", commission1, addr1, pk1, coinNeg, false}, } for _, tc := range tests { @@ -96,8 +93,6 @@ func TestMsgCreateValidatorOnBehalfOf(t *testing.T) { {"empty validator address", "a", "b", "c", "d", commission2, sdk.AccAddress(addr1), emptyAddr, pk2, coinPos, false}, {"empty pubkey", "a", "b", "c", "d", commission1, sdk.AccAddress(addr1), addr2, emptyPubkey, coinPos, true}, {"empty bond", "a", "b", "c", "d", commission2, sdk.AccAddress(addr1), addr2, pk2, coinZero, false}, - {"negative bond", "a", "b", "c", "d", commission1, sdk.AccAddress(addr1), addr2, pk2, coinNeg, false}, - {"negative bond", "a", "b", "c", "d", commission2, sdk.AccAddress(addr1), addr2, pk2, coinNeg, false}, } for _, tc := range tests { @@ -136,7 +131,6 @@ func TestMsgDelegate(t *testing.T) { {"empty delegator", sdk.AccAddress(emptyAddr), addr1, coinPos, false}, {"empty validator", sdk.AccAddress(addr1), emptyAddr, coinPos, false}, {"empty bond", sdk.AccAddress(addr1), addr2, coinZero, false}, - {"negative bond", sdk.AccAddress(addr1), addr2, coinNeg, false}, } for _, tc := range tests {