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.

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: reregistration delay #246

Merged
merged 9 commits into from
Apr 30, 2024
Merged

feat: reregistration delay #246

merged 9 commits into from
Apr 30, 2024

Conversation

0x0aa0
Copy link
Contributor

@0x0aa0 0x0aa0 commented Apr 26, 2024

Adds a delay for registration to any quorum after an operator has been ejected

@0x0aa0 0x0aa0 marked this pull request as ready for review April 26, 2024 17:10
Copy link
Contributor

@ChaoticWalrus ChaoticWalrus left a comment

Choose a reason for hiding this comment

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

I believe there's a ~medium-severity issue here -- see my comment.
I would definitely like to resolve this discussion before merging.

@0x0aa0 0x0aa0 requested review from ChaoticWalrus and anupsv April 27, 2024 02:15
Comment on lines +373 to +377
if(
operatorInfo.status == OperatorStatus.REGISTERED &&
!quorumsToRemove.isEmpty() &&
quorumsToRemove.isSubsetOf(currentBitmap)
){
Copy link
Contributor

Choose a reason for hiding this comment

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

so there's still some weird race condition where an operator can deregister from a subset of quorums to avoid being ejected for more, right?
I guess I can accept that, but it would certainly be nice to resolve if there's an easy solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we can just take the submitted 'quorumNumbers' and actually pass on a combination ('and'ing) of them with the operator's current quorums in the subsequent call?

@ChaoticWalrus
Copy link
Contributor

I could see merging this now. Am pushing a bit on fixing the remaining race condition here, at least if it's easy enough, because it would be a nice improvement.
Otherwise LGTM

@0x0aa0 0x0aa0 requested a review from ChaoticWalrus April 29, 2024 20:37
* @dev The owner can eject operators without recording of stake ejection
*/
function ejectOperators(bytes32[][] memory _operatorIds) external returns (uint32[] memory){
function ejectOperators(bytes32[][] memory _operatorIds) external {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wdyt about refactoring the ejection logic

  1. That doesn't enforce the rate limit that is callable by the owner
  2. That is callable by the ejector and enforces the rate limit

This should clean up some of the branchy conditionals in this function

@0x0aa0 0x0aa0 merged commit 74c86d2 into dev Apr 30, 2024
2 of 3 checks passed
@0x0aa0 0x0aa0 deleted the feat/reregistration-delay branch April 30, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants