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

types.Coins hardening: NewCoin and NewDecCoin should fail if denom is empty #3614

Closed
4 tasks
alessio opened this issue Feb 12, 2019 · 4 comments · Fixed by #3607
Closed
4 tasks

types.Coins hardening: NewCoin and NewDecCoin should fail if denom is empty #3614

alessio opened this issue Feb 12, 2019 · 4 comments · Fixed by #3607

Comments

@alessio
Copy link
Contributor

alessio commented Feb 12, 2019

NewCoin and NewDecCoin functions should panic if the supplied denomination is empty in order to reduce surface of vulnerabilities that could potentially be introduced by lack of preemptive validation (e.g. by not calling IsValid())

On the other hand, ParseCoin functions should check for empty denoms and appropriately handle empty denoms zero-coin cases as per argument supplied by the caller. ParseCoin functions might take an additional parameter strict for instance to either return a ZeroCoin() or fail in zero-coin cases.
Alternatively, we may want to provide an alternative implementation for each case, e.g. ParsePositiveCoins, ParseNonNegativeCoins - either ways the client would not be required to call IsZero or IsPositive on the return value.

/cc @alexanderbez @cwgoes @sunnya97 @jackzampolin


For Admin Use

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

cwgoes commented Feb 12, 2019

Sure; we should also harden the UnmarshalJSON functions if we want to defensively-code in this way.

@alexanderbez
Copy link
Contributor

alexanderbez commented Feb 12, 2019

Adding validation is great to the new coin constructors! But ParseCoin/s already does validate denom via the regex: [[:alpha:]][[:alnum:]]{2,15}

@jackzampolin
Copy link
Member

I like all these defensive coding ideas.

@cwgoes
Copy link
Contributor

cwgoes commented Feb 12, 2019

Adding validation is great to the new coin constructors! But ParseCoin/s already does validate denom via the regex: [[:alpha:]][[:alnum:]]{2,15}

Ah excellent, let's use the same validation in the NewXYZ constructors then, as long as it isn't too expensive.

alessio pushed a commit that referenced this issue Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants