Feat: combined operator registration flow #100
Merged
Conversation
This file contains 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
…stryCoordinator also make a version of `RegistryCoordinator. registerOperator` that accepts these inputs new function passes these inputs on appropriately TODO: remove old `registerOperator` function and fix associated tests
…ubkey registration inputs also fix tests to use the new method
note: code is currently not compiling due to a stack-too-deep error
error here:
src/RegistryCoordinator.sol:254:69:
|
254 | _deregisterOperator(operatorKickParams[i].operator, quorumNumbers[i:i+1]);
wadealexc
reviewed
Dec 12, 2023
wadealexc
reviewed
Dec 12, 2023
| @@ -118,33 +116,33 @@ contract BLSApkRegistry is BLSApkRegistryStorage { | |||
| ); | |||
|
|
|||
| // H(m) | |||
| BN254.G1Point memory messageHash = getMessageHash(msg.sender); | |||
| BN254.G1Point memory messageHash = getMessageHash(operator); | |||
There was a problem hiding this comment.
I wonder if the message hash should use the registry coordinator address rather than the BLSApkRegistry, since the registry coordinator is "the AVS" (as far as the Dmgr is concerned, anyway)
There was a problem hiding this comment.
i guess we're registering within the context of the AVS though so maybe it makes sense as-is
There was a problem hiding this comment.
i guess we're registering within the context of the AVS though so maybe it makes sense as-is
yeah I agree with this.
will make the message signature more EIP712-compatible, but I think using the BLSApkRegistry address here is OK
wadealexc
reviewed
Dec 12, 2023
wadealexc
reviewed
Dec 12, 2023
wadealexc
reviewed
Dec 12, 2023
wadealexc
approved these changes
Dec 12, 2023
see reasoning here #100 (comment) this allows undoing a previous fix for a stack-too-deep error, which... helps make the logic a little clearer / easier to parse here.
NOTE: this changes the IBLSApkRegistry interface to make `registerBLSPublicKey` return a bytes32 object
`pubkeyRegistrationParams` => `params` see discussion here: #100 (comment)
|
merging this PR |
ChaoticWalrus
merged commit ffc25f7
into
feat-adjust-compendium-for-per-avs-deployment
3 checks passed
stevennevins
pushed a commit
that referenced
this pull request
Dec 18, 2023
see reasoning here #100 (comment) this allows undoing a previous fix for a stack-too-deep error, which... helps make the logic a little clearer / easier to parse here.
stevennevins
pushed a commit
that referenced
this pull request
Dec 18, 2023
`pubkeyRegistrationParams` => `params` see discussion here: #100 (comment)
stevennevins
added a commit
that referenced
this pull request
Dec 18, 2023
* fix: remove husky command that only needs to be run once * update nonSignerForQuorumIndex outside of loop * chore: add MacOS indexing file to gitignore * chore: correct addresses for M2 Goerli deployment switch these addresses over from pre-prod environment to the addresses that EigenDA is using now * chore: test out relative import * chore: all relative imports * chore: fix remaining relative imports * fix: update missed import * fix: additional missed imports * Update README.md to avoid confusing phrasing (#75) * style: use for loop indices when the index is just an index * refactor: Use a signed delta value in StakeRegistry to remove tons of unneeded code * style: use over , and simplify * feat: only update total history for nonzero delta, and dont push update if last update was in current block * style: use uint256 in registry coordinator * test: fix broken tests for StakeRegistry changes generally i just removed testing of specific stake histories in favor of testing net outcomes. we can revisit these tests in a few weeks. * refactor: wip refactor to move "createQuorum" to the registry coordinator * style: use for loop indices when the index is just an index * refactor: Use a signed delta value in StakeRegistry to remove tons of unneeded code * style: use over , and simplify * feat: only update total history for nonzero delta, and dont push update if last update was in current block * style: use uint256 in registry coordinator * test: fix broken tests for StakeRegistry changes generally i just removed testing of specific stake histories in favor of testing net outcomes. we can revisit these tests in a few weeks. * fix: fix compilation issues and tests * refactor: wip refactor for index and blspubkey registries to simplify naming and logic * style: pull out common logic to a helper method * style: pulled additional logic out into a helper method * refactor: simplify registry coord state variable names and clean logic and comments for deregistration * fix: enforce invariant that existing indice have nonzero length history. also fix tests * style: shorten state variable and function naming in registry coordinator * style: shorten state variable and function naming in stake registry * style: remove unused index registry function * refactor: pk compendium stores operator pubkeys and can look them up ... which means pubkey parameters for various functions/structs can be removed * style: shorten state variable and function names to be more consistent with other registry contracts * style: removed redundant check and swapped param order to be consistent with corresponding state lookup * style: remove redundant checks from pubkey compendium * fix: replace borked state variable * refactor: move updateStakes to registry coordinator * fix: fixes several compiler issues still running into "stack too deep" * fix: stack too deep issue addressed also addressed review comments: - natspec comments in IStakeRegistry - incorrect boolean in StakeRegistry * fix: actually commit natspec changes * style: stakeregistry naming is more consistent with other contracts * style: rename upper bound in bitmap utils, and rename Operator to OperatorInfo * fix: operator has minimum if weight is equal to minimum * test: update tests to support refactor * style: reword comments and remove duplicate check * style: remove unneeded parenthesis and fix _registerOperator naming * style: rename OperatorStakeUpdate struct to StakeUpdate, and swap event to match (#61) * cleanup imports (#78) * chore: merge master into m2-mainnet (#79) --------- Co-authored-by: steven <[email protected]> Co-authored-by: gpsanant <[email protected]> Co-authored-by: ChaoticWalrus <[email protected]> Co-authored-by: Gautham Anant <[email protected]> * build: point eigenlayer gitmodule to m2-mainnet (#80) * ci: add rule to run CI on m2-mainnet branch * chore: remove old tree file (#83) Co-authored-by: steven <[email protected]> * chore: rename several contracts and fix interfaces and tests (#95) - BLSRegistryCoordinatorWithIndices is now RegistryCoordinator - BLSOperatorStateRetriever is now OperatorStateRetriever - BLSPubkeyRegistry is now BLSApkRegistry * chore: copy BLSPublicKeyCompendium storage and functions into BLSApkRegistry * chore: delete BLSPublicKeyCompendium and associated interface requires changing a bunch of files to make things work. I had to comment out a few calls in tests (now marked with a `TODO`) to get this past the compiler. Will have to do additional cleanup to get this ready for review. * feat: create harness for BLSApkRegistry and use it in tests These changes appear to fix all existing tests. I've done what I can to at least comment specifically where harnessed functions are used, as this pattern could give some false assurance and I'd like to avoid that. * chore: delete unused mock file I created this mock while trying to sort out compilation issues and test failures This mock is no longer being used anywhere, so I am deleting it for cleanup * chore: cleanup unused file and reduce mutability of test utils functions this commit fixes a ton of compiler warnings on build, in particular. * feat: make BLSApkRegistry. registerBLSPublicKey only callable by RegistryCoordinator also make a version of `RegistryCoordinator. registerOperator` that accepts these inputs new function passes these inputs on appropriately TODO: remove old `registerOperator` function and fix associated tests * chore: remove `RegistryCoordinator.registerOperator` method without pubkey registration inputs also fix tests to use the new method * feat: create struct for pubkey registration params note: code is currently not compiling due to a stack-too-deep error error here: src/RegistryCoordinator.sol:254:69: | 254 | _deregisterOperator(operatorKickParams[i].operator, quorumNumbers[i:i+1]); * fix: address stack-too-deep issue in RegistryCoordinator * chore: delete unused index see reasoning here #100 (comment) this allows undoing a previous fix for a stack-too-deep error, which... helps make the logic a little clearer / easier to parse here. * feat: optimization for fetching operatorId NOTE: this changes the IBLSApkRegistry interface to make `registerBLSPublicKey` return a bytes32 object * chore: shorten variable name that is quite clear from context `pubkeyRegistrationParams` => `params` see discussion here: #100 (comment) * feat: use EIP712 for the pubkeyRegistrationMessageHash this commit also moves the definition to the RegistryCoordinator contract and updates the function naming to be clearer * chore: add NatSpec to getter function * Feat(M2-Mainnet): StakeRegistry pull updates per quorum (#62) * feat: initial implementation w/o timestamp * feat: enforce stake updates for quorum in sigcheck * fix: update operator stake for just one quorum Additionally - renamed updateOperatorsPerQuorum to updateOperatorsForQuorum - sort by operator addresses instead of operatorIds * fix: requested changes and optimizations - sorted by ascending address instead of operatorId - sanitization checks on quorumNumbers, added quorumsAllExist modifier - performing `OperatorStatus.REGISTERED` checks now - Removed uneccesary bitmapToBytes calls and doing bytes slicing for quorumNumbers - using internal function and scoping to fix stack-too-deep * refactor: move quorumUpdateTimestamp to reg coord * refactor: remove modifier in favor of helper method (#64) * fix: initializedQuorumBitmap fix small off by 1 error, for example of quorumCount = 1,3 1 << 1 = 2, bit rep is 10 1 << 1 - 1 = 1, bit rep is 01 1 << 3 = 8, bit rep is 1000 1 << 3 - 1 = 7, bit rep is 0111 We should be subtracting by 1 instead of 2 here to get the bitmap * fix: bitshifting error, needs to shift first before subtracting * fix: moved 1 weeks to a constant for now May change this in the future to be configurable * chore: require statement * refactor: move shared logic to `_updateOperator` * refactor: added status check to `_updateOperator` * feat: Delegation.withdrawalDelayBlocks in sigcheck * fix: use blocknumber instead of timestamp * fix: unused constant and require error * feat: timestamp requirement is able to be toggled * fix: interface imports * chore: renamed to staleStakesForbidden * chore: typo with BlockNumber * fix: rebase remove deleted file * fix: prevOperatorAddress not being updated --------- Co-authored-by: Alex <[email protected]> * feat: issue templates (#87) Co-authored-by: steven <[email protected]> * fix typo BLSPublicKeyCompendium.md * fix typo BLSSignatureChecker.md * fix typo StakeRegistry.md * fix typo README.md * chore: remove ServiceManagerBase and add RegistryCoordinator owner (#98) * Feat: Add AVS/Operator Registration Support in RegistryCoordinator (#99) * chore: fix merge artifacts / broken calls these weren't caught in merging but were causing compiler errors * chore: re-implement stack-too-deep fix I previously implemented this fix, then reverted it as not needed. Now upon merging with m2-mainnet, `registerOperatorWithChurn` has an additional input. This is reintroducing the same stack-too-deep error so I am reintroducing the same fix (one less local var) * chore: de-duplicate code into an internal function see suggestion here: #102 (comment) saves ~0.25kb in code size * fix: actually use the `operator` input to `_getOrCreateOperatorId` also reduce memory copying operations (I think, at least) in the `registerOperator` function specifically, only copy the `numOperatorsPerQuorum` returned by the internal `_registerOperator` function * feat: add optimizer runs count to foundry config * chore: remove redundant check and return data The `operatorId` in this check is already fetched from the `blsApkRegistry` With the other changes, this return data is no longer necessary at all * chore: fewer memory operations(?) I believe this change cuts down on the memory copying being done here * fix: reduce optimizer runs to meet contract code size limits Likely not the ideal fix, but it gets the job done for the moment, at least. * chore: rename file to reflect it only being used in tests `RegistryCoordinatorHarness.sol` -> `RegistryCoordinatorHarness.t.sol` * chore: delete unused (memory) variable * fix: have the harness import Test so it gets ignored in build sizes * fix: remove circular dependency (#108) * fix: address multiple issues in BLSSignatureChecker, optimize, and clean (#104) * fix: swap sign on sig checker withdrawal delay check * fix: tiny_scalar_mul in BN254 and additional cleanup - unify validateXAtBlockNumber style checks in registries - checkSignatures changes below - rename variables to be more readable - validate input lengths for quorum and nonsigner params - create signingQuorumBitmap using method with additional validation to avoid duplicates - clarify commenting and style to be more consistent with rest of codebase - cache state to avoid lookups in loop - move total stake query to initial loop * fix: refactor checkSignatures to remove an unneeded loop - also optimizes BN254.hashG1Point - also refactors checkSignatures to avoid negating pubkeys multiple times * fix: stale stakes check should care about referenceBlockNumber * fix: remove repeated storage read from loop * chore: remove TODOs * docs: add wip docs and clean main README (#96) * docs: add docs very wip * docs: wip add docs and main README * docs: wip main README * docs: more wip * docs: get docs to a good ish state * docs: address feedback * chore: add eigenda goerli deployment (#111) * feat: concentrate eigenlayer avs interactions into service manager (#109) * feat: reintroduce IServiceManager & ServiceManagerBase These are useful concepts to form the single interaction point where AVSs push data to EigenLayer. The previous version focused on (unused) Slashing interactions, whereas this version focused on the newer registration interactions. TODO: modify the RegistryCoordinator to call the ServiceManagerBase instead of calling the DelegationManager directly. * feat: integrate RegistryCoordinator with ServiceManager(Base) achieves the goal of a single interaction point with EigenLayer (per AVS). all calls which push data to EigenLayer core are now forwarded via the ServiceManager(Base) * fix: use correct addresses in test setup + functions simple follow-up commit to fix breaking tests from last commit * chore: restore optimizer_runs to 200 other changes in this PR remove enough from the size of the RegistryCoordinator contract to make this possible. 200 runs is a wider industry default + this should slightly decrease gas costs for users. we can switch back to 100 in the future if we ever need to * chore: add `setMetadataURI` function to IServiceManager interface * feat: add operator strategy indexing to service manager (#110) --------- Co-authored-by: Yash Patil <[email protected]> * fix: correctly set bit in stake registry (#112) * feat: add restakeable strategies to service manager for indexing (#113) * chore: resolve imports * fix: remove eigenlayer deployer * chore: additional relative imports * chore: remove whitespace diff * chore: clean up remappings * chore: remove whitespace diff * chore: stragglers --------- Co-authored-by: steven <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: gpsanant <[email protected]> Co-authored-by: ChaoticWalrus <[email protected]> Co-authored-by: Gautham Anant <[email protected]> Co-authored-by: Bowen Li <[email protected]> Co-authored-by: wadealexc <[email protected]> Co-authored-by: quaq <[email protected]> Co-authored-by: Michael Sun <[email protected]> Co-authored-by: iwantanode <[email protected]> Co-authored-by: Yash Patil <[email protected]>
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.
Opening this is a "sub-PR" / add-on to #97
Now that the pubkey compendium has been merged into other middleware contracts / is on a per-AVS basis, we can make the call to register a new pubkey permissioned + make a single function that combines what was previously 2 calls.