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

Make Churner work on v2 smart contracts #213

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

jianoaix
Copy link
Contributor

@jianoaix jianoaix commented Jan 26, 2024

Why are these changes needed?

In the v2 contracts, there is no pubkey compendium registration before registering the operator. As a result, we cannot convert operatorID to address.

The reason we did this was to avoid spams. However, since we set the rate limit to 5mins per request, it should be safe enough to counter the spams.

We may come up with stronger measures if this isn't the case (e.g. requiring a eth address with min balance).

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@mooselumph
Copy link
Collaborator

Unfortunately, I don't think this design works because it doesn't enforce any connection between the operatorAddress and the PublicKeys.

In particular, it allows for the operator to register with the churner using one operatorAddress and PubKey (one with a lot of stake) and then call the RegisterOperator contract method with a different address.

I think the simplest solution may be to include both the address and the operatorId in the messaage signed by the churner. However, this may require smart contract changes.

@0x0aa0, @gpsanant

@jianoaix
Copy link
Contributor Author

Good point! The message it signs right now is exactly the request hash (of the ChurnRequest), so it looks straightforward to extend from here.

@mooselumph
Copy link
Collaborator

Good point! The message it signs right now is exactly the request hash (of the ChurnRequest), so it looks straightforward to extend from here.

Just make sure to check the smart contract side. The message will need to be updated there as well, so we probably need @0x0aa0, @gpsanant involved for this to make it into the middleware contracts.

@jianoaix
Copy link
Contributor Author

@jianoaix jianoaix requested a review from 0x0aa0 January 29, 2024 18:43
@jianoaix jianoaix changed the title Drop the pubkey compendium check as it doesn't exist now Make Churner work on v2 smart contracts Jan 30, 2024
@@ -588,7 +589,7 @@ func (t *Transactor) CalculateOperatorChurnApprovalDigestHash(
}
return t.Bindings.RegistryCoordinator.CalculateOperatorChurnApprovalDigestHash(&bind.CallOpts{
Context: ctx,
}, operatorId, opKickParams, salt, expiry)
}, operatorAddress, operatorId, opKickParams, salt, expiry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0x0aa0 This is where we depend on the onchain change

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm!

Copy link
Collaborator

@mooselumph mooselumph left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Contributor Author

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

Going to merge this. We may test and fix as we have all pieces ready in the m2 branch.

@@ -33,6 +33,8 @@ message ChurnRequest {
// - If any of the quorum fails to register, this entire request will fail.
// The IDs must be in range [0, 255].
repeated uint32 quorum_ids = 5;
// The Ethereum address (in hex like "0x123abcdef...") of the operator.
string operator_address = 6;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be better to place at the top. It's just about having to change the field number, which is breaking.

@jianoaix jianoaix merged commit 5e911e0 into Layr-Labs:m2-mainnet-contracts Jan 30, 2024
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