Swap out ln and exp for Solmate versions
#1992
Conversation
TomAFrench
added
the
do not merge
|
An alternative to explore, we could use PRBMath's implementation. It doesn't have the crazy optimisation theory maths but switching from using base e to base 2 should definitely save gas. |
There was a problem hiding this comment.
I'm in favor of this renaming, but I'd do this in two steps. On the first PR, we create NewLogExpMath and rename the existing one to ReferenceLogExpMath (changing the imports as well). We have equivalence tests etc. Then, on a second PR (once we have confidence in the new impl), we rename NewLogExpMath to LogExpMath, and change the imports from Reference to LogExpMath.
This may be slightly annoying as we'll need to touch the imports twice, but it clearly defines the point in which we change impls, and provides a clean diff (since we'll just have renames, unlike in this PR where the reference is a seemingly new file, and the new impl is also hard to review since the diff is messy (because it replaces old code).
| int256 expectedLn = ReferenceLogExpMath.ln(a); | ||
| int256 actualLn = LogExpMath.ln(a); | ||
|
|
||
| assertApproxEqAbs(actualLn, expectedLn, 2); |
There was a problem hiding this comment.
Wow, I'm surprised this passes with such little difference.
| int256 expectedExp = ReferenceLogExpMath.exp(a); | ||
| int256 actualExp = LogExpMath.exp(a); | ||
|
|
||
| assertApproxEqRel(actualExp, expectedExp, 1e2); |
There was a problem hiding this comment.
It's good to test that they are quite close, but we also definitely want some ffi testing to test that the maximum error remains what it used to be.
|
Have you performed any gas measurements to compare them? Also bytecode size differences. |
Not as of yet. My major concern is the removal of the 36 digit variant of |
Description
This swaps out the exp and ln implementations for the solmate version.
Type of change
Checklist:
master, or there's a description of how to mergeIssue Resolution