Skip to content

Commit

Permalink
fix: sdk.Coins.Add (#14715)
Browse files Browse the repository at this point in the history 
Co-authored-by: Julien Robert <julien@rbrt.fr>
(cherry picked from commit 32bb7f6)

# Conflicts:
#	CHANGELOG.md
  • Loading branch information
@robert-zaremba @mergify
robert-zaremba authored and mergify[bot] committed Jan 26, 2023
1 parent 591f1be commit c1fa331
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 36 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,19 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf

### Bug Fixes

<<<<<<< HEAD
* (x/auth) [#13838](https://github.com/cosmos/cosmos-sdk/pull/13838) Fix calling `String()` and `MarshalYAML` panics when pubkey is set on a `BaseAccount`.
=======
* (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.
* (server) [#14441](https://github.com/cosmos/cosmos-sdk/pull/14441) Fix `--log_format` flag not working.
* (x/upgrade) [#13936](https://github.com/cosmos/cosmos-sdk/pull/13936) Make downgrade verification work again
* (x/group) [#13742](https://github.com/cosmos/cosmos-sdk/pull/13742) Fix `validate-genesis` when group policy accounts exist.
* (x/auth) [#13838](https://github.com/cosmos/cosmos-sdk/pull/13838) Fix calling `String()` when pubkey is set on a `BaseAccount`.
>>>>>>> 32bb7f63a (fix: sdk.Coins.Add (#14715))
* (rosetta) [#13583](https://github.com/cosmos/cosmos-sdk/pull/13583) Misc fixes for cosmos-rosetta.
* (x/evidence) [#13740](https://github.com/cosmos/cosmos-sdk/pull/13740) Fix evidence query API to decode the hash properly.
* (bank) [#13691](https://github.com/cosmos/cosmos-sdk/issues/13691) Fix unhandled error for vesting account transfers, when total vesting amount exceeds total balance.
Expand Down
3 changes: 3 additions & 0 deletions types/coin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,9 @@ func (coins Coins) safeAdd(coinsB Coins) (coalesced Coins) {
coalesced = append(coalesced, comboCoin)
}
}
if coalesced == nil {
return Coins{}
}
return coalesced.Sort()
}

Expand Down
53 changes: 47 additions & 6 deletions types/coin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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{}
}

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -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)
})
}
}
Expand Down Expand Up @@ -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},
Expand Down
31 changes: 16 additions & 15 deletions x/auth/vesting/types/vesting_account_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
var (
stakeDenom = "stake"
feeDenom = "fee"
emptyCoins = sdk.Coins{}
)

type VestingAccountTestSuite struct {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion x/authz/simulation/operations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion x/bank/types/send_authorization.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
29 changes: 16 additions & 13 deletions x/feegrant/periodic_fee_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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),
},
Expand All @@ -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
},
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit c1fa331

Please sign in to comment.