morpho-org / morpho-optimizers

Core contracts of Morpho V1 Optimizers.
https://app.morpho.org
GNU Affero General Public License v3.0
137 stars 22 forks source link

refactor: remove supply and borrow guard #1629

Closed pakim249CAL closed 1 year ago

pakim249CAL commented 1 year ago

Pull Request

Issue(s) fixed

This pull request fixes #1628

MathisGD commented 1 year ago

Oh but no we need it because the asset could not be borrowable on Aave

Just saw that, sorry

MathisGD commented 1 year ago

I don't understand why you closed it ?

Here you explained me why we should remove them.

MerlinEgalite commented 1 year ago

I don't understand why you closed it ?

Here you explained me why we should remove them.

Yes but:

I think that the getBorrowingEnabled was there before, and is actually very important ! (stETH)

Rubilmax commented 1 year ago

From my understanding:

Note that aave governance can setBorrowingEnabled anytime So we keep isFrozen check because the save governance have the exact same power over it and the impact would be similar

But then we should also consider behaving the same way on Morpho-Compound??

I genuinely don't understand our reasoning as a team here

MerlinEgalite commented 1 year ago

From my understanding:

  • getBorrowingEnabled is necessary because otherwise users could borrow from Morpho & borrow would revert on the pool but users could borrow 100% peer-to-peer and lenders wouldn't be able to withdraw (because pool would revert)

Note that aave governance can setBorrowingEnabled anytime So we keep isFrozen check because the save governance have the exact same power over it and the impact would be similar

But then we should also consider behaving the same way on Morpho-Compound??

I genuinely don't understand our reasoning as a team here

You're suggesting to do the borrow check on Morpho-Compound, correct?

MathisGD commented 1 year ago

I think that the getBorrowingEnabled was there before, and is actually very important ! (stETH)

I'm talking about the isFrozen check. It seems (https://github.com/morpho-dao/morpho-v1/issues/1628) that a decisions has been made to remove those checks, and now we say we don't remove them (and even add the to Morpho-Compound). Both solutions are ok to me, but I don't understand our logic here.

MerlinEgalite commented 1 year ago

I think that the getBorrowingEnabled was there before, and is actually very important ! (stETH)

I'm talking about the isFrozen check. It seems (#1628) that a decisions has been made to remove those checks, and now we say we don't remove them (and even add the to Morpho-Compound). Both solutions are ok to me, but I don't understand our logic here.

Ok I think I asked to close the PR but in fact we should re-open it but only remove the frozen flags and keep the borrowing enabled flag because it's necessary

Rubilmax commented 1 year ago

Ok I think I asked to close the PR but in fact we should re-open it but only remove the frozen flags and keep the borrowing enabled flag because it's necessary

Agreed that it is the logical conclusion we should have taken

Though I think we're doing something weird here as we are not behaving the same way whether the aave governance decides to setBorrowingEnabled or setFrozen, which have the same impact to me: we would allow people to borrow peer-to-peer and make lenders funds illiquid on Morpho

I know we have the same debate over & over but this is a new argument coming up: we're removing isFrozen but not isBorrowingEnabled

MerlinEgalite commented 1 year ago

Ok I think I asked to close the PR but in fact we should re-open it but only remove the frozen flags and keep the borrowing enabled flag because it's necessary

Agreed that it is the logical conclusion we should have taken

Though I think we're doing something weird here as we are not behaving the same way whether the aave governance decides to setBorrowingEnabled or setFrozen, which have the same impact to me: we would allow people to borrow peer-to-peer and make lenders funds illiquid on Morpho

I know we have the same debate over & over but this is a new argument coming up: we're removing isFrozen but not isBorrowingEnabled

isBorrowingEnabled is generally set at the asset listing event, so I think the behavior is a bit different between those 2 flags