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

Stake handler must truncate decimal input #887

Closed
rigelrozanski opened this issue Apr 20, 2018 · 5 comments
Closed

Stake handler must truncate decimal input #887

rigelrozanski opened this issue Apr 20, 2018 · 5 comments

Comments

@rigelrozanski
Copy link
Contributor

To ensure that a transaction can't pass in huge rational values (and cause slow math, generally slowing down the blockchain) we must truncate the decimal-string value which comes in with transactions to the stake handler

@martindale martindale added this to the 1.0 Code Freeze milestone Apr 24, 2018
@cwgoes cwgoes modified the milestone: 1.0 Code Freeze Apr 27, 2018
@ebuchman ebuchman added this to To do in Stability via automation Jun 20, 2018
@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 26, 2018

Do we want this to be true for all modules, or just staking. (I.e. should we alter NewRatFromDecimal, or wherever its called in staking)

Also what precision would be reasonable, 5 values after the decimal point?

@rigelrozanski
Copy link
Contributor Author

Yeah I mean this is only really used when parsing input from the CLI or from LCD - I think we can add a new input to the function NewRatFromDecimal which specifies the number of decimal places. this way the module/application can define what it wants - we probably will define one integer object (probably set to 8) which should be used for displaying the shares as well as parsing the input - for ethermint this will probably be greater

@ValarDragon
Copy link
Contributor

Cool! I'll write a PR for this once unbonding is merged.

@ValarDragon ValarDragon self-assigned this Jun 26, 2018
@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 27, 2018

Wait fixing the UI layer doesn't prevent txs with large rationals from verifying. (Since txs could be sent w/o going through the UI) Should size of the rationals also be enforced in the handler, and if so, how should that be done?

@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 27, 2018

I think this should additionally checked in validate basic for MsgBeginUnbonding and MsgBeginRedelegate. (UI layer should also be fixed, so that it always generates valid txs) I think the better way to specify this however is to ensure that the numerator and denominator both are less than a certain precision. (I'd suggest 32 bits), and for the ui layer, use the previously described truncation method. If this sounds good, I'll go ahead and implement it.

ValarDragon added a commit that referenced this issue Jun 27, 2018
This commit sets the maximum number of decimal points that can be
passed in from messages. This is enforced on the validate basic of
MsgBeginUnbonding and MsgBeginRedelegation. The cli has been
updated to truncate the user input to the specified precision. This
also updates types/rational to return big ints for Num() and Den().

Closes #887
@ValarDragon ValarDragon moved this from To do to In progress in Stability Jun 28, 2018
Stability automation moved this from In progress to Done Jun 29, 2018
cwgoes pushed a commit that referenced this issue Jun 29, 2018
* x/stake: Limit the size of rationals from user input

This commit sets the maximum number of decimal points that can be
passed in from messages. This is enforced on the validate basic of
MsgBeginUnbonding and MsgBeginRedelegation. The cli has been
updated to truncate the user input to the specified precision. This
also updates types/rational to return big ints for Num() and Den().

Closes #887

* Switch NewFromDecimal to error instead of truncating
adrianbrink pushed a commit that referenced this issue Jul 2, 2018
* x/stake: Limit the size of rationals from user input

This commit sets the maximum number of decimal points that can be
passed in from messages. This is enforced on the validate basic of
MsgBeginUnbonding and MsgBeginRedelegation. The cli has been
updated to truncate the user input to the specified precision. This
also updates types/rational to return big ints for Num() and Den().

Closes #887

* Switch NewFromDecimal to error instead of truncating
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

4 participants