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

chore: deprecate earningsReceiver #562

Merged
merged 6 commits into from
May 29, 2024
Merged

Conversation

8sunyuan
Copy link
Collaborator

@8sunyuan 8sunyuan commented May 23, 2024

Description
IDelegationManager.OperatorDetails.earningsReceiver is not being used as part of payments so to avoid confusion with the upcoming release of Payments we are deprecating this address slot in the OperatorDetails struct.

Since a non-zero address of earningsReceiver is being used to check whether an address is an operator or not, the isOperator(operator) function is now changed to simply check delegatedTo[operator] == operator. That is a operator is an operator iff they are delegated to themselves.

Changes

  • renamed earningsReceiver to __deprecated_earningsReceiver
  • isOperator checks an address is self delegated now
  • _delegate() no longer checks the staker is not delegated and that operator is an operator. It assumes these checks are made prior to the internal call. This was needed to fix operator registration because of how isOperator was changed.
  • bunch of unit test fixes and docs
  • deleted commented out tests in Delegation.t.sol

Comment on lines -101 to -104
require(
_operatorDetails[msg.sender].earningsReceiver == address(0),
"DelegationManager.registerAsOperator: operator has already registered"
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should default to setting the value of this deprecated field to zero.

We dont need to keep this require here, but if we plan to repurpose this field later, we need to make sure people aren't already setting it to a nonzero value.

Copy link
Contributor

Choose a reason for hiding this comment

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

it has already been set to a nonzero value by all existing users 😞
one option I suggested was setting it to the operator's own address, but I think setting it to zero could also be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since all existing registered operators have nonzero values I thought its best to not make assumptions on whatever value this field is and let it be whatever people have it set to.

If we use this field later, we need to allow for a period for operators to update it and then assume afterwards that it's configured.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah right i missed that we currently require this to be nonzero. yikes!

if we use this field later... we could do that or we might want to migrate and set it to 0. or set it to a sane default.

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

See comment above - if we want to leave the option to use this field, we need to ensure it stays zeroed out.

@ChaoticWalrus
Copy link
Contributor

Looks like this PR breaks a Prover rule
https://prover.certora.com/output/83341/72a015ef94bd4d09b5b8422ed3c36059?anonymousKey=f746a2604541cb957aa0857fc48b48d920cf0af8
operatorCannotUnregister is breaking on a call to because delegateTo, ultimately because EigenPodManager.podOwnerShares is resulting in a HAVOC summary; I'm pretty sure this can be fixed by using a NONDET summary for this call in the spec file

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'm personally OK with this, but I would like to see the Prover error fixed prior to merging, as it seems like merging this as-is would cause our CI to fail consistently

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

LGTM

@8sunyuan 8sunyuan merged commit 8905de3 into dev May 29, 2024
13 of 16 checks passed
@8sunyuan 8sunyuan deleted the chore/deprecate-earners-address branch May 29, 2024 15:13
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