-
Notifications
You must be signed in to change notification settings - Fork 670
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
✨ FixedMathLib: expWadDown() #171
Conversation
Hmm... that should have been a ~10 gas reduction. |
Can't inline the variables |
We can always go back to the original formula for I have a similar scheme for
but that one somehow ends up costing 1 gas more than the plain formula in my local tests. |
want me to commit? seems like the scheme for p and the original formula for q is the fastest |
lol! In my repo it's exact opposite! Yeah, go a head an use whatever works best. But I wonder what's going on though. |
assertEq( | ||
FixedPointMathLib.expWadDown(0), | ||
999999999999999999 | ||
// True value: 1000000000000000000 (off by 1 wei) |
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.
This actually annoys me quite a bit.
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.
People know a^0 = 1 from their algebra class, and might write contracts that depends on fp.mul(x, fp.exp(0)) == x
. Would be nice if this held.
Easy fix is to add 1 wei to the output. Can probably do that at zero extra gas by adding to the constant term of the p polynomial. It should not hurt the accuracy of other values much. Let me fiddle with this.
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.
Hmm that will give the effect of rounding up in some areas I believe
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.
lmk adding to the constant term works without impacting the other vars
another way thats only a couple extra opcodes would be doing
assembly {
r := add(r, iszero(x))
}
right at the end
is it optimizer run related? we use 1m in here |
// p is made monic, we'll multiply by a scale factor later. | ||
int256 y = x + 1346386616545796478920950773328; | ||
y = ((y * x) >> 96) + 57155421227552351082224309758442; | ||
int256 p = y + x - 94201549194550492254356042504812; | ||
p = ((p * y) >> 96) + 28719021644029726153956944680412240; | ||
p = p * x + (4385272521454847904632057985693276 << 96); |
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.
should we add a comment about using knuth's method here?
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.
Sure, it's the first method from page 491. He mentions a second one which has one fewer addition, but leaves the details as an exercise. I haven't bother to do that exercise yet :)
btw how hard would it be to implement a rounding up version of this? looks like this impl truncates (which is great but i'd like to have both a floor and ceil version) |
This should work (assuming we fix the exp(0) = 1 issue): function expCeil(int256 x) returns (int256) {
if (x == 0) return 1e18; // ceil(exp(0)) = floor(exp(0)) = 1
return expFloor(x) + 1;
} |
But I don't think it is guaranteed to return the truncated / floor result. For larger input values it doesn't have the precision required to do accurate rounding. It also doesn't make sense to increase the precision for large inputs, because the input itself is only accurate up to 1e-18. |
oh hmm yea maybe just got lucky with the test cases i used |
Description
Describe the changes made in your pull request here.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
dapp snapshot
?npm run lint
?forge test
?dapp test
?Pull requests with an incomplete checklist will be thrown out.