R4R: Fix coins.IsLT() impl

[jaekwon]

Addresses #2674

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
• Wrote tests
• Added entries in PENDING.md with issue #
• rereviewed Files changed in the github PR explorer

[cwgoes]

Tested ACK

Reading through, this makes sense, but the sdk.Coins API is probably going to be confusing to downstream users, as the operations we call less than, greater than, greater than or equal to, etc. do not obey the normal arithmetic laws. Are we sure we want these versions, instead of the ones which would obey normal arithmetic laws (A.LT(B) is true if the amount for any denom in A is less than the corresponding amount in B, for example)

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (develop@2779f4d). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             develop    #2686   +/-   ##
==========================================
  Coverage           ?   58.01%           
==========================================
  Files              ?      153           
  Lines              ?     9511           
  Branches           ?        0           
==========================================
  Hits               ?     5518           
  Misses             ?     3632           
  Partials           ?      361

[alessio]

This could be problematic too:

// IsGTE returns true if they are the same type and the receiver is
// an equal or greater value
func (coin Coin) IsGTE(other Coin) bool {
	return coin.SameDenomAs(other) && (!coin.Amount.LT(other.Amount))
}

// IsLT returns true if they are the same type and the receiver is
// a smaller value
func (coin Coin) IsLT(other Coin) bool {
	return !coin.IsGTE(other)
}

e.g.

coinA.IsLT(coinB)

would return true if the 2 coins were different denom, see:

func TestIsLTCoin(t *testing.T) {
	cases := []struct {
		inputOne Coin
		inputTwo Coin
		expected bool
	}{
		{NewInt64Coin("A", 1), NewInt64Coin("A", 1), false},
		{NewInt64Coin("A", 2), NewInt64Coin("A", 1), false},
		{NewInt64Coin("A", -1), NewInt64Coin("A", 5), true},
		{NewInt64Coin("a", 0), NewInt64Coin("b", 1), true},
	}

	for tcIndex, tc := range cases {
		res := tc.inputOne.IsLT(tc.inputTwo)
		require.Equal(t, tc.expected, res, "coin LT relation is incorrect, tc #%d", tcIndex)
	}
}

[alessio]

It'd be nice if we could fix the Coin.{LT()/GTE} idiosyncrasy too

[alexanderbez]

One minor godoc update and a question about the APIs which I don't quite understand.

[alexanderbez]

Suggested change

	// IsGTE returns True iff for every denom in coins, the denom is present at a
	// IsGT returns True iff for every denom in coins, the denom is present at a

[alexanderbez]

I also don't quite understand these APIs.

assert.True(t, Coins{{"A", one}, {"B", one}}.IsGT(Coins{{"B", one}}))

A is not present in the "other" coins. Shouldn't this return false?

[alessio]

IMHO no, the function's name should be AllGT to mean that the comparison is performed on all denoms.

[alexanderbez]

So the godoc should be updated.

[rigelrozanski]

local-net failing however

[alexanderbez]

@rigelrozanski yes, this is failing on develop (which Jae and I were okay with), but #2692 fixes that.