-
Notifications
You must be signed in to change notification settings - Fork 453
feat: remove semver + minor optimizations #1654
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
Conversation
ProtocolRegistry + one-click pause allProtocolRegistry + remove semver
7cc2203 to
e23d5c9
Compare
ProtocolRegistry + remove semver42af1f1 to
ecc69cf
Compare
fb163fb to
54b1047
Compare
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 may not want to update the Eigen token
fbcb025 to
d60643a
Compare
be7c1dc to
d17a568
Compare
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 changes to remove SemVerMixin look good from a correctness standpoint.
That said, taking a step back, I'm not sure removing SemVerMixin for the 300 bytes return is ideal. It's a strange property for some contracts to have a version attributed to them, and for others not to.
If we're to make a change here, I recommend we have some replacement, e.g. a registry with either a global protocol version or versions for each contract. That way, we don't have a strange situation where some contracts have version while others don't.
| ) external onlyWhenNotPaused(PAUSED_MODIFY_ALLOCATIONS) { | ||
| // Check that the caller is allowed to modify allocations on behalf of the operator | ||
| // We do not use a modifier to avoid `stack too deep` errors | ||
| require(_checkCanCall(operator), InvalidCaller()); |
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.
Curious to know what the need for these changes to the PermissionControllerMixin and related contracts was in this PR is, as it seems unrelated to the SemVerMixin. We lose some granularity in error specification here -- not the worst situation as InvalidPermissions is fairly explanatory for a given function, but recommend we save this for a different PR.
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.
srry squashed, commit history not as useful now
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.
Do we still have this update in this PR?
|
|
||
| // If we're not newly registered, check that the caller (not the delegationManager) is authorized to set the allocation delay for the operator | ||
| if (!newlyRegistered) { | ||
| require(_checkCanCall(operator), InvalidCaller()); |
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 not use require(_canCall() here? When would we use _checkCanCall as opposed to require(_canCall())?
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.
codesize
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.
How much do we save? IMO not worth to have two different patterns of use within the same contract (or even across the protocol). We also do canCall not in a modifier below:
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.
eg.
| require(_canCall(params.operator) || _canCall(params.avs), InvalidCaller()); |
4830de3 to
c1caa7d
Compare
**Motivation:** The `AllocationManager` contract was hitting the 24KB bytecode size limit, which would have blocked deployment. We needed a solution to reduce the contract size while maintaining backwards compatibility with existing integrations that call view functions on `AllocationManager`. **Modifications:** - Created a new `SplitContractMixin` that uses a fallback to delegate unmatched function calls via `delegatecall` to a secondary contract - Split `AllocationManager` into two contracts: the main contract handles state-mutating operations, while `AllocationManagerView` handles all read-only view functions - Both contracts inherit from `AllocationManagerStorage` to share the same storage layout, enabling the view contract to read from the main contract's storage - Updated `AllocationManager` constructor to accept and store the `AllocationManagerView` address - Modified all deployment scripts (devnet, local, integration) to deploy both proxies and implementations - Updated `ExistingDeploymentParser`, test harnesses, and unit/integration tests to work with the split architecture **Result:** - The `AllocationManager` is now about ~ 4.8KB under the 24KB limit with room to grow. - The `AllocationManagerView` contract has 16KB of available space for future view functions. TODO: Add a ci check (I don't have privs but the code is written locally) 1) Get list of all contracts ending in View.sol in the repo. 2) Use forge inspect abi and check all mutability fields == view|pure. 3) Basic search over the file to see if sstore or delegatecall is used on a for additional sanity. --------- Co-authored-by: eigenmikem <[email protected]> squash squash feat: split alm wip wip wip wip wip wip wip unit tests passing remove extsload wip tests passing chore: git mv storage to folder wip ci docs todo cleanup ci ci rename `secondHalf` rm extsload move storage add protocol registry to zeus squash feat: split alm wip wip wip wip wip wip wip unit tests passing remove extsload wip tests passing chore: git mv storage to folder wip ci wip wip wip wip wip remove extsload wip tests passing wip passing perf: wrap modifier logic to save codesize chore: forge fmt refactor: remove semver (#1641) **Motivation:** We want to consolidate semver logic into a single purpose built contract. **Modifications:** - Removed `SemVerMixin` import from all contracts. **Result:** More codesize savings. make fmt refactor: expand `ProtocolRegistry` to list all contracts + pause rm gas snapshots rm extsload rebase ci revert eigen changes remove stale scripts
c1caa7d to
96e51bc
Compare
090df60
into
release-dev/slashing-ux-improvements
**Motivation:** We want to reduce codesize throughout our core contracts. **Modifications:** - Removed `SemVerMixin` from every contract that doesn't inherit `SignatureUtilsMixin`. - Added non-revert `checkCanCall` function `PermissionController` for space savings **Result:** ~300 bytes of codesize saved per contract. --------- Co-authored-by: Yash Patil <[email protected]>
Motivation:
We want to reduce codesize throughout our core contracts.
Modifications:
SemVerMixinfrom every contract that doesn't inheritSignatureUtilsMixin.checkCanCallfunctionPermissionControllerfor space savingsResult:
~300 bytes of codesize saved per contract.