Conversation
jordanschalm
left a comment
There was a problem hiding this comment.
Are you sure about limiting positions to one collateral/debt type? It seems a big change from the current implementation, which is built around the idea that a position can contain multiple collateral/debt balances (eg. using a map for balances field, queuedDeposits).
If we do really want to make Position limited to one collateral/debt balance, I think there are more targeted ways to accomplish this:
- add a collateralType/debtType field to
Position - replace
balanceswithcollateralBalance,debtBalance - make
queuedDepositsjust a vault
cadence/contracts/FlowALPv0.cdc
Outdated
| // MOET cannot be seized as collateral (it should never exist as collateral) | ||
| if seizeType == Type<@MOET.Vault>() { | ||
| panic("Cannot seize MOET as collateral. MOET should not exist as collateral in any position.") | ||
| } |
There was a problem hiding this comment.
Are you sure it is the desired behaviour, that MOET cannot be used as collateral?
There was a problem hiding this comment.
@dete do we want to allow MOET as a collateral?
It creates a vulnerability for the protocol, since MOET is a fully synthetic token and not directly backed by any stable coin.
There was a problem hiding this comment.
Yes, MOET should be allowed as collateral. Of course, you can't borrow MOET against MOET, so we're not creating a "stack of turtles" here. If you try to "withdraw" MOET when you have a MOET balance, you just draw down your collateral.
Note that it's the lending protocol itself that makes MOET safe, so it's not unreasonable for the lending protocol to assume that its safe (so long as MOET isn't backed by MOET, which we will never allow).
Like other assets, there should be deposit interest on MOET collateral (which is always < the borrow interest on the same asset). Note that since all MOET is created by borrowing, and all borrowed MOET pays interest, the interest we pay on MOET deposits are always less than the MOET we earn by charging interest on debts. So, you can think of the interest we pay on MOET deposits as passing directly through the protocol from the original borrower to the current holder.
Critically: We don't allow USDC or PyUSD0 or any other stables to be deposited as collateral. So folks will have to "swap into" MOET if they want to have USD-denominated collateral. (Again, this has been modelled and is HEALTHY for the protocol, not risky.)
Thanks for being cautious, Alex! But this is part of the design and included in our stability analysis. 🙇
|
@jordanschalm this is a soft limitation until we figure out how to do rebalancing and position closing with multiple collaterals and debts. |
|
Hey @nialexsan, should this be targeting nialexsan/pre-refactor instead of |
zhangchiqing
left a comment
There was a problem hiding this comment.
LGTM, some minor suggestions
cadence/contracts/FlowALPv0.cdc
Outdated
| let positionBalance = position.getBalance(seizeType) | ||
| if positionBalance == nil { | ||
| // Liquidation is seizing collateral - validate single collateral type | ||
| position.validateCollateralType(seizeType) |
There was a problem hiding this comment.
I noticed the validation is always done before setBalance, does it make sense to simply add the validation to setBalance? That way, we don't need to add it everywhere.
There was a problem hiding this comment.
by Jordan's suggestion I moved the check to post condition
cadence/contracts/FlowALPv0.cdc
Outdated
| // Re-fetch balance to check if direction changed | ||
| positionBalance = position.getBalance(type) | ||
| if wasCredit && positionBalance!.direction == FlowALPModels.BalanceDirection.Debit { | ||
| position.validateDebtType(type) |
There was a problem hiding this comment.
could we validate before the recordWithdrawal? So that instead of returning the error message "Internal error, Position has multiple debt types", which is confusing, we could provide better error message, saying "Position already has debt type X. Cannot borrow Y"
if currentBalance.direction == Credit && currentBalance.wouldFlipToDebit(amount, tokenState) {
position.validateDebtType(type)
}
position.borrowBalance(type)!.recordWithdrawal(...)
There was a problem hiding this comment.
Follow-up for this validation-order path is in PR #200:
We now validate debt type before recordWithdrawal on credit->debit flip flows, so invalid attempts fail with the intended domain error instead of the internal invariant assert.
cadence/contracts/FlowALPv0.cdc
Outdated
| let seizeState = self._borrowUpdatedTokenState(type: seizeType) | ||
| if position.getBalance(seizeType) == nil { | ||
| let positionBalance = position.getBalance(seizeType) | ||
| if positionBalance == nil { |
There was a problem hiding this comment.
Same here: it doesn't make sense to seize collateral from a position which does not have a balance for that collateral type, so we should remove the nil check and let it panic.
cadence/contracts/FlowALPModels.cdc
Outdated
| /// If nil, the Pool will not pull underflown value, and liquidation may occur. | ||
| access(EImplementation) fun setTopUpSource(_ source: {DeFiActions.Source}?) | ||
|
|
||
| /// Returns the current collateral types for this position based on existing Credit balances |
There was a problem hiding this comment.
The current implementation injects checks in all places where we might add/remove a debt/collateral balance, and uses the validate* functions below to guard against adding more balances than we want to support.
I am wondering if we can use post-conditions and these getCollateralTypes functions to simplify this new logic and localize the temporary balance constraint to a smaller cross-section of the code. For example, if we added something like the following post-conditions to all functions which are able to modify position balances:
// we can localize the temporary constraint to one validation function, and document the reasoning behind it here
fun positionSatisfiesTemporaryBalanceConstraint(pid): Bool {
return getCollateralTypes(pid) <= 1 && getDebtTypes(pid) <= 1
}
// then, in functions which might violate the constraint, we can add a post-condition
post {
self.positionSatisfiesTemporaryBalanceConstraint(pid): "..."
}This way, we don't need to make any modifications to the business logic of affected functions, we just let them execute as normal and revert if their end state conflicts with the constraint.
cadence/contracts/FlowALPv0.cdc
Outdated
| self._queuePositionForUpdateIfNecessary(pid: pid) | ||
|
|
||
| let withdrawn <- reserveVault.withdraw(amount: amount) | ||
| // Get tokens either by minting (MOET) or from reserves (other tokens) |
There was a problem hiding this comment.
Dete suggested an abstraction to help simplify this different behaviour between MOET and other tokens in this thread. I think we should apply that, or a similar idea here to minimize the additional complexity we are adding.
This function was already ~150 LOC before this PR, which is beyond the threshold where a person can reasonably understand its behaviour, in my opinion. This is a serious technical risk for introducing and hiding security vulnerabilities. We should ideally be working on improving code quality over time, but at a minimum we need to stop making the problem worse. If you find yourself adding new functionality inline to a >100LOC function, I think that should be a strong signal that we can afford to spend the time to decompose the function into more manageable components as part of adding that functionality.
There was a problem hiding this comment.
added abstraction with interfaces for reserve handlers
cadence/contracts/FlowALPv0.cdc
Outdated
| assert(self.state.borrowReserve(type) != nil, message: "Cannot withdraw \(type.identifier) - no reserves available") | ||
| let reserveVault = self.state.borrowReserve(type)! | ||
| assert(reserveVault.balance >= amount, message: "Insufficient reserves for \(type.identifier): need \(amount), have \(reserveVault.balance)") |
There was a problem hiding this comment.
If we remove these two assert statements, won't withdraw panic anyway?
cadence/contracts/FlowALPv0.cdc
Outdated
| ) | ||
| let sinkVault <- FlowALPv0._borrowMOETMinter().mintTokens(amount: sinkAmount) | ||
|
|
||
| // Get tokens either by minting (MOET) or from reserves (other tokens) |
There was a problem hiding this comment.
This seems very similar to the logic on lines 1597-1608 - can we make a shared helper function to avoid duplicating big chunks of code inline?
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
| let positionBalance = position.getBalance(type) | ||
| // Determine if this is a repayment or collateral deposit | ||
| // based on the current balance state | ||
| let isRepayment = positionBalance != nil && positionBalance!.direction == FlowALPModels.BalanceDirection.Debit |
There was a problem hiding this comment.
Follow-up fix for this logic is available in #196 .
isRepayment is computed from pre-deposit state, so MOET over-repayments (deposit > current debt) route the full amount through depositRepayment and burn all of it, while accounting still records surplus as credit collateral. PR #196 splits the accepted deposit into repayment vs surplus collateral before reserve routing to keep reserves/accounting aligned.
There was a problem hiding this comment.
a post check should address this issue
cadence/contracts/FlowALPv0.cdc
Outdated
| if positionBalance == nil { | ||
| // Validate single collateral type constraint | ||
| position.validateCollateralType(type) | ||
|
|
There was a problem hiding this comment.
Follow-up for the collateral-type bypass case is in #197 .
When a deposit starts as repayment, deposit > current debt can flip surplus into Credit and create a second collateral type unless validated before recording. PR #197 adds that pre-check in _depositEffectsOnly and includes a regression test (testCannotCreateSecondCollateralTypeByOverRepayingDebt).
There was a problem hiding this comment.
a post check should address this issue
cadence/contracts/FlowALPModels.cdc
Outdated
| access(all) fun getCollateralTypes(): [Type] { | ||
| let types: [Type] = [] | ||
| for type in self.balances.keys { | ||
| if self.balances[type]!.direction == BalanceDirection.Credit { |
There was a problem hiding this comment.
Follow-up fix: this type discovery now needs to ignore zero balances. Otherwise exact repay/full withdraw can leave a phantom type that still trips validateDebtType/validateCollateralType. Addressed in PR #199
Clear phantom debt/collateral types after exact zeroing
|
don't need this any more, after QnA |
|
@jordanschalm I'll clean up the PR and leave reserve handing without all the single token restrictions |
Thanks -- also take a look at this thread if you haven't seen it: https://flow-foundation.slack.com/archives/C08QF29F7TK/p1772730444226649. We can simplify that reserve interface to two methods. |
Closes: #???
Description
support borrowing tokens from the protocol reserves, not just minting MOET
temporary limitations:
this limitation is required to simplify rebalancing, closing position logic, and liquidations, otherwise the rebalancer would need to decide which tokens to move first.
There are multiple ways to implement this logic:
Which debt to repay first when deleveraging?
Which collateral to sell first to fund repay?
For contributor use:
masterbranchFiles changedin the Github PR explorer