Feat/base 1 #12

pakim249CAL · 2022-12-17T16:48:31Z

A proposed initial layout of the contract structure.

MerlinEgalite

Love the ideas. I added some comments too

.prettierrc.json

MerlinEgalite · 2022-12-18T11:28:59Z

src/interfaces/aave/IAToken.sol

@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GNU AGPLv3


should be accessible from lib

Not sure what you mean. How can a lib cannot access this?

from the imported library aave-core-v3 I mean

MerlinEgalite · 2022-12-18T11:29:06Z

src/interfaces/aave/IPool.sol

@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GNU AGPLv3


MerlinEgalite · 2022-12-18T11:31:25Z

src/libraries/Libraries.sol

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: UNLICENSED


I don't really get the usage of this file tbh

This library is meant to aggregate the libraries we use in Morpho. This IMO is important in our case so that we are less likely to run into the issue of using two versions of the same library accidentally. It doesn't have to be this way, but I think this is one way we can softly enforce what libraries we use in our contracts.

Ok we can go with it for now

src/libraries/MarketLib.sol

src/MatchingEngine.sol

src/MorphoInternal.sol

MerlinEgalite

I'm globally ok with the changes :)

Note that I didn't check the in-depth logic, security, etc.

Rubilmax

I really like the internal-by-default pattern
I also like the library-based plug-in logic

There a lot of conventions introduced in this PR, which I am for or not against, but we shouldn't consider them set in stone as quick as this PR is merged because we (at least I) need to test how they behave

src/interfaces/IERC1155.sol

Rubilmax · 2022-12-19T06:06:41Z

src/libraries/Errors.sol

@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.17;

-library Errors {}
+library Errors {


This library will be used much like an enum and the common naming standard of enum is singular
This is obviously not mandatory

Whatever works for you guys

Do you mean having the library renamed to be a single letter? @Rubilmax

Sorry, I should have been more specific. I had in mind to name it Error instead of Errors, to use it like revert Error.MarketNotCreated()

Rubilmax · 2022-12-19T06:07:07Z

src/libraries/Events.sol


-library Events {}
+library Events {


This library will be used much like an enum and the common naming standard of enum is singular
This is obviously not mandatory

src/libraries/Constants.sol

Rubilmax · 2022-12-19T06:10:16Z

src/libraries/Types.sol

+        uint256 p2pSupplyIndex; // 256 bits
+        uint256 p2pBorrowIndex; // 256 bits
+        BorrowMask borrowMask; // 256 bits
+        Delta deltas; // 1024 bits
+        uint32 lastUpdateTimestamp; // 32 bits
+        uint112 poolSupplyIndex; // 112 bits
+        uint112 poolBorrowIndex; // 112 bits
+        address underlying; // 168 bits
+        address variableDebtToken; // 168 bits
+        uint16 reserveFactor; // 16 bits
+        uint16 p2pIndexCursor; // 16 bits
+        PauseStatuses pauseStatuses; // 64 bits


Could we count in bytes? 😁

EDIT: Actually I don't know if it's a good idea

Bytes seems okay as this is the minimal unit of each variable, but I prefer bits since it requires no translation between something like uint256 to bytes.

Rubilmax · 2022-12-19T06:46:18Z

src/libraries/MarketMaskLib.sol

+    function setSupplying(Types.UserMarkets memory userMarkets, Types.BorrowMask memory borrowMask, bool supplying)
+        internal
+        pure
+        returns (Types.UserMarkets memory newUserMarket)


Could we modify the input struct and return it? Or is it bad software design?

Rubilmax · 2022-12-19T06:47:33Z

src/libraries/Types.sol

+        ThreeHeapOrdering.HeapArray suppliersP2P; // in scaled unit
+        ThreeHeapOrdering.HeapArray suppliersPool; // in scaled unit
+        ThreeHeapOrdering.HeapArray borrowersP2P; // in scaled unit
+        ThreeHeapOrdering.HeapArray borrowersPool; // in scaled unit


Suggested change

ThreeHeapOrdering.HeapArray suppliersP2P; // in scaled unit

ThreeHeapOrdering.HeapArray suppliersPool; // in scaled unit

ThreeHeapOrdering.HeapArray borrowersP2P; // in scaled unit

ThreeHeapOrdering.HeapArray borrowersPool; // in scaled unit

ThreeHeapOrdering.HeapArray p2pSuppliers; // in scaled unit

ThreeHeapOrdering.HeapArray poolSuppliers; // in scaled unit

ThreeHeapOrdering.HeapArray p2pBorrowers; // in scaled unit

ThreeHeapOrdering.HeapArray poolBorrowers; // in scaled unit

Rubilmax · 2022-12-19T06:48:28Z

src/libraries/Types.sol

+    struct AssetLiquidityData {
+        uint256 decimals; // The number of decimals of the underlying token.
+        uint256 tokenUnit; // The token unit considering its decimals.
+        uint256 liquidationThreshold; // The liquidation threshold applied on this token (in basis point).
+        uint256 ltv; // The LTV applied on this token (in basis point).
+        uint256 underlyingPrice; // The price of the token (In base currency in wad).
+    }
+
+    struct LiquidityData {
+        uint256 collateral; // The collateral value (In base currency in wad).
+        uint256 maxDebt; // The max debt value (In base currency in wad).
+        uint256 liquidationThresholdValue; // The liquidation threshold value (In base currency in wad).
+        uint256 debt; // The debt value (In base currency in wad).
+    }


These could be updated to match branch upgrade-morpho-1

Hmm remember we're dealing with Aave V3. So we first need to make sure that it's using the same units

And it depends on the network lol... So value may be the best keyword in this case (in place of Usd or Eth). Any better proposal?

src/libraries/Types.sol

Rubilmax · 2022-12-19T06:51:35Z

src/libraries/Types.sol

+    struct UserMarkets {
+        bytes32 data;
+    }
+
+    struct BorrowMask {
+        bytes32 data;
+    }


Why wrap these inside structs?

I believe that the UserMarkets and BorrowMask should be treated as strongly typed, similar to what aave does. This should prevent any developer mistakes when working with these variables (including us)

Note that we might come back to the way we're handling it with Morpho-Compound (namely keeping the list of user's entered markets)

Rubilmax

Approving this to merge, but keeping conversations unresolved for future reference

pakim249CAL marked this pull request as ready for review December 17, 2022 16:50

pakim249CAL requested review from MerlinEgalite, Rubilmax and a team December 17, 2022 16:50

MerlinEgalite requested changes Dec 18, 2022

View reviewed changes

pakim249CAL force-pushed the feat/base-1 branch 4 times, most recently from dfe2958 to 64a999c Compare December 18, 2022 15:52

build: Build out initial contract structure and add internal functions.

47a2dbf

pakim249CAL force-pushed the feat/base-1 branch from 64a999c to 47a2dbf Compare December 18, 2022 15:53

chore: remove unused type

c3d1aff

MerlinEgalite approved these changes Dec 18, 2022

View reviewed changes

pakim249CAL requested a review from a team December 18, 2022 16:33

Rubilmax reviewed Dec 19, 2022

View reviewed changes

morpho-org deleted a comment from MerlinEgalite Dec 19, 2022

Rubilmax approved these changes Dec 19, 2022

View reviewed changes

refactor: split and reorg libs, rename some variables

6c8c802

pakim249CAL merged commit 138c8bd into feat/base Dec 19, 2022

Rubilmax deleted the feat/base-1 branch December 19, 2022 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/base 1 #12

Feat/base 1 #12

pakim249CAL commented Dec 17, 2022 •

edited

Loading

MerlinEgalite left a comment

MerlinEgalite Dec 18, 2022

pakim249CAL Dec 18, 2022

MerlinEgalite Dec 19, 2022

MerlinEgalite Dec 18, 2022

MerlinEgalite Dec 18, 2022

pakim249CAL Dec 18, 2022 •

edited

Loading

MerlinEgalite Dec 19, 2022

MerlinEgalite left a comment •

edited

Loading

Rubilmax left a comment

Rubilmax Dec 19, 2022

MerlinEgalite Dec 19, 2022

pakim249CAL Dec 19, 2022

Rubilmax Dec 19, 2022

Rubilmax Dec 19, 2022

Rubilmax Dec 19, 2022 •

edited

Loading

pakim249CAL Dec 19, 2022

Rubilmax Dec 19, 2022

Rubilmax Dec 19, 2022

Rubilmax Dec 19, 2022

MerlinEgalite Dec 19, 2022

Rubilmax Dec 19, 2022

Rubilmax Dec 19, 2022

pakim249CAL Dec 19, 2022 •

edited

Loading

MerlinEgalite Dec 19, 2022

Rubilmax left a comment

Feat/base 1 #12

Feat/base 1 #12

Conversation

pakim249CAL commented Dec 17, 2022 • edited Loading

MerlinEgalite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakim249CAL Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MerlinEgalite left a comment • edited Loading

Choose a reason for hiding this comment

Rubilmax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rubilmax Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakim249CAL Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rubilmax left a comment

Choose a reason for hiding this comment

pakim249CAL commented Dec 17, 2022 •

edited

Loading

pakim249CAL Dec 18, 2022 •

edited

Loading

MerlinEgalite left a comment •

edited

Loading

Rubilmax Dec 19, 2022 •

edited

Loading

pakim249CAL Dec 19, 2022 •

edited

Loading