feat: min and max operators on coins

[JimLarson]

Description

Closes: #10995

Adds Min() and Max() operations on sdk.Coins for per-denom minimum and maximum.

Replaced an example of manual low-level construction of Coins with higher-level operators.
Upcoming enhancements to vesting accounts make heavy use of this pattern.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[JimLarson]

Tag @ValarDragon

[JimLarson]

Does this merit mention in CHANGELOG.md? Is it a "Feature" or "Improvement"?

[ValarDragon]

LGTM!

I'd say put this as a changelog item in "Improvement"

Also I think it can be marked ready for review / merged.

[peterbourgon]

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}}},

[JimLarson]

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.

[ValarDragon]

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.

[peterbourgon]

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

[JimLarson]

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

[peterbourgon]

Not including but excluding, right?

[JimLarson]

The missing denom is included in the calculation of minimums, but excluded from the result because it's zero. I was trying to draw a contrast with your earlier interpretation. I'd be open to better wording if you have any to suggest.

[peterbourgon]

The methods in this PR operate on the union set of two sets of coins, on a per-denom basis. Therefore, intuitively, as long as a denom exists in one of those input sets, it should also be represented in the output set.

If a denom has to be present in both sets in order to be represented in the output set, then say exactly that in the docs 👍

[JimLarson]

I think the sparse-set implementation of Coins is causing confusion here, since any Coins value includes quantities for all denominations, since AmountOf() always returns a number instead of occasional errors. Would this phrasing be better?

// Min returns a Coins value that contains, for each denom, the minimum of the amount in either of its inputs.

[ValarDragon]

That sounds correct to me. AmountOf will return 0 if the denom isn't present.

I guess if we wanted to be very explicitly, we could add a Note saying that the length of the returned object can be less than the length of both inputs. I don't think such a note is necessary, but I am also quite normalized to the sparse representation of the coins object.

[peterbourgon]

A multiset of denom strings can be equivalently envisioned as a set that allows repeats ({"atom", "atom", "atom", "eth"})

✔︎

or as a set where each element has a nonzero multiplicity ({("atom", 3), ("eth", 1)})

✔︎

or as a mapping from the entire space of values to nonnegatives ({("atom", 3), ("bld", 0), ("btc", 0), ..., ("eth", 1), ("hex": 0), ...}).

✘

Firstly, this isn't equivalent. The cardinality of this proposed set would be equal to the cardinality of the entire space of possible elements, but (per Wikipedia) its cardinality is the number of extant elements. At least, that's how I read it.

The cardinality of a multiset is constructed by summing up the multiplicities of all its elements. For example, in the multiset {a, a, b, b, b, c} the multiplicities of the members a, b, and c are respectively 2, 3, and 1, and therefore the cardinality of this multiset is 6.

Secondly, for Coins in particular, the entire set of possible denoms is infinite and unknowable, which would make Coins an infinite set. This is nonintuitive, and produces surprising results for common set operations — like min! :)

I'm not saying operations which treat Coins as infinite sets shouldn't exist, just that (a) this shouldn't be the default assumption, and (b) those operations should be unambiguously named and documented regarding this assumption and its consequences.

[JimLarson]

In the alternative you object to, I explicitly said "mapping", not "implementation". To be fair, I should have written "atom" -> 3, "bld" -> 0, "btc" -> 0, ..., "eth" -> 1, "hex" -> 0, ... to make it super clear that I'm describing a mapping. This is exactly the mulitiplicity function from the cited article, and is implemented (in finite space) by AmountOf(). Despite the infinite domain of possible denom strings, all these examples are finite multisets and have finite cardinality. Specifying the mapping "btc" -> 0 does not increase the cardinality.

Every other Coins operation works with the semantics implemented for Min() - the result is, for every one of the infinite denoms, the equivalent scalar operation for that denom:

AmountOf(denom, a.Add(b)) == a.AmountOf(denom).Add(b.AmountOf(denom))
AmountOf(denom, a.Sub(b)) == a.AmountOf(denom).Sub(b.AmountOf(denom))

[peterbourgon]

I guess there's lots of surprising properties about Coin/Coins. A Coin with a zero amount should definitely be valid. A zero value Coins, or equivalently a Coins without any Coins in it, should definitely be valid. Negative amounts should be representable. And so on. So maybe that's the windmill I'm tilting against here, really. In any case, I defer 👍

[robert-zaremba]

I think it is just an optimization: we don't store zero balances in the state (to free the state), nor we don't store zero coins in messages, because they don't change any value.

[JimLarson]

If you're talking about a zero quantity for a denom in a Coins value, then I'd put it even more strongly - you can't possibly represent them explicitly since there are too many possible denom strings.

If you're talking about the all-zeroes Coins value, then it doesn't make sense for some operations, e.g. a send of empty Coins between accounts seems odd. On the other hand, in Issue 11223 I gave a long list of places where an all-zeroes Coins value is essential.

[JimLarson]

I keep getting lucky with the numbers - see #11223 for a proposal to clean up some inconsistencies in Coins and DecCoins in line with the changes here.

[robert-zaremba]

• Let's rethink the Min semantic. I don't think it should be the same as Intersect and would rather expect that in Min the "missing" coins are included in the result.
• Need to update the docs.

[robert-zaremba]

So, we require not only that the coins are sorted, but also they are unified, ie - there is no duplicate denom.
Could you update the documentation to:

• clarify that coins must not have duplicate (otherwise wrong result will be returned) - so it's not really a multiset.
• please add an example. By first reading I thought we will take a max denom, not a set of denoms.

[JimLarson]

The only real description of the Coins type forbids duplicates:

// Coins is a set of Coin, one per currency

I'll update the docs to clarify that it expects "valid" input (and produces valid output), which lets Validate() contain all the checks, which are:

• valid denoms
• sorted order
• no duplicates
• positive quantities

I'll also add examples.

[robert-zaremba]

thanks. It's better to link or be explicit than face more risk of bugs.

[robert-zaremba]

doing this naively with a for loop will be faster and require less allocations

[JimLarson]

I don't expect Max() to be used as much as Min(), but sure.

[ValarDragon]

I feel like optimizing for readability is way better right now than performance for this operation as well

[robert-zaremba]

I had to think more to confirm that this works. With short for loop it would be easier.

[JimLarson]

I had updated it to loop over the Coins. Max() is a little trickier than Min() since you need to iterate over any leftovers once the overlap has been processed.

I thought about getting fancy and writing a zip() helper to iterate over two lists at once, calling processing function to handle the elements. Then Min, Max, Add, and Sub could use this common iterator. It could even check for out-of-order or duplicate entries without much overhead. But it's a little harder to get the substructure sharing behavior of an explicit loop.

[robert-zaremba]

As above, let's update the docs and ALSO:

• better document a case where a coin is missing, it seams it's skipped: for example {coinA, coinB}.Min({coinC}) == {} the result is empty slice.

I'm not sure if omitting a coin which is only present in one set is the right semantic, personally I would expect that the coins which are only present in one of the multi sets will be included. So in the example above, I would rather expect: {coinA, coinB}.Min({coinC}) == {coinA, coinB, coinC}

[JimLarson]

Would you change your expectation if the example were the following?

NewCoins(NewCoin(denomA, one), NewCoin(denomB, one), NewCoin(denomC, zero))
    .Min(NewCoins(NewCoin(denomA, zero), NewCoin(denomB, zero), newCoin(denomC, one)))

[JimLarson]

This contribution comes at the request of reviewers of our implementation of clawback vesting. They noticed that we heavily used a utility function coinsMin(a,b), and thought it ought to go in coins.go. The semantics are exactly what we require. I'll see what I can do with documentation and I'm open to simply going with the Intersect name, if there's a use case for different semantics that also want the name Min() - but I'd like to see the compelling use case, because it really seems like the right name for the functionality.

[JimLarson]

So min/max are now documented in terms of AmountOf() and illuminated by examples, and that's as clear as I think I can make it. See Issue 11223 for further discussion. I'm leaving it to others to tweak the analogous DecCoins methods if needed.

PTAL

[JimLarson]

Linking in @marbar3778

[ValarDragon]

I remain in favor of merging with any semantics for Intersect / Min.

Not including the zero coins is also what I expected from using Coins in the past, and think it can be resolved via a comment. I would significantly prefer unblocking the vesting work, and revisiting this in a future issue/PR prior to the release.

Getting vesting + clawback in code has far higher positive UX impact than a negative impact we will have here with any API decision. Especially as we have rethinking all the methods on coins in the roadmap.

[alexanderbez]

LGTM

[JimLarson]

@alexanderbez @ValarDragon @robert-zaremba How can we deal with the failing test? It seems to be unrelated to the content of this PR. I don't think I have the permissions for troubleshooting or fixing.

[alexanderbez]

If you mean this, then you can ignore it :) at least I do. Idk why it always fails on PR. Should be fixed.

[robert-zaremba]

Not going to block this PR. But, please:

1. Don't create aliases and "book" more API. We should restrict creating function aliases for dealing with legacy code. So lets create only one method: either Min or Intersect
2. IMHO, your definition of Min/Intersect is closer to Intersect from my mathematical feeling. For me.

• {2A, 3B}.Min({1B, 4C}) == {2A, 1B, 1C}
• {2A, 3B}.Intersect({1B, 4C}) == {1B}
  But not going to block on that.

[robert-zaremba]

Suggested change

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

[robert-zaremba]

thanks for the updates.