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

prysmctl: validator exit #11515

Merged
merged 25 commits into from
Oct 17, 2022
Merged

prysmctl: validator exit #11515

merged 25 commits into from
Oct 17, 2022

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Sep 29, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This feature migrates the command to exit a validator to the prysmctl and is part of a greater effort to support message signing in the prysmctl.

Which issues(s) does this PR fix?

Fixes #11478

out of scope but related #11451

Other notes for review

@james-prysm james-prysm added Web3Signer Web3Signer related tasks UX cosmetic / user experience related prysmctl labels Sep 29, 2022
@james-prysm james-prysm self-assigned this Sep 29, 2022
@@ -16,7 +16,7 @@ import (
const backupPromptText = "Enter the directory where your backup.zip file will be written to"

func accountsBackup(c *cli.Context) error {
w, km, err := walletWithKeymanager(c)
w, km, err := walletWithKeymanager(c, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels a bit weird to pass nil into a function call like this, what is the purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree I was trying to think of a better way to clean this up but just had a hard time figuring out how I can pass the genesis validators root here.

at first, I had the node client abstracted away in the function. so that if web3signer was used it would make an API call to get the genesis information but I had a hard time unit testing it, but the more I think of it maybe I can just abstract the logic out into its own function, and not have the unit test run the connection...

I also tried doing something like walletWithKeymanager(c *cli.Context, conns ...*grpc.Connection)but had a hard time mocking the genesis call. open for other suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of using the connection i could also hardcode a map like potuz suggested and get it from the cli, but i was thinking other chains that use prysm like gnosis might want this capability too without having exclusive support

@kasey
Copy link
Contributor

kasey commented Oct 17, 2022

I see you trying to play with the capitalization of prysmctl - I will fight you to the death!

@james-prysm james-prysm changed the title PrysmCTL: validator exit prysmctl: validator exit Oct 17, 2022
@james-prysm james-prysm marked this pull request as ready for review October 17, 2022 19:56
@james-prysm james-prysm requested a review from a team as a code owner October 17, 2022 19:56
rauljordan
rauljordan previously approved these changes Oct 17, 2022
rauljordan
rauljordan previously approved these changes Oct 17, 2022
@james-prysm james-prysm merged commit df694aa into develop Oct 17, 2022
@delete-merged-branch delete-merged-branch bot deleted the validator-exit-improvement branch October 17, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prysmctl UX cosmetic / user experience related Web3Signer Web3Signer related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do an exit of your validators if you use a external signer
3 participants