-
Notifications
You must be signed in to change notification settings - Fork 407
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
feat: posting ECDSARegistry signature to s3 bucket for validators #3674
Conversation
|
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.
directionally lgtm!
#[derive(Clone, Debug, Default)] | ||
pub struct StakingConf { | ||
/// Address of the HyperlaneServiceManager contract | ||
pub service_managers: HashMap<u32, H160>, |
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.
will there be stuff in here other than the service_managers?
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.
Yes, I imagine we'll have more config once we have native staking which will also have some form of registration announcement in a json form.
@@ -40,6 +41,8 @@ pub struct ValidatorSettings { | |||
pub reorg_period: u64, | |||
/// How frequently to check for new checkpoints | |||
pub interval: Duration, | |||
|
|||
pub staking_config: StakingConf, |
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.
Do you expect to ever have this be set via settings?
if we won't ever have a different config other than the default_staking_config
, imo you could just not even have this in ValidatorSettings, and just create it in impl BaseAgent for Validator {
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.
given the above comment, I was anticipating we may need to set it via settings as you mentioned
@@ -122,6 +124,7 @@ impl BaseAgent for Validator { | |||
agent_metrics, | |||
chain_metrics, | |||
core_metrics: metrics, | |||
staking_config: settings.staking_config, |
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.
imo instead of storing a config in the Validator that you can then lookup
let service_manager = self
.staking_config
.service_managers
.get(&avs_domain)
it'd be nicer to just do that lookup in here when building the Validator once, and then store the service_manager
as the config in the Validator
Description
Drive-by changes
Related issues
Backward compatibility
Testing