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

Check for overflow in Coin.Multiply and return error #326

Closed
ethanfrey opened this issue Feb 19, 2019 · 1 comment · Fixed by #351
Closed

Check for overflow in Coin.Multiply and return error #326

ethanfrey opened this issue Feb 19, 2019 · 1 comment · Fixed by #351
Assignees

Comments

@ethanfrey
Copy link
Contributor

ethanfrey commented Feb 19, 2019

Update In the end, we just add a check to Multiply to see if it will overflow. If it will overflow, return an error instead. (Using normal int64 math or big to check it).

Then we can guarantee no overflows can happen

Is your feature request related to a problem? Please describe.

This came up in a PR review comment #322 (comment)

Describe the solution you'd like

Never overflow when calculating with Coins

Describe alternatives you've considered

We can use big, or we can just limit the ranges to make sure we never overflow int64. I think the second case is quite easy, as total token supply should generally fit in int32 and int64 was for super-safety already.

But let's discuss if this is needed and how urgent

@ethanfrey ethanfrey added the design Needs design work label Feb 19, 2019
@ethanfrey
Copy link
Contributor Author

My proposal:

Limit each account to store 2^32 in Whole (MaxInt32).
Limit any multiplier to 2^31 (fee dist)
We can never have an overflow.

I think this is allows lots of math using the normal ints and restricting an account to 4 billion tokens shouldn't be a big problem (seriously, even if you had more, please put them behind multiple private keys)

@ethanfrey ethanfrey added question and removed design Needs design work question labels Feb 19, 2019
@ethanfrey ethanfrey changed the title Use Big numbers for coin calculations Check for overflow in Coin.Multiply and return error Feb 20, 2019
@husio husio self-assigned this Feb 22, 2019
husio added a commit that referenced this issue Feb 25, 2019
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

Successfully merging a pull request may close this issue.

2 participants