From be321373da2c9c856c7fb7c83bdb5d91fb999342 Mon Sep 17 00:00:00 2001
From: Ethan Frey <ethanfrey@users.noreply.github.com>
Date: Mon, 22 May 2017 11:22:41 +0200
Subject: [PATCH] 87: Sort coin order on ParseCoins to be less fragile

---
 types/coin.go      | 19 +++++++++-
 types/coin_test.go | 92 +++++++++++++++++++++++++++++++++++-----------
 2 files changed, 87 insertions(+), 24 deletions(-)

diff --git a/types/coin.go b/types/coin.go
index e34f7d1bdb..0499e58c0f 100644
--- a/types/coin.go
+++ b/types/coin.go
@@ -3,8 +3,11 @@ package types
 import (
 	"fmt"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
+
+	"github.com/pkg/errors"
 )
 
 type Coin struct {
@@ -53,9 +56,8 @@ func (coins Coins) String() string {
 }
 
 func ParseCoins(str string) (Coins, error) {
-
 	split := strings.Split(str, ",")
-	var coins []Coin
+	var coins Coins
 
 	for _, el := range split {
 		if len(el) > 0 {
@@ -67,6 +69,12 @@ func ParseCoins(str string) (Coins, error) {
 		}
 	}
 
+	// ensure they are in proper order, to avoid random failures later
+	coins.Sort()
+	if !coins.IsValid() {
+		return nil, errors.Errorf("ParseCoins invalid: %#v", coins)
+	}
+
 	return coins, nil
 }
 
@@ -195,3 +203,10 @@ func (coins Coins) IsNonnegative() bool {
 	}
 	return true
 }
+
+/*** Implement Sort interface ***/
+
+func (c Coins) Len() int           { return len(c) }
+func (c Coins) Less(i, j int) bool { return c[i].Denom < c[j].Denom }
+func (c Coins) Swap(i, j int)      { c[i], c[j] = c[j], c[i] }
+func (c Coins) Sort()              { sort.Sort(c) }
diff --git a/types/coin_test.go b/types/coin_test.go
index 8cbc708a49..bf99d0caab 100644
--- a/types/coin_test.go
+++ b/types/coin_test.go
@@ -4,7 +4,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestCoins(t *testing.T) {
@@ -56,30 +55,79 @@ func TestCoins(t *testing.T) {
 
 //Test the parse coin and parse coins functionality
 func TestParse(t *testing.T) {
-	assert, require := assert.New(t), require.New(t)
+	assert := assert.New(t)
 
-	makeCoin := func(str string) Coin {
-		coin, err := ParseCoin(str)
-		require.Nil(err)
-		return coin
+	cases := []struct {
+		input    string
+		valid    bool  // if false, we expect an error on parse
+		expected Coins // if valid is true, make sure this is returned
+	}{
+		{"", true, nil},
+		{"1foo", true, Coins{{"foo", 1}}},
+		{"10bar", true, Coins{{"bar", 10}}},
+		{"99bar,1foo", true, Coins{{"bar", 99}, {"foo", 1}}},
+		{"98 bar , 1 foo  ", true, Coins{{"bar", 98}, {"foo", 1}}},
+		{"2foo, 97 bar", true, Coins{{"bar", 97}, {"foo", 2}}},
 	}
 
-	makeCoins := func(str string) Coins {
-		coin, err := ParseCoins(str)
-		require.Nil(err)
-		return coin
+	for _, tc := range cases {
+		res, err := ParseCoins(tc.input)
+		if !tc.valid {
+			assert.NotNil(err, tc.input)
+		} else if assert.Nil(err, "%s: %+v", tc.input, err) {
+			assert.Equal(tc.expected, res)
+		}
 	}
 
-	//testing ParseCoin Function
-	assert.Equal(Coin{}, makeCoin(""), "ParseCoin makes bad empty coin")
-	assert.Equal(Coin{"fooCoin", 1}, makeCoin("1fooCoin"), "ParseCoin makes bad coins")
-	assert.Equal(Coin{"barCoin", 10}, makeCoin("10 barCoin"), "ParseCoin makes bad coins")
-
-	//testing ParseCoins Function
-	assert.True(Coins{{"fooCoin", 1}}.IsEqual(makeCoins("1fooCoin")),
-		"ParseCoins doesn't parse a single coin")
-	assert.True(Coins{{"barCoin", 99}, {"fooCoin", 1}}.IsEqual(makeCoins("99barCoin,1fooCoin")),
-		"ParseCoins doesn't properly parse two coins")
-	assert.True(Coins{{"barCoin", 99}, {"fooCoin", 1}}.IsEqual(makeCoins("99 barCoin, 1 fooCoin")),
-		"ParseCoins doesn't properly parse two coins which use spaces")
+}
+
+func TestSortCoins(t *testing.T) {
+	assert := assert.New(t)
+
+	good := Coins{
+		Coin{"GAS", 1},
+		Coin{"MINERAL", 1},
+		Coin{"TREE", 1},
+	}
+	empty := Coins{
+		Coin{"GOLD", 0},
+	}
+	badSort1 := Coins{
+		Coin{"TREE", 1},
+		Coin{"GAS", 1},
+		Coin{"MINERAL", 1},
+	}
+	badSort2 := Coins{ // both are after the first one, but the second and third are in the wrong order
+		Coin{"GAS", 1},
+		Coin{"TREE", 1},
+		Coin{"MINERAL", 1},
+	}
+	badAmt := Coins{
+		Coin{"GAS", 1},
+		Coin{"TREE", 0},
+		Coin{"MINERAL", 1},
+	}
+	dup := Coins{
+		Coin{"GAS", 1},
+		Coin{"GAS", 1},
+		Coin{"MINERAL", 1},
+	}
+
+	cases := []struct {
+		coins         Coins
+		before, after bool // valid before/after sort
+	}{
+		{good, true, true},
+		{empty, false, false},
+		{badSort1, false, true},
+		{badSort2, false, true},
+		{badAmt, false, false},
+		{dup, false, false},
+	}
+
+	for _, tc := range cases {
+		assert.Equal(tc.before, tc.coins.IsValid())
+		tc.coins.Sort()
+		assert.Equal(tc.after, tc.coins.IsValid())
+	}
 }