cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.3k stars 3.64k forks source link

Min operation on Coins #10995

Closed JimLarson closed 2 years ago

JimLarson commented 2 years ago

Summary

Incorporate a new Coins method: func (c Coins) Min(other Coins) Coins for utility and consistency.

Problem Definition

When working with Coins, it's sometimes useful to compute the minimum of two values, defined as the minimum of each denom, but there's no existing library method for it.

Example: toTransfer := want.Min(spendable) Example: to compute max(a.Sub(b), zeroCoin) without panicking, compute a.Sub(a.Min(b))

With IsAllLTE() defining a partial order on Coins, Min() becomes the "meet" operator of a lattice, tying the operations into centuries of mathematical precedent.

Proposal

Implement func (c Coins) Min(other Coins) Coins with tests and documentation for its properties.

Max() should be added for symmetry, with the property that a.Add(b) == (a.Max(b)).Add(a.Min(b)), so it could be defined as

func (c Coins) Max(other Coins) Coins {
    return c.Add(other).Sub(c.Min(other))
}

Where 0 is the empty Coins value, <= is IsAllLTE(), and == is a non-panicking variant of IsEqual(), for arbitrary Coins values a, b, and c, we have the properties:


For Admin Use

ValarDragon commented 2 years ago

This is a great idea imo, just will need a clear comment so people realize that the following can pass:

c := A.Min(B)
assert c != A && c != B

Perhaps this can be done by calling it "MinSet" or something? (Surely theres some standard naming for taking the entry-wise minimum of two lists)

Feel free to tag me as a reviewer for such a PR

JimLarson commented 2 years ago

The literature seems to call this "multiset intersection" or "bag intersection", or "infemum" or "greatest common divisor". See wikipedia entry.

I can definitely document it clearly. For a name, let me see how it looks in context. Briefer seems better. So far, Min() seems the most intuitive, but maybe Inf() is just as good?

ValarDragon commented 2 years ago

Oh can we call it intersection?

If we prefer a 3-4 letter name, then Min also SGTM

JimLarson commented 2 years ago

If we call this Intersection(), we'd want to rename IsAllGTE() to Contains() to be consistent. :-) But I think we'll want to stick with an "ordering" theme rather than a "set theory" theme. The latter is technically correct, but I don't think it's how we think about Coins.

I'll try to find some places to use this in the codebase, so seeing the uses in context will help with the naming discussion.

ValarDragon commented 2 years ago

Ok that sounds good to me! I'm down for Min under this framing. (Using an ordering interpretation rather than a set theory one)

I don't think theres harm in duplicate functions for both SetTheory naming + normal naming, but we can easily make that a separate issue after Min() is in

JimLarson commented 2 years ago

@ValarDragon I've tagged you on #11200 which is in draft form, per CONTRIBUTING.md. Please advise on next steps.

JimLarson commented 2 years ago

Note that sdk.DecCoins has an Intersect() call with the same semantics as Min(). I'll add it as another name for Min().

JimLarson commented 2 years ago

See #11223 for other changes which could make Coins and DecCoins more consistent.