-
Notifications
You must be signed in to change notification settings - Fork 2
single debt and collateral per position #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
f8e0c64
921efff
cfbcd1a
7b1a144
c492f20
a7f0828
4c9e452
a916839
aa091eb
2b1ed97
7187d1e
1a6b4ec
093d823
e3dcb84
8153647
368dbaa
a414472
385387f
3641469
ebffab0
5d9b84c
6e95bab
af6b9d7
5058488
9fbd813
b65f0ff
c0713a7
416a9d2
23603b4
ee601c1
08fd6fd
8d48045
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -576,15 +576,31 @@ access(all) contract FlowALPv0 { | |
| let debtState = self._borrowUpdatedTokenState(type: debtType) | ||
|
|
||
| if position.getBalance(debtType) == nil { | ||
| // Liquidation is repaying debt - validate single debt type | ||
| position.validateDebtType(debtType) | ||
|
|
||
| position.setBalance(debtType, FlowALPModels.InternalBalance(direction: FlowALPModels.BalanceDirection.Debit, scaledBalance: 0.0)) | ||
| } | ||
|
nialexsan marked this conversation as resolved.
Outdated
|
||
| position.borrowBalance(debtType)!.recordDeposit(amount: UFix128(repayAmount), tokenState: debtState) | ||
|
|
||
| // Withdraw seized collateral from position and send to liquidator | ||
| let seizeState = self._borrowUpdatedTokenState(type: seizeType) | ||
| if position.getBalance(seizeType) == nil { | ||
| // 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.") | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure it is the desired behaviour, that MOET cannot be used as collateral?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dete do we want to allow MOET as a collateral?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 🙇 |
||
|
|
||
| // Liquidation is seizing collateral - validate single collateral type | ||
| position.validateCollateralType(seizeType) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by Jordan's suggestion I moved the check to post condition |
||
|
|
||
| position.setBalance(seizeType, FlowALPModels.InternalBalance(direction: FlowALPModels.BalanceDirection.Credit, scaledBalance: 0.0)) | ||
| } | ||
| // Additional safety check: MOET should never be seized as collateral | ||
| if seizeType == Type<@MOET.Vault>() && position.balances[seizeType]!.direction == BalanceDirection.Credit { | ||
| panic("Cannot seize MOET as collateral. MOET should not exist as collateral in any position.") | ||
| } | ||
|
|
||
| position.borrowBalance(seizeType)!.recordWithdrawal(amount: UFix128(seizeAmount), tokenState: seizeState) | ||
| let seizeReserveRef = self.state.borrowReserve(seizeType)! | ||
| let seizedCollateral <- seizeReserveRef.withdraw(amount: seizeAmount) | ||
|
|
@@ -1323,7 +1339,15 @@ access(all) contract FlowALPv0 { | |
| } | ||
|
|
||
| // If this position doesn't currently have an entry for this token, create one. | ||
| if position.getBalance(type) == nil { | ||
| if position.balances[type] == nil { | ||
|
nialexsan marked this conversation as resolved.
Outdated
|
||
| // MOET cannot be used as collateral (only as debt) | ||
| if type == Type<@MOET.Vault>() { | ||
| panic("MOET cannot be deposited as collateral. MOET can only be borrowed (debt), not used as collateral.") | ||
| } | ||
|
|
||
| // Validate single collateral type constraint | ||
| position.validateCollateralType(type) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up for the collateral-type bypass case is in #197 .
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a post check should address this issue |
||
| position.setBalance(type, FlowALPModels.InternalBalance( | ||
| direction: FlowALPModels.BalanceDirection.Credit, | ||
| scaledBalance: 0.0 | ||
|
|
@@ -1341,6 +1365,13 @@ access(all) contract FlowALPv0 { | |
| // This only records the portion of the deposit that was accepted, not any queued portions, | ||
| // as the queued deposits will be processed later (by this function being called again), and therefore | ||
| // will be recorded at that time. | ||
|
|
||
| // MOET cannot be deposited as collateral (Credit direction) | ||
| // It can only be deposited to repay debt (Debit direction) | ||
| if type == Type<@MOET.Vault>() && position.balances[type]!.direction == BalanceDirection.Credit { | ||
| panic("MOET cannot be deposited as collateral. MOET can only be borrowed (debt), not used as collateral.") | ||
| } | ||
|
|
||
| let acceptedAmount = from.balance | ||
| position.borrowBalance(type)!.recordDeposit( | ||
| amount: UFix128(acceptedAmount), | ||
|
|
@@ -1527,20 +1558,28 @@ access(all) contract FlowALPv0 { | |
|
|
||
| // If this position doesn't currently have an entry for this token, create one. | ||
| if position.getBalance(type) == nil { | ||
| // When withdrawing a token that doesn't exist in position yet, | ||
| // we'll be creating a debit (debt). Validate single debt type. | ||
| position.validateDebtType(type) | ||
|
|
||
| position.setBalance(type, FlowALPModels.InternalBalance( | ||
| direction: FlowALPModels.BalanceDirection.Credit, | ||
| scaledBalance: 0.0 | ||
| )) | ||
| } | ||
|
|
||
| let reserveVault = self.state.borrowReserve(type)! | ||
|
|
||
| // Reflect the withdrawal in the position's balance | ||
| let wasCredit = position.balances[type]!.direction == BalanceDirection.Credit | ||
| let uintAmount = UFix128(amount) | ||
| position.borrowBalance(type)!.recordWithdrawal( | ||
| amount: uintAmount, | ||
| tokenState: tokenState | ||
| ) | ||
|
|
||
| // If we flipped from Credit to Debit, validate debt type constraint | ||
| if wasCredit && position.balances[type]!.direction == BalanceDirection.Debit { | ||
| position.validateDebtType(type) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we validate before the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up for this validation-order path is in PR #200: |
||
| } | ||
| // Attempt to pull additional collateral from the top-up source (if configured) | ||
| // to keep the position above minHealth after the withdrawal. | ||
| // Regardless of whether a top-up occurs, the position must be healthy post-withdrawal. | ||
|
|
@@ -1565,18 +1604,30 @@ access(all) contract FlowALPv0 { | |
| // Queue for update if necessary | ||
| self._queuePositionForUpdateIfNecessary(pid: pid) | ||
|
|
||
| let withdrawn <- reserveVault.withdraw(amount: amount) | ||
| // Get tokens either by minting (MOET) or from reserves (other tokens) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added abstraction with interfaces for reserve handlers |
||
| var withdrawn: @{FungibleToken.Vault}? <- nil | ||
| if type == Type<@MOET.Vault>() { | ||
| // For MOET, mint new tokens | ||
| withdrawn <-! FlowALPv0._borrowMOETMinter().mintTokens(amount: amount) | ||
| } else { | ||
| // For other tokens, withdraw from reserves | ||
| assert(self.reserves[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)") | ||
| withdrawn <-! reserveVault.withdraw(amount: amount) | ||
| } | ||
| let unwrappedWithdrawn <- withdrawn! | ||
|
|
||
| FlowALPEvents.emitWithdrawn( | ||
| pid: pid, | ||
| poolUUID: self.uuid, | ||
| vaultType: type, | ||
| amount: withdrawn.balance, | ||
| withdrawnUUID: withdrawn.uuid | ||
| amount: unwrappedWithdrawn.balance, | ||
| withdrawnUUID: unwrappedWithdrawn.uuid | ||
| ) | ||
|
|
||
| self.unlockPosition(pid) | ||
| return <- withdrawn | ||
| return <- unwrappedWithdrawn | ||
| } | ||
|
|
||
| /////////////////////// | ||
|
|
@@ -1951,40 +2002,56 @@ access(all) contract FlowALPv0 { | |
| let sinkCapacity = drawDownSink.minimumCapacity() | ||
| let sinkAmount = (idealWithdrawal > sinkCapacity) ? sinkCapacity : idealWithdrawal | ||
|
|
||
| // TODO(jord): we enforce in setDrawDownSink that the type is MOET -> we should panic here if that does not hold (currently silently fail) | ||
| if sinkAmount > 0.0 && sinkType == Type<@MOET.Vault>() { | ||
| let tokenState = self._borrowUpdatedTokenState(type: Type<@MOET.Vault>()) | ||
| if position.getBalance(Type<@MOET.Vault>()) == nil { | ||
| position.setBalance(Type<@MOET.Vault>(), FlowALPModels.InternalBalance( | ||
| // Support multiple token types: MOET (minted) or other tokens (from reserves) | ||
| if sinkAmount > 0.0 { | ||
| let tokenState = self._borrowUpdatedTokenState(type: sinkType) | ||
| if position.getBalance(sinkType) == nil { | ||
| // Rebalancing is borrowing/withdrawing to push to sink - validate single debt type | ||
| position.validateDebtType(sinkType) | ||
|
|
||
| position.setBalance(sinkType, FlowALPModels.InternalBalance( | ||
| direction: FlowALPModels.BalanceDirection.Credit, | ||
| scaledBalance: 0.0 | ||
| )) | ||
| } | ||
| // record the withdrawal and mint the tokens | ||
| // Record the withdrawal | ||
| let uintSinkAmount = UFix128(sinkAmount) | ||
| position.borrowBalance(Type<@MOET.Vault>())!.recordWithdrawal( | ||
| position.borrowBalance(sinkType)!.recordWithdrawal( | ||
| amount: uintSinkAmount, | ||
| tokenState: tokenState | ||
| ) | ||
| let sinkVault <- FlowALPv0._borrowMOETMinter().mintTokens(amount: sinkAmount) | ||
|
|
||
| // Get tokens either by minting (MOET) or from reserves (other tokens) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| var sinkVault: @{FungibleToken.Vault}? <- nil | ||
| if sinkType == Type<@MOET.Vault>() { | ||
| // For MOET, mint new tokens | ||
| sinkVault <-! FlowALPv0._borrowMOETMinter().mintTokens(amount: sinkAmount) | ||
| } else { | ||
| // For other tokens, withdraw from reserves | ||
| assert(self.reserves[sinkType] != nil, message: "Cannot withdraw \(sinkAmount) of \(sinkType.identifier) - token not in reserves") | ||
| let reserveVault = (&self.reserves[sinkType] as auth(FungibleToken.Withdraw) &{FungibleToken.Vault}?)! | ||
| assert(reserveVault.balance >= sinkAmount, message: "Insufficient reserves for \(sinkType.identifier): available \(reserveVault.balance), needed \(sinkAmount)") | ||
| sinkVault <-! reserveVault.withdraw(amount: sinkAmount) | ||
| } | ||
| let unwrappedSinkVault <- sinkVault! | ||
|
|
||
| FlowALPEvents.emitRebalanced( | ||
| pid: pid, | ||
| poolUUID: self.uuid, | ||
| atHealth: balanceSheet.health, | ||
| amount: sinkVault.balance, | ||
| amount: unwrappedSinkVault.balance, | ||
| fromUnder: false | ||
| ) | ||
|
|
||
| // Push what we can into the sink, and redeposit the rest | ||
| drawDownSink.depositCapacity(from: &sinkVault as auth(FungibleToken.Withdraw) &{FungibleToken.Vault}) | ||
| if sinkVault.balance > 0.0 { | ||
| drawDownSink.depositCapacity(from: &unwrappedSinkVault as auth(FungibleToken.Withdraw) &{FungibleToken.Vault}) | ||
| if unwrappedSinkVault.balance > 0.0 { | ||
| self._depositEffectsOnly( | ||
| pid: pid, | ||
| from: <-sinkVault, | ||
| from: <-unwrappedSinkVault, | ||
| ) | ||
| } else { | ||
| Burner.burn(<-sinkVault) | ||
| Burner.burn(<-unwrappedSinkVault) | ||
| } | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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