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

Follow up to #2686 -- Coins API #2713

Closed
4 tasks
alexanderbez opened this issue Nov 7, 2018 · 3 comments
Closed
4 tasks

Follow up to #2686 -- Coins API #2713

alexanderbez opened this issue Nov 7, 2018 · 3 comments

Comments

@alexanderbez
Copy link
Contributor

Summary

I believe the changes introduced in #2686 may lead to a confusing API to most users. Take for example:

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

The coin A is not in the "other" set, but based off of the godoc, it would make you seem to believe this should return false.

We should at the very least update the godocs.

/cc @jaekwon @alessio


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alessio
Copy link
Contributor

alessio commented Nov 7, 2018

IMHO we should remove all comparison ops from the type Coins and encourage the use of AmountOf + Coin's comparison ops.

@alexanderbez
Copy link
Contributor Author

@alessio this is what we do mostly in x/bank I believe. But yes, good point. The API is still confusing though.

@jackzampolin
Copy link
Member

This code has undergone significant refactor by @alessio since this issue. Going to go ahead and close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants