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

Add path to Keystore #10152

Merged
merged 7 commits into from
Feb 14, 2022
Merged

Conversation

rootulp
Copy link
Contributor

@rootulp rootulp commented Jan 29, 2022

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Adds path to the Keystore struct because it is a required JSON field per EIP-2335

Which issues(s) does this PR fix?

Fixes #10107

@rootulp rootulp requested a review from a team as a code owner January 29, 2022 05:04
@CLAassistant
Copy link

CLAassistant commented Jan 29, 2022

CLA assistant check
All committers have signed the CLA.

@rootulp
Copy link
Contributor Author

rootulp commented Feb 2, 2022

Thanks for the feedback @rkapka. I addressed so this PR is ready for re-review.

Copy link
Contributor

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

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

looks like you need to run the gazelle prefix "github.com/prysmaticlabs/prysm/validator/keymanager" command in the validators keymanager folder to fix the linter, must be from added dependencies for test. thanks for the contribution LGTM after that.

Ran `bazel run //:gazelle -- fix`
@rootulp
Copy link
Contributor Author

rootulp commented Feb 12, 2022

Thanks for the tip @james-prysm !

On a slightly related note, brew install bazel installs bazel 5.0.0 and it looks like this project is using 4.2.2 per

4.2.2

Should I open a new issue to track upgrading to bazel 5.0.0 because it doesn't look like such an issue exists yet (search).

@james-prysm
Copy link
Contributor

Thanks for the tip @james-prysm !

On a slightly related note, brew install bazel installs bazel 5.0.0 and it looks like this project is using 4.2.2 per

4.2.2

Should I open a new issue to track upgrading to bazel 5.0.0 because it doesn't look like such an issue exists yet (search).

let me double check with the team I'm sure they know about bazel 5.0.0 but might not be ready to upgrade yet ( assumption) i'd hold off on it. happy to continue the conversation on discord if interested.

@rootulp
Copy link
Contributor Author

rootulp commented Feb 13, 2022

Sounds good, I'll hold off on creating a GH issue.

@rootulp rootulp requested a review from james-prysm February 13, 2022 06:30
@rkapka
Copy link
Contributor

rkapka commented Feb 14, 2022

This PR should definitely not update Bazel.

@james-prysm
Copy link
Contributor

will test out UI with this change so see backwards compatability before merging

Copy link
Contributor

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

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

Approving this PR after testing out the following

  1. backup account returns a keystore with path property
  2. import keystore with new path property still works
  3. import keystore without new path property still works
  4. backup keystore without new path property returns a keystore with path property

@james-prysm james-prysm merged commit f550a96 into prysmaticlabs:develop Feb 14, 2022
@rootulp
Copy link
Contributor Author

rootulp commented Feb 14, 2022

This PR should definitely not update Bazel.

Agreed. My intention was to create a separate GH issue to track upgrading to Bazel 5.0.0 but defer to you / the rest of the maintainers on how best to track that effort.

Thanks for reviewing and merging! Keep up the great work on Prysm.

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.

Validator accounts backup missing path as defined in EIP-2335
4 participants