-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
Your PR was set to target |
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 left a few comments and suggestion. Something worth fixing in this PR and the previous is the use of validator
for the relayer
role. As mentioned we don't need to be validators to relay a checkpoint, and this may lead to confusion.
#[arg(long, short, help = "The number of seconds to submit checkpoint")] | ||
pub checkpoint_interval_sec: Option<u64>, | ||
#[arg(long, short, help = "The hex encoded address of the validator")] | ||
pub validator: Option<String>, |
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.
Wait, relayers don't need to be validators in the subnet, let's call this address
as we do for the rest of the subnet.
I also don't understand why we need the PubKey
here, I think we should pass the address and leverage the wallet to sign or any interactions required with the parent network.
pub validator: Option<String>, | |
pub address: Option<String>, |
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 see, I think the public key side of things is actually a typo due to the copy-pasta, right?
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 still have the impresseion that only validators can submit, I will change it to submitter
|
||
let mut keystore = new_evm_keystore_from_path(&config_path)?; | ||
let validator = match (arguments.validator.as_ref(), keystore.get_default()?) { | ||
(Some(validator), _) => Address::from_str(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.
As mentioned below, let's call this the address
.
use std::sync::{Arc, RwLock}; | ||
use std::time::Duration; | ||
|
||
const DEFAULT_CHECKPOINT_INTERVAL: u64 = 15; |
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.
This is not the checkpoint interval but the polling time in the process loop, right? I think we should rename it and clarify in a comment.
const DEFAULT_CHECKPOINT_INTERVAL: u64 = 15; | |
const DEFAULT_POLLING_INTERVAL: u64 = 15; |
.checkpoint_interval_sec | ||
.unwrap_or(DEFAULT_CHECKPOINT_INTERVAL), | ||
); | ||
manager.run(validator, interval).await; |
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.
Out of a curiosity, how are we handling killing the process? We should ensure that ctrl-c
here gracefully kills the process.
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.
Yeah, ctrl+c should be able to kill the process.
ipc/provider/src/checkpoint.rs
Outdated
}); | ||
|
||
tokio::time::sleep(submission_interval).await; | ||
} | ||
} | ||
|
||
/// Submit the checkpoint from the target validator address | ||
pub async fn submit_checkpoint(&self, validator: &Address) -> Result<()> { |
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.
We should remove validator
from all the functions as the argument (I think I missed it in my previous review). We don't need to be a validator
to act as a relayer in the system.
pub async fn submit_checkpoint(&self, validator: &Address) -> Result<()> { | |
pub async fn submit_checkpoint(&self, relayer: &Address) -> Result<()> { |
Co-authored-by: adlrocha <[email protected]>
Co-authored-by: adlrocha <[email protected]>
…agent into relayer-cli
Adding relayer cli plus some small fixes.