close position from strategies by nialexsan · Pull Request #182 · onflow/FlowYieldVaults

nialexsan · 2026-02-23T22:15:39Z

Closes: #???

Description

implement close position logic in strategies to integrate with FlowALP close position logic

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

cadence/contracts/mocks/MockStrategies.cdc

zhangchiqing · 2026-03-04T17:56:21Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            totalDebtAmount = totalDebtAmount + repaymentBuffer
+
+            // Step 3: If no debt, pass empty vault array
+            if totalDebtAmount == 0.0 {


With the repaymentBuffer, this will never be true, so maybe this instead?

Suggested change

if totalDebtAmount == 0.0 {

if totalDebtAmount == repaymentBuffer {

or create a new variable, and then we can check if the original debt amount without buffer is 0

let totalDebtAmountWithBuffer = totalDebtAmount + repaymentBuffer

the close position logic should always overpay debt slightly

No. Look at this logic, totalDebtAmount should always > 0, and never be 0.0. So if totalDebtAmount == 0.0 will never become true.

let repaymentBuffer: UFix64 = 0.00000001 // 1e-8 totalDebtAmount = totalDebtAmount + repaymentBuffer // Step 3: If no debt, close with empty sources array if totalDebtAmount == 0.0 {

it seems to be an artifact from one of the previous versions, there is no repayment buffer any more

zhangchiqing · 2026-03-04T18:11:09Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+                ?? panic("Could not create external source from AutoBalancer")
+
+            // Step 5: Retrieve yield→MOET swapper from contract config
+            let swapperKey = "yieldToMoetSwapper_".concat(self.id()!.toString())


better to create a swapperKeyByID(self.id()!) function

zhangchiqing · 2026-03-04T18:22:07Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+                    )
+                    collateralVault.deposit(from: <-swappedCollateral)
+                } else {
+                    destroy dustVault


Note we will destroy a non-empty collateral vault regardless the amount. This could is techinically possible since the returned vaults can have pending deposits, and overpay vaults.

I think we should do:

if dustVault.balance == 0 { destroy dustVault } if dustVault.getType == collateralType { collateralVault.deposit(from: <- dustVault) } // dustVault.getType() != collateralType // swap and then deposit ...

zhangchiqing · 2026-03-04T18:33:47Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            assert(resultVaults.length > 0, message: "No vaults returned from closePosition")
+
+            // First vault should be collateral
+            var collateralVault <- resultVaults.removeFirst()


Better double check the collateral type?

put a post check

zhangchiqing · 2026-03-04T18:51:10Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            collateralType: Type,
+            uniqueID: DeFiActions.UniqueIdentifier
+        ): UniswapV3SwapConnectors.Swapper {
+            // Reverse the swap path: collateral -> yield (opposite of yield -> collateral)


Does this reverse computation happen a lot? Does it make sense to store the reversed path within the CollateralConfig?

no, it's calculated only once during position creation, storing it separately would be redundant and it will increase risk of misconfiguration

we'll need to store another path though: moet to collateral

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

jordanschalm · 2026-03-06T19:13:56Z

cadence/contracts/mocks/MockFlowALPConsumer.cdc

-                repaymentSource: repaymentSource,
-                pushToDrawDownSink: pushToDrawDownSink
-            )
+        let poolCap = MockFlowALPConsumer.account.storage.load<Capability<auth(FlowALPv0.EParticipant, FlowALPv0.EPosition) &FlowALPv0.Pool>>(


Suggested change

let poolCap = MockFlowALPConsumer.account.storage.load<Capability<auth(FlowALPv0.EParticipant, FlowALPv0.EPosition) &FlowALPv0.Pool>>(

let poolCap = MockFlowALPConsumer.account.storage.load<Capability<auth(FlowALPv0.EParticipant) &FlowALPv0.Pool>>(

See onflow/FlowALP#175

jordanschalm · 2026-03-06T19:23:28Z

cadence/contracts/mocks/MockStrategies.cdc

+            // Step 1: Get debt amounts from position - returns {Type: UFix64} dictionary
+            let debtsByType = self.position.getTotalDebt()
+
+            // Step 2: Calculate total debt amount across all debt types


This doesn't really make sense to me:

getTotalDebt returns a map from token type to the amount of debt denominated in that token type

if there are actually multiple debt types, we need to handle them separately, we can't just add up multiple debts of different currencies, without considering what the currencies are

further down, we assume the debt is all MOET and only provide a MOET repayment source

implemented multy-debt type logic

jordanschalm · 2026-03-06T19:25:24Z

cadence/contracts/mocks/MockStrategies.cdc

+                ?? panic("Could not create external source from AutoBalancer")
+
+            // Step 5: Withdraw ALL available YT from AutoBalancer to avoid losing funds when Strategy is destroyed
+            // Use minimumAvailable() to get the actual available amount (UFix64.max might not work as expected)


UFix64.max might not work as expected

Why does it not work as expected?

this is a debugging artifact to fully withdraw from the position, I'll remove it

jordanschalm · 2026-03-06T19:44:54Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            // Step 2: Calculate total debt amount across all debt types
+            var totalDebtAmount: UFix64 = 0.0
+            for debtAmount in debtsByType.values {
+                totalDebtAmount = totalDebtAmount + debtAmount
+            }


Same here. This structure and documentation implies that it will handle multiple debt types, but it will only work when there is exactly one debt type, and it is MOET.

now this is just a check whether there is any debt balance

jordanschalm · 2026-03-06T19:49:25Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+                )
+                // Extract the first vault (should be collateral)
+                assert(resultVaults.length > 0, message: "No vaults returned from closePosition")
+                let collateralVault <- resultVaults.removeFirst()


If we ever have more than one collateral type in the future, we will destroy it here. I'd add an assertion that we got exactly one collateral vault back (if we're certain that is the only allowed initial state), or handle multiple collateral vaults being returned.

this strategy will work only with one collateral and one debt type at a time, I added assertions

jordanschalm · 2026-03-06T23:51:06Z

cadence/tests/rebalance_scenario5_test.cdc

+}
+
+access(all)
+fun setup() {


Can we consolidate this setup function with the one used in scenario4?

jordanschalm · 2026-03-07T00:13:13Z

cadence/tests/rebalance_scenario5_test.cdc

+	log("  MOET debt:       \(debtAfterFlowDrop) MOET")
+	log("  Health:          \(healthAfterRebalance)")
+
+	if healthAfterRebalance >= 1.3 {


Let's assert whichever of these we expect to actually happen!

jordanschalm · 2026-03-07T00:14:17Z

cadence/tests/rebalance_scenario5_test.cdc

+	log("  MOET debt:       \(debtBefore) MOET")
+	log("  Health:          \(healthBeforeRebalance)")
+
+	if healthBeforeRebalance < 1.0 {


Is observing this health value expected behaviour? Unexpected behaviour? Let's assert what we expect the health to actually be.

jordanschalm · 2026-03-07T00:15:02Z

cadence/tests/rebalance_scenario5_test.cdc

+}
+
+access(all)
+fun test_RebalanceYieldVaultScenario5() {


Same comment here. This test has almost no assertions. Automated tests are useful because they pass when the component behaves as expected, and fail when the component doesn't behave as expected!

Logging as the only strategy for surfacing the behaviour of the component we are testing is counterproductive. It gives the illusion of test coverage, but will not actually perform the purpose of a test suite, by letting us know when the component's behaviour regresses.

jordanschalm · 2026-03-07T00:41:38Z

cadence/tests/tracer_strategy_test.cdc

+	// This is significantly better than without rebalanceSource (would be ~94% loss)
+	// but still substantial due to the extreme 90% price crash.
+	let expectedMaxBalance = fundingAmount * 0.9  // Allow for up to 10-20% loss
+	Test.assert((flowBalanceAfter-flowBalanceBefore) <= expectedMaxBalance,


The comment above states:

User ends up with ~65-70 FLOW (30-35% loss)

Can we make the assertion reflect this expectation about the final balance more precisely? "Somewhere between 0-90%" is a really wide range for what should be a deterministic test.

nialexsan · 2026-03-07T20:36:20Z

@jordanschalm I addressed all comments ~~except of FlowYieldVaultsStrategiesV2, they will be addressed in a separate PR. I need to rework the whole strategy to handle different collateral types and a special case for PYUSD0~~
nvm, the PYUSD0 won't be used for FUSDEV - FlowALP, it simplified the logic

nialexsan · 2026-03-08T04:16:58Z

cadence/contracts/mocks/MockStrategies.cdc

+            // Otherwise, use MockSwapper.quoteIn to pre-swap YT → debt token and wrap in a VaultSource.
+            // Pre-swapping with quoteIn guarantees the exact debt amount is delivered to the pool.
+            var repaymentSources: [{DeFiActions.Source}] = []
+            var debtCaps: [Capability<auth(FungibleToken.Withdraw) &{FungibleToken.Vault}>] = []


using this awkward solution to precisely match the debt amounts.
just using swap source directly optimizes for inAmount and has precision issue in this test

liobrasil · 2026-03-09T17:23:16Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+
+            // Store yield→MOET swapper in contract config for later access during closePosition
+            let yieldToMoetSwapperKey = FlowYieldVaultsStrategiesV2.getYieldToMoetSwapperConfigKey(uniqueID)!
+            FlowYieldVaultsStrategiesV2.config[yieldToMoetSwapperKey] = yieldToMoetSwapper


This writes per-vault runtime state into a contract-global dictionary, and nothing removes it on close/burn, so every opened+closed vault leaks a yieldToMoetSwapper_<id> entry.

I opened a follow-up fix in #201

That PR moves the swapper onto FUSDEVStrategy itself and removes the global runtime config/key path.

I'll fix that, I missed this one during config partitioning

that's right, this contract is already deployed, and this was the original implementation
the top level config with nested maps is a workaround to keep the contract updatable

I've addressed this issue in #183

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

jordanschalm · 2026-03-09T16:27:27Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            // Handle any overpayment dust (MOET) returned as the second vault
            while resultVaults.length > 0 {
                let dustVault <- resultVaults.removeFirst()
                if dustVault.balance > 0.0 {


The current logic is to destroy any funds we get back that aren't the collateral type without checking their type or balance, which seems dangerous to me. On line 215 above, we assert that there are either 1 or 2 result vaults, so this while loop is operating on at most one vault in resultVaults.

If it is still possible to have overpayment dust, when closePosition can pull the exact debt repayment it wants, then I would replace the while loop with:

let dustVault <- resultVaults.removeFirst() assert(resultVaults.length == 0) destroy <-resultVaults // We may receive a small amount of MOET in addition to collateral, which represents dust overpayment from repaying the position's debt. // Because this is a trivial amount of funds, we do not attempt to recover it. assert(dustVault.getType() == Type<MOET>) assert(dustVault.balance <= maximumPossibleDustBalance) destroy <-dustVault

If it isn't possible to have overpayment dust, then we can change line 215 to assert resultVaults.length == 1 and remove this loop.

I implemented dust handling in #183, so the strategy will swap overpayment dust back to collateral

OK, I would still avoid destroying vaults without checking them. The current code handles the first collateral vault (👍) but can destroy subsequent vaults without checking them. I'd just add assertions prior to destroying those vaults, to make sure they are the expected dust (expected type and expected small value).

jordanschalm · 2026-03-09T19:54:43Z

cadence/tests/rebalance_scenario4_test.cdc

+	log("  MOET debt:       \(debtBefore) MOET")
+
+	rebalanceYieldVault(signer: flowYieldVaultsAccount, id: yieldVaultIDs![0], force: true, beFailed: false)
+	rebalancePosition(signer: protocolAccount, pid: pid, force: true, beFailed: false)


The YV rebalance is doing all the work here, right? We expect rebalancePosition to basically be a no-op? (Otherwise I would expect the FLOW collateral below to change after the rebalance.)

Suggested change

rebalancePosition(signer: protocolAccount, pid: pid, force: true, beFailed: false)

// Since we rebalance the yield vault first, which pays down debt, the position rebalance is a no-op

rebalancePosition(signer: protocolAccount, pid: pid, force: true, beFailed: false)

cadence/tests/rebalance_scenario4_test.cdc

nialexsan added 14 commits February 23, 2026 17:14

rebalance test edge cases

44854d6

Merge branch 'main' into nialexsan/dust-debug-flowalp

d4c07bb

close position

87f1fe4

mock swap rounding

94e6013

remove doc

79f79e4

update ref

1e6acc3

close position in strategy

21c6c5a

remove comments

190e1c3

Merge branch 'main' into nialexsan/dust-debug-flowalp

8e3c733

fix closing

341461d

switch to upgradeable change

d154cea

add autobalancer

226c86a

fix tracer strategy

6a6b85c

remove unused method

934319a

nialexsan commented Feb 26, 2026

View reviewed changes

cadence/contracts/mocks/MockStrategies.cdc Outdated Show resolved Hide resolved

nialexsan added 5 commits February 26, 2026 12:42

Apply suggestion from @nialexsan

1c61628

update ref

41b24de

sync implementation with FlowALP

c26627c

fix deposit rate

a92ce68

split PMStrategies test

884e899

nialexsan marked this pull request as ready for review March 1, 2026 18:06

nialexsan requested a review from a team as a code owner March 1, 2026 18:06

vishalchangrani requested review from Kay-Zee, holyfuchs, jordanschalm and liobrasil March 3, 2026 16:37

vishalchangrani mentioned this pull request Mar 3, 2026

Enable PyUSD0 as a collateral for the Flow Looping Vault strategy. #184

Open

zhangchiqing reviewed Mar 4, 2026

View reviewed changes

nialexsan added 2 commits March 5, 2026 12:53

update FlowALP ref

3452d17

revert ci/cd

a763eac

liobrasil reviewed Mar 6, 2026

View reviewed changes

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc Outdated Show resolved Hide resolved

Fix zero-debt close path state tracking

e2787e0

nialexsan commented Mar 6, 2026

View reviewed changes

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc Outdated Show resolved Hide resolved

nialexsan and others added 3 commits March 6, 2026 16:04

Apply suggestion from @nialexsan

ca91632

chore: mark strategy config key helpers as view

9b105b1

fix: publish receiver capabilities in yield vault txns

27edd22

jordanschalm requested changes Mar 7, 2026

View reviewed changes

nialexsan added 3 commits March 7, 2026 14:44

address PR comments

268bbaa

combine tests

2d8788b

remove duplicated methods

6a2915e

nialexsan added 5 commits March 7, 2026 20:50

address strategy comments

9cf0944

assert first vault is collateral[C

49b0a17

tweaks

f88fff4

tweak assertion

f74da70

tweak mock

068457d

nialexsan commented Mar 8, 2026

View reviewed changes

holyfuchs mentioned this pull request Mar 9, 2026

Fix supervisor: report vault execution so stuck-scan order isn't fixed #187

Open

liobrasil reviewed Mar 9, 2026

View reviewed changes

Merge branch 'main' into nialexsan/dust-debug-flowalp

bfc1fc0

nialexsan changed the title ~~rebalance test edge cases~~ close position from strategies Mar 9, 2026

nialexsan requested a review from jordanschalm March 9, 2026 21:18

jordanschalm reviewed Mar 9, 2026

View reviewed changes

liobrasil and others added 2 commits March 9, 2026 19:34

test: use canonical position health helper

2dddb68

abstract hardcoded values

8ea43dd

jordanschalm approved these changes Mar 10, 2026

View reviewed changes

nialexsan merged commit f9daf70 into main Mar 10, 2026
7 checks passed

nialexsan deleted the nialexsan/dust-debug-flowalp branch March 10, 2026 00:57

liobrasil mentioned this pull request Mar 11, 2026

Production hardening for PR183 strategy deployment #209

Open

	if totalDebtAmount == 0.0 {
	if totalDebtAmount == repaymentBuffer {

	let poolCap = MockFlowALPConsumer.account.storage.load<Capability<auth(FlowALPv0.EParticipant, FlowALPv0.EPosition) &FlowALPv0.Pool>>(
	let poolCap = MockFlowALPConsumer.account.storage.load<Capability<auth(FlowALPv0.EParticipant) &FlowALPv0.Pool>>(

	rebalancePosition(signer: protocolAccount, pid: pid, force: true, beFailed: false)
	// Since we rebalance the yield vault first, which pays down debt, the position rebalance is a no-op
	rebalancePosition(signer: protocolAccount, pid: pid, force: true, beFailed: false)

Conversation

nialexsan commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nialexsan Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nialexsan commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liobrasil Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nialexsan commented Feb 23, 2026 •

edited

Loading

nialexsan Mar 7, 2026 •

edited

Loading

nialexsan commented Mar 7, 2026 •

edited

Loading

liobrasil Mar 9, 2026 •

edited

Loading