morpho-org / morpho-utils

Repository gathering useful libraries and contracts.
GNU Affero General Public License v3.0
64 stars 1 forks source link

Factorize math constants #112

Closed QGarchery closed 1 year ago

QGarchery commented 1 year ago

The purpose of this PR is to avoid defining the same constants multiple times, so they are all defined at the same place and used when needed

github-actions[bot] commented 1 year ago

Changes to gas cost

Generated at commit: a0a313dcc95e300398837df939413721dd2d5c9c, compared to commit: 61edcbd0a364ba9acd1b3e7caf8b65c719a3997f

๐Ÿงพ Summary (20% most significant diffs)

Contract Method Avg (+/-) %
MathRef contract log2Naive +343 โŒ +0.41%

Full diff report ๐Ÿ‘‡
| Contract | Deployment Cost (+/-) | Method | Min (+/-) | % | Avg (+/-) | % | Median (+/-) | % | Max (+/-) | % | # Calls (+/-) | |:-|-:|:-|-:|-:|-:|-:|-:|-:|-:|-:|-:| | **MathRef contract** | 238,081 (0) | _log2Naive_
_safeAbs_ | 84,964 (+343)
422 (+80) | **+0.41%**
**+23.39%** | 84,964 (+343)
422 (+80) | **+0.41%**
**+23.39%** | 84,964 (+343)
422 (+80) | **+0.41%**
**+23.39%** | 84,964 (+343)
422 (+80) | **+0.41%**
**+23.39%** | 1 (0)
1 (0) |
MathisGD commented 1 year ago

I'm not a big big fan. Btw, I'm still in favor of #57, which would naturally fix this issue.

MerlinEgalite commented 1 year ago

I'm not a big big fan. Btw, I'm still in favor of #57, which would naturally fix this issue.

not a big fan as well tbh

QGarchery commented 1 year ago

I don't have a strong opinion on it, but could you explain what you don't like about it @MathisGD @MerlinEgalite ?

QGarchery commented 1 year ago

Closing as it's not completely necessary, and it may be worth it to not factorize simple libraries