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

Validator manager commands for the Keymanager APIs #6261

Open
wants to merge 62 commits into
base: unstable
Choose a base branch
from

Conversation

chong-he
Copy link
Member

Issue Addressed

Proposed Changes

Add commonds to lighthouse vm to support the Keymanager API to manage local keystores, i.e., list, import and delete

Additional Info

Building on top of @pahor167 PR: #5347

@michaelsproul michaelsproul added the v6.0.0 New major release for hierarchical state diffs label Aug 16, 2024
@eserilev
Copy link
Collaborator

eserilev commented Aug 16, 2024

If it's not too much of a hassle it'd be cool to define the new cli flags here w/ clap derive. Alternatively, I can make a separate PR to migrate to derive once this is merged

EDIT: I think we can just migrate to clap derive later after this PR is merged. thanks for taking a look CK!

@chong-he chong-he added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 16, 2024
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

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

This looks very clean!
I do have a few suggestions for you which range from easy fixes to large rewrites 😁

validator_manager/src/list_validators.rs Outdated Show resolved Hide resolved
validator_manager/src/list_validators.rs Outdated Show resolved Hide resolved
validator_manager/src/delete_validator.rs Outdated Show resolved Hide resolved
validator_manager/src/delete_validator.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

There's a decent amount of code duplication between this and the import_validators.rs file.

It would be nice if would could merge the two into the same command, with maybe a --use-standard-format flag or something in order to specify which format we are using (next level would be detecting which format it is automagically. I believe we can also make certain flags mutually exclusive so we can disable the ones that are not applicable for each format.

This may not be worth the complexity, but it's just quite unintuitive (imo) to have an import subcommand and then also an import-standard subcommand.

@chong-he chong-he added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Sep 2, 2024
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs val-client Relates to the validator client binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants