-
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 12 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 |
|---|---|---|
|
|
@@ -1903,6 +1903,22 @@ access(all) contract FlowALPModels { | |
| /// Sets the top-up source. See borrowTopUpSource for additional details. | ||
| /// 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 | ||
| /// Returns empty array if no Credit balance exists yet (allows any collateral type for first deposit) | ||
| access(all) fun getCollateralTypes(): [Type] | ||
|
|
||
| /// Returns the current debt types for this position based on existing Debit balances | ||
| /// Returns empty array if no Debit balance exists yet (allows any debt type for first borrow) | ||
| access(all) fun getDebtTypes(): [Type] | ||
|
|
||
| /// Validates that the given token type can be used as collateral for this position | ||
| /// Panics if position already has a different collateral type | ||
| access(EImplementation) fun validateCollateralType(_ type: Type) | ||
|
|
||
| /// Validates that the given token type can be used as debt for this position | ||
| /// Panics if position already has a different debt type | ||
| access(EImplementation) fun validateDebtType(_ type: Type) | ||
| } | ||
|
|
||
| /// InternalPositionImplv1 is the concrete implementation of InternalPosition. | ||
|
|
@@ -2071,6 +2087,78 @@ access(all) contract FlowALPModels { | |
| access(EImplementation) fun setTopUpSource(_ source: {DeFiActions.Source}?) { | ||
| self.topUpSource = source | ||
| } | ||
|
|
||
| /// Returns the current collateral types for this position based on existing Credit balances | ||
| /// Returns empty array if no Credit balance exists yet (allows any collateral type for first deposit) | ||
| access(all) fun getCollateralTypes(): [Type] { | ||
| let types: [Type] = [] | ||
| for type in self.balances.keys { | ||
| if self.balances[type]!.direction == BalanceDirection.Credit { | ||
|
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 fix: this type discovery now needs to ignore zero balances. Otherwise exact repay/full withdraw can leave a phantom type that still trips |
||
| types.append(type) | ||
| } | ||
| } | ||
| return types | ||
| } | ||
|
|
||
| /// Returns the current debt types for this position based on existing Debit balances | ||
| /// Returns empty array if no Debit balance exists yet (allows any debt type for first borrow) | ||
| access(all) fun getDebtTypes(): [Type] { | ||
| let types: [Type] = [] | ||
| for type in self.balances.keys { | ||
| if self.balances[type]!.direction == BalanceDirection.Debit { | ||
| types.append(type) | ||
| } | ||
| } | ||
| return types | ||
| } | ||
|
|
||
| /// Validates that the given token type can be used as collateral for this position | ||
| /// Panics if position already has a different collateral type | ||
| access(EImplementation) fun validateCollateralType(_ type: Type) { | ||
| let existingTypes = self.getCollateralTypes() | ||
|
|
||
| // Constraint: For now, only one collateral type is allowed per position | ||
| // This assertion ensures the invariant is maintained | ||
| assert(existingTypes.length <= 1, message: "Internal error: Position has multiple collateral types") | ||
|
|
||
| if existingTypes.length == 0 { | ||
| // No collateral yet, allow any type | ||
| return | ||
| } | ||
|
|
||
| // Check if type already exists (idempotent) | ||
| if existingTypes.contains(type) { | ||
| return | ||
| } | ||
|
|
||
| // For now, only one collateral type is allowed per position | ||
| // This restriction can be removed in the future to support multiple collateral types | ||
| panic("Position already has collateral type ".concat(existingTypes[0].identifier).concat(". Cannot deposit ").concat(type.identifier).concat(". Only one collateral type allowed per position.")) | ||
| } | ||
|
|
||
| /// Validates that the given token type can be used as debt for this position | ||
| /// Panics if position already has a different debt type | ||
| access(EImplementation) fun validateDebtType(_ type: Type) { | ||
| let existingTypes = self.getDebtTypes() | ||
|
|
||
| // Constraint: For now, only one debt type is allowed per position | ||
| // This assertion ensures the invariant is maintained | ||
| assert(existingTypes.length <= 1, message: "Internal error: Position has multiple debt types") | ||
|
|
||
| if existingTypes.length == 0 { | ||
| // No debt yet, allow any type | ||
| return | ||
| } | ||
|
|
||
| // Check if type already exists (idempotent) | ||
| if existingTypes.contains(type) { | ||
| return | ||
| } | ||
|
|
||
| // For now, only one debt type is allowed per position | ||
| // This restriction can be removed in the future to support multiple debt types | ||
| panic("Position already has debt type ".concat(existingTypes[0].identifier).concat(". Cannot borrow ").concat(type.identifier).concat(". Only one debt type allowed per position.")) | ||
| } | ||
| } | ||
|
|
||
| /// Factory function to create a new InternalPositionImplv1 resource. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -576,15 +576,23 @@ 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 { | ||
| let positionBalance = position.getBalance(seizeType) | ||
| if positionBalance == nil { | ||
|
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. 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. |
||
| // 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)) | ||
| } | ||
|
|
||
| position.borrowBalance(seizeType)!.recordWithdrawal(amount: UFix128(seizeAmount), tokenState: seizeState) | ||
| let seizeReserveRef = self.state.borrowReserve(seizeType)! | ||
| let seizedCollateral <- seizeReserveRef.withdraw(amount: seizeAmount) | ||
|
|
@@ -1322,8 +1330,12 @@ access(all) contract FlowALPv0 { | |
| position.depositToQueue(type, vault: <-queuedForUserLimit) | ||
| } | ||
|
|
||
| let positionBalance = position.getBalance(type) | ||
| // If this position doesn't currently have an entry for this token, create one. | ||
| if position.getBalance(type) == nil { | ||
| if positionBalance == nil { | ||
| // 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 +1353,7 @@ 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. | ||
|
|
||
| let acceptedAmount = from.balance | ||
| position.borrowBalance(type)!.recordDeposit( | ||
| amount: UFix128(acceptedAmount), | ||
|
|
@@ -1525,22 +1538,37 @@ access(all) contract FlowALPv0 { | |
| panic("Cannot withdraw \(amount) of \(type.identifier) from position ID \(pid) - Insufficient funds for withdrawal") | ||
| } | ||
|
|
||
| var positionBalance = position.getBalance(type) | ||
|
|
||
| // If this position doesn't currently have an entry for this token, create one. | ||
| if position.getBalance(type) == nil { | ||
| if positionBalance == 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)! | ||
| // Re-fetch the balance after creating it | ||
| positionBalance = position.getBalance(type) | ||
| } | ||
|
|
||
| // Reflect the withdrawal in the position's balance | ||
| let wasCredit = positionBalance!.direction == FlowALPModels.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 | ||
| // Re-fetch balance to check if direction changed | ||
| positionBalance = position.getBalance(type) | ||
| if wasCredit && positionBalance!.direction == FlowALPModels.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 +1593,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.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)") | ||
|
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. If we remove these two assert statements, won't withdraw panic anyway? |
||
| 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 +1991,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.state.borrowReserve(sinkType) != nil, message: "Cannot withdraw \(sinkAmount) of \(sinkType.identifier) - token not in reserves") | ||
| let reserveVault = self.state.borrowReserve(sinkType)! | ||
| 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.
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
getCollateralTypesfunctions 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: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.