Validator manager commands for the Keymanager APIs #6261
Conversation
…anager-standard-keystore
| let validators: Vec<ValidatorSpecification> = serde_json::from_reader(&validators_file) | ||
| .map_err(|e| { | ||
|
|
||
| let validators: Vec<ValidatorSpecification> = if standard_format { |
There was a problem hiding this comment.
Rather than using an extra --standard-format flag, I wonder if we should use a different flag name like --keystore-file for keystores in the standard format.
The reason being that the --validators-file name isn't really accurate for a single keystore, and the validators.json file generated by Lighthouse is a very different format to the standard format.
We could have --keystore-file and --validators-file conflict so that the user is only able to set 1.
There was a problem hiding this comment.
Revised in aa110d5, that are some questions that I am a little unsure which I will highlight them in a "self-review" later
| If this value is not supplied then a 'dry run' will be conducted where \ | ||
| no changes are made to the validator client.", |
There was a problem hiding this comment.
Is this doc accurate? It looks like it was copy-pasted from the --confirm flag
There was a problem hiding this comment.
I removed this description in e5a618c for all import, list, and delete. Because from testing, without supplying vc-url flag, it still able to make changes to the VC, using the default VC HTTP url.
| result.push('\n'); | ||
| } | ||
|
|
||
| Ok(result) |
There was a problem hiding this comment.
It's a bit strange that we return a string here, but never do anything with it other than string.contains(pubkey). We could return the validators which is a Vec<SingleKeystoreResponse> instead, which is cleaner and simpler.
There was a problem hiding this comment.
This is revised in c652724, thank you for your help!
I think I understand what you mean, now that we return Vec from the run function, which we can obtain the validating_pubkey from one of the field in the struct, which is more natural. We can then use that to compare with local public keys.
Feel free to point out anything that's still weird or incorrect
michaelsproul
added
waiting-on-author
Co-authored-by: Michael Sproul <micsproul@gmail.com>
| let validators: Vec<ValidatorSpecification> = | ||
| if let Some(validators_format_path) = &validators_file_path { |
There was a problem hiding this comment.
As validators-file and keystore-file are now only either one that will be used, these flags become optional now. So I use the Some() to "extract" the file path so that I can use the value inside the Option<PathBuf>.
Feel free to point out if this is not the correct/standard way to do this.
The same goes for keystore_file_path below.
| CommandLineTest::validators_import() | ||
| .flag("--vc-token", Some("./token.json")) | ||
| .flag("--validators-file", Some("./vals.json")) | ||
| .flag("--keystore-file", Some("./keystore.json")) |
There was a problem hiding this comment.
This test is failing: https://github.com/sigp/lighthouse/actions/runs/11404357702/job/31733366200
I found that this is because the .required(true) for the validators-file flag is removed: https://github.com/sigp/lighthouse/pull/6261/files#diff-e54fe894fb02bdd0c2175fbb4d7cc6d04fa7c8b460b30e9512edb814f569f97eL41
(as it now requires either --validators-file or --keystore-file)
The test caught it because now that the hard requirement for --validators-file is dropped, so missing the validators file flag will not return a false from the test. I change this test to testing that if both flags are used, then the test fails.
Not sure if this is good?
chong-he
added
ready-for-review
Issue Addressed
Proposed Changes
Add commonds to
lighthouse vmto support the Keymanager API to manage local keystores, i.e.,list,importanddeleteAdditional Info
Building on top of @pahor167 PR: #5347
Some other modifications:
importlogging where only successful import will display the log (previously it will display the log once the HTTP API is called even though the import is not successful, e.g., incorrect password)delete