Validator manager import to allow overriding fields with CLI flag #7684
+183
−46
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,8 +112,7 @@ pub fn cli_app() -> Command { | |
| .value_name("ETH1_ADDRESS") | ||
| .help("When provided, the imported validator will use the suggested fee recipient. Omit this flag to use the default value from the VC.") | ||
| .action(ArgAction::Set) | ||
| .display_order(0), | ||
| ) | ||
| .arg( | ||
| Arg::new(GAS_LIMIT) | ||
|
|
@@ -122,8 +121,7 @@ pub fn cli_app() -> Command { | |
| .help("When provided, the imported validator will use this gas limit. It is recommended \ | ||
| to leave this as the default value by not specifying this flag.",) | ||
| .action(ArgAction::Set) | ||
| .display_order(0), | ||
| ) | ||
| .arg( | ||
| Arg::new(BUILDER_PROPOSALS) | ||
|
|
@@ -132,8 +130,7 @@ pub fn cli_app() -> Command { | |
| blocks via builder rather than the local EL.",) | ||
| .value_parser(["true","false"]) | ||
| .action(ArgAction::Set) | ||
| .display_order(0), | ||
| ) | ||
| .arg( | ||
| Arg::new(BUILDER_BOOST_FACTOR) | ||
|
|
@@ -144,8 +141,7 @@ pub fn cli_app() -> Command { | |
| when choosing between a builder payload header and payload from \ | ||
| the local execution node.",) | ||
| .action(ArgAction::Set) | ||
| .display_order(0), | ||
| ) | ||
| .arg( | ||
| Arg::new(PREFER_BUILDER_PROPOSALS) | ||
|
|
@@ -154,8 +150,16 @@ pub fn cli_app() -> Command { | |
| constructed by builders, regardless of payload value.",) | ||
| .value_parser(["true","false"]) | ||
| .action(ArgAction::Set) | ||
| .display_order(0), | ||
| ) | ||
| .arg( | ||
| Arg::new(ENABLED) | ||
| .long(ENABLED) | ||
| .help("When provided, the imported validator will be \ | ||
| enabled or disabled.",) | ||
| .value_parser(["true","false"]) | ||
| .action(ArgAction::Set) | ||
| .display_order(0), | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -225,48 +229,112 @@ async fn run(config: ImportConfig) -> Result<(), String> { | |
| enabled, | ||
| } = config; | ||
|
|
||
| let validators: Vec<ValidatorSpecification> = if let Some(validators_format_path) = | ||
| &validators_file_path | ||
| { | ||
| if !validators_format_path.exists() { | ||
| return Err(format!( | ||
| "Unable to find file at {:?}", | ||
| validators_format_path | ||
| )); | ||
| } | ||
|
|
||
| let validators_file = fs::OpenOptions::new() | ||
| .read(true) | ||
| .create(false) | ||
| .open(validators_format_path) | ||
| .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_path, e))?; | ||
|
|
||
| // Define validators as mutable so that if a relevant flag is supplied, the fields can be overridden. | ||
| let mut validators: Vec<ValidatorSpecification> = serde_json::from_reader(&validators_file) | ||
| .map_err(|e| { | ||
| format!( | ||
| "Unable to parse JSON in {:?}: {:?}", | ||
| validators_format_path, e | ||
| ) | ||
| })?; | ||
|
|
||
| // Log the overridden note when one or more flags is supplied | ||
| if !validators.is_empty() { | ||
| if let Some(override_fee_recipient) = fee_recipient { | ||
| eprintln!( | ||
| "Please note! --suggested-fee-recipient is provided. This will override existing fee recipient defined in validators.json with: {:?}", | ||
| override_fee_recipient | ||
| ); | ||
| } | ||
| if let Some(override_gas_limit) = gas_limit { | ||
| eprintln!( | ||
| "Please note! --gas-limit is provided. This will override existing gas limit defined in validators.json with: {}", | ||
| override_gas_limit | ||
| ); | ||
| } | ||
| if let Some(override_builder_proposals) = builder_proposals { | ||
| eprintln!( | ||
| "Please note! --builder-proposals is provided. This will override existing builder proposal setting defined in validators.json with: {}", | ||
| override_builder_proposals | ||
| ); | ||
| } | ||
| if let Some(override_builder_boost_factor) = builder_boost_factor { | ||
| eprintln!( | ||
| "Please note! --builder-boost-factor is provided. This will override existing builder boost factor defined in validators.json with: {}", | ||
| override_builder_boost_factor | ||
| ); | ||
| } | ||
| if let Some(override_prefer_builder_proposals) = prefer_builder_proposals { | ||
| eprintln!( | ||
| "Please note! --prefer-builder-proposals is provided. This will override existing prefer builder proposal setting defined in validators.json with: {}", | ||
| override_prefer_builder_proposals | ||
| ); | ||
| } | ||
| if let Some(override_enabled) = enabled { | ||
| eprintln!( | ||
| "Please note! --enabled flag is provided. This will override existing setting defined in validators.json with: {}", | ||
| override_enabled | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Override the fields in validators.json file if the flag is supplied | ||
| for validator in &mut validators { | ||
| if let Some(override_fee_recipient) = fee_recipient { | ||
| validator.fee_recipient = Some(override_fee_recipient); | ||
|
Member
There was a problem hiding this comment. We could move the logging inside the for loop? I doubt we need to log anything in the case that
Member
Author
There was a problem hiding this comment. Thanks for the review. I got what you mean. Previously, before consolidating the Now, after moving in the Revise in 6237ab3 |
||
| } | ||
| if let Some(override_gas_limit) = gas_limit { | ||
| validator.gas_limit = Some(override_gas_limit); | ||
| } | ||
| if let Some(override_builder_proposals) = builder_proposals { | ||
| validator.builder_proposals = Some(override_builder_proposals); | ||
| } | ||
| if let Some(override_builder_boost_factor) = builder_boost_factor { | ||
| validator.builder_boost_factor = Some(override_builder_boost_factor); | ||
| } | ||
| if let Some(override_enabled) = enabled { | ||
| validator.enabled = Some(override_enabled); | ||
| } | ||
| } | ||
|
|
||
| validators | ||
| } else if let Some(keystore_format_path) = &keystore_file_path { | ||
| vec![ValidatorSpecification { | ||
| voting_keystore: KeystoreJsonStr( | ||
| Keystore::from_json_file(keystore_format_path).map_err(|e| format!("{e:?}"))?, | ||
| ), | ||
| voting_keystore_password: password.ok_or_else(|| { | ||
| "The --password flag is required to supply the keystore password".to_string() | ||
| })?, | ||
| slashing_protection: None, | ||
| fee_recipient, | ||
| gas_limit, | ||
| builder_proposals, | ||
| builder_boost_factor, | ||
| prefer_builder_proposals, | ||
| enabled, | ||
| }] | ||
| } else { | ||
| return Err(format!( | ||
| "One of the flag --{VALIDATORS_FILE_FLAG} or --{KEYSTORE_FILE_FLAG} is required." | ||
| )); | ||
| }; | ||
|
|
||
| let count = validators.len(); | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so as a consequence of moving the logs inside the for loop, this will log a separate instance of each
Please note!log for every validator in yourvalidator.jsonfile. So we should either:validators.len() == 0.I think I'm fine with either, it's just a matter of which we feel is more ergonomic (1 log per validator vs 1 log total for all validators). For 100s of validators, 1 log per validator would likely be too much. But if you had 2 validators, having a reminder that the override will apply to both might be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaning towards no. 2, I will revise
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the check
if !validators.is_empty()(forvalidators.len() == 0), but I am thinking that this may not be exactly accurate?For example, when we import new validators and the VC has no validators, then the logs
Please note! ...wouldn't be shown, which is not really the intention. i.e., it's not only about modifying existing validators, it can also be used to modify the fields for newly added validators (including the scenario when the VC has no validators)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the
if !validators.is_empty()condition with reason stated above: 2239b55Let me know if this is no good and I am happy to revert the change