x/stake: Limit the size of rationals from user input #1415
Conversation
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
Codecov Report
@@ Coverage Diff @@
## develop #1415 +/- ##
===========================================
- Coverage 63.18% 63.09% -0.09%
===========================================
Files 118 118
Lines 6554 6563 +9
===========================================
Hits 4141 4141
- Misses 2144 2151 +7
- Partials 269 271 +2 |
types/int.go
Outdated
| @@ -214,6 +214,11 @@ func (i Int) Neg() (res Int) { | |||
| return Int{neg(i.i)} | |||
| } | |||
|
|
|||
| // String converts int to string | |||
| func (i Int) String() string { | |||
| return i.i.String() | |||
There was a problem hiding this comment.
Does the tendermint-lint require a comment on this function? I don't think it does - pretty straight forward probably can remove the comment (unless this needs to be added to our linter)
types/rational.go
Outdated
| @@ -61,6 +61,9 @@ func NewRatFromDecimal(decimalStr string) (f Rat, err Error) { | |||
| if len(str[0]) == 0 || len(str[1]) == 0 { | |||
| return f, ErrUnknownRequest("not a decimal string") | |||
| } | |||
| if len(str[1]) > prec { | |||
| str[1] = str[1][:prec] | |||
| } | |||
There was a problem hiding this comment.
I think we should actually cause an error if somebody is sending in to much precision not just truncate it - we shouldn't fail silently should let user know
| numBig.Neg(numBig) | ||
| } | ||
|
|
||
| return NewRatFromBigInt(numBig, big.NewInt(denom)), nil |
There was a problem hiding this comment.
oh whoa so now we could take in huge stuff? cool!
types/rational.go
Outdated
| @@ -70,7 +73,17 @@ func NewRatFromDecimal(decimalStr string) (f Rat, err Error) { | |||
|
|
|||
| num, errConv := strconv.Atoi(numStr) | |||
| if errConv != nil { | |||
| return f, ErrUnknownRequest(errConv.Error()) | |||
| // resort to big int, don't make this default option for efficiency | |||
There was a problem hiding this comment.
I feel like a better thing to do would be to check the specific type of error - only if it was an (overflow?) error then we would go to this next step... shouldn't have to go inside here if the user is attempting to convert "alskgndnalsg" into a rational
types/rational_test.go
Outdated
| @@ -31,7 +33,10 @@ func TestNewFromDecimal(t *testing.T) { | |||
| {"1.1", false, NewRat(11, 10)}, | |||
| {"0.75", false, NewRat(3, 4)}, | |||
| {"0.8", false, NewRat(4, 5)}, | |||
| {"0.11111", false, NewRat(11111, 100000)}, | |||
| {"0.11111", false, NewRat(1111, 10000)}, | |||
| {"628240629832763.5738930323617075341", false, NewRat(3141203149163817869, 5000)}, | |||
There was a problem hiding this comment.
As I mentioned earlier we should send an error if there are to many decimal places
|
Addressed PR comments |
|
Huh, test_unit doesn't fail on my machine. I'll rerun on circle to see if its a non-deterministic failure. |
|
It was a non-deterministic failure. Re-ran on circle and it passed. |
|
now test cover failing |
|
I think that was non-deterministic also, re-ran and it passed. |
* 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
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