refactor: use early return in _accrueInterests

[julien-devatom]

Pull request

Use early return instead of wrapping multiple lines of code with a if block.

This eases the code reading

⚠️ PR Update

The totalBorrow[id] != 0 is not useful. All the logic is ok since you're adding 0 interests.
The only consideration of letting all the logic wrapped is in case of a non-borrowed market, you'll Emit an AccrueInterest event with 0 interest generated, which is an edge case in fact.

So I suggest just to add 0 if the market is not borrowed

[Rubilmax]

This eases the code reading

Lol have you talked with @MathisGD?

[QGarchery]

I'm ok with either, with a slight preference to applying the changes of this PR.

[MathisGD]

This early return (but same for the previous if):

• saves a lot of gas when you _accrueInterest and the totalBorrow is zero
• cost a little bit of gas to any other interaction (not sure how much)

Assuming that a market very rarely is at zero total borrow it might not be worth from a total gas spent pov. Wdyt ?

[Rubilmax]

If we assume this check costs 1 warm SLOAD + 1 equality check + some stack management, it should cost 150 gas max
Whereas it would save a cold CALL to the IRM when totalBorrow is zero (which should only be the case when a market is just created)

Because of the order of magnitude of the gas cost and how often a market has no borrow, it's not much of a matter to me but I prefer having it to decrease the gas cost of suppliers at the beginning of a market (early returns or if conditions are dangerous to me when a software evolves a lot ; which is not the case of Blue thanks to your fight for minimalism)

[MerlinEgalite]

Maybe we can roll back and store totalBorrow[id] on the stack before the early return?

[Rubilmax]

This early return (but same for the previous if):

• saves a lot of gas when you _accrueInterest and the totalBorrow is zero
• cost a little bit of gas to any other interaction (not sure how much)

Assuming that a market very rarely is at zero total borrow it might not be worth from a total gas spent pov. Wdyt ?

Actually I just realized that removing the condition now makes Blue emit AccrueInterests event with 0 interests. It may be ok though

[makcandrov]

I don't understand why the early return is removed if totalBorrow is null. It saves a lot of gas when there is no borrow (it avoids the call to the IRM).

Moreover, if this check isn't done in Blue, it will need to be done in the IRM, because the IRM usually divides by totalSupply (and we have the invariant totalSupply >= totalBorrow).

https://github.com/morpho-labs/morpho-blue/blob/827096eac6748eec73ae3ad86ffbed3d139fdc8a/src/mocks/IrmMock.sol#L22

Therefore, this change doesn't save any gas in the general case.

[Rubilmax]

Moreover, if this check isn't done in Blue, it will need to be done in the IRM, because the IRM usually divides by totalSupply (and we have the invariant totalSupply >= totalBorrow).

I support your point but I don't get this. Dividing by totalSupply is not an issue when totalBorrow == 0?

[makcandrov]

totalBorrow > 0 => totalSupply > 0 thus we can safely divide by totalSupply in the IRM if (and not only if) totalBorrow > 0

[Rubilmax]

totalBorrow > 0 => totalSupply > 0 thus we can safely divide by totalSupply in the IRM if (and not only if) totalBorrow > 0

Indeed, but the IRM cannot assume it'll be queried only when totalSupply > 0 (e.g. offchain staticcalls) so this check will be performed anyway to prevent reverts, right?

[makcandrov]

so this check will be performed anyway to prevent reverts, right?

I guess it depends on the point of view. Both are ok for me.

Currently, that's not the case with our mock IRM (which is why the hardhat tests fail btw). But anyway, I think we should skip the IRM call if there is no borrow.

[MathisGD]

Agree with Maxime in the end, let's close this