-
Notifications
You must be signed in to change notification settings - Fork 23
Feat/slashing yn eth #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/slashing yn eth #220
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
* 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]>
Also fix some formtting issues
This is for test compat
On Holesky Failing tests: Encountered 1 failing test in test/scenarios/fork/ynETH/ProtocolUpgrade-Scenario.spec.sol:ProtocolUpgradeScenario Encountered 1 failing test in test/scenarios/fork/ynETH/Withdrawals.t.sol:M3WithdrawalsTest Encountered 1 failing test in test/scenarios/fork/ynETH/WithdrawalsWithRewards-Scenario.t.sol:M3WithdrawalsWithRewardsTest Encountered 2 failing tests in test/scenarios/ynEIGEN/Delegation.t.sol:YnEigenDelegationScenarioTest |
The |
* fix: Fix delegation tests * fix: Rename to token staking node * fix: Update actors
87a6d76
to
130fc17
Compare
@@ -665,10 +671,6 @@ contract StakingNodesManager is | |||
uint256 updatedTotalETHStaked = 0; | |||
IStakingNode[] memory allNodes = getAllNodes(); | |||
for (uint256 i = 0; i < allNodes.length; i++) { | |||
if (!allNodes[i].isSynchronized()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this deleted? can the node not go out of sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct here. this shouldn't be deleted as it introduce the attack vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/StakingNodesManager.sol
Outdated
} | ||
emit TotalETHStakedUpdated(updatedTotalETHStaked); | ||
totalETHStaked = updatedTotalETHStaked; | ||
// TODO: commenting this for now because getETHBalance() is not available in current deployed version of stakingNode on holesky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this break the deployment?
How does this even work?
If it's not available an upgrade strategy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't call updateTotalETHStaked
on holesky right now because of breaking changes by eigenlayer. Currently calling updatedTotalETHStaked
will revert. Also, stakingNode.getETHBalance
reverts on currently deployed version of holesky. Current holesky deployment is already broken. Now, if we upgrade StakingNodeImplementation
on holesky and if it calls getETHBalance
in initializeV3
, the upgrade will fail. So, for holesky, plan is to comment this out and upgrade StakingNodeImplementation and then call updateTotalETHStaked
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, understood i'm trying to see here how we don't have commented out code and things looking unfinished and in draft state when we deploy things
If this is ready for audit but we can't ship this branch to audit with commented out code
So i'm thinking here what is cleaner to do here, have a HoleskyStakingNodesManager.sol contract that just inherits and overrides this? to have the mainnet version be this contract and be clean for audit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. I will create different version of Holesky so that we don't have commented code for audits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created HoleskyStakingNodesManager.sol which inherits initializeV3.
test/integration/StakingNode.t.sol
Outdated
allocationManager.createOperatorSets(avs, createSetParams); | ||
allocationManager.updateAVSMetadataURI(avs, "ipfs://some-metadata-uri"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has to be done before creating the set:
/// @inheritdoc IAllocationManager
function createOperatorSets(address avs, CreateSetParams[] calldata params) external checkCanCall(avs) {
// Check that the AVS exists and has registered metadata
require(_avsRegisteredMetadata[avs], InvalidAVSWithNoMetadataRegistered());
for (uint256 i = 0; i < params.length; i++) {
OperatorSet memory operatorSet = OperatorSet(avs, params[i].operatorSetId);
// Create the operator set, ensuring it does not already exist
require(_operatorSets[avs].add(operatorSet.id), InvalidOperatorSet());
emit OperatorSetCreated(OperatorSet(avs, operatorSet.id));
// Add strategies to the operator set
bytes32 operatorSetKey = operatorSet.key();
for (uint256 j = 0; j < params[i].strategies.length; j++) {
_operatorSetStrategies[operatorSetKey].add(address(params[i].strategies[j]));
emit StrategyAddedToOperatorSet(operatorSet, params[i].strategies[j]);
}
}
}
The createOperatorSets checks that the avs has metadata. Updating it afterwards will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. This was introduced in new version. I will update it accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This branch has conflicts with base vranch that need to be resolved |
src/StakingNode.sol
Outdated
bytes32 withdrawalRoot = delegationManager.calculateWithdrawalRoot(withdrawals[i]); | ||
uint256 withdrawableShares = shares[i][0]; | ||
withdrawableSharesForWithdrawalRoot[withdrawalRoot] = withdrawableShares; | ||
queuedSharesAmount += withdrawableShares; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not gas optimized it keeps writing storage every iteration of the loop which is wasteful
queuedSharesAmount should be written to storage exactly once in this function. It's written once at the beginning to 0 and then once in every iteration.
Storage Write is the most expensive operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will make the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
src/StakingNode.sol
Outdated
* @param withdrawal The withdrawal struct | ||
* @return totalWithdrawableShareForWithdrawalRoot The total withdrawable shares for the withdrawal root | ||
*/ | ||
function _decreaseQueuedSharesOnCompleteQueuedWithdrawalAndGetTotalWithdrawableShare( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AndGetTotalWithdrawableShare
let's remove this from the name
and make a named return value like returns (uint256 totalWithdrawableShare)
if you'd like to have that name clarity there, accompanied by the right natspec.
The return value is not normally mentioned in the function name, which now makes this name very long, but normally named in the returns (..)
syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
src/StakingNode.sol
Outdated
* @dev Allows only a whitelisted address to configure the contract | ||
* @dev The amount of shares queued for withdrawal that were queued before the upgrade | ||
*/ | ||
uint256 public legacyQueuedSharesAmount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call this something more specific as its specific to the slashing upgrade
preELIP002QueuedSharesAmount ?
or any other code name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the naming of variable
return delegatedTo == delegationManager.delegatedTo(address(this)); | ||
} | ||
|
||
function synchronize(uint256 queuedShares, uint32 undelegateBlockNumber) public onlyDelegator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great to see this whole thing gone.
It was useful while it lasted since that's how the old eigenlayer worked but glad to see we can now just sync queued shares
src/StakingNode.sol
Outdated
withdrawableSharesForWithdrawalRoot[withdrawalRoot] = 0; | ||
queuedSharesAmount -= withdrawableSharesForWithdrawalRootValue; | ||
} else { | ||
// If the withdrawableSharesForWithdrawalRootValue are 0, that means withdrawal root doesn't exist in withdrawableSharesForWithdrawalRoot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this function assume that the function that calls it reverts if the withdrawal has already been completed or does not exist?
withdrawableSharesForWithdrawalRootValue is 0 if it was never set or is set here to 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. the if withdrawal has already completed or doesn't exist, the eigenlayer code will revert. https://github.com/Layr-Labs/eigenlayer-contracts/blob/v1.0.3/src/contracts/core/DelegationManager.sol#L527
If pendingWithdrawal is true, then only withdrawal gets completed in eigenlayer.
src/ynEIGEN/WithdrawalsProcessor.sol
Outdated
@@ -7,7 +7,7 @@ import {AccessControlUpgradeable} from | |||
"lib/openzeppelin-contracts-upgradeable/contracts/access/AccessControlUpgradeable.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test file is now very large at 2k lines
Is it possible to separate the tests here into logical units into multiple files to help with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/ynViewer.sol
Outdated
@@ -41,8 +41,9 @@ contract ynViewer is IynViewer { | |||
function withdrawalDelayBlocks(address _strategy) external view returns (uint256) { | |||
IDelegationManager _delegationManager = stakingNodesManager.delegationManager(); | |||
uint256 _minDelay = _delegationManager.minWithdrawalDelayBlocks(); | |||
uint256 _strategyDelay = _delegationManager.strategyWithdrawalDelayBlocks(IStrategy(_strategy)); | |||
return _minDelay > _strategyDelay ? _minDelay : _strategyDelay; | |||
// uint256 _strategyDelay = _delegationManager.__deprecated_strategyWithdrawalDelayBlocks(IStrategy(_strategy)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this commented out?
Final code ready for review should not have commented out blocks like this, they are either useful or they are removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the commented code because now withdrawalDelayBlocks
is same regardless of strategy. Is this contract used by frontend? Is it possible to change it's interface?
src/StakingNode.sol
Outdated
IEigenPodManager eigenPodManager = IEigenPodManager(IStakingNodesManager(stakingNodesManager).eigenPodManager()); | ||
|
||
if (delegatedTo == address(0)) { | ||
beaconChainSlashingFactor = eigenPodManager.beaconChainSlashingFactor(address(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please declare variables where they're used
beaconChainSlashingFactor is not used outside of this if block, so is beaconChainSlashingFactor
shouldn't be declared outside of it in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done the changes. moved beaconChainSlashingFactor
and eigenPodManager
in the block
@@ -545,18 +614,26 @@ contract StakingNode is IStakingNode, StakingNodeEvents, ReentrancyGuardUpgradea | |||
|
|||
function getETHBalance() public view returns (uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's please add natspec to this, it wasn't there to begin with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
|
||
// Shares are no longer queued; decrease what was queued for withdrawal | ||
queuedSharesAmount -= totalWithdrawalAmount; | ||
if (actualWithdrawalAmount != totalWithdrawableShares) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the deleted comment and cod is still valid to my knowledge
actualWithdrawalAmount may be < totalWithdrawalAmount in case of slashing
The slashing there was referring to beacon chain slashing
what happens here when there is beacon chain slashing?
How was this tested?
The current change looks like it doesn't account for beacon chain slashing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed on discord, this is correct because eigenlayer introduced beaconChainSlashingfactor.
src/StakingNode.sol
Outdated
uint256 withdrawableShares; | ||
bytes32[] memory fullWithdrawalRoots; | ||
|
||
if (delegatedTo == address(0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there an if clause here? Why not just compute withdrawableShares
in one way, why have branching here?
Unless there's a good reason to do it we should have only one codepath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
computation of withdrawableShares
depends on whether staker is delegated to some operator or not. So, it requires if clause for that check. If the staker is not delegated to operator, we just need to consider beaconChainSlashingFactor
. If staker is delegated to operator, we need to consider beaconChainSlashingFactor
and operatorSlashingFactor
. But if staker is delegated to operator, on queueing of withdrawal, the shares delegated to operator gets reduced by withdrawable share so that's easier way out. we can check this condition in eigenlayer codebase here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if it was delegated up until the previous block and now it has been undelegated? Why wouldn't you do the same computation?
What does it mean that's easier way out.
?
.gitmodules
Outdated
@@ -17,4 +17,4 @@ | |||
[submodule "lib/eigenlayer-contracts"] | |||
path = lib/eigenlayer-contracts | |||
url = https://github.com/yieldnest/eigenlayer-contracts | |||
branch = v1.1.1-v4.9.0 | |||
branch = v1.4.0-testnet-holesky-v4.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use v1.3.0-v4.9.0 instead given that 1.4 will not reach mainnet when the slashing upgrade occurs
No description provided.