feat(irm): skip address(0) #640

Rubilmax · 2023-12-07T13:28:57Z

This PR is not related to a cantina issue.

Fixes IRM zero #631

it takes 85k gas to create an idle market while it takes 175k gas to create a standard market

and here is the gas diff between standard markets (left) and idle markets (right):

on avg, it costs -9k to supply/withdraw to/from an idle market

src/Morpho.sol

MerlinEgalite

Should be tested i think

MerlinEgalite · 2023-12-07T16:50:39Z

it breaks certora though

Rubilmax

Do it in the external library too

src/Morpho.sol

The base branch was changed.

…skip-irm-0

MerlinEgalite

Well not sure now we should do it

MathisGD · 2023-12-11T14:02:02Z

I just reopened this discussion, please wait a little bit before merging

…skip-irm-0

src/libraries/periphery/MorphoBalancesLib.sol

MerlinEgalite · 2023-12-12T14:11:56Z

Should we wait for your branch to be merged in it @MathisGD ?

Rubilmax · 2023-12-12T14:58:38Z

I think these 2 PRs are independent

StErMi · 2023-12-21T06:15:07Z

src/Morpho.sol


-        emit EventsLib.AccrueInterest(id, borrowRate, interest, feeShares);
+            emit EventsLib.AccrueInterest(id, borrowRate, interest, feeShares);
+        }

        // Safe "unchecked" cast.
        market[id].lastUpdate = uint128(block.timestamp);


What's the "meaning" of lastUpdate? I'm asking because there's no natspec documentation on the interface, and previously it was updated when there was an interest accrual.

So if I need to give a meaning from the previous code logic, lastUpdate represented the last time that interest was accrued or at least triggered (maybe there was no accrual because of market condition or rounding)

With this change instead, lastUpdate is always updated even if no interest accrual is triggered, and we know for sure that the market has not changed.

Should the market[id].lastUpdate = uint128(block.timestamp); execution be moved inside the if branch?

I think now it refers to the timestamp of the block where there was the last interaction that could change the interest. So mostly the main lending entry points except supplyCollateral. I agree that it does not really make the spec simpler in the end, so maybe it's better to use the previous solution of skipping the all function _accrueInterest as soon as the irm is 0

Do you mean something like if (elapsed == 0 || marketParams.irm == address(0)) return;?

We thought about returning even earlier, before computing elapsed, so that we would not have to touch the fee/lastUpdate slot. But this is not such a big optimization actually because this slot is warm anyway (when checking if the market is created). So we thought about your suggestion also.

In the end, because optimizations are not so big, we went for the solution where markets with 0 irm function similarly to regular markets

I think that lastUpdate should always be updated (current situation) because it makes the spec simpler (it's always updated when accrueInterest is called). Also I was in favor (but not big deal) of always emitting the event for the same reason. For me when irm=0 we can say that the interest is accrued, but with rate=0.

StErMi · 2023-12-21T06:20:06Z

src/Morpho.sol

@@ -160,7 +160,7 @@ contract Morpho is IMorphoStaticTyping {
        emit EventsLib.CreateMarket(id, marketParams);

        // Call to initialize the IRM in case it is stateful.
-        IIrm(marketParams.irm).borrowRate(marketParams, market[id]);
+        if (marketParams.irm != address(0)) IIrm(marketParams.irm).borrowRate(marketParams, market[id]);


From my understanding, this PR is to allow the creation of "idle" markets where there's no IRM. Is my understanding correct?

This allows anyone to create a market where no interest will accrue.

borrowers do not need to pay any interest to suppliers

suppliers do not accrue and earn any earning from borrowers

bad debt socialization can't be "neutralized" by interest accrued in the past

Isn't there any better solution to allow the creation of "idle" markets? Because with this modification and the fact that you are not checking during market creation if the loanToken and collateralToken exists (!= address(0) or address(token).code.length > 0) it could lead to some unwanted behaviors.

Given that you are shaping MB to allow the MM use case (idle markets), I would suggest thinking about:

find the conditions that only allow the creation of an idle market (supply loanToken without allowing borrowing, only withdraw)

does not allow the creation of arbitrary markets that do no make sense for a landing platform

document inside the code that those checks and those markets should be used only for idle liquidity purposes

As I mentioned previously, IMHO, Morpho blue should not be designed and shaped for the needs of externals actors like MetaMorpho. MetaMorpho should be shaped around the basic building block that MB should be.

Is my understanding correct?

Correct

As I mentioned previously, IMHO, Morpho blue should not be designed and shaped for the needs of externals actors like MetaMorpho. MetaMorpho should be shaped around the basic building block that MB should be.

Indeed, it's 100% aligned with our general philosophy but we think it makes more sense from a product perspective and we won't change it.

This allows anyone to create a market where no interest will accrue.

borrowers do not need to pay any interest to suppliers
suppliers do not accrue and earn any earning from borrowers
bad debt socialization can't be "neutralized" by interest accrued in the past

correct, but this is not the expected use-case: for an idle market, one would put address zero as collateral and no borrower would be able to deposit collateral (and so borrow). It's only a way to make these idle depositors pay less gas.

I'm confused because this change has been done specifically to allow the creation of "idle market" but it also allows the creation of unwanted (as far as I get) markets.

Why don't you narrow the creation of those market to just the one that makes sense to be used as an idle market (as you said)?

agree that it feels weird, but in Blue you can also created unwanted markets in multiple ways (tokens are not checked, oracles are not checked) so why checking for this thing specifically

agree that it feels weird, but in Blue you can also created unwanted markets in multiple ways (tokens are not checked, oracles are not checked) so why checking for this thing specifically

Because this modification has been made deliberately to allow the creation of an idle market, with the side effects that it allows the creation of non-lending markets.

The final decision is obviously up to you, I just wanted to point out the side effects and that at least those side effects need to be documented.

Rubilmax requested review from adhusson, QGarchery, MerlinEgalite, MathisGD and Jean-Grimal December 7, 2023 13:28

Rubilmax marked this pull request as ready for review December 7, 2023 13:29

Rubilmax linked an issue Dec 7, 2023 that may be closed by this pull request

IRM zero #631

Closed

QGarchery reviewed Dec 7, 2023

View reviewed changes

src/Morpho.sol Outdated Show resolved Hide resolved

Rubilmax requested a review from QGarchery December 7, 2023 14:39

QGarchery previously approved these changes Dec 7, 2023

View reviewed changes

Jean-Grimal approved these changes Dec 7, 2023

View reviewed changes

MerlinEgalite requested changes Dec 7, 2023

View reviewed changes

Rubilmax requested a review from MerlinEgalite December 7, 2023 16:31

MerlinEgalite approved these changes Dec 7, 2023

View reviewed changes

Rubilmax commented Dec 8, 2023

View reviewed changes

MerlinEgalite previously approved these changes Dec 8, 2023

View reviewed changes

MathisGD requested changes Dec 10, 2023

View reviewed changes

src/Morpho.sol Outdated Show resolved Hide resolved

MathisGD reviewed Dec 10, 2023

View reviewed changes

src/Morpho.sol Outdated Show resolved Hide resolved

Rubilmax force-pushed the feat/skip-irm-0 branch from de60058 to 09b2dee Compare December 11, 2023 08:56

Rubilmax changed the base branch from post-cantina to fix/issue-259 December 11, 2023 08:58

Rubilmax added 5 commits December 11, 2023 09:59

feat(irm): skip address(0)

1176e03

fix(irm): skip before elapsed

fc91741

test(irm): add irm zero test

0e0ebdb

fix(lib): skip irm in external lib

598c43e

fix(accrue-interest): skip address(0)

b9fe01c

Rubilmax force-pushed the feat/skip-irm-0 branch from 09b2dee to b9fe01c Compare December 11, 2023 08:59

Rubilmax requested review from MathisGD, QGarchery and MerlinEgalite December 11, 2023 09:00

Base automatically changed from fix/issue-259 to post-cantina December 11, 2023 09:49

Rubilmax requested a review from Jean-Grimal December 11, 2023 10:03

Jean-Grimal approved these changes Dec 11, 2023

View reviewed changes

Merge branch 'post-cantina' of github.com:morpho-labs/blue into feat/…

87d973a

…skip-irm-0

MathisGD assigned Rubilmax Dec 11, 2023

Rubilmax added 2 commits December 11, 2023 11:45

test(hardhat): add idle market tests

c1ed84b

test(forge): fix create market tests

936971d

MerlinEgalite approved these changes Dec 11, 2023

View reviewed changes

MerlinEgalite reviewed Dec 11, 2023

View reviewed changes

Rubilmax added 2 commits December 12, 2023 10:31

Merge branch 'post-cantina' of github.com:morpho-labs/blue into feat/…

d960aa8

…skip-irm-0

fix(accrue-interest): update lastUpdate

a7d3ffd

MathisGD reviewed Dec 12, 2023

View reviewed changes

src/libraries/periphery/MorphoBalancesLib.sol Outdated Show resolved Hide resolved

docs(balances-lib): update comment

5ae9ff2

Rubilmax requested a review from MathisGD December 12, 2023 10:41

MathisGD approved these changes Dec 12, 2023

View reviewed changes

MathisGD requested review from MerlinEgalite and Jean-Grimal December 12, 2023 11:30

MerlinEgalite approved these changes Dec 12, 2023

View reviewed changes

Jean-Grimal approved these changes Dec 12, 2023

View reviewed changes

MerlinEgalite merged commit 184fc24 into post-cantina Dec 12, 2023
15 checks passed

MerlinEgalite deleted the feat/skip-irm-0 branch December 12, 2023 15:11

MathisGD mentioned this pull request Dec 12, 2023

post-cantina #633

Merged

StErMi reviewed Dec 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(irm): skip address(0) #640

feat(irm): skip address(0) #640

Rubilmax commented Dec 7, 2023 •

edited by MerlinEgalite

Loading

MerlinEgalite left a comment

MerlinEgalite commented Dec 7, 2023

Rubilmax left a comment

MerlinEgalite left a comment

MathisGD commented Dec 11, 2023

MerlinEgalite commented Dec 12, 2023

Rubilmax commented Dec 12, 2023

StErMi Dec 21, 2023

QGarchery Dec 21, 2023

StErMi Dec 21, 2023

QGarchery Dec 22, 2023

MathisGD Dec 23, 2023

StErMi Dec 21, 2023 •

edited

Loading

MerlinEgalite Dec 22, 2023

MathisGD Dec 23, 2023

StErMi Dec 23, 2023

MathisGD Dec 23, 2023

StErMi Dec 23, 2023

feat(irm): skip address(0) #640

feat(irm): skip address(0) #640

Conversation

Rubilmax commented Dec 7, 2023 • edited by MerlinEgalite Loading

MerlinEgalite left a comment

Choose a reason for hiding this comment

MerlinEgalite commented Dec 7, 2023

Rubilmax left a comment

Choose a reason for hiding this comment

MerlinEgalite left a comment

Choose a reason for hiding this comment

MathisGD commented Dec 11, 2023

MerlinEgalite commented Dec 12, 2023

Rubilmax commented Dec 12, 2023

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

StErMi Dec 21, 2023 • 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

Rubilmax commented Dec 7, 2023 •

edited by MerlinEgalite

Loading

StErMi Dec 21, 2023 •

edited

Loading