fix: sdk.Coins.Add (#14715)

Co-authored-by: Julien Robert <julien@rbrt.fr>

CHANGELOG.md

@@ -252,6 +252,8 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf
 
 ### Bug Fixes
 
+* (types/coin) [#14715](https://github.com/cosmos/cosmos-sdk/pull/14715) `sdk.Coins.Add` now returns an empty set of coins `sdk.Coins{}` if both coins set are empty.
+    * This is a behavior change, as previously `sdk.Coins.Add` would return `nil` in this case.
 * (types/coin) [#14739](https://github.com/cosmos/cosmos-sdk/pull/14739) Deprecate the method `Coin.IsEqual` in favour of  `Coin.Equal`. The difference between the two methods is that the first one results in a panic when denoms are not equal. This panic lead to unexpected behaviour
 * (x/bank) [#14538](https://github.com/cosmos/cosmos-sdk/pull/14538) Validate denom in bank balances GRPC queries.
 * (baseapp) [#14505](https://github.com/cosmos/cosmos-sdk/pull/14505) PrepareProposal and ProcessProposal now use deliverState for the first block in order to access changes made in InitChain.

types/coin.go

@@ -340,6 +340,9 @@ func (coins Coins) safeAdd(coinsB Coins) (coalesced Coins) {
 			coalesced = append(coalesced, comboCoin)
 		}
 	}
+	if coalesced == nil {
+		return Coins{}
+	}
 	return coalesced.Sort()
 }
 

types/coin_test.go

@@ -21,6 +21,7 @@ var (
 type coinTestSuite struct {
 	suite.Suite
 	ca0, ca1, ca2, ca4, cm0, cm1, cm2, cm4 sdk.Coin
+	emptyCoins                             sdk.Coins
 }
 
 func TestCoinTestSuite(t *testing.T) {
@@ -35,6 +36,7 @@ func (s *coinTestSuite) SetupSuite() {
 
 	s.ca0, s.ca1, s.ca2, s.ca4 = sdk.NewCoin(testDenom1, zero), sdk.NewCoin(testDenom1, one), sdk.NewCoin(testDenom1, two), sdk.NewCoin(testDenom1, four)
 	s.cm0, s.cm1, s.cm2, s.cm4 = sdk.NewCoin(testDenom2, zero), sdk.NewCoin(testDenom2, one), sdk.NewCoin(testDenom2, two), sdk.NewCoin(testDenom2, four)
+	s.emptyCoins = sdk.Coins{}
 }
 
 // ----------------------------------------------------------------------------
@@ -524,24 +526,56 @@ func (s *coinTestSuite) TestEqualCoins() {
 }
 
 func (s *coinTestSuite) TestAddCoins() {
+	cA0M0 := sdk.Coins{s.ca0, s.cm0}
+	cA0M1 := sdk.Coins{s.ca0, s.cm1}
+	cA1M1 := sdk.Coins{s.ca1, s.cm1}
 	cases := []struct {
 		name     string
 		inputOne sdk.Coins
 		inputTwo sdk.Coins
 		expected sdk.Coins
+		msg      string
 	}{
-		{"{1atom,1muon}+{1atom,1muon}", sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca2, s.cm2}},
-		{"{0atom,1muon}+{0atom,0muon}", sdk.Coins{s.ca0, s.cm1}, sdk.Coins{s.ca0, s.cm0}, sdk.Coins{s.cm1}},
-		{"{2atom}+{0muon}", sdk.Coins{s.ca2}, sdk.Coins{s.cm0}, sdk.Coins{s.ca2}},
-		{"{1atom}+{1atom,2muon}", sdk.Coins{s.ca1}, sdk.Coins{s.ca1, s.cm2}, sdk.Coins{s.ca2, s.cm2}},
-		{"{0atom,0muon}+{0atom,0muon}", sdk.Coins{s.ca0, s.cm0}, sdk.Coins{s.ca0, s.cm0}, sdk.Coins(nil)},
+		{"adding two empty lists", s.emptyCoins, s.emptyCoins, s.emptyCoins, "empty, non list should be returned"},
+		{"empty list + set", s.emptyCoins, cA0M1, sdk.Coins{s.cm1}, "zero coins should be removed"},
+		{"empty list + set", s.emptyCoins, cA1M1, cA1M1, "zero + a_non_zero = a_non_zero"},
+		{"set + empty list", cA0M1, s.emptyCoins, sdk.Coins{s.cm1}, "zero coins should be removed"},
+		{"set + empty list", cA1M1, s.emptyCoins, cA1M1, "a_non_zero + zero  = a_non_zero"},
+		{
+			"{1atom,1muon}+{1atom,1muon}", cA1M1, cA1M1,
+			sdk.Coins{s.ca2, s.cm2},
+			"a + a = 2a",
+		},
+		{
+			"{0atom,1muon}+{0atom,0muon}", cA0M1, cA0M0,
+			sdk.Coins{s.cm1},
+			"zero coins should be removed",
+		},
+		{
+			"{2atom}+{0muon}",
+			sdk.Coins{s.ca2},
+			sdk.Coins{s.cm0},
+			sdk.Coins{s.ca2},
+			"zero coins should be removed",
+		},
+		{
+			"{1atom}+{1atom,2muon}",
+			sdk.Coins{s.ca1},
+			sdk.Coins{s.ca1, s.cm2},
+			sdk.Coins{s.ca2, s.cm2},
+			"should be correctly added",
+		},
+		{
+			"{0atom,0muon}+{0atom,0muon}", cA0M0, cA0M0, s.emptyCoins,
+			"sets with zero coins should return empty set",
+		},
 	}
 
 	for _, tc := range cases {
 		s.T().Run(tc.name, func(t *testing.T) {
 			res := tc.inputOne.Add(tc.inputTwo...)
 			require.True(t, res.IsValid(), fmt.Sprintf("%s + %s = %s", tc.inputOne, tc.inputTwo, res))
-			require.Equal(t, tc.expected, res, "sum of coins is incorrect")
+			require.Equal(t, tc.expected, res, tc.msg)
 		})
 	}
 }
@@ -573,12 +607,19 @@ func TestCoinsAddCoalescesDuplicateDenominations(t *testing.T) {
 }
 
 func (s *coinTestSuite) TestSubCoins() {
+	cA0M0 := sdk.Coins{s.ca0, s.cm0}
+	cA0M1 := sdk.Coins{s.ca0, s.cm1}
 	testCases := []struct {
 		inputOne    sdk.Coins
 		inputTwo    sdk.Coins
 		expected    sdk.Coins
 		shouldPanic bool
 	}{
+		{s.emptyCoins, s.emptyCoins, s.emptyCoins, false},
+		{cA0M0, s.emptyCoins, s.emptyCoins, false},
+		{cA0M0, sdk.Coins{s.cm0}, s.emptyCoins, false},
+		{sdk.Coins{s.cm0}, cA0M0, s.emptyCoins, false},
+		{cA0M1, s.emptyCoins, sdk.Coins{s.cm1}, false},
 		// denoms are not sorted - should panic
 		{sdk.Coins{s.ca2}, sdk.Coins{s.cm2, s.ca1}, sdk.Coins{}, true},
 		{sdk.Coins{s.cm2, s.ca2}, sdk.Coins{s.ca1}, sdk.Coins{}, true},

x/auth/vesting/types/vesting_account_test.go

@@ -25,6 +25,7 @@ import (
 var (
 	stakeDenom = "stake"
 	feeDenom   = "fee"
+	emptyCoins = sdk.Coins{}
 )
 
 type VestingAccountTestSuite struct {
@@ -97,7 +98,7 @@ func TestGetVestingCoinsContVestingAcc(t *testing.T) {
 
 	// require no coins vesting at the end of the vesting schedule
 	vestingCoins = cva.GetVestingCoins(endTime)
-	require.Nil(t, vestingCoins)
+	require.Equal(t, emptyCoins, vestingCoins)
 
 	// require 50% of coins vesting
 	vestingCoins = cva.GetVestingCoins(now.Add(12 * time.Hour))
@@ -173,14 +174,14 @@ func TestTrackUndelegationContVestingAcc(t *testing.T) {
 	cva.TrackDelegation(now, origCoins, origCoins)
 	cva.TrackUndelegation(origCoins)
 	require.Nil(t, cva.DelegatedFree)
-	require.Nil(t, cva.DelegatedVesting)
+	require.Equal(t, emptyCoins, cva.DelegatedVesting)
 
 	// require the ability to undelegate all vested coins
 	cva = types.NewContinuousVestingAccount(bacc, origCoins, now.Unix(), endTime.Unix())
 
 	cva.TrackDelegation(endTime, origCoins, origCoins)
 	cva.TrackUndelegation(origCoins)
-	require.Nil(t, cva.DelegatedFree)
+	require.Equal(t, emptyCoins, cva.DelegatedFree)
 	require.Nil(t, cva.DelegatedVesting)
 
 	// require no modifications when the undelegation amount is zero
@@ -204,7 +205,7 @@ func TestTrackUndelegationContVestingAcc(t *testing.T) {
 
 	// undelegate from the other validator that did not get slashed
 	cva.TrackUndelegation(sdk.Coins{sdk.NewInt64Coin(stakeDenom, 50)})
-	require.Nil(t, cva.DelegatedFree)
+	require.Equal(t, emptyCoins, cva.DelegatedFree)
 	require.Equal(t, sdk.Coins{sdk.NewInt64Coin(stakeDenom, 25)}, cva.DelegatedVesting)
 }
 
@@ -237,7 +238,7 @@ func TestGetVestingCoinsDelVestingAcc(t *testing.T) {
 
 	// require no coins vesting at schedule maturation
 	vestingCoins = dva.GetVestingCoins(endTime)
-	require.Nil(t, vestingCoins)
+	require.Equal(t, emptyCoins, vestingCoins)
 }
 
 func TestSpendableCoinsDelVestingAcc(t *testing.T) {
@@ -315,13 +316,13 @@ func TestTrackUndelegationDelVestingAcc(t *testing.T) {
 	dva.TrackDelegation(now, origCoins, origCoins)
 	dva.TrackUndelegation(origCoins)
 	require.Nil(t, dva.DelegatedFree)
-	require.Nil(t, dva.DelegatedVesting)
+	require.Equal(t, emptyCoins, dva.DelegatedVesting)
 
 	// require the ability to undelegate all vested coins
 	dva = types.NewDelayedVestingAccount(bacc, origCoins, endTime.Unix())
 	dva.TrackDelegation(endTime, origCoins, origCoins)
 	dva.TrackUndelegation(origCoins)
-	require.Nil(t, dva.DelegatedFree)
+	require.Equal(t, emptyCoins, dva.DelegatedFree)
 	require.Nil(t, dva.DelegatedVesting)
 
 	// require no modifications when the undelegation amount is zero
@@ -412,7 +413,7 @@ func TestGetVestingCoinsPeriodicVestingAcc(t *testing.T) {
 
 	// require no coins vesting at the end of the vesting schedule
 	vestingCoins = pva.GetVestingCoins(endTime)
-	require.Nil(t, vestingCoins)
+	require.Equal(t, emptyCoins, vestingCoins)
 
 	// require 50% of coins vesting
 	vestingCoins = pva.GetVestingCoins(now.Add(12 * time.Hour))
@@ -428,7 +429,7 @@ func TestGetVestingCoinsPeriodicVestingAcc(t *testing.T) {
 
 	// require 0% of coins vesting after vesting complete
 	vestingCoins = pva.GetVestingCoins(now.Add(48 * time.Hour))
-	require.Nil(t, vestingCoins)
+	require.Equal(t, emptyCoins, vestingCoins)
 }
 
 func TestSpendableCoinsPeriodicVestingAcc(t *testing.T) {
@@ -530,21 +531,21 @@ func TestTrackUndelegationPeriodicVestingAcc(t *testing.T) {
 	pva.TrackDelegation(now, origCoins, origCoins)
 	pva.TrackUndelegation(origCoins)
 	require.Nil(t, pva.DelegatedFree)
-	require.Nil(t, pva.DelegatedVesting)
+	require.Equal(t, emptyCoins, pva.DelegatedVesting)
 
 	// require the ability to undelegate all vested coins at the end of vesting
 	pva = types.NewPeriodicVestingAccount(bacc, origCoins, now.Unix(), periods)
 
 	pva.TrackDelegation(endTime, origCoins, origCoins)
 	pva.TrackUndelegation(origCoins)
-	require.Nil(t, pva.DelegatedFree)
+	require.Equal(t, emptyCoins, pva.DelegatedFree)
 	require.Nil(t, pva.DelegatedVesting)
 
 	// require the ability to undelegate half of coins
 	pva = types.NewPeriodicVestingAccount(bacc, origCoins, now.Unix(), periods)
 	pva.TrackDelegation(endTime, origCoins, periods[0].Amount)
 	pva.TrackUndelegation(periods[0].Amount)
-	require.Nil(t, pva.DelegatedFree)
+	require.Equal(t, emptyCoins, pva.DelegatedFree)
 	require.Nil(t, pva.DelegatedVesting)
 
 	// require no modifications when the undelegation amount is zero
@@ -568,7 +569,7 @@ func TestTrackUndelegationPeriodicVestingAcc(t *testing.T) {
 
 	// undelegate from the other validator that did not get slashed
 	pva.TrackUndelegation(sdk.Coins{sdk.NewInt64Coin(stakeDenom, 50)})
-	require.Nil(t, pva.DelegatedFree)
+	require.Equal(t, emptyCoins, pva.DelegatedFree)
 	require.Equal(t, sdk.Coins{sdk.NewInt64Coin(stakeDenom, 25)}, pva.DelegatedVesting)
 }
 
@@ -667,14 +668,14 @@ func TestTrackUndelegationPermLockedVestingAcc(t *testing.T) {
 	plva.TrackDelegation(now, origCoins, origCoins)
 	plva.TrackUndelegation(origCoins)
 	require.Nil(t, plva.DelegatedFree)
-	require.Nil(t, plva.DelegatedVesting)
+	require.Equal(t, emptyCoins, plva.DelegatedVesting)
 
 	// require the ability to undelegate all vesting coins at endTime
 	plva = types.NewPermanentLockedAccount(bacc, origCoins)
 	plva.TrackDelegation(endTime, origCoins, origCoins)
 	plva.TrackUndelegation(origCoins)
 	require.Nil(t, plva.DelegatedFree)
-	require.Nil(t, plva.DelegatedVesting)
+	require.Equal(t, emptyCoins, plva.DelegatedVesting)
 
 	// require no modifications when the undelegation amount is zero
 	plva = types.NewPermanentLockedAccount(bacc, origCoins)

x/authz/simulation/operations.go

@@ -110,7 +110,7 @@ func SimulateMsgGrant(cdc *codec.ProtoCodec, ak authz.AccountKeeper, bk authz.Ba
 		}
 
 		spendLimit := spendableCoins.Sub(fees...)
-		if spendLimit == nil {
+		if len(spendLimit) == 0 {
 			return simtypes.NoOpMsg(authz.ModuleName, TypeMsgGrant, "spend limit is nil"), nil, nil
 		}
 

x/bank/types/send_authorization.go

@@ -62,7 +62,7 @@ func (a SendAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptRes
 
 // ValidateBasic implements Authorization.ValidateBasic.
 func (a SendAuthorization) ValidateBasic() error {
-	if a.SpendLimit == nil {
+	if len(a.SpendLimit) == 0 {
 		return sdkerrors.ErrInvalidCoins.Wrap("spend limit cannot be nil")
 	}
 	if !a.SpendLimit.IsAllPositive() {

x/feegrant/periodic_fee_test.go

@@ -25,6 +25,7 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 	leftAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 512))
 	oneAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 1))
 	eth := sdk.NewCoins(sdk.NewInt64Coin("eth", 1))
+	emptyCoins := sdk.Coins{}
 
 	now := ctx.BlockTime()
 	oneHour := now.Add(1 * time.Hour)
@@ -61,11 +62,12 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 				PeriodSpendLimit: smallAtom,
 				PeriodReset:      now.Add(30 * time.Minute),
 			},
-			blockTime:   now,
-			valid:       true,
-			accept:      true,
-			remove:      false,
-			periodReset: now.Add(30 * time.Minute),
+			blockTime:     now,
+			valid:         true,
+			accept:        true,
+			remove:        false,
+			remainsPeriod: emptyCoins,
+			periodReset:   now.Add(30 * time.Minute),
 		},
 		"mismatched currencies": {
 			allow: feegrant.PeriodicAllowance{
@@ -94,7 +96,7 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 			blockTime:     now,
 			accept:        true,
 			remove:        false,
-			remainsPeriod: nil,
+			remainsPeriod: emptyCoins,
 			remains:       leftAtom,
 			periodReset:   now.Add(1 * time.Hour),
 		},
@@ -113,7 +115,7 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 			blockTime:     now.Add(1 * time.Hour),
 			accept:        true,
 			remove:        false,
-			remainsPeriod: nil,
+			remainsPeriod: emptyCoins,
 			remains:       smallAtom,
 			periodReset:   oneHour.Add(tenMinutes), // one step from last reset, not now
 		},
@@ -142,12 +144,13 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 				PeriodReset:      now,
 				PeriodSpendLimit: atom,
 			},
-			valid:       true,
-			fee:         atom,
-			blockTime:   oneHour,
-			accept:      true,
-			remove:      false,
-			periodReset: oneHour.Add(tenMinutes), // one step from last reset, not now
+			valid:         true,
+			fee:           atom,
+			blockTime:     oneHour,
+			accept:        true,
+			remove:        false,
+			remainsPeriod: emptyCoins,
+			periodReset:   oneHour.Add(tenMinutes), // one step from last reset, not now
 		},
 		"expired": {
 			allow: feegrant.PeriodicAllowance{