feat(irm): skip address(0) #640
Conversation
|
it breaks certora though |
de60058 to
09b2dee
Compare
09b2dee to
b9fe01c
Compare
The base branch was changed.
|
I just reopened this discussion, please wait a little bit before merging |
|
Should we wait for your branch to be merged in it @MathisGD ? |
|
I think these 2 PRs are independent |
|
|
||
| emit EventsLib.AccrueInterest(id, borrowRate, interest, feeShares); | ||
| emit EventsLib.AccrueInterest(id, borrowRate, interest, feeShares); | ||
| } | ||
|
|
||
| // Safe "unchecked" cast. | ||
| market[id].lastUpdate = uint128(block.timestamp); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Do you mean something like if (elapsed == 0 || marketParams.irm == address(0)) return;?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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]); | |||
There was a problem hiding this comment.
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
loanTokenwithout 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This PR is not related to a cantina issue.
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