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?

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

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.

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?

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nialexsan/fusdev strategy#215

Nialexsan/fusdev strategy#215
nialexsan wants to merge 21 commits intomainfrom
nialexsan/fusdev-strategy

nialexsan commented Mar 16, 2026 •

edited

Loading

Uh oh!

nialexsan Mar 19, 2026

Uh oh!

nialexsan commented Mar 19, 2026

Uh oh!

claude bot commented Mar 19, 2026 •

edited

Loading

Uh oh!

nialexsan commented Mar 20, 2026 •

edited

Loading

Uh oh!

jribbink Mar 20, 2026

Uh oh!

zhangchiqing Mar 20, 2026

Uh oh!

zhangchiqing Mar 20, 2026

Uh oh!

zhangchiqing Mar 20, 2026

Uh oh!

zhangchiqing Mar 20, 2026

Uh oh!

jordanschalm Mar 20, 2026

Uh oh!

Uh oh!

jordanschalm Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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!

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