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

Update cli.rs #11148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
104 changes: 48 additions & 56 deletions tools/mirror/src/cli.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use anyhow::Context;
use anyhow::{Context, Result};
use near_primitives::types::BlockHeight;
use std::cell::Cell;
use std::path::PathBuf;
use tokio::runtime::Runtime;

#[derive(clap::Parser)]
pub struct MirrorCommand {
Expand All @@ -15,43 +15,37 @@ enum SubCommand {
Run(RunCmd),
}

/// initialize a target chain with genesis records from the source chain, and
/// then try to mirror transactions from the source chain to the target chain.
#[derive(clap::Parser)]
struct RunCmd {
/// source chain home dir
#[clap(long)]
source_home: PathBuf,
/// target chain home dir

#[clap(long)]
target_home: PathBuf,
/// file containing an optional secret as generated by the
/// `prepare` command. Must be provided unless --no-secret is given
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were these descriptions removed?


#[clap(long)]
secret_file: Option<PathBuf>,
/// Equivalent to passing --secret-file <FILE> where <FILE> is a
/// config that indicates no secret should be used. If this is
/// given, and --secret-file is also given and points to a config
/// that does contain a secret, the mirror will refuse to start

#[clap(long)]
no_secret: bool,
/// Start a NEAR node for the source chain, instead of only using
/// whatever's currently stored in --source-home

#[clap(long)]
online_source: bool,
/// If provided, we will stop after sending transactions coming from
/// this height in the source chain

#[clap(long)]
stop_height: Option<BlockHeight>,

#[clap(long)]
config_path: Option<PathBuf>,
}

impl RunCmd {
fn run(self) -> anyhow::Result<()> {
//returning a Result instead of unwrapping, to handle any error properly
//===================================
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please follow the rustdoc documentation guidelines. Some things that stand out immediately:

  • ===== is very seldom used -- this is my first time seeing it used for Rust documentation;
  • /// denotes a doc-comment and should be used here;
  • I don't think code comments need to motivate a specific return value as you did here, nor they should be used to keep a changelog to the functions – we have git for that after all.

//The RunCmd::run method is changed to asynchronous and properly awaits the asynchronous operations.
async fn run(self) -> Result<()> {
openssl_probe::init_ssl_cert_env_vars();
let runtime = tokio::runtime::Runtime::new().context("failed to start tokio runtime")?;

let runtime = Runtime::new().context("failed to start tokio runtime")?;
let secret = if let Some(secret_file) = &self.secret_file {
let secret = crate::secret::load(secret_file)
.with_context(|| format!("Failed to load secret from {:?}", secret_file))?;
Expand All @@ -67,16 +61,17 @@ impl RunCmd {
}
None
};

let system = new_actix_system(runtime);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing whitespace is unnecessary. Have you run cargo fmt after making your changes?

system
.block_on(async move {
let _subscriber_guard = near_o11y::default_subscriber(
near_o11y::EnvFilterBuilder::from_env().finish().unwrap(),
&near_o11y::Options::default(),
)
.global();
actix::spawn(crate::run(
system.block_on(async move {
let _subscriber_guard = near_o11y::default_subscriber(
near_o11y::EnvFilterBuilder::from_env().finish().unwrap(),
&near_o11y::Options::default(),
)
.global();

/*
actix::spawn(crate::run(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We generally strive to not have commented-out code… You probably should remove this.

self.source_home,
self.target_home,
secret,
Expand All @@ -87,41 +82,39 @@ impl RunCmd {
.await
})
.unwrap()
changes will be made to the above code to handle the Result properly
*/
crate::run(
self.source_home,
self.target_home,
secret,
self.stop_height,
self.online_source,
self.config_path,
).await
})?;

Ok(())
}
}

/// Write a new genesis records file where the public keys have been
/// altered so that this binary can sign transactions when mirroring
/// them from the source chain to the target chain
#[derive(clap::Parser)]
struct PrepareCmd {
/// A genesis records file as output by `neard view-state
/// dump-state --stream`
#[clap(long)]
records_file_in: PathBuf,
/// Path to the new records file with updated public keys

#[clap(long)]
records_file_out: PathBuf,
/// If this is provided, don't use a secret when mapping public
/// keys to new source chain private keys. This means that anyone
/// will be able to sign transactions for the accounts in the
/// target chain corresponding to accounts in the source chain. If
/// that is okay, then --no-secret will make the code run slightly
/// faster, and you won't have to take care to not lose the
/// secret.

#[clap(long)]
no_secret: bool,
/// Path to the secret. Note that if you don't pass --no-secret,
/// this secret is required to sign transactions for the accounts
/// in the target chain corresponding to accounts in the source
/// chain. This means that if you lose this secret, you will no
/// longer be able to mirror any traffic.

#[clap(long)]
secret_file_out: PathBuf,
}

impl PrepareCmd {
fn run(self) -> anyhow::Result<()> {
fn run(self) -> Result<()> {
crate::genesis::map_records(
&self.records_file_in,
&self.records_file_out,
Expand All @@ -131,26 +124,25 @@ impl PrepareCmd {
}
}

// copied from neard/src/cli.rs
fn new_actix_system(runtime: tokio::runtime::Runtime) -> actix::SystemRunner {
// `with_tokio_rt()` accepts an `Fn()->Runtime`, however we know that this function is called exactly once.
// This makes it safe to move out of the captured variable `runtime`, which is done by a trick
// using a `swap` of `Cell<Option<Runtime>>`s.
let runtime_cell = Cell::new(Some(runtime));
//using tokio::runtime::Runtime directly
//ensuring consistent error handling are also made.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here as above.

fn new_actix_system(runtime: Runtime) -> actix::SystemRunner {
let runtime_cell = std::cell::Cell::new(Some(runtime));
actix::System::with_tokio_rt(|| {
let r = Cell::new(None);
let r = std::cell::Cell::new(None);
runtime_cell.swap(&r);
r.into_inner().unwrap()
})
}

//MirrorCommand::run is now asynchronous, and it awaits the result of the RunCmd::run method when applicable.
impl MirrorCommand {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above.

pub fn run(self) -> anyhow::Result<()> {
pub async fn run(self) -> Result<()> {
tracing::warn!(target: "mirror", "the mirror command is not stable, and may be removed or changed arbitrarily at any time");

match self.subcmd {
SubCommand::Prepare(r) => r.run(),
SubCommand::Run(r) => r.run(),
SubCommand::Run(r) => r.run().await,
}
}
}