[N23] Unused code #2905
[N23] Unused code #2905
Conversation
| * Test add(maxFixedAdd(),maxFixedAdd()) equals maxFixedAdd() + maxFixedAdd() | ||
| * Test add(maxFixedAdd()+1,maxFixedAdd()+1) throws | ||
| */ | ||
| function maxFixedAdd() internal pure returns (uint256) { |
There was a problem hiding this comment.
Hmm, so I think this could still be a potentially useful number to keep in this file, at least in a comment. Can be useful information for consumers of this library.
There was a problem hiding this comment.
+1 to keeping as a comment
| @@ -214,6 +191,7 @@ library FixidityLib { | |||
| * Test multiply(maxFixedMul()+1,maxFixedMul()+1) fails | |||
| */ | |||
| function multiply(Fraction memory x, Fraction memory y) internal pure returns (Fraction memory) { | |||
| require(x.value <= maxFixedMul() && y.value <= maxFixedMul()); | |||
There was a problem hiding this comment.
I believe there are unit tests that verify this reverts anyways without this check when the inputs are too large. I understand being conservative, as this library aims to be safe, but it also aims to be efficient, do we need an additional require here?
There was a problem hiding this comment.
Good point. Will make the functions that expose these values external, so that clients that consume this code can do input sanitation more easily.
There was a problem hiding this comment.
Actually nevermind, as this increases bytecode size. Going to move everything to comments.
Codecov Report
@@ Coverage Diff @@
## master #2905 +/- ##
========================================
Coverage 73.37% 73.37%
========================================
Files 570 570
Lines 14277 14277
Branches 1722 1482 -240
========================================
Hits 10476 10476
+ Misses 3516 3514 -2
- Partials 285 287 +2
Continue to review full report at Codecov.
|
Description
This PR removes unused code, except for FractionUtil, which is removed in #2890.
Note that the maximum values were being exposed for testing purposes, and the existing code already has tests showing transactions that would cause overflows properly revert. For posterity, these maximum values are instead preserved as part of the docstring on the relevant functions.
Tested
Not tested
Other changes
Describe any minor or "drive-by" changes here.
Related issues
Backwards compatibility
Yes