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.

Sign up for GitHub

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

Jump to bottom

validator/accounts: don't ask for keystore path if specified #5641

Merged
merged 5 commits into from
Apr 27, 2020
Merged

validator/accounts: don't ask for keystore path if specified #5641

merged 5 commits into from
Apr 27, 2020

Conversation

q9f
Copy link
Contributor

@q9f q9f commented Apr 27, 2020

What type of PR is this?

Usability

What does this PR do? Why is it needed?

If you run a validator with --keystore-path it still asks you for a keystore path.

This PR improves usability in two ways:

  1. It does not ask you to confirm the path if you already specified one.
  2. It clarifies the wording that user interaction is necessary ("Please specify ...").

Which issues(s) does this PR fix?

N/A

Other notes for review

As discussed on Discord with @nisdas

@q9f q9f requested a review from a team as a code owner April 27, 2020 09:13
@q9f q9f requested review from shayzluf, 0xKiwi and nisdas April 27, 2020 09:13
@CLAassistant
Copy link

CLAassistant commented Apr 27, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@q9f q9f changed the title validator/accounts: don't ask for keystore path of specified validator/accounts: don't ask for keystore path if specified Apr 27, 2020
nisdas
nisdas previously approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

LGTM

terencechain
terencechain previously approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Thanks!

0xKiwi
0xKiwi suggested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@0xKiwi 0xKiwi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for doing this! I think with NewValidatorAccount inside the if statement, it appears creating new accounts won't work unless theres no path set.

@q9f q9f dismissed stale reviews from terencechain and nisdas via 0622e35 April 27, 2020 16:03
@q9f
Copy link
Contributor Author

q9f commented Apr 27, 2020

Well spotted 👍

@prylabs-bulldozer prylabs-bulldozer bot merged commit f19aa93 into prysmaticlabs:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xKiwi 0xKiwi 0xKiwi approved these changes

@nisdas nisdas nisdas left review comments

@terencechain terencechain terencechain left review comments

@shayzluf shayzluf Awaiting requested review from shayzluf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@q9f @CLAassistant @0xKiwi @terencechain @nisdas