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

Remove Zero Coins from Parsing #4095

Closed
1 of 4 tasks
alexanderbez opened this issue Apr 10, 2019 · 8 comments
Closed
1 of 4 tasks

Remove Zero Coins from Parsing #4095

alexanderbez opened this issue Apr 10, 2019 · 8 comments
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code)

Comments

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 10, 2019

ParseCoins and ParseDecCoins will currently return an error if a zero (dec) coin is provided as input (via the IsValid check). Arguably, it's a better dev UX to simply remove all the zero (dec) coins after parsing/sorting and not return an error.

/cc @jaekwon


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez alexanderbez added the T: Dev UX UX for SDK developers (i.e. how to call our code) label Apr 10, 2019
@alessio
Copy link
Contributor

alessio commented Apr 11, 2019

I'd argue that

  1. it's legitimate to instantiate a zero-Coin via API
  2. unless there's a use case for ParseCoins() to consider a zero input legit (and AFAIK there isn't any), I would not fix it as it's not broken.

Re: it makes sense to not fail on parsing zero (empty) DecCoins as AFAICR we need to parse config file options where supplying 0 is actually a legit value.

@alessio
Copy link
Contributor

alessio commented Apr 15, 2019

How do we about this then @alexanderbez ?

@alexanderbez
Copy link
Contributor Author

So under further thought, I think allowing zero coins through ParseCoins and simply removing them is not desirable. Doing so would allow zero amount txs to be valid and pass (unless certain checks are added in place). However, ParseDecCoins might be a different case. Perhaps we should allow zero coins to be removed.

@alessio
Copy link
Contributor

alessio commented Apr 15, 2019

Can we parametrise the removal?

func ParseDecCoin(coinStr string, stripZeroes bool) (coin DecCoin, err error)

@alexanderbez
Copy link
Contributor Author

Yeah, good idea @alessio.

@alessio
Copy link
Contributor

alessio commented Apr 16, 2019

Does ParseDecCoins ever need to fail when the underlying slice is empty? Cause we could parametrise ParseDecCoins as well by adding an extra argument failIfZero

@alexanderbez
Copy link
Contributor Author

I'm not sure. I'm honestly tempted to just leave as-is unless a demand for it appears.

@alessio
Copy link
Contributor

alessio commented Apr 16, 2019

No demand, no need to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
None yet
Development

No branches or pull requests

2 participants