-
Notifications
You must be signed in to change notification settings - Fork 100
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: batch operator id conversions #248
Conversation
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.
LG
@@ -178,4 +178,25 @@ contract OperatorStateRetriever { | |||
} | |||
return quorumBitmaps; | |||
} | |||
|
|||
function getBatchOperatorId( |
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.
These two methods will revert if any of the ID/address doesn't exist?
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 written, these methods currently will not revert, but instead return bytes32(0)
as the ID of any operator who has never registered, or address(0)
for any operatorID that has never been used.
relevant code:
https://github.com/Layr-Labs/eigenlayer-middleware/blob/feat/batch-operatorid-view/src/BLSApkRegistry.sol#L275-L276
https://github.com/Layr-Labs/eigenlayer-middleware/blob/feat/batch-operatorid-view/src/RegistryCoordinator.sol#L796-L804
@jianoaix what would your preferred behavior here be?
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.
Yep, that's more desirable! Could we put this in the comment of the function?
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.
Looks fine. Would be good to resolve the thread around reversions + add some documentation, prior to merging.
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
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.
FYI, the 2 automated test failures are unreleated to this PR and are getting fixed in a separate PR.
Adds two view functions to OperatorStateRetriever: