Skip to content

feat: operations, argument validation, and args for global mode#148

Closed
rikonor wants to merge 21 commits intomainfrom
or-args
Closed

feat: operations, argument validation, and args for global mode#148
rikonor wants to merge 21 commits intomainfrom
or-args

Conversation

@rikonor
Copy link
Copy Markdown
Contributor

@rikonor rikonor commented Oct 17, 2025

This change introduces a few new things:

  1. Begins the separation of commands and operations. Commands act as an entrypoint for the cli, and in turn make use of lower-level operations that don't require knowledge of icp projects. This allows us to:
  2. Introduce global and project mode - this means that one should be able to run the cli tool in both project context, as well as outside of a project. This enables one to use operations that should not necessarily be limited to a project's scope.
  3. Introduce a crappy little framework for cli command validation, since more extensive validation is needed (e.g some things are allowed in global mode, but not project mode, and vice-versa).

The above are done for several initial commands, with the assumption that the same work can take place for the rest of the cli footprint (currently global mode and arg validation is implemented for icp canister start/stop and token transfer/balance).

@rikonor rikonor self-assigned this Oct 17, 2025
@rikonor rikonor force-pushed the or-args branch 2 times, most recently from 18028ae to 23ca480 Compare October 20, 2025 15:48
@rikonor rikonor marked this pull request as ready for review October 20, 2025 16:27
@rikonor rikonor requested a review from a team as a code owner October 20, 2025 16:27
Comment on lines +111 to +114
// Argument (Canister)
let args::Canister::Name(name) = &args.canister else {
return Err(CommandError::Args);
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should forbid using principals in projects. Otherwise to talk to a specific canister on a non-default network one has to somehow hook up the principal to a name just for one-off calls

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternate suggestion: #157


// Ensure canister is included in the environment
if !env.canisters.contains_key(&args.name) {
if !env.canisters.contains_key(name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similarly, this should probably only happen if a non-principal was supplied

Comment on lines +28 to +29
#[error("an invalid argument was provided")]
Args,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is enough detail to give a decent user experience. It's ok to leave this as-is, but at least make a ticket to improve the situation. We can easily say 'only provide one of network and environment, not both' here

Comment on lines +20 to +44
// #[cfg(test)]
// pub(crate) trait IntoOptions<T> {
// fn into_options(self) -> Vec<Option<T>>;
// }

// #[cfg(test)]
// impl<T> IntoOptions<T> for Vec<T> {
// fn into_options(self) -> Vec<Option<T>> {
// self.into_iter().fold(vec![None], |mut acc, cur| {
// acc.push(Some(cur));
// acc
// })
// }
// }

pub(crate) fn all_modes() -> Vec<Mode> {
vec![Mode::Global, Mode::Project("dir".into())]
}

// pub(crate) fn all_networks() -> Vec<args::Network> {
// vec![
// args::Network::Name("my-network".to_string()),
// args::Network::Url("http::/www.example.com".to_string()),
// ]
// }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// #[cfg(test)]
// pub(crate) trait IntoOptions<T> {
// fn into_options(self) -> Vec<Option<T>>;
// }
// #[cfg(test)]
// impl<T> IntoOptions<T> for Vec<T> {
// fn into_options(self) -> Vec<Option<T>> {
// self.into_iter().fold(vec![None], |mut acc, cur| {
// acc.push(Some(cur));
// acc
// })
// }
// }
pub(crate) fn all_modes() -> Vec<Mode> {
vec![Mode::Global, Mode::Project("dir".into())]
}
// pub(crate) fn all_networks() -> Vec<args::Network> {
// vec![
// args::Network::Name("my-network".to_string()),
// args::Network::Url("http::/www.example.com".to_string()),
// ]
// }
pub(crate) fn all_modes() -> Vec<Mode> {
vec![Mode::Global, Mode::Project("dir".into())]
}

}

const PLEASE_PROVIDE_A_CANISTER_PRINCIPAL_IN_GLOBAL_MODE: &str = r#"
Please provide a canister principal in global mode.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not convinced this is the best wording. global mode makes me think of a flag that I could use to switch to local mode.

Suggested change
Please provide a canister principal in global mode.
{user_provided_string} is not a principal. Cannot use non-principals outside of a project.

}

const ENVIRONMENTS_ARE_NOT_AVAILABLE_IN_GLOBAL_MODE: &str = r#"
Environments are not available in global mode.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Environments are not available in global mode.
Environments are not available in outside of projects.

}

const A_NETWORK_URL_IS_REQUIRED_IN_GLOBAL_MODE: &str = r#"
A network `url` is required in global mode.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
A network `url` is required in global mode.
A network `url` is required outside of a project.

@raymondk raymondk marked this pull request as draft October 23, 2025 18:47
@raymondk
Copy link
Copy Markdown
Contributor

We moved away from this approach because it uses icp.yaml to decide the mode. Instead of adopted an approach where we decide the mode based on the command and arguments provided.

@raymondk raymondk closed this Oct 30, 2025
@raymondk raymondk deleted the or-args branch February 24, 2026 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants