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: support remote signer in voluntary exit command #6132

Merged
merged 18 commits into from
Dec 1, 2023

Conversation

eth2353
Copy link
Contributor

@eth2353 eth2353 commented Nov 27, 2023

Motivation

The desire to submit voluntary exit messages for validators loaded not directly in the Lodestar VC, but loaded in a remote signer.

Description

Voluntary exits were only possible for validators imported locally. It was not possible to sign or submit voluntary exit messages for validators loaded in a remote signer.

This PR makes it possible to exit validators loaded in a remote signer.

The command to run looks like this, it supports all the flags for local signers (e.g. --pubkeys) as well:

validator voluntary-exit --network=goerli --externalSigner.url=http://signer:9000 --externalSigner.fetch=true --beaconNodes=http://beacon:9596

This was much easier to implement than I'd expected, speaks to the quality of your codebase!

Testing

Added an e2e test for this.
I have also manually tested running the command on Goerli and mainnet successfully.

I had to create a keystore for the interop validator secret keys. I used the Holesky genesis fork version & validators root because I couldn't locate the values to use. I don't think that should cause any issues but I can update that if needed if you point me to the values to use.

This is a draft PR, even though it's ready for review, I wanted to ask you guys if you even want to support thos.

@CLAassistant
Copy link

CLAassistant commented Nov 27, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, I think this is a nice feature to have.

Looks pretty good overall, left a few minor remarks.

packages/cli/src/cmds/validator/voluntaryExit.ts Outdated Show resolved Hide resolved
packages/cli/test/e2e/voluntaryExitRemoteSigner.test.ts Outdated Show resolved Hide resolved
packages/cli/test/e2e/voluntaryExitRemoteSigner.test.ts Outdated Show resolved Hide resolved
packages/cli/test/e2e/voluntaryExitRemoteSigner.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

Thanks @eth2353 for the updates. The refactoring looks great, I pushed a few changes to clean up some minor things (hope you don't mind).

Generally, looks good to me. Let's mark this as ready for review to get feedback from the team

packages/cli/src/cmds/validator/voluntaryExit.ts Outdated Show resolved Hide resolved
@nflaig nflaig marked this pull request as ready for review November 30, 2023 11:36
@nflaig nflaig requested a review from a team as a code owner November 30, 2023 11:36
Co-authored-by: Nico Flaig <[email protected]>
@eth2353
Copy link
Contributor Author

eth2353 commented Nov 30, 2023

I don't mind at all, I am not that experienced within the TypeScript ecosystem so I expected to have done a few things suboptimally.

When I made this change initially, I actually wanted to skip writing a test for this since the actual changes in the code are so minimal (and trivial) 😄

But in the end I'm glad I didn't skip it, I think the refactored external signer test-util came out nice indeed!

nflaig
nflaig previously approved these changes Nov 30, 2023
nflaig
nflaig previously approved these changes Nov 30, 2023
dapplion
dapplion previously approved these changes Dec 1, 2023
Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

PR looks generally correct! Thanks for the contribution and the e2e test 👌

@nflaig nflaig dismissed stale reviews from dapplion and themself via fcc8493 December 1, 2023 11:41
@nflaig nflaig enabled auto-merge (squash) December 1, 2023 11:43
@nflaig nflaig merged commit 5911739 into ChainSafe:unstable Dec 1, 2023
15 checks passed
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.13.0 🎉

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.

5 participants