refacto: improve node cli

[tdelabro]

Does this introduce a breaking change?

Yes, some arguments and behaviors in the CLI changed

Sumup

• feat(cli): run is the by default command when running the madara bin
• refacto(cli): run and setup commands are defined in their own files
• refacto(cli): run.testnet argument removed in favor of the substrate native chain arg
• feat(cli): run.fetch_chain_spec argument removed in favor of the substrate native chain arg
• feat(cli): setup require a source file, either from an url or a path on the local filesystem
• chore(cli): use Url, Path and PathBuf types rather than String
• refacto(cli): moved the pallet/chain_spec/utils methods to the node crate
• feat(cli): madara_path arg has been remove, we use the substrate native base_path arg instead
• feat(cli): sharingan chain specs are loaded during the compilation, not downloaded from github
• refacto(pallet/starknet): GenesisLoader refactored as GenesisData + a base_path field

How to use?

You use the same chain arg in both setup and run, and you will be fine.
It will setup the chain spec for this specific chain, and then use those to run.

[kalaninja]

Looks very good to me.

[kalaninja]

I also can propose to decompose the big if-clause in setup module. For example the following way:

struct SetupRunner;
trait RunSetup<S> {
    fn run(&self, setup: S) -> Result<()>;
}

struct LocalSetupParams {
    src_dir: PathBuf,
    dest_dir: PathBuf,
}
impl RunSetup<LocalSetupParams> for SetupRunner {
    fn run(&self, setup: LocalSetupParams) -> Result<()> {
        let src_index_path = setup.src_dir.join(CONFIG_INDEX_FILE_NAME);
        let index_json = std::fs::read_to_string(src_index_path).map_err(|e| Error::Application(Box::new(e)))?;
        let index_config: Configs = serde_json::from_str(&index_json)
            .map_err(|e| Error::Input(format!("Invalid `{CONFIG_INDEX_FILE_NAME}` content: {e}")))?;
        write_to_file(index_json, &setup.dest_dir.join(CONFIG_INDEX_FILE_NAME))?;

        let src_genesis_assets_dir = setup.src_dir.join(GENESIS_ASSETS_DIR_NAME);
        let dest_genesis_assets_dir = setup.dest_dir.join(GENESIS_ASSETS_DIR_NAME);
        for asset in index_config.genesis_assets {
            force_copy_file(&src_genesis_assets_dir.join(asset.name), &dest_genesis_assets_dir)?;
        }

        Ok(())
    }
}

struct RemoteSetupParams {
    index_url: Url,
    dest_dir: PathBuf,
}
impl RunSetup<RemoteSetupParams> for SetupRunner {
    fn run(&self, setup: RemoteSetupParams) -> Result<()> {
        let index_bytes = reqwest::blocking::get(setup.index_url)
            .and_then(|response| response.bytes())
            .map_err(|e| Error::Application(Box::new(e)))?;
        let index_config: Configs =
            serde_json::from_slice(&index_bytes[..]).map_err(|e| Error::Application(Box::new(e)))?;
        write_to_file(index_bytes, &setup.dest_dir.join(CONFIG_INDEX_FILE_NAME))?;

        let base_url = Url::parse(&index_config.remote_base_path)
            .and_then(|url| url.join(format!("{GENESIS_ASSETS_DIR_NAME}/").as_str()))
            .map_err(|e| Error::Application(Box::new(e)))?;
        let dest_genesis_assets_dir = setup.dest_dir.join(GENESIS_ASSETS_DIR_NAME);
        for asset in index_config.genesis_assets {
            safe_download_genesis_asset(&base_url, asset, &dest_genesis_assets_dir)?;
        }

        Ok(())
    }
}

enum Setup {
    Local(LocalSetupParams),
    Remote(RemoteSetupParams),
}

impl RunSetup<Setup> for SetupRunner {
    fn run(&self, setup: Setup) -> Result<()> {
        match setup {
            Setup::Local(params) => self.run(params),
            Setup::Remote(params) => self.run(params),
        }
    }
}

fn write_to_file<T: AsRef<[u8]>>(content: T, dest_file: &Path) -> Result<()> {
    let parent = dest_file.parent().ok_or("Failed to get parent directory")?;
    std::fs::create_dir_all(parent)?;
    std::fs::File::create(dest_file)?.write_all(content.as_ref())?;

    Ok(())
}

fn force_copy_file(src_file: &Path, dest_path: &Path) -> Result<()> {
    let file_name = src_file.file_name().ok_or("File name not found")?;
    if !src_file.exists() {
        return Err(format!("Source file '{}' does not exist", src_file.display()).into());
    }

    std::fs::create_dir_all(dest_path)?;
    std::fs::copy(src_file, dest_path.join(file_name))?;

    Ok(())
}

fn safe_download_genesis_asset(base_url: &Url, file: FileInfos, dest_path: &Path) -> Result<()> {
    let full_url = base_url.join(&file.name).map_err(|e| Error::Application(Box::new(e)))?;
    println!("Fetching '{}'", &full_url);

    let file_content = reqwest::blocking::get(full_url.clone())
        .and_then(|response| response.bytes())
        .map_err(|e| Error::Application(Box::new(e)))
        .and_then(|bytes| write_to_file(&bytes, &dest_path.join(file.name)).map(|_| bytes))?;

    if let Some(expected_hash) = file.md5 {
        let digest = md5::compute(file_content);
        let actual_hash = format!("{:x}", digest);
        if actual_hash != expected_hash {
            return Err(format!("MD5 mismatch for file '{full_url}': {actual_hash} != {expected_hash}").into());
        }
    }

    Ok(())
}

So SetupCmd can be implemented the following way:

impl SetupCmd {
    pub fn run(&self) -> Result<()> {
        log::info!("setup cmd: {:?}", self);

        let is_dev = self.shared_params().is_dev();
        let chain_id = self.shared_params().chain_id(is_dev);
        let dest_dir = self
            .shared_params()
            .base_path()?
            .unwrap_or_else(|| BasePath::from_project("", "", &Cli::executable_name()))
            .config_dir(&chain_id);
        log::info!("Setting up madara config at '{}'", dest_dir.display());

        let setup = if let Some(path) = &self.source.from_local {
            Setup::Local(LocalSetupParams { src_dir: PathBuf::from(path), dest_dir })
        } else if let Some(url) = &self.source.from_remote {
            let index_url =
                Url::parse(url).map_err(|e| Error::Input(format!("Invalid url for `{CONFIG_INDEX_FILE_NAME}`:{e}")))?;
            println!("Fetching chain config from '{}'", &index_url);

            Setup::Remote(RemoteSetupParams { index_url, dest_dir })
        } else {
            unreachable!(
                "clap::Args is derived upon `SetupSource` in a way that guarantee that either `from_remote` or \
                 `from_local` is present"
            );
        };

        SetupRunner.run(setup)
    }
}

[tdelabro]

I also can propose to decompose the big if-clause in setup module. For example the following way:

I understand its appeal. But I prefer to keep it as it is for now.
The two cases are not really factorizable. You propose to replace the if/else with types.
It creates more abstraction (and therefore complexity) and a higher code segmentation.

I prefer to have both close so it reads easily and straightforwardly.

I added a few comments tho.

[kalaninja]

I understand its appeal. But I prefer to keep it as it is for now.

I agree. The proposed change makes more sense if you are going to add more types of setup in the future.