-
Notifications
You must be signed in to change notification settings - Fork 431
feat(release): redistribution #1355
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AllocationManager
redistribution support (#1346)2145f11
to
5f342d7
Compare
ypatil12
requested changes
May 22, 2025
ypatil12
reviewed
May 22, 2025
0xClandestine
commented
May 22, 2025
wadealexc
reviewed
May 22, 2025
a518026
to
58c1bb9
Compare
wadealexc
requested changes
May 22, 2025
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.
Before I forget, we need some stuff ASAP (before audit at the very least):
- Update interface docs for ALM.slashOperator to reflect any functional changes as well as the new return values
- Update contract docs for ALM/DM/SM/etc
- New/updated methods in ALM
- Updated method in DM
- New/updated methods in SM
- Create contract docs for SlashingWithdrawalRouter or any other new contracts
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 22, 2025
wadealexc
reviewed
May 23, 2025
wadealexc
reviewed
May 23, 2025
wadealexc
reviewed
May 23, 2025
wadealexc
reviewed
May 23, 2025
wadealexc
reviewed
May 23, 2025
wadealexc
reviewed
May 23, 2025
wadealexc
reviewed
May 23, 2025
**Motivation:** Slashing was deployed with using a `v` prefix, thus we're simply going to drop the prefix moving forward. **Modifications:** Prefix removed, and tests updated. **Result:** SemVerMixin no longer requires a `v` prefix.
**Motivation:** Current implementation is broken by rebase tokens. **Modifications:** - Rename SWR -> `SlashEscrowFactory`. - Add factory logic that deploys clones unique to their slash ID. **Result:** Funds will now be stored in clone contracts unique to their slash ID.
**Motivation:** We want to resolve any review issues that arise. **Modifications:** - Use larger of strategy or global delay: @non-fungible-nelson - Fix storage overrides noted from `Deprecated_OwnableUpgradeable` @wadealexc - Use `EnumerableSet` instead of `EnumerableSetUpgradaeable` since it doesn't contain storage - Add missing event in `initialize()`. - Prevent `address(0)` during `createRedistributableOperatorSets` for event sanitation. - Improve check legibility @wadealexc **Result:** Current review concerns have been addressed.
**Motivation:** Storage checker didn't have ALM added. Also we needed to fix the deprecated ownable mixing. **Modifications:** Fix mixing to inherit from `ContextUpgradeable`. Add ALM to storage-diff.json. **Result:** Correct storage checks. --------- Co-authored-by: Yash Patil <[email protected]>
**Motivation:** We want to use internal getters wherever possible for style. **Modifications:** - Use `getRedistributionRecipient` in `isOperatorRedistributable` - Update `isOperatorRedistributable` to get all allocated/registered sets and then check if slashable and redistributable set for each - More comprehensive unit tests **Result:** Cleaner code + tests passing
…ove poc code (#1397) **Motivation:** Burn shares naming is confusing, since shares are burnOrRedistributable **Modifications:** - For the new withdrawal path, call it `burnOrRedistributable`, so `burnOrRedistributableSharesIncreased` or `burnOrRedistributableSharesDecreased` - Bring back `burnableSharesDecreased` event for the legacy burn path - Rename `_operatorSetBurnableShares` to `_burnOrRedistributableShares` - Fix the storage gap, since `_burnOrRedistributableShares` is a mapping pointing to an enumerable map, not an enumerable map - Remove POC withdrawal queue code from the DM **Result:** Cleaner Redistribution code
**Motivation:** We have several code size optimizations in the DM/ALM. These are not needed anymore with the ownable deprecation. We can add in future upgrade if we want. **Modifications:** Remove all internal `_check` and modifiers. Make `slashOperatorShares` in the DM return to the original non-arrayified method. **Result:** Smaller code diff for redistribution.
**Motivation:** We want to ensure we have full coverage unit tests for `SlashEscrowFactory` and `SlashEscrow` in preparation for audits. **Modifications:** - Adds checks for all view methods. **Result:** Brings coverage up to 100% for all branches/fns/etc.
**Motivation:** We want to minimize the diff between slashing. **Modifications:** Revert calldata/memory types in DM. **Result:** Smaller diff
**Motivation:** Currently, there's a bug in the `SM` where if you loop through the burnable shares queue, you may not clear all due to swap and pop of an Enumerable Map. Furthermore, we also are constrained by a token transfer taking too much gas and blocking transfer out of funds. **Modifications:** - Iterate backwards on `decreaseBurnOrRedistributableShares ` - Overloaded `decreaseBurnableShares` with a version to pass in an index. This function will escrow a single share (called by above too). Now, we do not need a max strategy per opSet requirement - Unit tests for both `increaseBurnOrRedistributableShares` and `decreaseBurnOrRedistributableShares` - Added the following introspection: -- `getBurnOrRedistributableShares(operatorSet, slashId) returns (Strategy[] Strats, uint256[] shares) -- `getBurnOrRedistributableShares(operatorSet, slashId, strategy) returns (shares) -- `getBurnOrRedistributableCount(operaotrSet, slashed) returns (count)` **Result:** Correct code with unit tests
**Motivation:** The escrow delay currently always you to complete escrows for a portion of strategies if there exists a strategy with a larger delay. This makes our codebase more complex. We also want to have a view function for offchain cronjob that needs to be called only once. **Modifications:** - Update `releaseEscrow` to obey the maximum delay across all strategies for a slash - Add a `getBurnOrRedistributionDelay` view function - Add a convenience view function `getPendingEscrows` for offchain burn job. This returns all pending operatorSets, and their associated redistribution status, slashIds, and completeBlocks - Standardize `uint32` for delay everywhere - Make `deploySlashEscrow` a public function **Result:** Simpler & correct code.
**Motivation:** We use `burnOrRedistributable` everywhere. Let's just use escrow instead. Much simpler. **Modifications:** `burnOrRedistributable -> escrow`. **Result:** Better readability. --------- Co-authored-by: clandestine.eth <[email protected]>
**Motivation:** For clarity, let's deploy the `SlashEscrow` in `initiateEscrow`. **Modifications:** 1. In `initiateEscrow `, deploy the `SlashEscrow` if it hasn't been deployed 2. Also add the startBlock, opSet to pending, and slashId to pending in this if block **Result:** No more counterfactual. Less redundant updates for multiple strategies in a slashID.
**Motivation:** With the addition of non-arrified versions of `decreaseBurnableShares` and `releaseSlashEscrow` we want to avoid the use of `EnumerableSet.at()` as it reverts when attempting to retrieve indices that do not exist (> length). **Modifications:** - Use `EnumerableSet.tryGet` and `EnumerableSet.keys` instead which does not revert (but simply does nothing) if the index doesn't exist. - Rename `decreaseBurnOrRedistributableShares` -> `clearBurnOrRedistributableShares` to better reflect the functions purpose. **Result:** Protocol guarantees are maintained, and better naming.
**Motivation:** Minor nits. **Modifications:** - Add `slashOperator` return value natspec. - Remove `pause/unpauseRedistribution` codesize optimization. **Result:** Cleaner and shorter diff for audit.
**Motivation:** We want to ensure: 1. We don't need a max strategy in operatorSet count 2. Blacklisted ERC-20 tokens do not block the escrow release of _other_ tokens **Modifications:** 1. Added a `releaseEscrowByStrategy` functionality, which takes in an strategy & releases that specific strategy 2. Added helper methods that both `releaseEscrow` and `releaseEscrowByStrategy` use 3. Cleaned up some latent DM POC changes **Result:** Handling all edge cases :) --------- Co-authored-by: clandestine.eth <[email protected]>
**Motivation:** Small optimization found. **Modifications:** Checks whether a strategy has already been added via .set() return value rather than a separate .contains() check. **Result:** More optimal code.
**Motivation:** We want consistent style across the repo **Modifications:** 1. Use modifier to gate access control 2. Don't use burn solo anywhere 3. Don't use virtual functions if not needed **Result:** Cleaner code
**Motivation:** Redistribution nits from @nadir-akhtar **Modifications:** - Reduce diff - Style updates - Clearer comments **Result:** Cleaner code
**Motivation:** We want to update the smart contract docs for redistribution. **Modifications:** Updates docs for `AllocationManager`, `DelegationManager`, `StrategyManager`, `StrategyBase` and `EigenPodManager`. Also creates a new doc file for `SlashEscrow`. We also update interfaces and some functionality for clarity **Result:** Docs for redistribution. --------- Co-authored-by: clandestine.eth <[email protected]>
**Motivation:** We want integration test coverage for the redistribution relaese. **Modifications:** - Fixed view revert bug (see comment bellow). - Added timing tests (cannot release escrow before delay, can after) **Result:** Integration test coverage.
**Motivation:** We need to sanity test that slashes completed prior to redistribution can be burned after redistribution. **Modifications:** - Add redistribution upgrade test which tests: 1. A slash prior to redistribution can have its shares burned after the upgrade 2. A slash prior to redistribution can have its shares burned prior to upgrade & storage is cleared - Remove all User_m2/m1 functionality **Result:** Basic upgrade test.
**Motivation:** Add redistribution bindings. **Modifications:** Bindings **Result:** Up to date bindings
**Motivation:** Redistribution upgrade script. **Modifications:** **Deployment Process:** 1. `SlashingEscrowFactory` and `SlashEscrow` are two _new_ deployed contracts a. We deploy a new proxyAdmin to upgrade the `SlashingEscrowFactory`. The owner of this proxyAdmin is the community multisig 3. Upgrades the following contracts: a. DelegationManager b. AllocationManager c. Strategymanager d. EigenPodManager e. EigenStrategy f. StrategyBeacon g. StrategyBaseTVLLimits (pre-longtail strats) **Zeus Config Updates:** In Zeus config, we add a `GLOBAL_SLASH_DELAY` value. On all testnets (preprod, testnet, testnet-hoodi, tesnet-sepolia) this value is 5 blocks (1 min). On mainnet this value is 28,800 (4 days in blocks) Run `zeus env show <environment>` to double check. **Important Checks:** 1. Ensure the `_globalBurnOrRedistributionDelay` is properly set 2. Upgradability of the `SlashEscrowFactory` is controlled by a new `SlashEscrowProxyAdmin`. The owner of this contract is the `CommunityMultisig`. **Result:** Updated deploy scripts. --------- Co-authored-by: clandestine.eth <[email protected]> Co-authored-by: bowenli86 <[email protected]>
**Motivation:** Changelog for redistribution **Modifications:** Update `CHANGELOG-1.5.0.md` **Result:** Documented release notes. --------- Co-authored-by: clandestine.eth <[email protected]>
a7673f1
to
da4de65
Compare
ypatil12
approved these changes
May 30, 2025
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.
LGTM
All changes addresses via security review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Update core contracts to support redistribution.
Modifications:
We add a new contract,
SlashingWithdrawalRouter
, which handles the redistribution withdrawal queue.AllocationManager:
Deprecated_OwnableUpgradeable
createRedistributableOperatorSet
function, which creates an operatorSet with a 1-time settableredistributionRecipient
. Normal operator sets have aredistributionRecipient
of the default Burna addressslashOperator
now aggregates all magnitudes and then make a single call to theDelegationManager
for a slash--
getRedistributionRecipient
: gets theredistributionRecipient
of an operatorSet. Non-redistributing operatorSets will return thedefaultBurnAddress
--
isRedistributingOperatorSet
: whether an operatorSet is redistributable--
getSlashCount
: the number of slashes an operatorSet has done--
isOperatorRedistributable
: whether an operator is registered to a redistributable operatorSetDelegationManager:
slashOperator
to take inslashId
andoperatorSet
.ShareManager (SM, EPM):
IncreaseBurnableShares
function is updated to take in anoperatorSet
,slashId
StrategyBase:
amountOut
upon withdrawal. This enables programmatic handling of redistributed fundsSemVerMixin:
majorVersion
General Size Modifications:
AllocationManager
andDelegationManager
inherit fromDeprecated_OwnableUpgradeable
, which saves ~400B from both of these contracts.DelegationManager
andAllocationManager
use internal methods to conserve space. We likely can get rid of these modifications due to theDeprecated_OwnableUpgradeable
handling the majority of theseResult:
After your change, what will change.