Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: min and max operators on coins #11200

Merged
merged 6 commits into from
Feb 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (gov) [\#10854](https://github.com/cosmos/cosmos-sdk/pull/10854) v1beta2's vote doesn't include the deprecate `option VoteOption` anymore. Instead, it only uses `WeightedVoteOption`.
* (types) [\#11004](https://github.com/cosmos/cosmos-sdk/pull/11004) Added mutable versions of many of the sdk.Dec types operations. This improves performance when used by avoiding reallocating a new bigint for each operation.
* (x/auth) [\#10880](https://github.com/cosmos/cosmos-sdk/pull/10880) Added a new query to the tx query service that returns a block with transactions fully decoded.
* (types) [\#11200](https://github.com/cosmos/cosmos-sdk/pull/11200) Added `Min()` and `Max()` operations on sdk.Coins.

### Bug Fixes

Expand Down
85 changes: 85 additions & 0 deletions types/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,91 @@ func (coins Coins) SafeSub(coinsB Coins) (Coins, bool) {
return diff, diff.IsAnyNegative()
}

// Max takes two valid Coins inputs and returns a valid Coins result
// where for every denom D, AmountOf(D) of the result is the maximum
// of AmountOf(D) of the inputs. Note that the result might be not
// be equal to either input. For any valid Coins a, b, and c, the
// following are always true:
// a.IsAllLTE(a.Max(b))
// b.IsAllLTE(a.Max(b))
// a.IsAllLTE(c) && b.IsAllLTE(c) == a.Max(b).IsAllLTE(c)
// a.Add(b...).IsEqual(a.Min(b).Add(a.Max(b)...))
//
// E.g.
// {1A, 3B, 2C}.Max({4A, 2B, 2C} == {4A, 3B, 2C})
// {2A, 3B}.Max({1B, 4C}) == {2A, 3B, 4C}
// {1A, 2B}.Max({}) == {1A, 2B}
func (coins Coins) Max(coinsB Coins) Coins {
max := make([]Coin, 0)
indexA, indexB := 0, 0
for indexA < len(coins) && indexB < len(coinsB) {
coinA, coinB := coins[indexA], coinsB[indexB]
switch strings.Compare(coinA.Denom, coinB.Denom) {
case -1: // denom missing from coinsB
max = append(max, coinA)
indexA++
case 0: // same denom in both
maxCoin := coinA
if coinB.Amount.GT(maxCoin.Amount) {
maxCoin = coinB
}
max = append(max, maxCoin)
indexA++
indexB++
case 1: // denom missing from coinsA
max = append(max, coinB)
indexB++
}
}
for ; indexA < len(coins); indexA++ {
max = append(max, coins[indexA])
}
for ; indexB < len(coinsB); indexB++ {
max = append(max, coinsB[indexB])
}
return NewCoins(max...)
}

// Min takes two valid Coins inputs and returns a valid Coins result
// where for every denom D, AmountOf(D) of the result is the minimum
// of AmountOf(D) of the inputs. Note that the result might be not
// be equal to either input. For any valid Coins a, b, and c, the
// following are always true:
// a.Min(b).IsAllLTE(a)
// a.Min(b).IsAllLTE(b)
// c.IsAllLTE(a) && c.IsAllLTE(b) == c.IsAllLTE(a.Min(b))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// c.IsAllLTE(a) && c.IsAllLTE(b) == c.IsAllLTE(a.Min(b))
// c.IsAllLTE(a) && c.IsAllLTE(b) <=> c.IsAllLTE(a.Min(b))

// a.Add(b...).IsEqual(a.Min(b).Add(a.Max(b)...))
//
// E.g.
// {1A, 3B, 2C}.Min({4A, 2B, 2C} == {1A, 2B, 2C})
// {2A, 3B}.Min({1B, 4C}) == {1B}
// {1A, 2B}.Min({3C}) == empty
//
// See also DecCoins.Intersect().
func (coins Coins) Min(coinsB Coins) Coins {
min := make([]Coin, 0)
for indexA, indexB := 0, 0; indexA < len(coins) && indexB < len(coinsB); {
coinA, coinB := coins[indexA], coinsB[indexB]
switch strings.Compare(coinA.Denom, coinB.Denom) {
case -1: // denom missing from coinsB
indexA++
case 0: // same denom in both
minCoin := coinA
if coinB.Amount.LT(minCoin.Amount) {
minCoin = coinB
}
if !minCoin.IsZero() {
min = append(min, minCoin)
}
indexA++
indexB++
case 1: // denom missing from coins
indexB++
}
}
return NewCoins(min...)
}

// IsAllGT returns true if for every denom in coinsB,
// the denom is present at a greater amount in coins.
func (coins Coins) IsAllGT(coinsB Coins) bool {
Expand Down
27 changes: 27 additions & 0 deletions types/coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,33 @@ func (s *coinTestSuite) TestCoins_Validate() {
}
}

func (s *coinTestSuite) TestMinMax() {
one := sdk.OneInt()
two := sdk.NewInt(2)

cases := []struct {
name string
input1 sdk.Coins
input2 sdk.Coins
min sdk.Coins
max sdk.Coins
}{
{"zero-zero", sdk.Coins{}, sdk.Coins{}, sdk.Coins{}, sdk.Coins{}},
{"zero-one", sdk.Coins{}, sdk.Coins{{testDenom1, one}}, sdk.Coins{}, sdk.Coins{{testDenom1, one}}},
{"two-zero", sdk.Coins{{testDenom2, two}}, sdk.Coins{}, sdk.Coins{}, sdk.Coins{{testDenom2, two}}},
{"disjoint", sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom2, two}}, sdk.Coins{}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}},
{"overlap", sdk.Coins{{testDenom1, one}, {testDenom2, two}}, sdk.Coins{{testDenom1, two}, {testDenom2, one}},
sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, two}, {testDenom2, two}}},
Comment on lines +672 to +676

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for these methods claims

// {Min, Max} returns the {minimum, maximum} of each denom of its inputs.

Which means that every denomination present in either input set-of-coins will be represented in the output set-of-coins. But that's different than the current behavior. Concretely, shouldn't this be as follows?

Suggested change
{"zero-one", sdk.Coins{}, sdk.Coins{{testDenom1, one}}, sdk.Coins{}, sdk.Coins{{testDenom1, one}}},
{"two-zero", sdk.Coins{{testDenom2, two}}, sdk.Coins{}, sdk.Coins{}, sdk.Coins{{testDenom2, two}}},
{"disjoint", sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom2, two}}, sdk.Coins{}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}},
{"overlap", sdk.Coins{{testDenom1, one}, {testDenom2, two}}, sdk.Coins{{testDenom1, two}, {testDenom2, one}},
sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, two}, {testDenom2, two}}},
{"zero-one", sdk.Coins{}, sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom1, one}}},
{"two-zero", sdk.Coins{{testDenom2, two}}, sdk.Coins{}, sdk.Coins{{testDenom2, two}}, sdk.Coins{{testDenom2, two}}},
{"disjoint", sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom2, two}}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}},
{"overlap", sdk.Coins{{testDenom1, one}, {testDenom2, two}}, sdk.Coins{{testDenom1, two}, {testDenom2, one}},
sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, two}, {testDenom2, two}}},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, zero-quantity denominations are omitted from Coins, so for instance in the "zero-one" case, we're comparing 0 vs 1 of testDenom1, and the min is zero, hence omitted from the result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, zero must be ommitted from the result due to the guarantees expected on a valid coins object.

I believe whats implemented is correct, perhaps we just need to update the code comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's surprising behavior! But then I'd agree an update to the documentation would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I'll be filing an issue to clean up sdk.Coins to give it multiset semantics everywhere.

}

for _, tc := range cases {
min := tc.input1.Min(tc.input2)
max := tc.input1.Max(tc.input2)
s.Require().True(min.IsEqual(tc.min), tc.name)
s.Require().True(max.IsEqual(tc.max), tc.name)
}
}

func (s *coinTestSuite) TestCoinsGT() {
one := sdk.OneInt()
two := sdk.NewInt(2)
Expand Down
3 changes: 2 additions & 1 deletion types/dec_coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,9 @@ func (coins DecCoins) SafeSub(coinsB DecCoins) (DecCoins, bool) {
// Intersect will return a new set of coins which contains the minimum DecCoin
// for common denoms found in both `coins` and `coinsB`. For denoms not common
// to both `coins` and `coinsB` the minimum is considered to be 0, thus they
// are not added to the final set.In other words, trim any denom amount from
// are not added to the final set. In other words, trim any denom amount from
// coin which exceeds that of coinB, such that (coin.Intersect(coinB)).IsLTE(coinB).
// See also Coins.Min().
func (coins DecCoins) Intersect(coinsB DecCoins) DecCoins {
res := make([]DecCoin, len(coins))
for i, coin := range coins {
Expand Down
16 changes: 3 additions & 13 deletions x/auth/vesting/types/vesting_account.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,10 @@ func NewBaseVestingAccount(baseAccount *authtypes.BaseAccount, originalVesting s
//
// CONTRACT: Delegated vesting coins and vestingCoins must be sorted.
func (bva BaseVestingAccount) LockedCoinsFromVesting(vestingCoins sdk.Coins) sdk.Coins {
lockedCoins := sdk.NewCoins()

for _, vestingCoin := range vestingCoins {
vestingAmt := vestingCoin.Amount
delVestingAmt := bva.DelegatedVesting.AmountOf(vestingCoin.Denom)

max := sdk.MaxInt(vestingAmt.Sub(delVestingAmt), sdk.ZeroInt())
lockedCoin := sdk.NewCoin(vestingCoin.Denom, max)

if !lockedCoin.IsZero() {
lockedCoins = lockedCoins.Add(lockedCoin)
}
lockedCoins := vestingCoins.Sub(vestingCoins.Min(bva.DelegatedVesting))
if lockedCoins == nil {
return sdk.Coins{}
}

return lockedCoins
}

Expand Down