Skip to content
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

feat(protocol): LibFixedPointMath contract library license different MAX_EXP_INPUT values #14344

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

MarcusWentz
Copy link
Contributor

@MarcusWentz MarcusWentz commented Aug 2, 2023

Motivation

Add license to third party contract library as suggested by @d1onys1us.

Concerns

Expected return types and values seem slightly off.

PRB math exp():
https://github.com/PaulRBerg/prb-math/blob/main/src/sd59x18/Math.sol#L152-L167
type SD59x18 (signed integer) return type seems different than int256 and has

MAX_EXP_INPUT=133084258667509499440;

while our current exp() function supports has

MAX_EXP_INPUT=135305999368893231588;

With

exp(133084258667509499440)

original returns

6277101735386680963982882153334421099665277130643420769258879401421053268429

PRB converted to int256 returns

6277101735386680754977611748738314679353920434623901771623

Should we keep the old fixed point exp() function?
Where do we use exp() math? How critical is exp() resolution? Do we have test edge cases with exp()?

@MarcusWentz MarcusWentz requested a review from dantaik August 2, 2023 04:21
@MarcusWentz MarcusWentz marked this pull request as draft August 2, 2023 04:22
@adaki2004
Copy link
Contributor

adaki2004 commented Aug 3, 2023

@MarcusWentz
I get the point to not use unlicensed files, but first would be free to ask Remco if he would be willing to change the license to MIT - then we do not need to put engineering efforts into it.

This is a critical component (part of 1559 math) and i think would require thorough testing which I do not know we want to go for (?). Since i cannot really estimate the real workload it seems floating point notation differs a little bit, it can be tricky to be tailored. Also would be cool having it as an npm package as well - is it available ?

@adaki2004
Copy link
Contributor

adaki2004 commented Aug 3, 2023

Remco's answer:

You can also find it on my site with an MIT License in the footer.

2π.com/22/exp-ln

I recommed you take it from @transmissions11 Solmate V7 as it's maintained and has test coverage: 

[github.com/transmissions1…](https://github.com/transmissions11/solmate/blob/v7/src/utils/FixedPointMathLib.sol)

@adaki2004
Copy link
Contributor

Compared the expWad() (as Remco suggested) line by line from solmate v7 and only parameters (how to calculate p and q) changed but overall fixed 1e18 size notation and upper/lower limits are the same. Applied those numbers and we are under MIT license now.

Merged via the queue into main with commit c6e391d Aug 3, 2023
@dionysuzx dionysuzx deleted the fixedTest branch August 3, 2023 16:31
@github-actions github-actions bot mentioned this pull request Aug 3, 2023
2manslkh pushed a commit that referenced this pull request Aug 7, 2023
…MAX_EXP_INPUT values (#14344)

Co-authored-by: adaki2004 <[email protected]>
Co-authored-by: d1onys1us <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants