fix: sdk.Coins.Add (backport #14715) (#14791)

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Julien Robert <julien@rbrt.fr>

CHANGELOG.md

@@ -37,7 +37,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-## Improvements
+### Improvements
 
 * (x/gov) [#13010](https://github.com/cosmos/cosmos-sdk/pull/13010) Partial cherry-pick of this issue for adding proposer migration.
 * [#14691](https://github.com/cosmos/cosmos-sdk/pull/14691) Change behavior of `sdk.StringifyEvents` to not flatten events attributes by events type.
@@ -47,15 +47,19 @@ Ref: https://keepachangelog.com/en/1.0.0/
     * This updates the [ADR-028](https://docs.cosmos.network/main/architecture/adr-028-public-key-addresses) and enhance the `address.Module` API to support module addresses and sub-module addresses in a backward compatible way.
 * (snapshots) [#14608](https://github.com/cosmos/cosmos-sdk/pull/14608/) Deprecate unused structs `SnapshotKVItem` and `SnapshotSchema`.
 
-## State Machine Breaking
+### State Machine Breaking
 
 * (baseapp, x/auth/posthandler) [#13940](https://github.com/cosmos/cosmos-sdk/pull/13940) Update `PostHandler` to receive the `runTx` success boolean.
 
-
-## API Breaking Changes
+### API Breaking Changes
 
 * (x/gov) [#14782](https://github.com/cosmos/cosmos-sdk/pull/14782) Move the `metadata` argument in `govv1.NewProposal` alongside `title` and `summary`.
 
+### 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.
+
 ## [v0.47.0-rc1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.0-rc1) - 2022-01-09
 
 ### Features

types/coin.go

@@ -343,6 +343,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{}
 }
 
 // ----------------------------------------------------------------------------
@@ -536,24 +538,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)
 		})
 	}
 }
@@ -585,12 +619,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

@@ -23,6 +23,7 @@ import (
 var (
 	stakeDenom = "stake"
 	feeDenom   = "fee"
+	emptyCoins = sdk.Coins{}
 )
 
 type VestingAccountTestSuite struct {
@@ -95,7 +96,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))
@@ -171,14 +172,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
@@ -202,7 +203,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)
 }
 
@@ -235,7 +236,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) {
@@ -313,13 +314,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
@@ -410,7 +411,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))
@@ -426,7 +427,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) {
@@ -528,21 +529,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
@@ -566,7 +567,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)
 }
 
@@ -665,14 +666,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

@@ -24,6 +24,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)
@@ -60,11 +61,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{
@@ -93,7 +95,7 @@ func TestPeriodicFeeValidAllow(t *testing.T) {
 			blockTime:     now,
 			accept:        true,
 			remove:        false,
-			remainsPeriod: nil,
+			remainsPeriod: emptyCoins,
 			remains:       leftAtom,
 			periodReset:   now.Add(1 * time.Hour),
 		},
@@ -112,7 +114,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
 		},
@@ -141,12 +143,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{