-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add unit-tests for Math #775
base: develop
Are you sure you want to change the base?
Conversation
contracts/0.8.9/lib/Math.sol
Outdated
@@ -15,6 +15,8 @@ library Math { | |||
|
|||
/// @notice Tests if x ∈ [a, b) (mod n) | |||
/// | |||
/// It is known that this function is susceptable to underflow/overflow/division-by-zero if | |||
/// intending to use in an unchecked context, be considerate of these possibilities | |||
function pointInHalfOpenIntervalModN(uint256 x, uint256 a, uint256 b, uint256 n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lido does not use these functions, and since they contain potential problems, I suggest removing them from the library at all (and tests too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an interesting proposal 🤔
The bytecode of the contracts will remain the same (because the compiler doesn't emit a bytecode for the unused library functions unless referenced somehow in the code). However, we would need to re-verify the sources on Etherscan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed both the implementation and the associated tests for the unused function.
0e400e9
to
df4d875
Compare
This PR adds forge unit-tests for the Math.sol implementation and additionally adds comments