Nialexsan/fusdev strategy by nialexsan · Pull Request #215 · onflow/FlowYieldVaults

nialexsan · 2026-03-16T15:07:13Z

Closes: #???

Description

this PR contains bulk changes related only to FUSDEV strategy from #183
so it's easier to review just the strategy itself withoutt the second strategy

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 · 2026-03-19T17:22:28Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

-        /// Tracks whether the underlying FlowALP position has been closed. Once true,
-        /// availableBalance() returns 0.0 to avoid panicking when the pool no longer
-        /// holds the position (e.g. during YieldVault burnCallback after close).
-        access(self) var positionClosed: Bool


this is removed to keep the contract updatable

nialexsan · 2026-03-19T20:08:10Z

@claude

claude · 2026-03-19T20:08:27Z

Claude finished @nialexsan's task in 2m 38s —— View job

Code Review

Gather context
Read changed files
Analyze changes
Post review feedback

Summary

This PR extracts the FUSDEV strategy from #183 for focused review. The changes are well-motivated and the code is generally clean. Below are my findings, ordered by severity.

`FlowYieldVaultsStrategiesV2.cdc`

Close Position Architecture — Notable Improvements

The closePosition refactor is a meaningful improvement over the previous approach:

Swapper reconstruction vs. serialization — Instead of storing a {DeFiActions.Swapper} in config (which required serialization of a complex struct), swappers are now rebuilt from the stored CollateralConfig. This is cleaner and avoids potential issues with stale or unresolvable stored swappers.
BufferedSwapSource — The design correctly addresses ERC4626 rounding underestimates. The comment is clear, and the burn of zero-balance vaults is handled properly. One note: the minimumAvailable() implementation calls quoteOut which may not be a perfect lower bound in edge cases — but given the buffered approach is already the "pull everything" pattern, this is acceptable.
Pre-supplement logic — The 1% buffer (shortfall + shortfall / 100.0) is reasonable for handling swap slippage during close. The key constraint is correctly documented: this must happen before closePosition to avoid the reentrancy lock.

Issues

_buildDebtToCollateralSwapper assumes a direct MOET→YIELD AMM pool exists (line ~383 in diff)

let debtToYieldAMM = FlowYieldVaultsStrategiesV2._buildUniV3Swapper(
    tokenPath: [tokens.moetTokenEVMAddress, tokens.yieldTokenEVMAddress],
    feePath: [100],
    ...
)

FUSDEV is a Morpho vault token — this assumes a MOET/FUSDEV UniV3 pool exists with fee tier 100. If that pool ever has insufficient liquidity, the dust-handling path will fail silently (the quoteOut check will return 0.0 and the vault is burned). The comment in _buildYieldToDebtSwapper confirms this pool is expected to exist, but it's worth documenting explicitly as a deployment invariant.

closePosition returns an empty collateral vault if no collateral is found (line ~244)

var collateralVault <- DeFiActionsUtils.getEmptyVault(collateralType)
while resultVaults.length > 0 {
    let v <- resultVaults.removeFirst()
    if v.getType() == collateralType {
        collateralVault.deposit(from: <-v)
    } ...
}

If closePosition returns no vault matching collateralType (shouldn't happen in practice but could if the position had dust-only collateral rounded to zero), the function returns an empty vault silently. The old code had an explicit assert that the first vault was collateral type. Consider adding an assertion after the loop: Fix this →

_resolveTokenBundle makes EVM calls during closePosition — called via _resolveTokenBundle(yieldTokenEVMAddress:), which calls ERC4626Utils.underlyingAssetEVMAddress. This performs an EVM read during close, which is already expensive. Not a bug, but worth noting as a potential cost concern in production.

Minor

var collateralVault <- DeFiActionsUtils.getEmptyVault(collateralType) — let would work here since the binding is never reassigned (only deposits are made into the resource). Cosmetic only.
_getStoredCollateralConfig borrows directly from deployer account storage — couples closePosition to the issuer's storage path. Acceptable for now but worth documenting.
_resolveTokenBundle was previously _resolveTokenBundle(collateralConfig:) on the composer and hard-coded Type<@MOET.Vault>(). The new version uses _getPoolDefaultToken() to derive it dynamically — this is a correctness improvement worth calling out positively.

`FlowYieldVaultsStrategiesV2_FUSDEV_test.cdc`

Commented-out contract deployment (lines ~211–221)

// temporary commented until merged with syWFLOW strategy
// log("Deploying FlowYieldVaultsStrategiesV2...")
// err = Test.deployContract(...)

This means the fork tests are running against the already-deployed mainnet version of the contract, not the new code from this PR. The new BufferedSwapSource, pre-supplement logic, and swapper reconstruction are not tested by these tests. This is a known limitation but should be noted as a gap — the tests validate the strategy's end-to-end lifecycle, not the specific new code paths.

Shared wbtcUser / wethUser state — Both point to 0x68da18f20e98a7b6. The WBTC and WETH lifecycle tests share this account including vault IDs stored in module-level state. If one lifecycle test fails, subsequent tests for the other collateral type may fail spuriously. The pattern is consistent with the rest of the test file, so not a blocker.

Leaked open vault in testCannotDepositWrongTokenToYieldVault — A fresh WETH vault is created for the negative test but never closed. Minor test hygiene issue.

`upsert_strategy_config.cdc`

The removal of the composer parameter from addOrUpdateCollateralConfig → addOrUpdateMorphoCollateralConfig is clean and reduces the risk of passing the wrong composer type. The rename is well-aligned with the fact that this issuer only manages Morpho configs.

Overall Assessment

The design is sound. The key risk is the commented-out contract deployment in tests — the fork tests provide good coverage of the strategy lifecycle but don't exercise the new closePosition code paths against the new contract. This should be resolved (or explicitly tracked) before merging to main.

nialexsan · 2026-03-20T16:10:04Z

Swapper reconstruction vs. serialization — Instead of storing a {DeFiActions.Swapper} in config (which required serialization of a complex struct), swappers are now rebuilt from the stored CollateralConfig. This is cleaner and avoids potential issues with stale or unresolvable stored swappers.

this change fixes an issue with exceeding compute limit while reading through the dict and retrieving a big object (Swapper) from it. Also this approach makes the implementation more flexible, in case the swap paths are updated after a strategy is created.

jribbink · 2026-03-20T20:42:22Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+        access(FungibleToken.Withdraw) fun withdrawAvailable(maxAmount: UFix64): @{FungibleToken.Vault} {
+            if maxAmount == 0.0 {
+                return <- DeFiActionsUtils.getEmptyVault(self.getSourceType())
+            }
+            let availableIn = self.source.minimumAvailable()
+            if availableIn == 0.0 {
+                return <- DeFiActionsUtils.getEmptyVault(self.getSourceType())
+            }
+            // Pull ALL available yield tokens (not quoteIn-limited)
+            let sourceLiquidity <- self.source.withdrawAvailable(maxAmount: availableIn)
+            if sourceLiquidity.balance == 0.0 {
+                Burner.burn(<-sourceLiquidity)
+                return <- DeFiActionsUtils.getEmptyVault(self.getSourceType())
+            }
+            let swapped <- self.swapper.swap(quote: nil, inVault: <-sourceLiquidity)
+            assert(swapped.balance > 0.0, message: "BufferedSwapSource: swap returned zero despite available input")
+            return <- swapped
+        }


This is the same behaviour as a SwapSource, but ignores the maxAmount provided by the DeFiActions.Source interface.

Are we sure that this is necessary, and not masking an underlying bug/design flaw? I thought that DeFiActions used UFix64.max as a senteniel value for cases like this, or am I misunderstanding here?

It kinda compensates the rounding/conversion error on ERC4626 vault and design/implementation of _repayDebtsFromSources method in FlowALPv0 that withdraws exact debt amount.

If we change that _repayDebtsFromSources to withdraw UFix64.max, then the BufferedSwapSource could be replaces with a regular SwapSource, though in this case the FlowALP will always overwithdraw even if the repayment source provides exact debt amount.

zhangchiqing · 2026-03-20T20:55:09Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

                    } else {
-                        // @TODO implement swapping moet to collateral
-                        destroy dustVault
+                        Burner.burn(<-v)


Should we give us some visibility of the reason we burn the tokens? Because the decision depends on a 3rd party quote.

We have 2 (or more?) code paths that lead to token burning event: 1) vault has 0 balance 2) the token worth nothing.

Maybe it's useful to add a TokenBurned event with the amount and the quote as evidence of why we decided to burn it, in case we need to debug, since the swapper price might be either outdated or misbehaving?

zhangchiqing · 2026-03-20T21:00:36Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+        ): UniswapV3SwapConnectors.Swapper {
+            let yieldToCollPath = collateralConfig.yieldToCollateralUniV3AddressPath
+            let yieldToCollFees = collateralConfig.yieldToCollateralUniV3FeePath
+            assert(yieldToCollPath.length >= 2, message: "yieldToCollateral path must have at least 2 elements")


Suggested change

assert(yieldToCollPath.length >= 2, message: "yieldToCollateral path must have at least 2 elements")

assert(yieldToCollPath.length >= 2, message: "yieldToCollateral path requires at least yield and collateral tokens, got \(yieldToCollPath.length)")

zhangchiqing · 2026-03-20T21:01:27Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            uniqueID: DeFiActions.UniqueIdentifier
+        ): UniswapV3SwapConnectors.Swapper {
+            let yieldToCollPath = collateralConfig.yieldToCollateralUniV3AddressPath
+            let yieldToCollFees = collateralConfig.yieldToCollateralUniV3FeePath


Better validate this as well

assert( yieldToCollFees.length == yieldToCollPath.length - 1, message: "Fee array length (\(yieldToCollFees.length)) must equal path hops (\(yieldToCollPath.length - 1))" )

this assertion exists in collateralConfig init method

zhangchiqing · 2026-03-20T21:10:25Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

                    }
                } else {
-                    destroy dustVault
+                    Burner.burn(<-v)


could we mention the burning token behavior (the cleanup) in the comments of closePosition as well?

jordanschalm · 2026-03-20T23:04:05Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            return self.swapper.quoteOut(forProvided: avail, reverse: false).outAmount
+        }
+        /// Pulls ALL available yield tokens from the source and swaps them to the debt token.
+        /// Ignores quoteIn — avoids ERC4626 rounding underestimates that would leave us short.


What is the rounding underestimate caused by? Is it bounded? If the source contains much more funds than is needed, we'll swap much more than we need to.

current implementation of ERC4626 connector rounds down on quote it and redeem
it's bounded to 1 unit on the evm side, which is 0.000001 PYUSD0.
the source won't contain too much funds, just enough to cover the debt. Pre-supplement step buffers the debt by 2% of the shortfall to cover the swap fees

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

jordanschalm · 2026-03-20T23:11:22Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+                    collateralType: collateralType,
+                    uniqueID: self.uniqueID!
+                )
+                let shortfall = totalDebtAmount - expectedMOET


It seems like both these changes are related to the Swapper quotes being unreliable:

BufferedSwapSource swaps all funds available in the source, ignoring the quote, because it might contain rounding errors

This "pre-supplement extra moet" step adds 1% to account for slippage/rounding unaccounted for in the quote from collateralToMoetSwapper

But when we calculate the shortfall, we are using expectedMOET, which is the result of a swapper quote. Why do we trust this quote and not the others?

expectedMOET uses quoteOut, which is more precise than quoteIn in other cases
quoteOut in this case goes with the direction of the swap, and it rounds down all calculations, just like swap

I guess what I'm trying to drive at is: can we change the implementation of the Swapper/Source to handle these rounding errors more gracefully?

If my understanding is correct, currently the yieldTokenSource might estimate it can provide X tokens, but will actually provide X-epsilon tokens. From this comment, I understand that this epsilon is bounded.

Looking at this comment:

SwapSource computed 98 shares was enough to produce 100 PYUSD0 — it wasn't

Is there anything we can do in the SwapSource or its underlying Swapper, to correct its estimation of its own capacity, given the error is known and bounded? My understanding of the whole picture here still feels a bit fuzzy, but intuitively it seems possible to account for a known, bounded error. I feel like effort spent making the minimumAvailable and quote{In/Out} functions always accurate will be broadly beneficial and avoid further need for this kind of workaround. By accurate, I mean choosing the direction of error:

minimumAvailable should always err on the side of underestimating -> being able to draw more tokens from the source is OK, thinking you can draw more than are actually available is not

quoteIn should always err on the side of overestimating -> getting more tokens from the swap is OK, getting less is not

quoteOut should always err on the side of underestimating -> getting more tokens from the swap is OK, getting less is not

that would be easier to achieve if there was only one hop, but in our implementation there are at least two hops, and the rounding error accumulates regardless of the precision

separately from that, ERC4626 contract has six different preview/quote methods that have different rounding modes, and we're just using two of them for quote it and quote out without any additional flags, so fundamentally it will be super tricky to fit them in

zhangchiqing · 2026-03-23T15:23:53Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+                let quote = collateralToMoetSwapper.quoteIn(forDesired: buffered, reverse: false)
+                if quote.inAmount > 0.0 {
+                    let extraCollateral <- self.source.withdrawAvailable(maxAmount: quote.inAmount)
+                    assert(extraCollateral.balance > 0.0,


If user doesn't have enough collateral to pay for the debt, we will panic and don't let them close the strategy?

Could you explain why we decided to panic, instead of continue closing the strategy?

if user doesn't have enough collateral to cover the debt, the position is unhealthy and it should be under liquidation

zhangchiqing · 2026-03-23T15:26:40Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+                // Add 1% buffer to account for swap slippage/rounding in the collateral→MOET leg
+                let buffered = shortfall + shortfall / 100.0
+                let quote = collateralToMoetSwapper.quoteIn(forDesired: buffered, reverse: false)
+                if quote.inAmount > 0.0 {


Is quote.inAmount == 0 an edge case?

(nit) To me, this should never happen (you can get out token by putting 0 in token), but even if this is possible, the quote.inAmount == 0 case only matters for the self.source.withdrawAvailable, so technically it only needs the if quote.inAmount > 0 check to wrap the withdraw, and continue the swap with the quote and deposit regardless the inAmount value. But that would be weird (you get desired MOET tokens by providing nothing), it's almost like a bug in the swapper. So if we don't think quote.inMount == 0 would ever make sense, we could just panic by replacing with an assert .

zhangchiqing · 2026-03-23T15:31:57Z

cadence/contracts/FlowYieldVaultsStrategiesV2.cdc

+            )
+        }
+        access(contract) view fun copyID(): DeFiActions.UniqueIdentifier? { return self.uniqueID }
+        access(contract) fun setID(_ id: DeFiActions.UniqueIdentifier?) { self.uniqueID = id }


Once set with a non-nil uniqueID, does it make sense to be set with nil ID again? Do we want to prevent that or keeping it flexible?

this is a standard implementation across the project
It's not usually reset the id to nil, in our code. I'd rather keep if flexible for now

…tegy

nialexsan · 2026-03-23T16:36:51Z

@jribbink @jordanschalm
here's a claude generated explanation why pre-supplement and buffered swapper are required:

● Setup (UniV3 fee exaggerated to 1% for readability; FUSDEV redemption is free)

  ---
  Open position

  Borrow:                     100 MOET
  MOET→PYUSD0 (1% UniV3):     floor(100 × 0.99) = 99 PYUSD0
  Deposit into fresh FUSDEV:  receive 99 shares at 1:1

  Vault:  totalAssets = 99,  totalSupply = 99

  Yield accrues

  Vault:  totalAssets = 101,  totalSupply = 99  (2 PYUSD0 yield)

  ---
  Close — Step 1: check if yield covers debt

  quoteOut(99 shares):
    FUSDEV→PYUSD0 (free): floor(99 × 101 / 99) = 101 PYUSD0
    PYUSD0→MOET (1% fee): floor(101 × 0.99)    = 99 MOET

  99 < 100 debt  →  shortfall = 1 MOET

  The 2% round-trip fee exceeds the 2% yield by a rounding unit.

  ---
  Close — Step 2: pre-supplement

  buffered = 1 × 1.01 = 1.01 MOET
  Pull PYUSD0 collateral, swap → deposit 1 MOET to reduce debt

  Remaining debt:  99 MOET

  ---
  Close — Step 3a: repay with SwapSource — fails

  quoteIn(99 MOET):
    PYUSD0 needed: ceil(99 / 0.99)        = 100 PYUSD0
    shares needed: floor(100 × 99 / 101)  = floor(98.01) = 98 shares

  Redeem 98 shares (free ERC4626):
    floor(98 × 101 / 99) = floor(99.97) = 99 PYUSD0

  PYUSD0→MOET (1% fee):
    floor(99 × 0.99) = floor(98.01) = 98 MOET

  98 < 99 debt  ✗

  SwapSource computed 98 shares was enough to produce 100 PYUSD0 — it wasn't. ERC4626 floor
  division turned 100 into 99, and the UniV3 fee turned 99 into 98.

  ---
  Close — Step 3b: repay with BufferedSwapSource — succeeds

  Pull all 99 shares unconditionally:
    FUSDEV→PYUSD0: floor(99 × 101 / 99) = 101 PYUSD0
    PYUSD0→MOET:   floor(101 × 0.99)    = 99 MOET

  99 >= 99 debt  ✓

zhangchiqing · 2026-03-23T18:34:29Z

cadence/tests/FlowYieldVaultsStrategiesV2_FUSDEV_test.cdc

+    let vaultBalAfter = _executeScript("../scripts/flow-yield-vaults/get_yield_vault_balance.cdc", [wbtcUser.address, wbtcVaultID])
+    Test.expect(vaultBalAfter, Test.beSucceeded())
+    Test.assert(vaultBalAfter.returnValue == nil, message: "WBTC vault should no longer exist after close")
+    log("WBTC yield vault closed successfully")


Can we also add assertion to check that after closing the yield vault, the debt is reduced.

We might want to assert that after opening and closing the yield vault, the total collateral amount should be very close to the original amount, there should be only a tiny amount loss due to running into the code path of expectedMOET < totalDebtAmount where some collateral has to be sold, right?

nialexsan added 8 commits March 16, 2026 11:03

isolated strategy contract from fyv-v2-strategy-deployment

90aa86f

remove More and syWFLOWv from strategy

0fc9afc

isolated FUSDEV test from fyv-v2-strategy-deployment

b7984d9

isolated FUSDEV test

05575b6

upsert strategy changes from fyv-v2-strategy-deployment

4aa9ad4

fix typo

c2cdcb1

provision script from fyv-v2-strategy-deployment

a55ad1c

deposit_wrong_token from fyv-v2-strategies-deployment

c6a0634

nialexsan force-pushed the nialexsan/fusdev-strategy branch from 6de117f to c6a0634 Compare March 16, 2026 16:13

nialexsan added 2 commits March 16, 2026 12:14

add missing clean up and remove unused preswap

c661c96

quote fix

009d413

onflow deleted a comment from claude bot Mar 16, 2026

nialexsan added 2 commits March 16, 2026 14:47

remove unused pre-swap logic

4d75cc2

fix typo

48ef07b

onflow deleted a comment from claude bot Mar 16, 2026

nialexsan force-pushed the nialexsan/fusdev-strategy branch from e55de25 to 48ef07b Compare March 16, 2026 19:46

onflow deleted a comment from claude bot Mar 16, 2026

nialexsan added 3 commits March 17, 2026 12:43

Merge branch 'main' into nialexsan/fusdev-strategy

1027932

remove fusdev related code

6b0d51a

cleaned fusdev strategy

be3dbbf

onflow deleted a comment from claude bot Mar 18, 2026

nialexsan added 5 commits March 18, 2026 12:24

Merge branch 'main' into nialexsan/fusdev-strategy

e9f80ee

tweak coa cap

fc38a29

apply suggestions

55adc6e

add panic to empty config

2ca5bd1

fix optional

417ded3

onflow deleted a comment from claude bot Mar 18, 2026

nialexsan marked this pull request as ready for review March 18, 2026 19:15

nialexsan requested a review from a team as a code owner March 18, 2026 19:15

safeguards

a6ee109

nialexsan commented Mar 19, 2026

View reviewed changes

vishalchangrani requested review from Kay-Zee, holyfuchs and jordanschalm March 20, 2026 14:48

jribbink reviewed Mar 20, 2026

View reviewed changes

zhangchiqing reviewed Mar 20, 2026

View reviewed changes

jordanschalm reviewed Mar 20, 2026

View reviewed changes

zhangchiqing reviewed Mar 23, 2026

View reviewed changes

nialexsan added 3 commits March 23, 2026 12:18

address PR comments

5268aeb

fix typo

0befc07

Merge remote-tracking branch 'origin/main' into nialexsan/fusdev-stra…

bd1dac3

…tegy

fix assertion message

5f05e4a

nialexsan requested review from jordanschalm, jribbink and zhangchiqing March 23, 2026 16:40

zhangchiqing reviewed Mar 23, 2026

View reviewed changes

	assert(yieldToCollPath.length >= 2, message: "yieldToCollateral path must have at least 2 elements")
	assert(yieldToCollPath.length >= 2, message: "yieldToCollateral path requires at least yield and collateral tokens, got \(yieldToCollPath.length)")

Conversation

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

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nialexsan commented Mar 19, 2026

Uh oh!

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

Uh oh!

Code Review

Summary

FlowYieldVaultsStrategiesV2.cdc

Close Position Architecture — Notable Improvements

Issues

Minor

FlowYieldVaultsStrategiesV2_FUSDEV_test.cdc

upsert_strategy_config.cdc

Overall Assessment

Uh oh!

nialexsan commented Mar 20, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordanschalm Mar 23, 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!

Choose a reason for hiding this comment

Uh oh!

nialexsan commented Mar 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nialexsan commented Mar 16, 2026 •

edited

Loading

claude bot commented Mar 19, 2026 •

edited

Loading

`FlowYieldVaultsStrategiesV2.cdc`

`FlowYieldVaultsStrategiesV2_FUSDEV_test.cdc`

`upsert_strategy_config.cdc`

nialexsan commented Mar 20, 2026 •

edited

Loading

jordanschalm Mar 23, 2026 •

edited

Loading