From ddbaaf0d77c05cd776c2d9d0b5e58e1abfa18a7f Mon Sep 17 00:00:00 2001 From: Greg Nazario Date: Tue, 27 Aug 2024 11:09:31 -0700 Subject: [PATCH] [cli] Add contribution guide --- crates/aptos/CONTRIBUTING.md | 247 +++++++++++++++++++++++++++++++++++ 1 file changed, 247 insertions(+) create mode 100644 crates/aptos/CONTRIBUTING.md diff --git a/crates/aptos/CONTRIBUTING.md b/crates/aptos/CONTRIBUTING.md new file mode 100644 index 0000000000000..7bd0fe7d976ee --- /dev/null +++ b/crates/aptos/CONTRIBUTING.md @@ -0,0 +1,247 @@ +# Aptos CLI Development Guide + +This is a list of design decisions and guidelines for adding commands to the Aptos CLI. + +## Command Groups + +Commands should be grouped into the existing categories. The current categories are: + +- account +- config +- genesis +- governance +- key +- move +- multisig +- node +- stake +- update + +All categories must have a doc comment that describes the command. It must also derive `Parser` and `Subcommand`. For +example: + +```rust +/// Tool for interacting with accounts +/// +/// This tool is used to create accounts, get information about the +/// account's resources, and transfer resources between accounts. +#[derive(Debug, Subcommand)] +pub enum AccountTool { + Create(create::CreateAccount), + CreateResourceAccount(create_resource_account::CreateResourceAccount), + DeriveResourceAccountAddress(derive_resource_account::DeriveResourceAccount), + FundWithFaucet(fund::FundWithFaucet), + Balance(balance::Balance), + List(list::ListAccount), + LookupAddress(key_rotation::LookupAddress), + RotateKey(key_rotation::RotateKey), + Transfer(transfer::TransferCoins), +} +``` + +Then it must also be added to the top level command structure: + +```rust +/// Command Line Interface (CLI) for developing and interacting with the Aptos blockchain +#[derive(Parser)] +#[clap(name = "aptos", author, version, propagate_version = true, styles = aptos_cli_common::aptos_cli_style())] +pub enum Tool { + #[clap(subcommand)] + Account(account::AccountTool), + #[clap(subcommand)] + Config(config::ConfigTool), + #[clap(subcommand)] + Genesis(genesis::GenesisTool), + #[clap(subcommand)] + Governance(governance::GovernanceTool), + Info(InfoTool), + Init(common::init::InitTool), + #[clap(subcommand)] + Key(op::key::KeyTool), + #[clap(subcommand)] + Move(move_tool::MoveTool), + #[clap(subcommand)] + Multisig(account::MultisigAccountTool), + #[clap(subcommand)] + Node(node::NodeTool), + #[clap(subcommand)] + Stake(stake::StakeTool), + #[clap(subcommand)] + Update(update::UpdateTool), +} +``` + +## Commands + +A command is a single top level command for the CLI. The CLI command must complete it's action in the single command +execution. + +### Command Names + +```rust +/// Compiles a package and returns the associated ModuleIds +#[derive(Parser)] +pub struct CompilePackage { + /// Save the package metadata in the package's build directory + /// + /// If set, package metadata should be generated and stored in the package's build directory. + /// This metadata can be used to construct a transaction to publish a package. + #[clap(long)] + pub(crate) save_metadata: bool, + + #[clap(flatten)] + pub(crate) included_artifacts_args: IncludedArtifactsArgs, + #[clap(flatten)] + pub(crate) move_options: MovePackageDir, +} +``` + +Command names should be simple, identifiable, and easy to use. For example, compilation is grouped in `move` and uses +the subcommand `compile`. + +```bash +aptos move compile +``` + +Once the new command is created, it should have `#[derive(Parser)]` added above. Additionally, it will need to be added +the higher level tool: + +```rust +#[derive(Subcommand)] +pub enum MoveTool { + #[clap(alias = "build")] + Compile(CompilePackage), + #[clap(alias = "build-script")] + CompileScript(CompileScript), + Init(Init), + // ... +} + +impl MoveTool { + pub async fn execute(self) -> CliResult { + match self { + MoveTool::Compile(tool) => tool.execute_serialized().await, + MoveTool::CompileScript(tool) => tool.execute_serialized().await, + MoveTool::Init(tool) => tool.execute_serialized_success().await, + } + } +} +``` + +Note that, there are two types of commands here `execute_serialized()` and `execute_serialized_success()`, if the +command must be returning a value, then it should call `execute_serialized()`, which will convert the input type as JSON +to `stdout`. + +Additionally, `alias` is allowed, but discouraged for new commands. This is mostly to provide either backwards +compatibility or reduce confusion for new users. + +### Command flags + +```rust +#[derive(Parser)] +pub struct CompilePackage { + /// Save the package metadata in the package's build directory + /// + /// If set, package metadata should be generated and stored in the package's build directory. + /// This metadata can be used to construct a transaction to publish a package. + #[clap(long)] + pub(crate) save_metadata: bool, + + // ... +} +``` + +Command inputs should always be documented for help to show up in the CLI. for example, below is the example for +`save_metadata`. They should be snake case, and will show up as a flag. Do not use `short` commands, as they can be +confused between different commands. + +```bash +aptos move compile --save-metadata +``` + +### Command flag groupings + +```rust +/// Compiles a package and returns the associated ModuleIds +#[derive(Parser)] +pub struct CompilePackage { + // ... + #[clap(flatten)] + pub(crate) included_artifacts_args: IncludedArtifactsArgs, + #[clap(flatten)] + pub(crate) move_options: MovePackageDir, +} +``` + +Command flags can be grouped into common structs to be used across multiple commands. These should be flattened by +adding the struct associated and using `#[clap(flatten)]` like above. These should not have a doc comment, and any doc +comments will not end up in the command. Instead, document the structs directly like so: + +```rust +#[derive(Parser)] +pub struct IncludedArtifactsArgs { + /// Artifacts to be generated when building the package + /// + /// Which artifacts to include in the package. This can be one of `none`, `sparse`, and + /// `all`. `none` is the most compact form and does not allow to reconstruct a source + /// package from chain; `sparse` is the minimal set of artifacts needed to reconstruct + /// a source package; `all` includes all available artifacts. The choice of included + /// artifacts heavily influences the size and therefore gas cost of publishing: `none` + /// is the size of bytecode alone; `sparse` is roughly 2 times as much; and `all` 3-4 + /// as much. + #[clap(long, default_value_t = IncludedArtifacts::Sparse)] + pub(crate) included_artifacts: IncludedArtifacts, +} +``` + +### Command Implementation + +```rust +#[async_trait] +impl CliCommand> for CompilePackage { + fn command_name(&self) -> &'static str { + "CompilePackage" + } + + async fn execute(self) -> CliTypedResult> { + let build_options = BuildOptions { + install_dir: self.move_options.output_dir.clone(), + ..self + .included_artifacts_args + .included_artifacts + .build_options( + self.move_options.dev, + self.move_options.skip_fetch_latest_git_deps, + self.move_options.named_addresses(), + self.move_options.override_std.clone(), + self.move_options.bytecode_version, + self.move_options.compiler_version, + self.move_options.language_version, + self.move_options.skip_attribute_checks, + self.move_options.check_test_code, + ) + }; + let pack = BuiltPackage::build(self.move_options.get_package_path()?, build_options) + .map_err(|e| CliError::MoveCompilationError(format!("{:#}", e)))?; + if self.save_metadata { + pack.extract_metadata_and_save()?; + } + let ids = pack + .modules() + .map(|m| m.self_id().to_string()) + .collect::>(); + // TODO: Also say how many scripts are compiled + Ok(ids) + } +} +``` + +Commands should implement the `CliCommand` trait for the package. This allows it to be called upstream generically +and `T` will automatically be serialized to JSON for the output. This allows for typed testing in unit tests, while +still having output converted for the total CLI. + +It's an anti-pattern to `panic`, please avoid panicking, and instead provide `CliError` or `CliError` conversion for the +current types. + +All output from the CLI should use `eprintln!()`, rather than `println!()`. `stdout` is reserved for the JSON output at +the end of the command, `stderr` is used for the rest of the output.