Skip to content

single debt and collateral per position#184

Draft
nialexsan wants to merge 32 commits intomainfrom
nialexsan/multi-debt
Draft

single debt and collateral per position#184
nialexsan wants to merge 32 commits intomainfrom
nialexsan/multi-debt

Conversation

@nialexsan
Copy link
Collaborator

@nialexsan nialexsan commented Feb 27, 2026

Closes: #???

Description

support borrowing tokens from the protocol reserves, not just minting MOET
temporary limitations:

  • one position supports one type of collateral and one type of debt
    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?

  • high interest rate
  • high liquidation penalty / hard-to-repay
  • high volatility relative to collateral
  • large share of total borrow

Which collateral to sell first to fund repay?

  • lowest liquidation threshold contribution per $ (i.e. “worst LT efficiency”)
  • lowest yield / least strategic
  • most liquid / lowest slippage

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@nialexsan nialexsan requested a review from a team as a code owner February 27, 2026 16:34
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 balances with collateralBalance, debtBalance
  • make queuedDeposits just a vault

// 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.")
}
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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. 🙇

@nialexsan
Copy link
Collaborator Author

@jordanschalm this is a soft limitation until we figure out how to do rebalancing and position closing with multiple collaterals and debts.
there are multiple ways of how to do rebalancing of multi token positions, but currently I don't know which one is more suited for our platform

@jordanschalm
Copy link
Member

Hey @nialexsan, should this be targeting nialexsan/pre-refactor instead of main?

Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some minor suggestions

let positionBalance = position.getBalance(seizeType)
if positionBalance == nil {
// Liquidation is seizing collateral - validate single collateral type
position.validateCollateralType(seizeType)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by Jordan's suggestion I moved the check to post condition

// Re-fetch balance to check if direction changed
positionBalance = position.getBalance(type)
if wasCredit && positionBalance!.direction == FlowALPModels.BalanceDirection.Debit {
position.validateDebtType(type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)

Copy link
Contributor

@liobrasil liobrasil Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let seizeState = self._borrowUpdatedTokenState(type: seizeType)
if position.getBalance(seizeType) == nil {
let positionBalance = position.getBalance(seizeType)
if positionBalance == nil {
Copy link
Member

Choose a reason for hiding this comment

The 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.

/// 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
Copy link
Member

@jordanschalm jordanschalm Mar 4, 2026

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 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.

self._queuePositionForUpdateIfNecessary(pid: pid)

let withdrawn <- reserveVault.withdraw(amount: amount)
// Get tokens either by minting (MOET) or from reserves (other tokens)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added abstraction with interfaces for reserve handlers

Comment on lines +1603 to +1605
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)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove these two assert statements, won't withdraw panic anyway?

)
let sinkVault <- FlowALPv0._borrowMOETMinter().mintTokens(amount: sinkAmount)

// Get tokens either by minting (MOET) or from reserves (other tokens)
Copy link
Member

Choose a reason for hiding this comment

The 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?

nialexsan and others added 2 commits March 4, 2026 13:54
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
Copy link
Contributor

@liobrasil liobrasil Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a post check should address this issue

if positionBalance == nil {
// Validate single collateral type constraint
position.validateCollateralType(type)

Copy link
Contributor

@liobrasil liobrasil Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a post check should address this issue

access(all) fun getCollateralTypes(): [Type] {
let types: [Type] = []
for type in self.balances.keys {
if self.balances[type]!.direction == BalanceDirection.Credit {
Copy link
Contributor

@liobrasil liobrasil Mar 4, 2026

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

Clear phantom debt/collateral types after exact zeroing
@nialexsan nialexsan mentioned this pull request Mar 5, 2026
6 tasks
@nialexsan nialexsan requested a review from jordanschalm March 5, 2026 03:18
@nialexsan nialexsan changed the title multi debt single debt and collateral per position Mar 5, 2026
@nialexsan
Copy link
Collaborator Author

don't need this any more, after QnA
the protocol uses top-up source for rebalancing

@nialexsan nialexsan closed this Mar 5, 2026
@nialexsan nialexsan reopened this Mar 5, 2026
@nialexsan nialexsan marked this pull request as draft March 5, 2026 18:44
@nialexsan
Copy link
Collaborator Author

@jordanschalm I'll clean up the PR and leave reserve handing without all the single token restrictions

@jordanschalm
Copy link
Member

@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.

@nialexsan nialexsan mentioned this pull request Mar 8, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants