Refactor: consolidate forum parameters and confirmation parameters in…

…to a single structure. (#1290)

What has happened here:

* Reduction of cyclic `use` dependencies.
* Auth stuff moved to `auth.rs`.
* `ExecutableCommand` stuff moved to `ctx` module, since (despite the name) it's just a vehicle to create a `Ctx` for commands to use.  I will revise this abstraction later on -- I think it may ultimately not be necessary.
* Consolidation of everything under `lib.rs` instead of double importing in `main.rs`.  Removes source of confusion and speeds up compilation.
* Confirmation options move to their own module `confirm.rs` to clear up confusion and the import graph.
* Forum submitter code from `forum.rs` moves to `submitter.rs` since it has dual responsibility (submit proposals and manage forum posts) so having it as a separate file adds clarity.
* Implementations of proposal executors from `proposal_executors.rs` move to `governance.rs` for the governance executor and `ic_admin.rs` for the ic-admin executor, breaking up an important import cycle.

One important change not in the list above:

The `SubmissionParameters` structure has sprung to existence to include the `ForumParameters` and `ConfirmationModeOptions` structs, and is now used when appropriate (in operations that require forum post parameters and can be affected by confirmation modes, e.g., dry run, confirm, unconditional).

These changes, together with prior changes that consolidate forum post management and proposal simulation / execution, make the code more maintainable, more explicit about what is implemented or happening and where it is, and pave the way for imbuing the `Proposable` types with knowledge about how to deal with the forum based on what the type of `Proposable`.

Noteworthy in light of the last sentence: now the Submitter type method `propose()` accepts a `ProposalExecution` (tied to a `Proposable`) next to a forum post type.  Soon, the type of forum post can be dictated by the `ProposalExecution` (effectively, the `Proposable`) itself, which makes sense, as we already have fixed forum post types for different proposals, but this knowledge is currently (incorrectly) hardcoded in an ad-hoc manner throughout various parts of the code.

**Important change: we hereby sunset the ability to pass `--yes`, `--no` and `--forum-post` stuff at arbitrary positions in the command line.  These are now accepted only after the subcommand name.  `clap` -- or perhaps our use of `clap` -- does not give us flexibility in parsing these flags anywhere in the command line.  I removed error-prone code that tried to guess (with only a moderate chance of success) under certain circumstances what the user meant.**

Find most current import graphs attached here:

![modules](https://github.com/user-attachments/assets/dfc1a781-ca1c-4956-aac8-8b8cc9cfc0f5)


_Not all cycles have been eliminated!_

rs/cli/src/auth.rs

@@ -1,3 +1,5 @@
+use clap::Args as ClapArgs;
+use clap_num::maybe_hex;
 use dialoguer::{console::Term, theme::ColorfulTheme, Password, Select};
 use ic_canisters::governance::GovernanceCanisterWrapper;
 use ic_canisters::parallel_hardware_identity::{hsm_key_id_to_int, HsmPinHandler, KeyIdVec, ParallelHardwareIdentity, PinHandlerError};
@@ -9,7 +11,84 @@ use keyring::{Entry, Error};
 use log::{debug, error, info, warn};
 use std::path::PathBuf;
 
-use crate::commands::{AuthOpts, AuthRequirement, HsmOpts, HsmParams};
+/// HSM authentication parameters
+#[derive(ClapArgs, Debug, Clone)]
+pub(crate) struct HsmParams {
+    /// Slot that HSM key uses, can be read with pkcs11-tool
+    #[clap(required = false,
+        conflicts_with = "private_key_pem",
+        long, value_parser=maybe_hex::<u64>, global = true, env = "HSM_SLOT")]
+    pub(crate) hsm_slot: Option<u64>,
+
+    /// HSM Key ID, can be read with pkcs11-tool
+    #[clap(required = false, conflicts_with = "private_key_pem", long, value_parser=maybe_hex::<u8>, global = true, env = "HSM_KEY_ID")]
+    pub(crate) hsm_key_id: Option<KeyIdVec>,
+}
+
+/// HSM authentication arguments
+/// These comprise an optional PIN and optional parameters.
+/// The PIN is used during autodetection if the optional
+/// parameters are missing.
+#[derive(ClapArgs, Debug, Clone)]
+pub(crate) struct HsmOpts {
+    /// Pin for the HSM key used for submitting proposals
+    // Must be present if slot and key are specified.
+    #[clap(
+        required = false,
+        alias = "hsm-pim",
+        conflicts_with = "private_key_pem",
+        long,
+        global = true,
+        hide_env_values = true,
+        env = "HSM_PIN"
+    )]
+    pub(crate) hsm_pin: Option<String>,
+    #[clap(flatten)]
+    pub(crate) hsm_params: HsmParams,
+}
+
+// The following should ideally be defined in terms of an Enum
+// as there is no conceivable scenario in which both a PEM file
+// and a set of HSM options can be used by the program.
+// Sadly, until ticket
+//   https://github.com/clap-rs/clap/issues/2621
+// is fixed, we cannot do this, and we must use a struct instead.
+// Note that group(multiple = false) has no effect, and therefore
+// we have to use conflicts and requires to specify option deps.
+#[derive(ClapArgs, Debug, Clone)]
+#[group(multiple = false)]
+/// Authentication arguments
+pub struct AuthOpts {
+    /// Path to private key file (in PEM format)
+    #[clap(
+        long,
+        required = false,
+        global = true,
+        conflicts_with_all = ["hsm_pin", "hsm_slot", "hsm_key_id"],
+        env = "PRIVATE_KEY_PEM",
+        visible_aliases = &["pem", "key", "private-key"]
+    )]
+    pub(crate) private_key_pem: Option<String>,
+    #[clap(flatten)]
+    pub(crate) hsm_opts: HsmOpts,
+}
+
+impl TryFrom<String> for AuthOpts {
+    type Error = anyhow::Error;
+
+    fn try_from(value: String) -> std::result::Result<Self, Self::Error> {
+        Ok(AuthOpts {
+            private_key_pem: Some(value),
+            hsm_opts: HsmOpts {
+                hsm_pin: None,
+                hsm_params: HsmParams {
+                    hsm_slot: None,
+                    hsm_key_id: None,
+                },
+            },
+        })
+    }
+}
 
 #[derive(Debug, Clone)]
 pub struct Neuron {
@@ -26,6 +105,13 @@ impl PartialEq for Neuron {
 
 impl Eq for Neuron {}
 
+#[derive(Clone)]
+pub enum AuthRequirement {
+    Anonymous, // for get commands
+    Signer,    // just authentication details used for signing
+    Neuron,    // Signer + neuron_id used for proposals
+}
+
 pub const STAGING_NEURON_ID: u64 = 49;
 pub const STAGING_KEY_PATH_FROM_HOME: &str = ".config/dfx/identity/bootstrap-super-leader/identity.pem";
 

rs/cli/src/commands/api_boundary_nodes/add.rs

@@ -2,13 +2,14 @@ use clap::Args;
 use ic_types::PrincipalId;
 
 use crate::{
-    commands::{AuthRequirement, ExecutableCommand},
-    forum::{ForumPostKind, Submitter},
+    auth::AuthRequirement,
+    exe::args::GlobalArgs,
+    exe::ExecutableCommand,
+    forum::ForumPostKind,
     ic_admin::{self},
+    submitter::{SubmissionParameters, Submitter},
 };
 
-use crate::forum::ForumParameters;
-
 #[derive(Args, Debug)]
 pub struct Add {
     /// Node IDs to turn into API BNs
@@ -24,7 +25,7 @@ pub struct Add {
     pub motivation: Option<String>,
 
     #[clap(flatten)]
-    pub forum_parameters: ForumParameters,
+    pub submission_parameters: SubmissionParameters,
 }
 
 impl ExecutableCommand for Add {
@@ -33,24 +34,23 @@ impl ExecutableCommand for Add {
     }
 
     async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> {
-        Submitter::from_executor_and_mode(
-            &self.forum_parameters,
-            ctx.mode.clone(),
-            ctx.ic_admin_executor().await?.execution(ic_admin::IcAdminProposal::new(
-                ic_admin::IcAdminProposalCommand::AddApiBoundaryNodes {
-                    nodes: self.nodes.to_vec(),
-                    version: self.version.clone(),
-                },
-                ic_admin::IcAdminProposalOptions {
-                    title: Some(format!("Add {} API boundary node(s)", self.nodes.len())),
-                    summary: Some(format!("Add {} API boundary node(s)", self.nodes.len())),
-                    motivation: self.motivation.clone(),
-                },
-            )),
-        )
-        .propose(ForumPostKind::Generic)
-        .await
+        Submitter::from(&self.submission_parameters)
+            .propose(
+                ctx.ic_admin_executor().await?.execution(ic_admin::IcAdminProposal::new(
+                    ic_admin::IcAdminProposalCommand::AddApiBoundaryNodes {
+                        nodes: self.nodes.to_vec(),
+                        version: self.version.clone(),
+                    },
+                    ic_admin::IcAdminProposalOptions {
+                        title: Some(format!("Add {} API boundary node(s)", self.nodes.len())),
+                        summary: Some(format!("Add {} API boundary node(s)", self.nodes.len())),
+                        motivation: self.motivation.clone(),
+                    },
+                )),
+                ForumPostKind::Generic,
+            )
+            .await
     }
 
-    fn validate(&self, _args: &crate::commands::Args, _cmd: &mut clap::Command) {}
+    fn validate(&self, _args: &GlobalArgs, _cmd: &mut clap::Command) {}
 }

rs/cli/src/commands/api_boundary_nodes/mod.rs

@@ -3,7 +3,7 @@ use clap::Args;
 use remove::Remove;
 use update::Update;
 
-use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand};
+use crate::exe::impl_executable_command_for_enums;
 
 mod add;
 mod remove;

rs/cli/src/commands/api_boundary_nodes/remove.rs

@@ -2,9 +2,12 @@ use clap::Args;
 use ic_types::PrincipalId;
 
 use crate::{
-    commands::{AuthRequirement, ExecutableCommand},
-    forum::{ForumParameters, ForumPostKind, Submitter},
+    auth::AuthRequirement,
+    exe::args::GlobalArgs,
+    exe::ExecutableCommand,
+    forum::ForumPostKind,
     ic_admin::{self},
+    submitter::{SubmissionParameters, Submitter},
 };
 
 #[derive(Args, Debug)]
@@ -18,7 +21,7 @@ pub struct Remove {
     pub motivation: Option<String>,
 
     #[clap(flatten)]
-    pub forum_parameters: ForumParameters,
+    pub submission_parameters: SubmissionParameters,
 }
 
 impl ExecutableCommand for Remove {
@@ -27,21 +30,20 @@ impl ExecutableCommand for Remove {
     }
 
     async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> {
-        Submitter::from_executor_and_mode(
-            &self.forum_parameters,
-            ctx.mode.clone(),
-            ctx.ic_admin_executor().await?.execution(ic_admin::IcAdminProposal::new(
-                ic_admin::IcAdminProposalCommand::RemoveApiBoundaryNodes { nodes: self.nodes.to_vec() },
-                ic_admin::IcAdminProposalOptions {
-                    title: Some(format!("Remove {} API boundary node(s)", self.nodes.len())),
-                    summary: Some(format!("Remove {} API boundary node(s)", self.nodes.len())),
-                    motivation: self.motivation.clone(),
-                },
-            )),
-        )
-        .propose(ForumPostKind::Generic)
-        .await
+        Submitter::from(&self.submission_parameters)
+            .propose(
+                ctx.ic_admin_executor().await?.execution(ic_admin::IcAdminProposal::new(
+                    ic_admin::IcAdminProposalCommand::RemoveApiBoundaryNodes { nodes: self.nodes.to_vec() },
+                    ic_admin::IcAdminProposalOptions {
+                        title: Some(format!("Remove {} API boundary node(s)", self.nodes.len())),
+                        summary: Some(format!("Remove {} API boundary node(s)", self.nodes.len())),
+                        motivation: self.motivation.clone(),
+                    },
+                )),
+                ForumPostKind::Generic,
+            )
+            .await
     }
 
-    fn validate(&self, _args: &crate::commands::Args, _cmd: &mut clap::Command) {}
+    fn validate(&self, _args: &GlobalArgs, _cmd: &mut clap::Command) {}
 }

rs/cli/src/commands/api_boundary_nodes/update.rs

@@ -2,9 +2,12 @@ use clap::Args;
 use ic_types::PrincipalId;
 
 use crate::{
-    commands::{AuthRequirement, ExecutableCommand},
-    forum::{ForumParameters, ForumPostKind, Submitter},
+    auth::AuthRequirement,
+    exe::args::GlobalArgs,
+    exe::ExecutableCommand,
+    forum::ForumPostKind,
     ic_admin::{self},
+    submitter::{SubmissionParameters, Submitter},
 };
 
 #[derive(Args, Debug)]
@@ -21,7 +24,7 @@ pub struct Update {
     pub motivation: Option<String>,
 
     #[clap(flatten)]
-    pub forum_parameters: ForumParameters,
+    pub submission_parameters: SubmissionParameters,
 }
 
 impl ExecutableCommand for Update {
@@ -30,24 +33,23 @@ impl ExecutableCommand for Update {
     }
 
     async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> {
-        Submitter::from_executor_and_mode(
-            &self.forum_parameters,
-            ctx.mode.clone(),
-            ctx.ic_admin_executor().await?.execution(ic_admin::IcAdminProposal::new(
-                ic_admin::IcAdminProposalCommand::DeployGuestosToSomeApiBoundaryNodes {
-                    nodes: self.nodes.to_vec(),
-                    version: self.version.to_string(),
-                },
-                ic_admin::IcAdminProposalOptions {
-                    title: Some(format!("Update {} API boundary node(s) to {}", self.nodes.len(), &self.version)),
-                    summary: Some(format!("Update {} API boundary node(s) to {}", self.nodes.len(), &self.version)),
-                    motivation: self.motivation.clone(),
-                },
-            )),
-        )
-        .propose(ForumPostKind::Generic)
-        .await
+        Submitter::from(&self.submission_parameters)
+            .propose(
+                ctx.ic_admin_executor().await?.execution(ic_admin::IcAdminProposal::new(
+                    ic_admin::IcAdminProposalCommand::DeployGuestosToSomeApiBoundaryNodes {
+                        nodes: self.nodes.to_vec(),
+                        version: self.version.to_string(),
+                    },
+                    ic_admin::IcAdminProposalOptions {
+                        title: Some(format!("Update {} API boundary node(s) to {}", self.nodes.len(), &self.version)),
+                        summary: Some(format!("Update {} API boundary node(s) to {}", self.nodes.len(), &self.version)),
+                        motivation: self.motivation.clone(),
+                    },
+                )),
+                ForumPostKind::Generic,
+            )
+            .await
     }
 
-    fn validate(&self, _args: &crate::commands::Args, _cmd: &mut clap::Command) {}
+    fn validate(&self, _args: &GlobalArgs, _cmd: &mut clap::Command) {}
 }

rs/cli/src/commands/der_to_principal.rs

@@ -2,7 +2,8 @@ use std::path::PathBuf;
 
 use clap::Args;
 
-use super::{AuthRequirement, ExecutableCommand};
+use crate::auth::AuthRequirement;
+use crate::exe::{args::GlobalArgs, ExecutableCommand};
 
 #[derive(Args, Debug)]
 pub struct DerToPrincipal {
@@ -21,5 +22,5 @@ impl ExecutableCommand for DerToPrincipal {
         Ok(())
     }
 
-    fn validate(&self, _args: &crate::commands::Args, _cmd: &mut clap::Command) {}
+    fn validate(&self, _args: &GlobalArgs, _cmd: &mut clap::Command) {}
 }