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

Fix lens visibility #1355

Merged
merged 1 commit into from
Oct 15, 2022
Merged

Fix lens visibility #1355

merged 1 commit into from
Oct 15, 2022

Conversation

MerlinEgalite
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 13, 2022

Morpho-aave-v2 gas impacts (eth-mainnet)

Generated at commit: 7c1fd7c87b07626ac3eb7315a65e5abc3538209d, compared to commit: ff8202702bc648392a1235961a4baf86f6830117

🧾 Summary

Contract Method Avg (+/-) %
Lens computeLiquidationRepayAmount
getAverageBorrowRatePerYear
getCurrentBorrowBalanceInOf
getCurrentUserBorrowRatePerYear
getIndexes
getMainMarketData
getMarketConfiguration
getNextUserSupplyRatePerYear
getRatesPerYear
getTotalSupply
getUserHealthFactor
-89 ✅
+22 ❌
+22 ❌
+22 ❌
+22 ❌
+22 ❌
+22 ❌
-89 ✅
-88 ✅
+22 ❌
+22 ❌
-0.08%
+0.06%
+0.12%
+0.04%
+0.09%
+0.04%
+0.06%
-0.13%
-0.26%
+0.01%
+0.03%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
Lens 4,056,848 (-20,426) computeLiquidationRepayAmount
getAverageBorrowRatePerYear
getCurrentBorrowBalanceInOf
getCurrentUserBorrowRatePerYear
getIndexes
getMainMarketData
getMarketConfiguration
getNextUserSupplyRatePerYear
getRatesPerYear
getTotalSupply
getUserHealthFactor
73,808 (-89)
27,755 (+22)
19,016 (+22)
49,882 (+22)
18,185 (+22)
59,624 (+22)
34,117 (+22)
50,142 (-89)
30,769 (-88)
221,654 (+22)
78,739 (+22)
-0.12%
+0.08%
+0.12%
+0.04%
+0.12%
+0.04%
+0.06%
-0.18%
-0.29%
+0.01%
+0.03%
104,913 (-89)
37,971 (+22)
19,021 (+22)
52,470 (+22)
23,257 (+22)
59,624 (+22)
34,117 (+22)
66,103 (-89)
33,994 (-88)
222,092 (+22)
82,876 (+22)
-0.08%
+0.06%
+0.12%
+0.04%
+0.09%
+0.04%
+0.06%
-0.13%
-0.26%
+0.01%
+0.03%
120,449 (-89)
28,420 (+22)
19,016 (+22)
49,933 (+22)
21,961 (+22)
59,624 (+22)
34,117 (+22)
62,322 (-89)
30,803 (-88)
222,092 (+22)
81,531 (+22)
-0.07%
+0.08%
+0.12%
+0.04%
+0.10%
+0.04%
+0.06%
-0.14%
-0.28%
+0.01%
+0.03%
128,398 (-89)
85,708 (+22)
19,033 (+22)
62,420 (+22)
36,800 (+22)
59,624 (+22)
34,117 (+22)
122,159 (-89)
82,022 (-88)
222,531 (+22)
93,707 (+22)
-0.07%
+0.03%
+0.12%
+0.04%
+0.06%
+0.04%
+0.06%
-0.07%
-0.11%
+0.01%
+0.02%
7 (0)
6 (0)
3 (0)
5 (0)
6 (0)
1 (0)
1 (0)
9 (0)
21 (0)
2 (0)
7 (0)

Copy link
Collaborator

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

But I think that

  • getCurrentP2PSupplyIndex is public but not used internally
  • getCurrentP2PBorrowIndex is public but not used internally
  • getIndexes does pretty much exactly what _getIndexes does...
  • _getTotalMarketSupply does pretty much exactly what getTotalMarketSupply does... and getTotalMarketSupply is public but not used internally
  • _getTotalMarketBorrow does pretty much exactly what getTotalMarketBorrow does... and getTotalMarketBorrow is public but not used internally
  • getCurrentUserSupplyRatePerYear is public but not used internally
  • getCurrentUserBorrowRatePerYear is public but not used internally
  • getCurrentUserBorrowRatePerYear is public but not used internally, and does pretty much exactly what _getCurrentUserBorrowRatePerYear does
  • same for borrow
  • getUserBalanceStates is public but not used internally

@Rubilmax
Copy link
Collaborator

It was previously addressed in this PR: https://github.com/morpho-dao/morpho-v1/pull/1273/files
getIndexes is moved from public to external and it's missing here

@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 13, 2022

getIndexes does pretty much exactly what _getIndexes does...
_getTotalMarketSupply does pretty much exactly what getTotalMarketSupply does...
_getTotalMarketBorrow does pretty much exactly what getTotalMarketBorrow does...
getCurrentUserBorrowRatePerYear is public but not used internally, and does pretty much exactly what _getCurrentUserBorrowRatePerYear does

yes but changing their return type is breaking
the goal of these was to not include the underlying token address to the response

@MathisGD
Copy link
Collaborator

yes but changing their return type is breaking the goal of these was to not include the underlying token address to the response

Of course I'm not saying that we should change it now. I'm just typing some remark as I go though it.

@MerlinEgalite MerlinEgalite merged commit b4ca7e1 into fix/lens-aave-v2 Oct 15, 2022
@MerlinEgalite MerlinEgalite deleted the fix/lens-visibility branch October 15, 2022 08:22
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.

None yet

5 participants