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.

Sign up for GitHub

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

Jump to bottom

Update solc versions #1364

Merged
merged 4 commits into from
Oct 26, 2022
Merged

Update solc versions #1364

merged 4 commits into from
Oct 26, 2022

Conversation

MerlinEgalite
Copy link
Contributor

@MerlinEgalite MerlinEgalite commented Oct 15, 2022 •
edited
Loading

Fixes #1351

Allow easier integrations by allowing more solc versions in libs and interfaces

@MerlinEgalite MerlinEgalite requested review from a team October 15, 2022 08:36
@MerlinEgalite MerlinEgalite self-assigned this Oct 15, 2022
@github-actions
Copy link

github-actions bot commented Oct 15, 2022 •
edited
Loading

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

Generated at commit: ff22e3bf454d737daf392eb11ffc1b9cec787c54, compared to commit: 029c29f5b0033a87f267221a694a721ec1507553

🧾 Summary

Contract Method Avg (+/-) %
Lens computeLiquidationRepayAmount +6,663 ❌ +6.78%
Morpho borrowBalanceInOf
deltas
market
p2pBorrowIndex
p2pSupplyIndex
poolIndexes
supplyBalanceInOf		 -2 ✅
-4 ✅
-1 ✅
-1 ✅
-1 ✅
-2 ✅
-2 ✅		 -0.11%
-0.24%
-0.09%
-0.13%
-0.12%
-0.20%
-0.14%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
Lens 4,056,848 (0) computeLiquidationRepayAmount 73,808 (0) 0.00% 104,913 (+6,663) +6.78% 120,449 (+33,467) +38.48% 128,398 (0) 0.00% 7 (0)
Morpho 3,436,193 (0) borrowBalanceInOf
deltas
market
p2pBorrowIndex
p2pSupplyIndex
poolIndexes
supplyBalanceInOf		 916 (0)
1,050 (0)
986 (0)
571 (0)
635 (0)
786 (0)
936 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 1,825 (-2)
1,679 (-4)
1,162 (-1)
767 (-1)
829 (-1)
1,004 (-2)
1,476 (-2)		 -0.11%
-0.24%
-0.09%
-0.13%
-0.12%
-0.20%
-0.14%		 916 (0)
1,050 (0)
986 (0)
571 (0)
635 (0)
786 (0)
936 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 4,916 (0)
9,050 (0)
2,986 (0)
2,571 (0)
2,635 (0)
2,786 (0)
4,936 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 330 (+1)
362 (+2)
318 (+2)
234 (+1)
236 (+1)
247 (+2)
318 (+1)

Rubilmax
Rubilmax reviewed Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@Rubilmax Rubilmax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • The Morpho-AaveV2 Lens solc version was updated: I believe it's a mistake
  • I'm not comfortable with allowing versions higher than 0.8.x in libraries, because we don't know how they'll behave with >=0.9.0. Even for Types libraries (so I would revert changes in libraries and only open interfaces)

MathisGD
MathisGD reviewed Oct 17, 2022
View reviewed changes
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.

I would have fixed the versions of the libraries, otherwise looks good !

Base automatically changed from fix/lens-aave-v2 to refactor/reset-state October 17, 2022 14:56
@MerlinEgalite
Copy link
Contributor Author

I would have fixed the versions of the libraries, otherwise looks good !

I don't thnk so it's a more pain for integrators than anything else

@MerlinEgalite MerlinEgalite changed the base branch from refactor/reset-state to dev October 17, 2022 16:36
@Rubilmax Rubilmax mentioned this pull request Oct 18, 2022
Merged
@MerlinEgalite MerlinEgalite changed the base branch from dev to upgrade-0 October 19, 2022 10:09
@MerlinEgalite MerlinEgalite linked an issue Oct 19, 2022 that may be closed by this pull request
Allow any solidity version in interfaces #1351
Closed
QGarchery
QGarchery reviewed Oct 24, 2022
View reviewed changes
Copy link
Collaborator

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

We could also fix the following files, they should be compiled with 0.8.13 I believe:
contracts/common/rewards-distribution/RewardsDistributor.sol
contracts/common/test/FakeToken.sol
test-foundry/aave-v2/TestRatesLens.t.sol
test-foundry/common/TestDoubleLinkedList.t.sol
test-foundry/common/helpers/MorphoToken.sol

@MerlinEgalite
Copy link
Contributor Author

Discussed with @QGarchery tests can be compiled with any 0.8 versions since the imported contracts will set the solc version anyway

@github-actions
Copy link

Morpho-compound gas impacts (eth-mainnet)

Generated at commit: ff22e3bf454d737daf392eb11ffc1b9cec787c54, compared to commit: 029c29f5b0033a87f267221a694a721ec1507553

🧾 Summary

Contract Method Avg (+/-) %
Morpho borrowBalanceInOf
lastPoolIndexes
liquidate
p2pBorrowIndex
p2pSupplyIndex
supplyBalanceInOf		 +5 ❌
+3 ❌
-2,277 ✅
+1 ❌
+1 ❌
+3 ❌		 +0.35%
+0.33%
-0.70%
+0.15%
+0.14%
+0.26%
PositionsManager liquidateLogic -1,961 ✅ -0.59%
Lens computeLiquidationRepayAmount
isLiquidatable		 +571 ❌
+180 ❌		 +0.53%
+0.31%
RewardsManager accrueUserBorrowUnclaimedRewards
accrueUserSupplyUnclaimedRewards		 +37 ❌
+33 ❌		 +0.16%
+0.12%
InterestRatesManager updateP2PIndexes +23 ❌ +0.08%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
Morpho 3,267,385 (0) borrowBalanceInOf
lastPoolIndexes
liquidate
p2pBorrowIndex
p2pSupplyIndex
supplyBalanceInOf		 894 (0)
742 (0)
4,065 (0)
571 (0)
569 (0)
936 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 1,424 (+5)
919 (+3)
322,433 (-2,277)
687 (+1)
692 (+1)
1,146 (+3)		 +0.35%
+0.33%
-0.70%
+0.15%
+0.14%
+0.26%		 894 (0)
742 (0)
388,716 (+42)
571 (0)
569 (0)
936 (0)		 0.00%
0.00%
+0.01%
0.00%
0.00%
0.00%		 4,894 (0)
2,742 (0)
556,854 (0)
2,571 (0)
2,569 (0)
4,936 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 520 (-5)
349 (-6)
28 (-1)
480 (-5)
453 (-5)
476 (-5)
PositionsManager 4,301,120 (0) liquidateLogic 2,901 (0) 0.00% 329,402 (-1,961) -0.59% 384,583 (+22) +0.01% 552,720 (0) 0.00% 27 (-1)
Lens 4,875,711 (0) computeLiquidationRepayAmount
isLiquidatable		 57,311 (0)
54,487 (0)		 0.00%
0.00%		 108,634 (+571)
59,180 (+180)		 +0.53%
+0.31%		 96,652 (0)
54,487 (0)		 0.00%
0.00%		 169,848 (0)
93,585 (0)		 0.00%
0.00%		 20 (-1)
25 (-1)
RewardsManager 972,578 (0) accrueUserBorrowUnclaimedRewards
accrueUserSupplyUnclaimedRewards		 2,107 (0)
2,129 (0)		 0.00%
0.00%		 23,228 (+37)
28,041 (+33)		 +0.16%
+0.12%		 23,802 (0)
25,824 (0)		 0.00%
0.00%		 56,277 (0)
57,058 (0)		 0.00%
0.00%		 576 (-1)
785 (-1)
InterestRatesManager 806,808 (0) updateP2PIndexes 618 (0) 0.00% 27,186 (+23) +0.08% 23,286 (0) 0.00% 102,847 (0) 0.00% 2,314 (-2)

@MathisGD
Copy link
Collaborator

I don't thnk so it's a more pain for integrators than anything else

But integrators should not need the libraries. They should only need the interfaces (otherwise we should refactor the files).

@MerlinEgalite
Copy link
Contributor Author

I don't thnk so it's a more pain for integrators than anything else

But integrators should not need the libraries. They should only need the interfaces (otherwise we should refactor the files).

I disagree, you can need Types for example

@MathisGD
Copy link
Collaborator

So why not putting Types as an interface. Idk why we would hardcode contracts versions but let libraries version be the one the integrator wants. There is not less risk than a normal contract, because there could be any code in it.

@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 24, 2022 •
edited
Loading

I don't thnk so it's a more pain for integrators than anything else

But integrators should not need the libraries. They should only need the interfaces (otherwise we should refactor the files).

Why would this be true? Take Uniswap's PathLib or BytesLib for example
On Morpho, an integrator could have the need for InterestRatesModel for example

I don't understand your POV as libraries are made for the purpose of sharing pure logic code... This means they should be made usable by third parties (outside the main application I mean, which in this context is Morpho)

So why not putting Types as an interface. Idk why we would hardcode contracts versions but let libraries version be the one the integrator wants. There is not less risk than a normal contract, because there could be any code in it.

The former objective of fixing contracts version is to indicate which version was used to compile contracts. But they don't have an inherent, fixed version. We only do so to fix a version to the protocol. But libraries are made to be usable outside the protocol, so there version can't be fixed

Allowing versions to be in 0.8.x is not a security threat to me. If we consider it a threat, then I agree libraries should have a fixed version, but it would come at a cost for integrators (but again, I would be ok for it).

@pakim249CAL
Copy link
Contributor

Ultimately, I would lean towards being lenient with the solidity versions for libs. If libs are being imported for use by integrators, it is their responsibility to make sure that there is nothing breaking, which is how I personally approach importing libs.

@MerlinEgalite MerlinEgalite merged commit 4f35c1a into upgrade-0 Oct 26, 2022
@MerlinEgalite MerlinEgalite deleted the refactor/solc-versions branch October 26, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rubilmax Rubilmax Rubilmax approved these changes

@QGarchery QGarchery QGarchery approved these changes

@pakim249CAL pakim249CAL pakim249CAL approved these changes

@MathisGD MathisGD Awaiting requested review from MathisGD

Assignees

@MerlinEgalite MerlinEgalite

Labels
👻 aave-v2 👻 aave-v3 🟢 compound 🏹 fuzzing 🧪 testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow any solidity version in interfaces
5 participants
@MerlinEgalite @MathisGD @Rubilmax @pakim249CAL @QGarchery