Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: slashing registry coordinator #361

Merged
merged 11 commits into from
Jan 24, 2025
Merged

refactor: slashing registry coordinator #361

merged 11 commits into from
Jan 24, 2025

Conversation

8sunyuan
Copy link
Collaborator

@8sunyuan 8sunyuan commented Jan 21, 2025 •
edited
Loading

Changes:

  • removed isUsingOperatorSets() and just read storage bool directly
  • setting isOperatorSetAVS to default to true in initialize
  • Moved all logic to SlashingRegistryCoordinator, RegistryCoordinator now only contains the legacy M2 interfaces registerOperator, registerOperatorWithChurn, deregisterOperator as well as enableOperatorSets for migration.
  • registerOperator hook call from the ALM was missing the maxOperatorCount check in the NORMAL RegistrationType
  • Cleaned up some internal interfaces and moved the _getOrCreateOperatorId call into registerOperator.
  • renamed deregistration function calling back to ALM as _forceDeregisterOperator for clarity, also added this to ejectOperator as it was missing there.

Register/Deregister Hook changes:

  • _beforeRegisterOperator Hook to allow for any pre-register logic in _registerOperator
  • _afterRegisterOperator Hook to allow for any post-register logic in _registerOperator
  • _beforeDeregisterOperator Hook to allow for any pre-deregister logic in _deregisterOperator
  • _afterDeregisterOperator Hook to allow for any post-deregister logic in _deregisterOperator
    • `RegistryCoordinator overrides this to call the ServiceManager to deregister if the operator is no quorums anymore.
  • Because the call to the ServiceManager for registering operators requires a signature, this was not used in a hook and was added to the external functions themselves in RegistryCoordinator.registerOperator and RegistryCoordinator.registerOperatorWithChurn

Note: will run a forge fmt src after finished review

ypatil12
ypatil12 reviewed Jan 21, 2025
View reviewed changes
src/SlashingRegistryCoordinator.sol Outdated
) external override onlyWhenNotPaused(PAUSED_REGISTER_OPERATOR) {
require(isOperatorSetAVS, OperatorSetsNotEnabled());
for (uint256 i = 0; i < operatorSetIds.length; i++) {
require(!isM2Quorum[uint8(operatorSetIds[i])], OperatorSetsNotSupported());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we not an M2Quorum by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are, only the inheriting RegistryCoordinator makes use of this check. Perhaps we can remove this check here and only have it in RegistryCoordinator and calling the super implementation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to assume all calls from the ALM are for "new" quorums and then remove this check

src/SlashingRegistryCoordinator.sol Outdated Show resolved Hide resolved
src/SlashingRegistryCoordinator.sol Outdated Show resolved Hide resolved
src/SlashingRegistryCoordinator.sol Outdated Show resolved Hide resolved
@8sunyuan 8sunyuan marked this pull request as draft January 22, 2025 17:08
ypatil12
ypatil12 reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Organizational comments

src/RegistryCoordinator.sol Outdated Show resolved Hide resolved
src/SlashingRegistryCoordinator.sol
bytes32 operatorId,
bytes memory quorumNumbers,
string memory socket
) internal virtual returns (RegisterResults memory results) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the logic here is basically the same as _registerOperator in the RegistryCoordinator. I think we can combine these. Th only thing we'd need to add is in the afterRegister hook on the RegistryCoordinator is to check the status and if registered then we call the SM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the _registerOperator function in RegistryCoordinator and reusing the logic. Rather than using the hook, I just added the SM call to the RegistryCoordinator itself since it requires an operatorSignature to be passed into the SM

src/SlashingRegistryCoordinator.sol Outdated
) external override onlyWhenNotPaused(PAUSED_REGISTER_OPERATOR) {
require(isOperatorSetAVS, OperatorSetsNotEnabled());
for (uint256 i = 0; i < operatorSetIds.length; i++) {
require(!isM2Quorum[uint8(operatorSetIds[i])], OperatorSetsNotSupported());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to assume all calls from the ALM are for "new" quorums and then remove this check

src/SlashingRegistryCoordinator.sol
return results;
}

function _registerOperatorWithChurn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also consolidate logic with the RegistryCoordinator for this function.

RegistryCoordinator just has to properly handle registration back to AVSD

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c462c88
reusing the churn logic here

@8sunyuan 8sunyuan force-pushed the slashing-registry-coordinator branch from c547c37 to 05abac6 Compare January 22, 2025 21:04
@8sunyuan
refactor: slashing registry coordinator
47b560d 
refactor: registry coordinator

refactor: remove sm calls

fix: tests wip
@8sunyuan 8sunyuan force-pushed the slashing-registry-coordinator branch from 05abac6 to 47b560d Compare January 22, 2025 22:41
@8sunyuan 8sunyuan marked this pull request as ready for review January 23, 2025 17:44
8sunyuan and others added 2 commits January 23, 2025 16:37
@8sunyuan
fix: call SM only based on m2 quorums (#369)
17c52df
@ypatil12
feat: update enableOperatorSets flow (#367)
57876ae 
* feat: update enableOperatorSets flow

* fix: dereg op check

* chore: renamed m2 quorums
@8sunyuan 8sunyuan merged commit 57f70ba into feat/slashing-release-branch Jan 24, 2025
5 of 6 checks passed
stevennevins added a commit that referenced this pull request Jan 28, 2025
@stevennevins @gpsanant
@shrimalmadhur @nadir-akhtar @0xClandestine @8sunyuan @ypatil12
feat: slashing release (#311)
11b0061 
* Update LICENSE (#285)

* chore: upgrade core to target operator set release

* chore: update mock contracts with latest interfaces (#293)

* chore: update mock contracts with latest interfaces

* chore: bump to latest operator set release commits

* chore: fixes for depedency bump

* feat: operator set migration-1-migration (#286)

* chore: checkout migration branch

* feat: implement migration function

* chore: update to release branch

* feat: operator set creation for each quorum number

* feat: migration with merge sorted array of operators and their quorums

* feat: operator set migration working

* chore: revert change from testing

* chore: revert change from testing

* chore: remove extra logging and commented out asserts

* chore: remove unused file

* fix: remove console logs

* refactor: to view functions

* chore: nit and remove unneeded function

* fix: remove duplication of looping for all the operators

* chore: remove comment

* feat: allow migrating in two transactions

* feat: finalization of migration

* chore: use string errors and fix migration issues

* chore: rename

* feat: use library for merge sort

* test: fuzz view function

* feat: operator set migration-2-create quorum (#287)

* chore: checkout migration branch

* feat: implement migration function

* chore: update to release branch

* feat: operator set creation for each quorum number

* feat: migration with merge sorted array of operators and their quorums

* feat: operator set migration working

* chore: revert change from testing

* chore: revert change from testing

* chore: remove extra logging and commented out asserts

* chore: remove unused file

* fix: remove console logs

* feat: create quorum post operator set migration

* test(wip): create quorum test adds new operator set

* test: migration create quorum

* refactor: to view functions

* chore: nit and remove unneeded function

* fix: remove duplication of looping for all the operators

* chore: remove comment

* feat: allow migrating in two transactions

* feat: finalization of migration

* chore: use string errors and fix migration issues

* chore: rename

* feat: use library for merge sort

* test: fuzz view function

* fix: updates from merge

* chore: use interface

* chore: bump operator set release dependency in core

* chore: updates from dependency bump

* feat: add natspec

* docs: add natspec

* feat: add checks operator was registered for quorums when migrating

* fix: resolve code size issue in RegistryCoordinator

* chore: update to latest core (#299)

* chore: update to latest eigenlayer-contracts feat/operator-set-release

* chore: add method to mock

* feat(op sets): update stakes when forceUnregister (#300)

* feat: update stakes handle direct deregistration on AVSDirectory

* test: update stake for quorum if operator directly unregistered from the AVSDirectory

* chore: simplify setup

* chore: simplify setup

* chore: make service manager immutable on stakeRegistry

* feat(op sets): register and deregister (#301)

* feat: register and deregister to operator sets

* fix: bytecode wrangling

* chore: shorter errors to reduce bytecode

* test: register after migration

* chore: remove stale todos

* chore: add back commented line

* chore: remain consistent with m2 events

* fix: side effect of merge from _deregister function

* fix: bytecode massaging

* chore: rename for clarity

* chore: remove comments and whitespace

* docs: add natspec to the library

* feat(op sets): upgrade and migrate script (#303)

* feat: upgrade and test pre prod upgrade and migration

* fix: add param introduced in merge

* chore: bump slashing core dependency (#312)

* chore: bump to slashing branch

* chore: bump compiler version

* fix: dep interface changes

* fix: compiler errors from interface changes and type changes

* fix: compiler errors

* chore: bump dependencies

* chore: bump core dependency and resolve issues

* chore: bump core dependency and fix compiler errors

* feat: integrate AllocationManager

* feat: add a slashing permission to the service manager

* chore: remove unneeded casting

* feat: implement a slasher permission and forward call to AllocationManager

* feat: add simiple slasher starting point

* chore: bump slashing magnitudes

* chore: bump core slashing-magnitudes branch

* feat: slasher templates / examples (#310)

* chore: bump to slashing branch

* chore: bump compiler version

* fix: dep interface changes

* fix: compiler errors from interface changes and type changes

* fix: compiler errors

* chore: bump dependencies

* chore: bump core dependency and resolve issues

* chore: bump core dependency and fix compiler errors

* feat: integrate AllocationManager

* feat: add a slashing permission to the service manager

* chore: remove unneeded casting

* feat: implement a slasher permission and forward call to AllocationManager

* feat: add simiple slasher starting point

* feat: slashers

* chore: change around slashed event

* fix: call dm

* feat: add proposal mechanism for updating slasher

* fix: set to completed instead of delete

* chore: use struct instead of params directly

* chore: clean up params more

* chore: simplify and organize files

* chore: cleanup logic and couple event with internal func

* fix: pass correct params

* chore: organize and add interface

* chore: nits

* chore: cleanup more nits

* fix: storage gap

* chore: nits refactor

* chore: go back to fulfill being onlySlasher

* test: fixes from core updates

* fix: use delegated stake per operator set instead of per AVS

* fix: update to 14 days

* feat: configurable lookahead and stake type

* chore: remove unused test util contracts (#319)

* feat: remove both option

* chore: remove unused test util contracts

* chore: remove diff

* feat: remove both option

* fix: storage gap remove one slot (#320)

* feat: track total slashable stake and total delegated stake per quorum (#317)

* feat: remove both option

* feat: total delegated stake and total slashable stake per quorum config

* test: resolve some breaking changes to tests

* chore: move stake type to file level definition

* chore: refactor loop

* test: add unit test for slashble stake quorum init

* test: assert on state and event

* test: delegated stake quorum and assertions

* fix: use libraries with only internal vis (#324)

* fix: revert making library function vis external

* fix: signature checker internal

* feat: avs registrar registration flow changes (#318)

* feat: remove both option

* feat: total delegated stake and total slashable stake per quorum config

* test: resolve some breaking changes to tests

* chore: bump core dependency

* chore: bump dependency

* chore: bump to latest slashing mags

* fix: creation of registry coordinator

* test: wip

* feat: integrate registrar interfaces

* test: add function to delegation mock to set operator status

* test: additional test case

* chore: bumping core dep and adding UAM (#325)

* test: use permission controller mock

* chore: label fuzz tests

* test: various fixes from config changes

* chore: remove comment

* test: fix permission controlled functions

* test: fix config issue in integration tests

* test: fix avs directory initialize

* feat: wip prevent m2 registration flows after migration

* feat: registration changes part 2

* chore: add note

* fix: remove handling of forceDeregistration

* fix: fix total delegated stake usage

* fix: integration tests

* test: fix remaining integration tests

* test: add back log check

* test: add additional tests for transition to operator sets

* test: add more test cases

* feat: record m2 quorums on migration

* chore: add note about churn support

* fix: prevent operator set registration changes for m2 quorums

* feat: require strings

* chore: add dev note and add require string

* chore: bump dependency for slashing mags updates (#329)

* fix: internal slashing security review (#332)

* docs: match file and library name, and add docstrings

* docs: update error strings to match function

* style: forge fmt + moving variables around + fixing error strings

* fix: enforce max quorum count for EjectionManager

* style: more error string fixes + formatting

* fix: correctly set StakeRegistryStorage gap

* style: more error string corrections + typos

* fix: revert cases

* chore: bump core dependency to pull in latest updates

* fix: slashing review fixes (#333)

* fix: withdrawal delay check

* fix: add operator set strategies (#334)

* fix: remove registerOperatorToOperatorSet interface function

* fix: update interface functions with alm interface

* fix: operator set strategies in ALM

* chore: remove todo

* fix: add strategies by stake registry

* fix: params for deregister

* chore: remove old migration functions

* chore: check quorum exists before setting params

* fix: deregister flow for operator set quorums

* test: fix test for DM withdrawal delay blocks

* chore: update all pragmas to 0.8.27 (#336)

* feat: custom require errors for registry coordinator (#337)

* feat: custom require errors for registry coordinator

* ci: update the ci to use the correct compiler settings

* chore: bump core dependency

* chore: bump forge-std

* docs: update readme with note on slashing (#341)

* chore: bump core fix iface changes (#352)

* chore: bump dep

* fix: iface changes and param names

* feat: ci storage reports (#347)

* feat: storage reports

* fix: storage report

* fix: storage report

* fix: ci

* fix: ci

* fix: ci

* fix: ci

* fix: ci

* fix: prevent calling enable operator sets twice and use internal function for checkALM

* chore: address review comments

* refactor: custom errors in middleware (#355)

* refactor: custom errors

* fix: unit tests

* chore: review remove setStakeType

* fix: wire up stake registry to call add and rm strategies for op set

* refactor: slashing UAM (#357)

* refactor: uam

* feat: add uam interfaces

* fix: lookahead period to blocks

* fix: tests

* chore: storage report

* feat: ci forge coverage (#349)

* feat: ci forge coverage

* fix: ci

* feat: add forge fmt to ci (#363)

* feat: ci forge fmt

* chore: forge fmt

* feat: add register with churn type (#360)

* feat: add register with churn type

* fix: tests for encoded registration type

* feat: add ejector support for operator sets

* refactor: handle operator set dereg

* fix: dereg on churn from ALM if needed

* refactor: slashing registry coordinator (#361)

* refactor: slashing registry coordinator

refactor: registry coordinator

refactor: remove sm calls

fix: tests wip

* fix: integration tests

* refactor: internal functions

* fix: remove unused block

* fix: comment out test for now

* build: fmt check run only on src contracts

* fix: call SM only based on m2 quorums (#369)

* feat: update enableOperatorSets flow (#367)

* feat: update enableOperatorSets flow

* fix: dereg op check

* chore: renamed m2 quorums

* fix: msg.sender -> operator

* chore: forge fmt

* fix: tests

---------

Co-authored-by: Yash Patil <[email protected]>

* fix: commented out view (#373)

* refactor: natspec + interface changes (#364)

* refactor: natspec + interfaces

refactor: natspec + interfaces

refactor: natspec + interfaces

docs: `RegistryCoordinator` natspec

chore: forge fmt

refactor: named mapping params

docs: natspec `BLSApkRegistry`

docs: natspec `BLSApkRegistry`

docs: natspec `BLSApkRegistry`

refactor: interface structure

refactor: separate `BLSSignatureChecker` storage

refactor: rename -> `IECDSAStakeRegistry`

docs: natspec

docs: natspec `ECDSAStakeRegistry`

docs: natspec `EjectionManager` + separate storage

docs: natspec

docs: natspec `IndexRegistry`

refactor: remove `IRegistry`

chore: forge fmt

chore: make storage-report

docs: natspec

refactor: remove `ISocketUpdater`

refactor: use

refactor: remove todo

refactor: `///` -> `/**`

refactor: improve comment

refactor: rename var

refactor: improve comment

* fix: compiling - tests failing

* refactor: test passing **mostly**

* refactor: add missing gap

* refactor: natspec

* fix: test - state changes moved out of internal

* chore: forge fmt

* refactor: reorganize errors

* refactor: remove override

* refactor: note avsd

* Yash/natspec - address comments (#372)

* feat: natpsec with inheritance

* feat: storage

* chore: format

* chore: fmt

* chore: format

* fix: test

* fix: test

---------

Co-authored-by: Michael Sun <[email protected]>
Co-authored-by: Yash Patil <[email protected]>

* chore: forge fmt

* feat: storage gap on socket registry

* chore: remove unused storage; update bindings

* chore: nuke storage report script for now on ci

---------

Co-authored-by: Gautham Anant <[email protected]>
Co-authored-by: Madhur Shrimal <[email protected]>
Co-authored-by: Nadir Akhtar <[email protected]>
Co-authored-by: clandestine.eth <[email protected]>
Co-authored-by: Michael Sun <[email protected]>
Co-authored-by: Yash Patil <[email protected]>
Co-authored-by: Michael Sun <[email protected]>
@stevennevins stevennevins deleted the slashing-registry-coordinator branch January 29, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ypatil12 ypatil12 ypatil12 left review comments

@stevennevins stevennevins stevennevins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@8sunyuan @stevennevins @ypatil12