From 2595c4da43944629bf9a705f72ec71fa4420cd5f Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Wed, 29 Jul 2026 15:12:05 +0000 Subject: [PATCH 1/3] wip --- .../src/engine_canister.rs | 79 +++++++++++ crates/icp-canister-interfaces/src/lib.rs | 1 + crates/icp-cli/src/operations/create.rs | 128 +++++++++++++++++- 3 files changed, 201 insertions(+), 7 deletions(-) create mode 100644 crates/icp-canister-interfaces/src/engine_canister.rs diff --git a/crates/icp-canister-interfaces/src/engine_canister.rs b/crates/icp-canister-interfaces/src/engine_canister.rs new file mode 100644 index 000000000..989b0d0f8 --- /dev/null +++ b/crates/icp-canister-interfaces/src/engine_canister.rs @@ -0,0 +1,79 @@ +//! Interface to the control-panel **engine-canister** — the registry that maps +//! an engine's subnet to its per-engine **engine-operator** canister. +//! +//! On a `SubnetType::CloudEngine` subnet, canister creation is delegated to the +//! subnet's engine-operator (which exposes a cycles-ledger-compatible +//! `create_canister`). To find that operator the CLI asks the engine-canister +//! "what is the engine-operator id for this subnet?" via +//! [`GET_ENGINE_OPERATOR_BY_SUBNET_METHOD`]. +//! +//! The argument and result are dedicated `opt`-field wrapper records +//! (`…Args` / `…Result`), mirroring the control-panel convention so the +//! interface can grow fields on either side without breaking the wire format. + +use std::env; + +use candid::{CandidType, Principal}; +use serde::Deserialize; + +/// Default engine-canister principal (staging/prod registry). +/// +/// Overridable at runtime via the [`ENGINE_CANISTER_ID_ENV`] environment +/// variable — see [`engine_canister_id`]. +pub const ENGINE_CANISTER_CID: &str = "q6cfj-fyaaa-aaaar-qb77q-cai"; + +/// Environment variable that overrides [`ENGINE_CANISTER_CID`]. +pub const ENGINE_CANISTER_ID_ENV: &str = "ENGINE_CANISTER_ID"; + +/// The engine-canister query that resolves a subnet to its engine-operator id. +pub const GET_ENGINE_OPERATOR_BY_SUBNET_METHOD: &str = "getEngineOperatorBySubnet"; + +/// Resolve the engine-canister principal to talk to. +/// +/// Uses the value of the `ENGINE_CANISTER_ID` environment variable when set and +/// non-empty, otherwise falls back to [`ENGINE_CANISTER_CID`]. Returns an error +/// only when the override is set but not a valid principal. +pub fn engine_canister_id() -> Result { + match env::var(ENGINE_CANISTER_ID_ENV) { + Ok(value) if !value.trim().is_empty() => Principal::from_text(value.trim()) + .map_err(|e| format!("invalid {ENGINE_CANISTER_ID_ENV}: {e}")), + // Unset, empty, or non-UTF-8: use the built-in default. + _ => Ok(Principal::from_text(ENGINE_CANISTER_CID) + .expect("ENGINE_CANISTER_CID is a valid principal")), + } +} + +/// Argument for [`GET_ENGINE_OPERATOR_BY_SUBNET_METHOD`]. +/// +/// A single `opt`-field record so the engine-canister can add inputs later +/// without changing the method's candid type. +#[derive(Clone, Debug, Default, CandidType, Deserialize)] +pub struct GetEngineOperatorBySubnetArgs { + /// The subnet whose engine-operator should be resolved. + pub subnet_id: Option, +} + +/// Result of [`GET_ENGINE_OPERATOR_BY_SUBNET_METHOD`]. +/// +/// `engine_operator_id` is `None` when no live engine is bound to the queried +/// subnet, or the matched engine has no operator recorded yet. The caller +/// treats a `None` here the same as "the subnet does not exist". +#[derive(Clone, Debug, Default, CandidType, Deserialize)] +pub struct GetEngineOperatorBySubnetResult { + /// The per-engine engine-operator canister id for the subnet, if any. + pub engine_operator_id: Option, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn default_engine_canister_id_parses() { + // The built-in default must always be a valid principal. + assert_eq!( + engine_canister_id().unwrap(), + Principal::from_text(ENGINE_CANISTER_CID).unwrap() + ); + } +} diff --git a/crates/icp-canister-interfaces/src/lib.rs b/crates/icp-canister-interfaces/src/lib.rs index 9a95c416e..938756a3b 100644 --- a/crates/icp-canister-interfaces/src/lib.rs +++ b/crates/icp-canister-interfaces/src/lib.rs @@ -1,6 +1,7 @@ pub mod candid_ui; pub mod cycles_ledger; pub mod cycles_minting_canister; +pub mod engine_canister; pub mod governance; pub mod icp_ledger; pub mod internet_identity; diff --git a/crates/icp-cli/src/operations/create.rs b/crates/icp-cli/src/operations/create.rs index 9bc823619..82bb89ffa 100644 --- a/crates/icp-cli/src/operations/create.rs +++ b/crates/icp-cli/src/operations/create.rs @@ -18,6 +18,10 @@ use icp_canister_interfaces::{ CYCLES_LEDGER_PRINCIPAL, CreateCanisterArgs, CreateCanisterResponse, CreationArgs, SubnetSelectionArg, }, + engine_canister::{ + GET_ENGINE_OPERATOR_BY_SUBNET_METHOD, GetEngineOperatorBySubnetArgs, + GetEngineOperatorBySubnetResult, engine_canister_id, + }, cycles_minting_canister::{ CYCLES_MINTING_CANISTER_CID, CYCLES_MINTING_CANISTER_PRINCIPAL, MEMO_CREATE_CANISTER, NotifyCreateCanisterArg, NotifyCreateCanisterResponse, NotifyError, SubnetSelection, @@ -27,7 +31,7 @@ use icp_canister_interfaces::{ use rand::seq::IndexedRandom; use snafu::{OptionExt, ResultExt, Snafu}; use tokio::{select, sync::OnceCell, time::sleep}; -use tracing::info; +use tracing::{info, warn}; use super::proxy::UpdateOrProxyError; use super::proxy_management; @@ -49,6 +53,20 @@ pub enum CreateOperationError { #[snafu(display("failed to get subnet for canister: {source}"))] GetSubnet { source: AgentError }, + #[snafu(display("invalid engine-canister id: {message}"))] + EngineCanisterId { message: String }, + + #[snafu(display("failed to query the engine-canister registry: {source}"))] + EngineCanisterQuery { source: AgentError }, + + #[snafu(display( + "no engine-operator registered for CloudEngine subnet {subnet} in engine-canister {engine_registry}" + ))] + NoEngineOperator { + subnet: Principal, + engine_registry: Principal, + }, + #[snafu(display("registry error: {message}"))] Registry { message: String }, @@ -214,7 +232,19 @@ impl CreateOperation { .await .context(GetSubnetSnafu)?; let cid = if let Some(SubnetType::CloudEngine) = subnet_info.subnet_type() { - self.create_mgmt(settings, selected_subnet).await? + // Optimistically try the engine-operator path, falling back to the + // legacy management-canister path until we're sure the latter is no + // longer needed. + match self.create_on_cloud_engine(settings, selected_subnet).await { + Ok(cid) => cid, + Err(e) => { + warn!( + "engine-operator creation failed ({e}); \ + falling back to management-canister creation" + ); + self.create_mgmt(settings, selected_subnet).await? + } + } } else { self.create_ledger(settings, selected_subnet).await? }; @@ -273,16 +303,100 @@ impl CreateOperation { Ok(cid) } + /// Creation on a `SubnetType::CloudEngine` subnet, via the subnet's + /// engine-operator canister (resolved through the engine-canister registry). + async fn create_on_cloud_engine( + &self, + settings: &CanisterSettings, + subnet: Principal, + ) -> Result { + let operator = self.resolve_engine_operator(subnet).await?; + self.create_via_operator(settings, operator).await + } + + /// Ask the engine-canister registry which engine-operator serves `subnet`. + async fn resolve_engine_operator( + &self, + subnet: Principal, + ) -> Result { + let engine_registry = + engine_canister_id().map_err(|message| CreateOperationError::EngineCanisterId { + message, + })?; + + let arg = GetEngineOperatorBySubnetArgs { + subnet_id: Some(subnet), + }; + let resp = self + .inner + .agent + .query(&engine_registry, GET_ENGINE_OPERATOR_BY_SUBNET_METHOD) + .with_arg(Encode!(&arg).context(CandidEncodeSnafu)?) + .call() + .await + .context(EngineCanisterQuerySnafu)?; + let resp = + Decode!(&resp, GetEngineOperatorBySubnetResult).context(CandidDecodeSnafu)?; + + resp.engine_operator_id.context(NoEngineOperatorSnafu { + subnet, + engine_registry, + }) + } + + /// Delegate creation to a subnet's engine-operator canister. The operator's + /// `create_canister` is byte-compatible with the cycles ledger, so we reuse + /// the same request/response types and simply target the operator principal. + async fn create_via_operator( + &self, + settings: &CanisterSettings, + operator: Principal, + ) -> Result { + let creation_args = CreationArgs { + // CloudEngine subnets only create on themselves; the operator + // ignores subnet selection, so leave it unset. + subnet_selection: None, + settings: Some(settings.clone()), + }; + let arg = CreateCanisterArgs { + from_subaccount: None, + created_at_time: None, + amount: Nat::from(self.cycles()), + creation_args: Some(creation_args), + }; + + let resp = self + .inner + .agent + .update(&operator, "create_canister") + .with_arg(Encode!(&arg).context(CandidEncodeSnafu)?) + .call_and_wait() + .await + .context(AgentSnafu)?; + let resp: CreateCanisterResponse = + Decode!(&resp, CreateCanisterResponse).context(CandidDecodeSnafu)?; + let cid = match resp { + CreateCanisterResponse::Ok { canister_id, .. } => canister_id, + CreateCanisterResponse::Err(err) => { + return CreateCanisterSnafu { + message: err.format_error(self.cycles()), + } + .fail(); + } + }; + Ok(cid) + } + + /// Legacy CloudEngine creation: call the management canister's + /// `create_canister`, routing to the target subnet by its id (subnet-scoped, + /// so no canister on that subnet is needed to derive an effective canister + /// id). Requires subnet-administrator permissions. Kept as a fallback for + /// the engine-operator path. async fn create_mgmt( &self, settings: &CanisterSettings, subnet: Principal, ) -> Result { - // Call the management canister's create_canister, routing the request to - // the target subnet by its id (subnet-scoped, so no canister on that - // subnet is needed to derive an effective canister id) and forwarding the - // settings as a whole via `with_canister_settings`. Subnet-scoped routing - // requires subnet-administrator permissions, which the CloudEngine path has. let mgmt = ManagementCanister::create(&self.inner.agent); let (canister_id,) = mgmt .create_canister() From 54ec20261736364ee8e3cb7bdab6d95833665166 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Wed, 29 Jul 2026 18:37:23 +0000 Subject: [PATCH 2/3] suggestions --- .../src/engine_canister.rs | 16 ++++-- crates/icp-cli/src/operations/create.rs | 50 ++++++++----------- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/crates/icp-canister-interfaces/src/engine_canister.rs b/crates/icp-canister-interfaces/src/engine_canister.rs index 989b0d0f8..ea9effbcc 100644 --- a/crates/icp-canister-interfaces/src/engine_canister.rs +++ b/crates/icp-canister-interfaces/src/engine_canister.rs @@ -11,7 +11,7 @@ //! (`…Args` / `…Result`), mirroring the control-panel convention so the //! interface can grow fields on either side without breaking the wire format. -use std::env; +use std::env::{self, VarError}; use candid::{CandidType, Principal}; use serde::Deserialize; @@ -32,13 +32,21 @@ pub const GET_ENGINE_OPERATOR_BY_SUBNET_METHOD: &str = "getEngineOperatorBySubne /// /// Uses the value of the `ENGINE_CANISTER_ID` environment variable when set and /// non-empty, otherwise falls back to [`ENGINE_CANISTER_CID`]. Returns an error -/// only when the override is set but not a valid principal. +/// when the override is set but invalid — either not a valid principal, or not +/// valid Unicode. Only an absent or empty (whitespace-only) variable uses the +/// default: a configured-but-invalid value must never silently route to the +/// built-in registry (and thus a different environment). pub fn engine_canister_id() -> Result { match env::var(ENGINE_CANISTER_ID_ENV) { Ok(value) if !value.trim().is_empty() => Principal::from_text(value.trim()) .map_err(|e| format!("invalid {ENGINE_CANISTER_ID_ENV}: {e}")), - // Unset, empty, or non-UTF-8: use the built-in default. - _ => Ok(Principal::from_text(ENGINE_CANISTER_CID) + // Set but non-Unicode is still a configured override — reject it rather + // than silently using the default and targeting the wrong environment. + Err(VarError::NotUnicode(_)) => Err(format!( + "invalid {ENGINE_CANISTER_ID_ENV}: not valid Unicode" + )), + // Unset or empty/whitespace-only: use the built-in default. + Ok(_) | Err(VarError::NotPresent) => Ok(Principal::from_text(ENGINE_CANISTER_CID) .expect("ENGINE_CANISTER_CID is a valid principal")), } } diff --git a/crates/icp-cli/src/operations/create.rs b/crates/icp-cli/src/operations/create.rs index 82bb89ffa..2fbe3763c 100644 --- a/crates/icp-cli/src/operations/create.rs +++ b/crates/icp-cli/src/operations/create.rs @@ -18,14 +18,14 @@ use icp_canister_interfaces::{ CYCLES_LEDGER_PRINCIPAL, CreateCanisterArgs, CreateCanisterResponse, CreationArgs, SubnetSelectionArg, }, - engine_canister::{ - GET_ENGINE_OPERATOR_BY_SUBNET_METHOD, GetEngineOperatorBySubnetArgs, - GetEngineOperatorBySubnetResult, engine_canister_id, - }, cycles_minting_canister::{ CYCLES_MINTING_CANISTER_CID, CYCLES_MINTING_CANISTER_PRINCIPAL, MEMO_CREATE_CANISTER, NotifyCreateCanisterArg, NotifyCreateCanisterResponse, NotifyError, SubnetSelection, }, + engine_canister::{ + GET_ENGINE_OPERATOR_BY_SUBNET_METHOD, GetEngineOperatorBySubnetArgs, + GetEngineOperatorBySubnetResult, engine_canister_id, + }, icp_ledger::{ICP_LEDGER_BLOCK_FEE_E8S, ICP_LEDGER_PRINCIPAL}, }; use rand::seq::IndexedRandom; @@ -232,18 +232,24 @@ impl CreateOperation { .await .context(GetSubnetSnafu)?; let cid = if let Some(SubnetType::CloudEngine) = subnet_info.subnet_type() { - // Optimistically try the engine-operator path, falling back to the - // legacy management-canister path until we're sure the latter is no - // longer needed. - match self.create_on_cloud_engine(settings, selected_subnet).await { - Ok(cid) => cid, - Err(e) => { + // Resolve the subnet's engine-operator first. Only a definitive + // "no operator registered for this subnet" resolution failure falls + // back to the legacy management-canister path — every other + // resolution error (invalid registry id, query/decode failure) is + // propagated. Crucially, the fallback is decided *before* any + // operator `create_canister` update is submitted: once we hand off + // to the operator, a failure may mean the canister was already + // created, so retrying via the management canister could + // double-create and double-charge. Those errors propagate too. + match self.resolve_engine_operator(selected_subnet).await { + Ok(operator) => self.create_via_operator(settings, operator).await?, + Err(e @ CreateOperationError::NoEngineOperator { .. }) => { warn!( - "engine-operator creation failed ({e}); \ - falling back to management-canister creation" + "{e}; falling back to management-canister creation on this CloudEngine subnet" ); self.create_mgmt(settings, selected_subnet).await? } + Err(e) => return Err(e), } } else { self.create_ledger(settings, selected_subnet).await? @@ -303,26 +309,13 @@ impl CreateOperation { Ok(cid) } - /// Creation on a `SubnetType::CloudEngine` subnet, via the subnet's - /// engine-operator canister (resolved through the engine-canister registry). - async fn create_on_cloud_engine( - &self, - settings: &CanisterSettings, - subnet: Principal, - ) -> Result { - let operator = self.resolve_engine_operator(subnet).await?; - self.create_via_operator(settings, operator).await - } - /// Ask the engine-canister registry which engine-operator serves `subnet`. async fn resolve_engine_operator( &self, subnet: Principal, ) -> Result { - let engine_registry = - engine_canister_id().map_err(|message| CreateOperationError::EngineCanisterId { - message, - })?; + let engine_registry = engine_canister_id() + .map_err(|message| CreateOperationError::EngineCanisterId { message })?; let arg = GetEngineOperatorBySubnetArgs { subnet_id: Some(subnet), @@ -335,8 +328,7 @@ impl CreateOperation { .call() .await .context(EngineCanisterQuerySnafu)?; - let resp = - Decode!(&resp, GetEngineOperatorBySubnetResult).context(CandidDecodeSnafu)?; + let resp = Decode!(&resp, GetEngineOperatorBySubnetResult).context(CandidDecodeSnafu)?; resp.engine_operator_id.context(NoEngineOperatorSnafu { subnet, From 1ed1f58ef7cfa85551b0cf7748c171781ea9cd36 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Wed, 29 Jul 2026 18:55:13 +0000 Subject: [PATCH 3/3] handling properly missing engine registry canister --- crates/icp-cli/src/operations/create.rs | 96 ++++++++++++++++++++++--- 1 file changed, 85 insertions(+), 11 deletions(-) diff --git a/crates/icp-cli/src/operations/create.rs b/crates/icp-cli/src/operations/create.rs index 2fbe3763c..d0f9163ed 100644 --- a/crates/icp-cli/src/operations/create.rs +++ b/crates/icp-cli/src/operations/create.rs @@ -3,7 +3,7 @@ use std::time::Duration; use bigdecimal::{BigDecimal, ToPrimitive}; use candid::{Decode, Encode, IDLArgs, IDLValue, Nat, Principal}; -use ic_agent::{Agent, AgentError, agent::SubnetType}; +use ic_agent::{Agent, AgentError, agent::RejectCode, agent::SubnetType}; use ic_ledger_types::{ AccountIdentifier, Memo, Subaccount, Tokens, TransferArgs, TransferError, TransferResult, }; @@ -60,7 +60,7 @@ pub enum CreateOperationError { EngineCanisterQuery { source: AgentError }, #[snafu(display( - "no engine-operator registered for CloudEngine subnet {subnet} in engine-canister {engine_registry}" + "could not resolve an engine-operator for CloudEngine subnet {subnet} via engine-canister {engine_registry} (no operator registered, or the registry is not deployed on this network)" ))] NoEngineOperator { subnet: Principal, @@ -233,13 +233,16 @@ impl CreateOperation { .context(GetSubnetSnafu)?; let cid = if let Some(SubnetType::CloudEngine) = subnet_info.subnet_type() { // Resolve the subnet's engine-operator first. Only a definitive - // "no operator registered for this subnet" resolution failure falls - // back to the legacy management-canister path — every other - // resolution error (invalid registry id, query/decode failure) is - // propagated. Crucially, the fallback is decided *before* any - // operator `create_canister` update is submitted: once we hand off - // to the operator, a failure may mean the canister was already - // created, so retrying via the management canister could + // "could not resolve an operator" resolution failure falls back to + // the legacy management-canister path — this covers both no operator + // being registered for the subnet and the engine-canister registry + // not being deployed on this network (a `DestinationInvalid` reject, + // mapped to `NoEngineOperator` in `resolve_engine_operator`). Every + // other resolution error (invalid registry id, other query/decode + // failures) is propagated. Crucially, the fallback is decided + // *before* any operator `create_canister` update is submitted: once + // we hand off to the operator, a failure may mean the canister was + // already created, so retrying via the management canister could // double-create and double-charge. Those errors propagate too. match self.resolve_engine_operator(selected_subnet).await { Ok(operator) => self.create_via_operator(settings, operator).await?, @@ -320,14 +323,30 @@ impl CreateOperation { let arg = GetEngineOperatorBySubnetArgs { subnet_id: Some(subnet), }; - let resp = self + let resp = match self .inner .agent .query(&engine_registry, GET_ENGINE_OPERATOR_BY_SUBNET_METHOD) .with_arg(Encode!(&arg).context(CandidEncodeSnafu)?) .call() .await - .context(EngineCanisterQuerySnafu)?; + { + Ok(resp) => resp, + // The engine-canister registry itself is not deployed on this + // network (e.g. a local/test env). The replica rejects the query + // with `DestinationInvalid` ("Canister ... not found", IC0301). + // Treat a missing registry the same as "no operator registered": + // this happens before any operator update is submitted, so the + // legacy management-canister fallback is safe. + Err(e) if is_canister_not_found(&e) => { + return NoEngineOperatorSnafu { + subnet, + engine_registry, + } + .fail(); + } + Err(e) => return Err(e).context(EngineCanisterQuerySnafu), + }; let resp = Decode!(&resp, GetEngineOperatorBySubnetResult).context(CandidDecodeSnafu)?; resp.engine_operator_id.context(NoEngineOperatorSnafu { @@ -647,6 +666,18 @@ pub(crate) fn shell_quote(value: &str) -> String { format!("'{}'", value.replace('\'', r"'\''")) } +/// Whether `err` is a replica rejection meaning the target canister does not +/// exist on this network — reject code `DestinationInvalid` (IC0301, +/// "Canister ... not found"). Used to treat a missing engine-canister registry +/// as "no operator registered" so the CloudEngine path can fall back safely. +fn is_canister_not_found(err: &AgentError) -> bool { + matches!( + err, + AgentError::CertifiedReject { reject, .. } | AgentError::UncertifiedReject { reject, .. } + if reject.reject_code == RejectCode::DestinationInvalid + ) +} + async fn get_available_subnets(agent: &Agent) -> Result, CreateOperationError> { let bs = agent .query(&CYCLES_MINTING_CANISTER_PRINCIPAL, "get_default_subnets") @@ -713,4 +744,47 @@ mod tests { // An embedded single quote is closed, escaped, and reopened via `'\''`. assert_eq!(shell_quote("a'b"), r"'a'\''b'"); } + + #[test] + fn detects_canister_not_found_rejections() { + use ic_agent::agent::RejectResponse; + + let not_found = |certified: bool| { + let reject = RejectResponse { + reject_code: RejectCode::DestinationInvalid, + reject_message: "Canister q6cfj-fyaaa-aaaar-qb77q-cai not found".to_string(), + error_code: Some("IC0301".to_string()), + }; + if certified { + AgentError::CertifiedReject { + reject, + operation: None, + } + } else { + AgentError::UncertifiedReject { + reject, + operation: None, + } + } + }; + + // A `DestinationInvalid` rejection means the registry canister is not + // deployed — both the certified and uncertified spellings count. + assert!(is_canister_not_found(¬_found(true))); + assert!(is_canister_not_found(¬_found(false))); + + // Other reject codes (e.g. a canister trap) must NOT be treated as + // "not found" — they should propagate rather than fall back. + assert!(!is_canister_not_found(&AgentError::CertifiedReject { + reject: RejectResponse { + reject_code: RejectCode::CanisterError, + reject_message: "trapped".to_string(), + error_code: None, + }, + operation: None, + })); + + // A non-reject error is never "not found". + assert!(!is_canister_not_found(&AgentError::InvalidReplicaStatus)); + } }