fix: threshold check in SD59x18 exp2 function #228
Conversation
There was a problem hiding this comment.
Thanks for your PR!
I have left some feedback below.
We also need to write tests for exp, not just exp2. Could you do this, please?
Also, I tried pushing to your fork, but I wasn't able to do it. This might be because you made the PR against your main branch.
Could you please push your changes to a different branch, cherry-pick the latest commit on my branch below, and open a new PR?
| @@ -2,16 +2,14 @@ | |||
| pragma solidity >=0.8.19 <0.9.0; | |||
|
|
|||
| import { sd } from "src/sd59x18/Casting.sol"; | |||
| import { E, EXP2_MAX_INPUT, MIN_SD59x18, MIN_WHOLE_SD59x18, PI, UNIT, ZERO } from "src/sd59x18/Constants.sol"; | |||
| import { E, EXP2_MAX_INPUT, MIN_SD59x18, MIN_WHOLE_SD59x18, PI, UNIT, ZERO, uEXP2_MIN_THRESHOLD, EXP2_MIN_THRESHOLD } from "src/sd59x18/Constants.sol"; | |||
There was a problem hiding this comment.
has to be ordered alphabetically
| sets.push(set({ x: EXP2_MIN_THRESHOLD, expected: 0.000000000000000001e18 })); | ||
| sets.push(set({ x: EXP2_MIN_THRESHOLD - sd(1), expected: 0 })); | ||
| sets.push(set({ x: -sd(2**255-1), expected: 0 })); |
There was a problem hiding this comment.
smallest value has to be at the top
| sets.push(set({ x: EXP2_MIN_THRESHOLD, expected: 0.000000000000000001e18 })); | ||
| sets.push(set({ x: EXP2_MIN_THRESHOLD - sd(1), expected: 0 })); | ||
| sets.push(set({ x: -sd(2**255-1), expected: 0 })); |
There was a problem hiding this comment.
We can use MIN_SD59x18 instead of -sd(2**255-1).
| @@ -172,6 +173,9 @@ function exp(SD59x18 x) pure returns (SD59x18 result) { | |||
| if (xInt > uEXP_MAX_INPUT) { | |||
| revert Errors.PRBMath_SD59x18_Exp_InputTooBig(x); | |||
| } | |||
| if (xInt < uEXP2_MIN_THRESHOLD) { | |||
There was a problem hiding this comment.
Unexplained. We need to add an explanatory comment
|
Also, wouldn't it be better to return 0 when the input is smaller than -41.45e18? |
|
Closing in favor of #229. |
Fixes #200
Changes include:
Overflow Prevention: Added a check in the exp2 function to return 0 when the input is below a certain threshold. This prevents an overflow error that could occur with large input numbers.
Test Cases: Added new test cases to verify the behavior of the exp2 function when handling inputs around the threshold. These tests ensure that the function behaves as expected and returns 0 when the input is below the threshold.
Threshold Constant: Added a new constant to represent the threshold value