feat: ynEIGEN withdrawals #164

johnnyonline · 2024-09-09T16:35:48Z

No description provided.

danoctavian · 2024-09-18T14:18:34Z

src/WithdrawalQueueManager.sol

@@ -291,6 +291,9 @@ contract WithdrawalQueueManager is IWithdrawalQueueManager, ERC721EnumerableUpgr

        uint256 unitOfAccountAmount = calculateRedemptionAmount(request.amount, redemptionRate);

+        // zero out the last decimal place


what is the reason to zero out the last decimal place?

it fails because of precision errors

removed at 7b35647.

danoctavian · 2024-09-18T14:49:02Z

src/WithdrawalQueueManager.sol

@@ -291,6 +291,9 @@ contract WithdrawalQueueManager is IWithdrawalQueueManager, ERC721EnumerableUpgr

        uint256 unitOfAccountAmount = calculateRedemptionAmount(request.amount, redemptionRate);

+        // zero out the last decimal place
+        unitOfAccountAmount = unitOfAccountAmount / 10 * 10 - 10; // @todo - is this safe? (maybe easier to just do `- 20`?)


> 18456789n / 10n * 10n - 10n 18456770n

From the first comment i'd asume you want the result here to be

18456789n -> 18456780n

if we can clarify first what's the objective here we can fix it.

yes objective is to not fail on this https://github.com/yieldnest/yieldnest-protocol/blob/yneigen-withdrawals/src/WithdrawalQueueManager.sol#L306

What if we just push a slight excess of tokens to the vault all the time, instead of tolerating wei differences?

yep probably better - 7b35647 (left my rpcurl there oops)

src/ynEIGEN/TokenStakingNode.sol

danoctavian · 2024-09-18T14:57:07Z

src/WithdrawalQueueManager.sol

@@ -382,7 +385,7 @@ contract WithdrawalQueueManager is IWithdrawalQueueManager, ERC721EnumerableUpgr
        uint256 amount,
        uint256 redemptionRate
    ) public view returns (uint256) {
-        return amount * redemptionRate / (10 ** redeemableAsset.decimals());
+        return amount * redemptionRate / (10 ** redeemableAsset.decimals()); // @todo


what is the @todo here?

Useful to add a clarifying comment

right. im in not process of cleaning up. give me another day here pls

no worries, i'll leave the comments anyway, with the understanding this is an early draft

removed at 7b35647.

danoctavian · 2024-09-18T16:24:24Z

src/ynEIGEN/RedemptionAssetsVault.sol

+     * @param amount The amount of assets to transfer.
+     * @dev Requires the caller to be the redeemer and the contract to not be paused.
+     */
+    function transferRedemptionAssets(address to, uint256 amount, bytes calldata /* data */) public onlyRedeemer whenNotPaused nonReentrant {


Another approach here is to send proportionate to the content of the vault. That way it's on the off-chain to decide the ratio and the on-chain simply uses what it has available without more complex logic.

uint256 totalRedemptionAssets = availableRedemptionAssets(); uint256 assetRatio = amount * YN_UNIT / totalRedemptionAssets; for (asset of assets) { uint256 amountToTransfer = balances[address(asset)] * assetRatio / YN_UNIT; // Do transfer }

We should be mindful about has here and repeated computation, we can duplicate code for less gas usage (Eg. availableRedemptionAssets logic)

suggested another approach on Discord. will update this thread once we have a decision.

danoctavian · 2024-09-18T16:30:59Z

src/ynEIGEN/EigenStrategyManager.sol

@@ -306,7 +305,15 @@ contract EigenStrategyManager is
                    asset,
                    strategies[asset].userUnderlyingView((address(node)))
                );
-                stakedBalances[j] += strategyBalance;
+


This function is the most gas consumptive function in the current implementation that poses problems because of the existing unbounded loops.

Let's discuss if we can include a solution for this in this current upgrade since we're updating it regardless.

This is a general problem that applies to all the multi-asset vaults that stake the assets within protocols but it's particularly pathological in Eigenlayer because of the StakingNodes pattern that is necessary to communicate to validators.

Idea here:

We could make the result of that inner loop cached in a storage variable per asset.

This variable will be updated all the time it when you allocate to StakinNodes or withdraw from StakingNodes.

The update function can also be permisionless - which would cover the slashing case.

This reduces it to a O(N) loop over the N assets which is reasonable for any multi-asset vault.

@johnnyonline this is not addressed - we'll need to cover this one

i'm not sure i understand what you mean here. but trying to optimize it a bit -

using function testTotalAssetsGas() public view {yneigen.totalAssets();}) as a test and forge snapshot.

testTotalAssetsGas() gas:

2ae7917 - 818370

be925af - 741084

471afc7 (using Solady PoC) - 740688 -- i can continue implementing Solady across the contracts, lmk

danoctavian · 2024-09-18T17:02:20Z

src/ynEIGEN/TokenStakingNode.sol

+        emit DeallocatedTokens(_amount, _token);
+    }
+
+    function _wrapIfNeeded(uint256 _amount, IERC20 _token) internal returns (uint256, IERC20) {


we should have this asset-specific logic that handles rebasing tokens not contained here

currently all those asset specific things are in EigenStrategyManager

We should consider refactoring things so it's all contained in 1 contract

@danoctavian we have this wrapping logic in the ynEigenDepositAdapter currently.

danoctavian · 2024-09-18T17:13:22Z

src/ynEIGEN/TokenStakingNodesManager.sol

+    //----------------------------------  WITHDRAWALS  -------------------------------------
+    //--------------------------------------------------------------------------------------
+
+    function processPrincipalWithdrawals(


What if we don't do this at all in ynEigen?

The reason this was a thing in ynETH is because

we had to take fees on withdrawals - therefore we had to insert fee collection logic on the withdrawal path + the trusted input on how much was rewards

there was accounting to change for each StakingNode (allocated staked eth needed to change)

The principal withdrawals came out in 32 ETH chunks (no further granularity) - therefore we had to have the "withdraw" and "reinvest" breakdown

As far as i can tell 1,2,3 don't apply here, but there is some accounting inside each of the TokenStakingNode instances.

What if simply have a role there that is able to do this and is constrained to send things to the vault, and leave this contract fully unchanged?

it will act on each TokenStakingNode as it does to trigger withdrawals and queuedWithdrawals

we may need to store just a reference to the redemptionAssetVault since the staking nodes query the Manager for data

Withdraw/Reinvest breakdown does have a use-case actually, that's reallocation.

That's fair.

Thinking how we can simplify the code as much as we can

danoctavian · 2024-09-18T17:33:13Z

src/ynEIGEN/TokenStakingNodesManager.sol

+    function initializeV2(
+        address _redemptionAssetsVault,
+        address _withdrawer
+    ) external onlyRole(DEFAULT_ADMIN_ROLE) reinitializer(2) notZeroAddress(_redemptionAssetsVault) {


Given the experience with ynETH and the issues with this, we should not use

onlyRole(DEFAULT_ADMIN_ROLE)

and call the initialize as part of upgradeAndCall().

Need to think carefully through this but i believe it would be correct.

danoctavian · 2024-09-18T17:54:01Z

src/ynEIGEN/EigenStrategyManager.sol

+
+                uint256 strategyWithdrawalQueueBalance;
+                uint256 queuedShares = node.queuedShares(strategies[asset]);
+                if (queuedShares > 0)


let's please use { } all the time, eliminates the possibility of making a block scope mistake.

danoctavian · 2024-09-18T18:24:11Z

Main thoughts here:

Instead of making changes to TokenStakingNodesManager,

If any reinvest/withdraw logic is still maintained we hold it in in EigenStrategyManager.

_wrapIfNeeded would also only be in EigenStrategyManager, to not add logic specific to that in TokenStakingNode.

function deallocateTokens(IERC20 _token, uint256 _amount) external onlyTokenStakingNodesManager

becomes

function deallocateTokens(IERC20 _token, uint256 _amount) external onlyYieldNestStrategyManager {

And TokenStakingNodesManager does not interfere with withdrawal.

Thinking along the lines of to be able to have TokenStakingNodesManager/TokenStakingNode something we can take and attach to other protocols without it being overloaded with functionality.

Logically, withdraw/reinvest belongs in the strategy.

danoctavian · 2024-09-18T22:05:33Z

Something like this. Let me know what you think!

johnnyonline · 2024-09-19T07:18:02Z

Main thoughts here:

Instead of making changes to TokenStakingNodesManager,

If any reinvest/withdraw logic is still maintained we hold it in in EigenStrategyManager.

_wrapIfNeeded would also only be in EigenStrategyManager, to not add logic specific to that in TokenStakingNode.

function deallocateTokens(IERC20 _token, uint256 _amount) external onlyTokenStakingNodesManager

becomes

function deallocateTokens(IERC20 _token, uint256 _amount) external onlyYieldNestStrategyManager {

And TokenStakingNodesManager does not interfere with withdrawal.

Thinking along the lines of to be able to have TokenStakingNodesManager/TokenStakingNode something we can take and attach to other protocols without it being overloaded with functionality.

Logically, withdraw/reinvest belongs in the strategy.

moved withdrawal logic to strategy manager 92540f4 (and removed comments here 4731a19).

refactoring _wrapIfNeeded next.

danoctavian · 2024-09-24T14:51:42Z

src/ynEIGEN/LSDWrapper.sol

+        }
+    }
+
+    // ============================================================================================


move toUserAssetAmount logic in here

danoctavian · 2024-09-24T14:58:18Z

src/ynEIGEN/LSDWrapper.sol

+    // ============================================================================================
+
+    /// @inheritdoc IWrapper
+    function wrap(uint256 _amount, IERC20 _token) external returns (uint256, IERC20) {


let's change the wrapper as discussed to be called with DELEGATECALL to avoid the double token transfer as discussed

which eliminates forceApproe

not as straight forward as it seemed, since wstETH doesnt allow to specify a sender, it would try to get funds from the tx.origin. discussing options on Discord.

Here i think you mean msg.sender not tx.origin

And what happens in this scenario is the Wrapper is msg.sender for wstETH (unlike the case with an external ibrary which still makes the contract that does DELEGATE call be the msg.sender still since the lib is not an actual contract).

And this makes it expect that funds are in there.

I want to follow up on this myself, i did a PoC and it worked for me for some reason.. might have had an error.

danoctavian · 2024-09-24T15:01:01Z

src/ynEIGEN/EigenStrategyManager.sol

+                uint256 strategyWithdrawalQueueBalance;
+                uint256 queuedShares = node.queuedShares(strategies[asset]);
+                if (queuedShares > 0) {
+                    strategyWithdrawalQueueBalance = toUserAssetAmount(asset, strategies[asset].sharesToUnderlyingView(queuedShares));


Change this loop to call toUserAssetAmount only once by summing

the results of queuedShares = node.queuedShares(strategies[asset]); and

uint256 strategyBalance = toUserAssetAmount( asset, strategies[asset].userUnderlyingView((address(node))) ); ```

danoctavian · 2024-09-24T15:17:28Z

src/ynEIGEN/RedemptionAssetsVault.sol

+            }
+        }
+
+        emit AssetTransferred(ETH_ASSET, msg.sender, to, amount);


the even should have the first value of amount, this one is potentially 0

danoctavian · 2024-09-24T15:20:45Z

src/ynEIGEN/RedemptionAssetsVault.sol

+     * @param amount The amount of assets to transfer.
+     * @dev Requires the caller to be the redeemer and the contract to not be paused.
+     */
+    function transferRedemptionAssets(address to, uint256 amount, bytes calldata /* data */) public onlyRedeemer whenNotPaused nonReentrant {


WE can move forward with strategy and explain the distribution strategy in the natspec

danoctavian · 2024-09-24T22:48:26Z

src/ynEIGEN/EigenStrategyManager.sol

-                stakedBalances[j] += strategyBalance;
+
+                uint256 strategyWithdrawalQueueBalance;
+                uint256 queuedShares = node.queuedShares(strategies[asset]);


we can do a bundled read for:

queuedShares and withdrawnAsset that returns a tuple so we don't do 2 separate calls to node

danoctavian · 2024-09-26T22:19:49Z

src/ynEIGEN/EigenStrategyManager.sol

+                }
+
+                stakedBalances[j] +=
+                    wrapper.toUserAssetAmount(asset, strategyBalance + strategyWithdrawalQueueBalance) +


for this optimization to be worthwhile, this contract call needs to be moved outside the inner loop.

Right now we still do 1 call per inner loop to wrapper.toUserAssetAmount(asset, strategyBalance + strategyWithdrawalQueueBalance)

To optimize this you need to rely on the property that

wrapper.toUserAssetAmount(asset, X) + wrapper.toUserAssetAmount(asset, Y) == wrapper.toUserAssetAmount(asset, X + Y)

Therefore you sum the uncoverted amounts inside the inner loop and convert the sum outside of the inner loop exactly once.

done here be925af

johnnyonline · 2024-09-27T13:38:44Z

implement caching for the getStakedAssetsBalances
complete the unit tests

danoctavian · 2024-09-29T14:25:49Z

src/ynEIGEN/EigenStrategyManager.sol

+    IRedemptionAssetsVaultExt public redemptionAssetsVault;
+    IWrapper public wrapper;
+
+    // @todo - struct


agreed struct also:

we could do the following:

strategiesBalance[strategy] = _strategiesBalance;
strategiesWithdrawalQueueBalance[strategy] = _strategiesWithdrawalQueueBalance

are stored as a sum. of the 2 things since they get converted together

using the Wrapper. This can be stored as as a uint128.

strategiesWithdrawnBalance gets stored as the other uint128. Thus both packed in the same slot. Make sure when writing that the slot gets written exactly one time (do the update of both at the same time).

danoctavian · 2024-10-02T09:20:50Z

src/ynEIGEN/EigenStrategyManager.sol

+        uint256 stakedBalance;
+        uint256 withdrawnBalance;
+    }
+    mapping(IStrategy => StrategyBalance) public strategiesBalance;


@xhad we cache the staked balances here

danoctavian · 2024-10-02T09:21:46Z

src/ynEIGEN/EigenStrategyManager.sol

+    /// @dev On a slashing events, users will have an incentive to call this function, to decrease the exchange rate.
+    /// @param asset The asset for which the balances are to be updated.
+    /// @param strategy The strategy for which the balances are to be updated. If not provided, we search for the strategy associated with the asset.
+    function updateTokenStakingNodesBalances(IERC20 asset, IStrategy strategy) public {


@xhad this updates the cached balances

in theory this can have another optimization done and that is:

take a list of assets as parameter and process them all in a single loop.

and hat woul entail we make versions of getQueuedSharesAndWithdrawn(strategy, asset)

that takes arrays as paramers and returns parameters

let's leave this for later i dont think there's enough time now

Deployment/yn ls de withdrawals

johnnyonline changed the base branch from main to danoctavian/sc-1162/implement-generalized-withdrawal-queue-manager September 9, 2024 16:36

danoctavian requested review from gearcat0, danoctavian and xhad September 17, 2024 12:38

danoctavian reviewed Sep 18, 2024

View reviewed changes

src/ynEIGEN/TokenStakingNode.sol Outdated Show resolved Hide resolved

danoctavian reviewed Sep 18, 2024

View reviewed changes

johnnyonline added a commit that referenced this pull request Sep 19, 2024

fix: #164 (comment)

7b35647

danoctavian reviewed Sep 24, 2024

View reviewed changes

danoctavian force-pushed the yneigen-withdrawals branch from 37d5cf6 to 24b72b5 Compare September 25, 2024 00:58

danoctavian reviewed Sep 26, 2024

View reviewed changes

danoctavian reviewed Sep 29, 2024

View reviewed changes

danoctavian reviewed Oct 2, 2024

View reviewed changes

danoctavian and others added 14 commits October 26, 2024 16:24

remove default calculation for unknown assets

d1345b6

adjust convertToAssets

bf08df0

add correct negation

8803efd

add test for previewReedeem unknown asset

187fd59

feat: schedule batch test

b9de00f

feat: add ynlsdadapter upgrade

b59be43

feat: add initialize eigenStrategyManager to batch

9eee371

feat: update withdrawal fee

99d82e7

add approval regardless of token

c9c9ad0

chore: holesky deployment

525fb6b

chore: holesky deployment with last commit

bc87545

feat: last changes

ec37bba

fix YnUpgrader to use timelock for proxies and correct roles

494f9ed

set test fee back to 0

4d66d0b

danoctavian force-pushed the yneigen-withdrawals branch from 156693d to 4d66d0b Compare October 26, 2024 13:30

danoctavian added 13 commits October 26, 2024 17:23

add deploy output mainnet + broadcast file

50e4e7b

add missing TokenStakingNode impl

17cb9d3

add ProxyAdmin addresses

43ab866

relax test assertion to 5 wei difference

d279bd2

add new implementations for YnLSDe

5fc832f

add parsing and verifying of withdrawal modules proxies

ec300e2

add ynLSDe withdrawals contracts for holesky

ab4b5f5

add all verifications to YnEigenVerifier

70e4dab

add new msig YNnWithdrawalsYnEigen

a217164

redemption assets vault PAUSER_ROLE is admin.ADMIN

630149e

add upgraded addresses and new contracts for withdrawals to ynLSDe

4babac0

Merge pull request #186 from yieldnest/deployment/ynLSDe-withdrawals

e9ff24a

Deployment/yn ls de withdrawals

cleanup solhint issues

9b4a2d6

danoctavian force-pushed the yneigen-withdrawals branch from 81c829b to 9b4a2d6 Compare October 29, 2024 23:32

danoctavian merged commit 7805347 into release-candidate Oct 29, 2024
1 check failed

		@@ -291,6 +291,9 @@ contract WithdrawalQueueManager is IWithdrawalQueueManager, ERC721EnumerableUpgr

		uint256 unitOfAccountAmount = calculateRedemptionAmount(request.amount, redemptionRate);

		// zero out the last decimal place

feat: ynEIGEN withdrawals #164

feat: ynEIGEN withdrawals #164

Conversation

johnnyonline commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

danoctavian commented Sep 18, 2024 • edited Loading

danoctavian commented Sep 18, 2024 • edited Loading

johnnyonline commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyonline commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian Sep 18, 2024 •

edited

Loading

danoctavian Sep 18, 2024 •

edited

Loading

danoctavian Sep 18, 2024 •

edited

Loading

danoctavian Sep 18, 2024 •

edited

Loading

danoctavian commented Sep 18, 2024 •

edited

Loading

danoctavian commented Sep 18, 2024 •

edited

Loading