From 7a63f454edce7a7f37534b58b3c00342e6f283c1 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Wed, 9 Sep 2026 10:53:56 -0700 Subject: [PATCH 01/13] refactor: reach canisters through a seam shaped like the plugin interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every operation in `icp-project` talked to canisters through an `ic_agent::Agent`, and threaded an `Option` proxy alongside it into about sixty signatures. Neither can exist where this crate is meant to end up running. `calls::CanisterCalls` is that surface now, modelled on what the sync-plugin WIT world already exposes to a guest — submit a call, read a certified fact — because that is the irreducible set everything else is built from. Certification is not the caller's business: a reader is *assumed* to return certified answers, and verifying whatever proof that took belongs to the implementation. Each certified fact gets its own method rather than a general state-tree read, since a caller running inside a canister cannot read the state tree and reaches the same facts through management-canister calls: `metadata_section`, `controllers`, `module_hash`, `subnet_of`, `subnet_uses_engine_operator`. `AgentCalls` in `icp-app` is the implementation, and it owns three things the project layer had been carrying: - **Proxy routing.** `--proxy` applies to a whole command, so it is a property of the caller, not of each call. It leaves every operation signature. A query through a proxy necessarily becomes an update, which is why the trait leaves how a query is answered to the implementation and `fetch_canister_logs` simply asks for the query it is. - **Subnet-scoped submission.** `RouteTo::Subnet` replaces the signed submit-and-poll dance `create.rs` spelled out. - **Distinguishing an absent metadata section from an absent canister**, by the controllers cross-check the plugin runtime already used. `CallError` says whether the network reached a verdict, which is the distinction callers actually branch on. The three classifiers that used to match `AgentError` variants — is this canister serving queries, does it have an `http_request`, was management access refused — now read a code off a rejection, and their tests got shorter for it. The snapshot transfer's retry rule follows from the same distinction: retry what reached no verdict, never a rejection. `operations/proxy.rs` is gone; its routing lives in the implementation and its typed-call helper in `calls`. What remains agent-shaped is the sync path: `Synchronize::sync` still takes one because the wasmtime plugin runtime does. That is the next stage, and it is why the trait above is shaped the way the WIT interface is — the runtime will take this same seam. --- crates/icp-app/src/calls.rs | 242 ++++++++++++ crates/icp-app/src/lib.rs | 1 + .../src/operations/snapshot_transfer.rs | 89 ++--- crates/icp-cli/src/call_output.rs | 5 +- crates/icp-cli/src/commands/canister/call.rs | 29 +- .../icp-cli/src/commands/canister/create.rs | 11 +- .../icp-cli/src/commands/canister/delete.rs | 10 +- .../icp-cli/src/commands/canister/install.rs | 12 +- crates/icp-cli/src/commands/canister/logs.rs | 18 +- .../icp-cli/src/commands/canister/metadata.rs | 3 +- .../src/commands/canister/migrate_id.rs | 17 +- .../src/commands/canister/settings/show.rs | 11 +- .../src/commands/canister/settings/sync.rs | 4 +- .../src/commands/canister/settings/update.rs | 8 +- .../src/commands/canister/snapshot/create.rs | 13 +- .../src/commands/canister/snapshot/delete.rs | 4 +- .../commands/canister/snapshot/download.rs | 24 +- .../src/commands/canister/snapshot/list.rs | 5 +- .../src/commands/canister/snapshot/restore.rs | 13 +- .../src/commands/canister/snapshot/upload.rs | 16 +- crates/icp-cli/src/commands/canister/start.rs | 4 +- .../icp-cli/src/commands/canister/status.rs | 159 +++----- crates/icp-cli/src/commands/canister/stop.rs | 4 +- crates/icp-cli/src/commands/deploy.rs | 94 ++--- crates/icp-cli/src/commands/message/send.rs | 5 +- crates/icp-cli/src/commands/sync.rs | 7 +- crates/icp-project/src/agent.rs | 74 ---- crates/icp-project/src/calls.rs | 270 +++++++++++++ crates/icp-project/src/defer.rs | 74 ++++ crates/icp-project/src/lib.rs | 3 +- .../src/operations/binding_env_vars.rs | 21 +- .../src/operations/candid_compat.rs | 17 +- crates/icp-project/src/operations/create.rs | 259 ++++++------ crates/icp-project/src/operations/deploy.rs | 133 +++---- crates/icp-project/src/operations/install.rs | 96 ++--- crates/icp-project/src/operations/misc.rs | 21 +- crates/icp-project/src/operations/mod.rs | 1 - crates/icp-project/src/operations/proxy.rs | 114 ------ .../src/operations/proxy_management.rs | 372 ++++++------------ .../src/operations/recover_cycles.rs | 36 +- crates/icp-project/src/operations/settings.rs | 32 +- crates/icp-project/src/store_id.rs | 2 +- 42 files changed, 1229 insertions(+), 1104 deletions(-) create mode 100644 crates/icp-app/src/calls.rs delete mode 100644 crates/icp-project/src/agent.rs create mode 100644 crates/icp-project/src/calls.rs create mode 100644 crates/icp-project/src/defer.rs delete mode 100644 crates/icp-project/src/operations/proxy.rs diff --git a/crates/icp-app/src/calls.rs b/crates/icp-app/src/calls.rs new file mode 100644 index 000000000..67f1526e8 --- /dev/null +++ b/crates/icp-app/src/calls.rs @@ -0,0 +1,242 @@ +//! Reaching canisters over the IC's HTTP interface. +//! +//! The implementation of [`icp_project::calls::CanisterCalls`] for a machine +//! with an `ic-agent`: it owns the identity the calls are signed with, the +//! endpoint they go to, and the root key their answers are verified against. +//! +//! It also owns *proxy routing*. `--proxy` names a canister that forwards +//! management calls on the caller's behalf, and it applies to the whole +//! command, so it is a property of the caller rather than of any one call. A +//! query made through a proxy necessarily becomes an update, which is why the +//! trait leaves how a query is answered to the implementation. + +use async_trait::async_trait; +use candid::{Encode, Nat, Principal}; +use ic_agent::{ + Agent, AgentError, + agent::{CallResponse, EffectiveId, SubnetType}, +}; +use icp_canister_interfaces::proxy::{ProxyArgs, ProxyResult}; +use icp_project::calls::{Call, CallError, CanisterCalls, RouteTo}; + +/// [`CanisterCalls`] over an `ic-agent`, optionally forwarding through a proxy +/// canister. +pub struct AgentCalls { + agent: Agent, + proxy: Option, + caller: Principal, +} + +impl AgentCalls { + /// Builds a caller from `agent`. `proxy`, when given, is the canister every + /// call is forwarded through. + /// + /// The caller's principal is read once here: it cannot change for the life + /// of the agent, and every call would otherwise ask for it again. + pub fn new(agent: Agent, proxy: Option) -> Result { + let caller = agent + .get_principal() + .map_err(|message| AgentError::MessageError(message.clone()))?; + Ok(Self { + agent, + proxy, + caller, + }) + } + + /// Turns an agent error into the shape the trait speaks in: a rejection is + /// a verdict and keeps its code, anything else reached none. + fn wrap(canister: Principal, method: &str, err: AgentError) -> CallError { + match &err { + AgentError::CertifiedReject { reject, .. } + | AgentError::UncertifiedReject { reject, .. } => CallError::Rejected { + canister, + method: method.to_owned(), + code: reject.error_code.clone(), + message: reject.reject_message.clone(), + }, + _ => CallError::failed(canister, method, err), + } + } + + /// Forwards `call` through the proxy canister, unwrapping its reply. + async fn through_proxy(&self, proxy: Principal, call: &Call) -> Result, CallError> { + let args = ProxyArgs { + canister_id: call.canister, + method: call.method.clone(), + args: call.arg.clone(), + cycles: Nat::from(call.cycles), + }; + let arg = Encode!(&args).map_err(|e| CallError::failed(proxy, "proxy", e))?; + let reply = self + .agent + .update(&proxy, "proxy") + .with_arg(arg) + .await + .map_err(|e| Self::wrap(proxy, "proxy", e))?; + let (result,): (ProxyResult,) = + candid::decode_args(&reply).map_err(|e| CallError::failed(proxy, "proxy", e))?; + match result { + ProxyResult::Ok(ok) => Ok(ok.result), + // The proxy reports the inner call's failure as text; it reached a + // verdict, so it is a rejection even though the code is lost. + ProxyResult::Err(err) => Err(CallError::Rejected { + canister: call.canister, + method: call.method.clone(), + code: None, + message: err.format_error(), + }), + } + } + + /// A subnet-scoped update: routed to a subnet rather than to any canister + /// on it, which the agent only exposes through a signed submission. + async fn to_subnet(&self, subnet: Principal, call: &Call) -> Result, CallError> { + let effective = EffectiveId::Subnet(subnet); + let fail = |e: AgentError| Self::wrap(call.canister, &call.method, e); + + let signed = self + .agent + .update(&call.canister, &call.method) + .with_arg(call.arg.clone()) + .sign() + .map_err(fail)?; + let response = self + .agent + .update_signed(effective, signed.signed_update) + .await + .map_err(fail)?; + match response { + CallResponse::Response(bytes) => Ok(bytes), + CallResponse::Poll(request_id) => { + let status = self + .agent + .sign_request_status(effective, request_id) + .map_err(fail)?; + Ok(self + .agent + .wait_signed(&request_id, effective, status.signed_request_status) + .await + .map_err(fail)? + .0) + } + } + } +} + +/// Wraps a resolved agent as the caller this workspace's operations take, +/// forwarding through `proxy` when one was asked for. +pub fn calls( + agent: Agent, + proxy: Option, +) -> Result, AgentError> { + Ok(std::sync::Arc::new(AgentCalls::new(agent, proxy)?)) +} + +#[async_trait] +impl CanisterCalls for AgentCalls { + fn caller(&self) -> Principal { + self.caller + } + + async fn update(&self, call: Call) -> Result, CallError> { + if let Some(proxy) = self.proxy { + return self.through_proxy(proxy, &call).await; + } + if let RouteTo::Subnet(subnet) = call.route { + return self.to_subnet(subnet, &call).await; + } + let mut builder = self + .agent + .update(&call.canister, &call.method) + .with_arg(call.arg.clone()); + if let RouteTo::Canister(effective) = call.route { + builder = builder.with_effective_canister_id(effective); + } + builder + .await + .map_err(|e| Self::wrap(call.canister, &call.method, e)) + } + + async fn query(&self, call: Call) -> Result, CallError> { + // A proxy only accepts updates, so a query through one becomes an + // update. The reply is the same either way. + if let Some(proxy) = self.proxy { + return self.through_proxy(proxy, &call).await; + } + let mut builder = self + .agent + .query(&call.canister, &call.method) + .with_arg(call.arg.clone()); + if let RouteTo::Canister(effective) = call.route { + builder = builder.with_effective_canister_id(effective); + } + builder + .call() + .await + .map_err(|e| Self::wrap(call.canister, &call.method, e)) + } + + async fn metadata_section( + &self, + canister: Principal, + path: &str, + ) -> Result>, CallError> { + match self + .agent + .read_state_canister_metadata(canister, path) + .await + { + Ok(bytes) => Ok(Some(bytes)), + Err(err) => { + // A path the certificate will not certify looks the same as a + // canister that was never created. `controllers` is written at + // creation, so its presence is what separates "no such section" + // from "no such canister". + if self + .agent + .read_state_canister_controllers(canister) + .await + .is_ok() + { + Ok(None) + } else { + Err(Self::wrap(canister, "read_state(metadata)", err)) + } + } + } + } + + async fn controllers(&self, canister: Principal) -> Result, CallError> { + self.agent + .read_state_canister_controllers(canister) + .await + .map_err(|e| Self::wrap(canister, "read_state(controllers)", e)) + } + + async fn module_hash(&self, canister: Principal) -> Result>, CallError> { + self.agent + .read_state_canister_module_hash(canister) + .await + .map(Some) + .map_err(|e| Self::wrap(canister, "read_state(module_hash)", e)) + } + + async fn subnet_of(&self, canister: Principal) -> Result { + Ok(self + .agent + .get_subnet_by_canister(&canister) + .await + .map_err(|e| Self::wrap(canister, "subnet_of", e))? + .id()) + } + + async fn subnet_uses_engine_operator(&self, subnet: Principal) -> Result { + let info = self + .agent + .get_subnet_by_id(&subnet) + .await + .map_err(|e| Self::wrap(subnet, "subnet_type", e))?; + Ok(matches!(info.subnet_type(), Some(SubnetType::CloudEngine))) + } +} diff --git a/crates/icp-app/src/lib.rs b/crates/icp-app/src/lib.rs index ff33ec2a4..a2fe0bd50 100644 --- a/crates/icp-app/src/lib.rs +++ b/crates/icp-app/src/lib.rs @@ -14,6 +14,7 @@ //! there and implemented here. pub mod agent; +pub mod calls; pub mod context; pub mod directories; pub mod identity; diff --git a/crates/icp-app/src/operations/snapshot_transfer.rs b/crates/icp-app/src/operations/snapshot_transfer.rs index 584dc1198..5009d24a5 100644 --- a/crates/icp-app/src/operations/snapshot_transfer.rs +++ b/crates/icp-app/src/operations/snapshot_transfer.rs @@ -1,11 +1,12 @@ +use icp_project::calls::{CanisterCalls, TypedCallError}; use std::{ collections::{BTreeMap, HashSet}, io::SeekFrom, }; use backoff::{ExponentialBackoff, backoff::Backoff}; +use candid::Principal; use futures::{StreamExt, stream::FuturesUnordered}; -use ic_agent::{Agent, AgentError, export::Principal}; use ic_management_canister_types::{ ChunkHash, ReadCanisterSnapshotDataArgs, ReadCanisterSnapshotMetadataArgs, ReadCanisterSnapshotMetadataResult, SnapshotDataKind, SnapshotDataOffset, @@ -13,7 +14,6 @@ use ic_management_canister_types::{ UploadCanisterSnapshotMetadataResult, }; -use icp_project::operations::proxy::UpdateOrProxyError; use icp_project::operations::proxy_management; use icp_project::operations::task::TaskReporter; use icp_project::{ @@ -107,43 +107,43 @@ pub enum SnapshotTransferError { #[snafu(display("Failed to read snapshot metadata for canister {canister_id}"))] ReadMetadata { canister_id: Principal, - #[snafu(source(from(UpdateOrProxyError, Box::new)))] - source: Box, + #[snafu(source(from(TypedCallError, Box::new)))] + source: Box, }, #[snafu(display("Failed to read snapshot data chunk at offset {offset}"))] ReadDataChunk { offset: u64, - #[snafu(source(from(UpdateOrProxyError, Box::new)))] - source: Box, + #[snafu(source(from(TypedCallError, Box::new)))] + source: Box, }, #[snafu(display("Failed to read WASM chunk with hash {hash}"))] ReadWasmChunk { hash: String, - #[snafu(source(from(UpdateOrProxyError, Box::new)))] - source: Box, + #[snafu(source(from(TypedCallError, Box::new)))] + source: Box, }, #[snafu(display("Failed to upload snapshot metadata for canister {canister_id}"))] UploadMetadata { canister_id: Principal, - #[snafu(source(from(UpdateOrProxyError, Box::new)))] - source: Box, + #[snafu(source(from(TypedCallError, Box::new)))] + source: Box, }, #[snafu(display("Failed to upload snapshot data chunk at offset {offset}"))] UploadDataChunk { offset: u64, - #[snafu(source(from(UpdateOrProxyError, Box::new)))] - source: Box, + #[snafu(source(from(TypedCallError, Box::new)))] + source: Box, }, #[snafu(display("Failed to upload WASM chunk with hash {hash}"))] UploadWasmChunk { hash: String, - #[snafu(source(from(UpdateOrProxyError, Box::new)))] - source: Box, + #[snafu(source(from(TypedCallError, Box::new)))] + source: Box, }, #[snafu(transparent)] @@ -384,28 +384,25 @@ impl BlobType { } } -/// Check if an agent error is retryable. -fn is_retryable_agent_error(error: &AgentError) -> bool { - matches!( - error, - AgentError::TimeoutWaitingForResponse() | AgentError::TransportError(_) - ) -} - -/// Check if an `UpdateOrProxyError` is retryable (by inspecting the inner agent error). -fn is_retryable(error: &UpdateOrProxyError) -> bool { +/// Whether a failed call is worth trying again. +/// +/// A rejection is the network's verdict and will be the same next time. Every +/// other failure means the call reached no verdict — a timeout, a dropped +/// connection — and the transfer can pick up where it left off. Encoding and +/// decoding failures fall on this side too; they are deterministic, so the +/// retry budget absorbs one wasted attempt rather than hiding a real problem. +fn is_retryable(error: &TypedCallError) -> bool { match error { - UpdateOrProxyError::DirectUpdateCall { source } - | UpdateOrProxyError::ProxyUpdateCall { source } => is_retryable_agent_error(source), - _ => false, + TypedCallError::Call { source } => !source.is_rejection(), + TypedCallError::Encode { .. } | TypedCallError::Decode { .. } => false, } } /// Execute an async operation with exponential backoff retry. -async fn with_retry(operation: F) -> Result +async fn with_retry(operation: F) -> Result where F: Fn() -> Fut, - Fut: std::future::Future>, + Fut: std::future::Future>, { let mut backoff = ExponentialBackoff { max_elapsed_time: Some(std::time::Duration::from_secs(60)), @@ -430,8 +427,7 @@ where /// Read snapshot metadata from a canister. pub async fn read_snapshot_metadata( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, snapshot_id: &[u8], ) -> Result { @@ -442,7 +438,7 @@ pub async fn read_snapshot_metadata( let metadata = with_retry(|| { let args = args.clone(); - async move { proxy_management::read_canister_snapshot_metadata(agent, proxy, args).await } + async move { proxy_management::read_canister_snapshot_metadata(calls, args).await } }) .await .context(ReadMetadataSnafu { canister_id })?; @@ -452,8 +448,7 @@ pub async fn read_snapshot_metadata( /// Upload snapshot metadata to create a new snapshot. pub async fn upload_snapshot_metadata( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, metadata: &ReadCanisterSnapshotMetadataResult, replace_snapshot: Option<&[u8]>, @@ -482,7 +477,7 @@ pub async fn upload_snapshot_metadata( let result = with_retry(|| { let args = args.clone(); - async move { proxy_management::upload_canister_snapshot_metadata(agent, proxy, args).await } + async move { proxy_management::upload_canister_snapshot_metadata(calls, args).await } }) .await .context(UploadMetadataSnafu { canister_id })?; @@ -496,8 +491,7 @@ pub async fn upload_snapshot_metadata( /// Tracks progress so gaps can be filled on resume. /// The agent handles rate limiting and semaphoring internally. pub async fn download_blob_to_file( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, snapshot_id: &[u8], blob_type: BlobType, @@ -559,9 +553,7 @@ pub async fn download_blob_to_file( in_progress.push(async move { let result = with_retry(|| { let args = args.clone(); - async move { - proxy_management::read_canister_snapshot_data(agent, proxy, args).await - } + async move { proxy_management::read_canister_snapshot_data(calls, args).await } }) .await .context(ReadDataChunkSnafu { @@ -611,8 +603,7 @@ pub async fn download_blob_to_file( /// Download a single WASM chunk by hash. pub async fn download_wasm_chunk( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, snapshot_id: &[u8], chunk_hash: &ChunkHash, @@ -631,7 +622,7 @@ pub async fn download_wasm_chunk( let result = with_retry(|| { let args = args.clone(); - async move { proxy_management::read_canister_snapshot_data(agent, proxy, args).await } + async move { proxy_management::read_canister_snapshot_data(calls, args).await } }) .await .context(ReadWasmChunkSnafu { hash: &hash_hex })?; @@ -647,8 +638,7 @@ pub async fn download_wasm_chunk( /// Saves progress after each successful chunk for resume support. /// Returns the final byte offset after all uploads complete. pub async fn upload_blob_from_file( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, snapshot_id: &[u8], blob_type: BlobType, @@ -709,9 +699,7 @@ pub async fn upload_blob_from_file( in_progress.push(async move { with_retry(|| { let args = args.clone(); - async move { - proxy_management::upload_canister_snapshot_data(agent, proxy, args).await - } + async move { proxy_management::upload_canister_snapshot_data(calls, args).await } }) .await .context(UploadDataChunkSnafu { offset })?; @@ -765,8 +753,7 @@ pub async fn upload_blob_from_file( /// Upload a single WASM chunk. pub async fn upload_wasm_chunk( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, snapshot_id: &[u8], chunk_hash: &[u8], @@ -786,7 +773,7 @@ pub async fn upload_wasm_chunk( with_retry(|| { let args = args.clone(); - async move { proxy_management::upload_canister_snapshot_data(agent, proxy, args).await } + async move { proxy_management::upload_canister_snapshot_data(calls, args).await } }) .await .context(UploadWasmChunkSnafu { hash: hash_hex })?; diff --git a/crates/icp-cli/src/call_output.rs b/crates/icp-cli/src/call_output.rs index d054b78fa..74d65e093 100644 --- a/crates/icp-cli/src/call_output.rs +++ b/crates/icp-cli/src/call_output.rs @@ -10,7 +10,6 @@ use candid::{IDLArgs, Principal, TypeEnv, types::Function}; use candid_parser::utils::CandidSource; use clap::ValueEnum; use dialoguer::console::Term; -use ic_agent::Agent; use icp_project::prelude::*; use serde::Serialize; use std::io::{self, Write}; @@ -173,10 +172,10 @@ pub(crate) fn print_candid_for_term(term: &mut Term, args: &IDLArgs) -> io::Resu /// - the IDL file can be parsed and type checked in Rust parser; /// - has an actor in the IDL file. If anything fails, it returns None. pub(crate) async fn get_candid_type( - agent: &Agent, + calls: &dyn icp_project::calls::CanisterCalls, canister_id: Principal, ) -> Option { - let candid_interface = fetch_canister_metadata(agent, canister_id, "candid:service").await?; + let candid_interface = fetch_canister_metadata(calls, canister_id, "candid:service").await?; CanisterInterface::from_text(candid_interface).ok() } diff --git a/crates/icp-cli/src/commands/canister/call.rs b/crates/icp-cli/src/commands/canister/call.rs index 8b93132c6..d69b7444e 100644 --- a/crates/icp-cli/src/commands/canister/call.rs +++ b/crates/icp-cli/src/commands/canister/call.rs @@ -8,6 +8,7 @@ use icp_app::context::{Context, NetworkSelection}; use icp_app::signed_message::{ self, CallType, Destination, Request, SignedMessage, Summary, WindowState, }; +use icp_project::calls::Call; use icp_project::host::EnvironmentSelection; use icp_project::manifest::ArgsFormat; use icp_project::network::{Configuration as NetworkConfiguration, RootKeySpec}; @@ -19,9 +20,7 @@ use time::{OffsetDateTime, format_description::well_known::Rfc3339}; use tracing::warn; use url::Url; -use icp_project::operations::{ - create::shell_quote, proxy::update_or_proxy_raw, wasm::extract_candid_service, -}; +use icp_project::operations::{create::shell_quote, wasm::extract_candid_service}; use crate::{ call_output::{ @@ -171,7 +170,10 @@ pub(crate) async fn exec(ctx: &Context, args: &CallArgs) -> Result<(), anyhow::E let candid_types = match (&args.candid, &agent) { (Some(path), _) => Some(load_candid_from_file(path)?), - (None, Some(agent)) => get_candid_type(agent, cid).await, + (None, Some(agent)) => { + let calls = icp_app::calls::calls(agent.clone(), None)?; + get_candid_type(calls.as_ref(), cid).await + } // Fetching `candid:service` is a network round trip, so signing falls // back to the interface of whatever this project last built. (None, None) => local_candid_type(ctx, &selections.canister).await, @@ -294,23 +296,12 @@ pub(crate) async fn exec(ctx: &Context, args: &CallArgs) -> Result<(), anyhow::E } let agent = agent.expect("an agent is built whenever the call is submitted"); + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; + let call = Call::new(cid, &method, arg_bytes).with_cycles(args.cycles.get()); let res = if args.query { - agent - .query(&cid, &method) - .with_arg(arg_bytes) - .call() - .await? + calls.query(call).await? } else { - update_or_proxy_raw( - &agent, - cid, - &method, - arg_bytes, - args.proxy, - None, - args.cycles.get(), - ) - .await? + calls.update(call).await? }; print_response(&res, args.output, declared_method.as_ref(), args.json) diff --git a/crates/icp-cli/src/commands/canister/create.rs b/crates/icp-cli/src/commands/canister/create.rs index a2502fd1e..efbd41f11 100644 --- a/crates/icp-cli/src/commands/canister/create.rs +++ b/crates/icp-cli/src/commands/canister/create.rs @@ -305,8 +305,10 @@ async fn create_canister(ctx: &Context, args: &CreateArgs) -> Result<(), anyhow: ) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; + let create_operation = - CreateOperation::new(agent, args.create_target(), args.funding(), vec![]); + CreateOperation::new(calls, args.create_target(), args.funding(), vec![]); let canister_settings = args.canister_settings(); @@ -370,8 +372,10 @@ async fn create_project_canister(ctx: &Context, args: &CreateArgs) -> Result<(), .await .map_err(|e| anyhow!(e))?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; + let create_operation = CreateOperation::new( - agent.clone(), + calls.clone(), args.create_target(), args.funding(), ids.values().copied().collect(), @@ -394,8 +398,7 @@ async fn create_project_canister(ctx: &Context, args: &CreateArgs) -> Result<(), icp_project::operations::settings::sync_controller_dependents( &ctx.host, - &agent, - args.proxy, + calls.as_ref(), &canister, &selections.environment, ) diff --git a/crates/icp-cli/src/commands/canister/delete.rs b/crates/icp-cli/src/commands/canister/delete.rs index 9d32cca9b..922962f4f 100644 --- a/crates/icp-cli/src/commands/canister/delete.rs +++ b/crates/icp-cli/src/commands/canister/delete.rs @@ -38,6 +38,8 @@ pub(crate) async fn exec(ctx: &Context, args: &DeleteArgs) -> Result<(), anyhow: &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -51,8 +53,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DeleteArgs) -> Result<(), anyhow: .get_principal() .map_err(|e| anyhow!("could not determine caller principal: {e}"))?; recover_cycles::recover_cycles_before_delete( - &agent, - args.proxy, + calls.as_ref(), cid, destination, crate::artifacts::get_recover_cycles_wasm(), @@ -62,9 +63,8 @@ pub(crate) async fn exec(ctx: &Context, args: &DeleteArgs) -> Result<(), anyhow: // delete_canister requires the canister be stopped; stopping an // already-stopped canister is a no-op, and the recovery step leaves it running. - proxy_management::stop_canister(&agent, args.proxy, CanisterIdRecord { canister_id: cid }) - .await?; - proxy_management::delete_canister(&agent, args.proxy, CanisterIdRecord { canister_id: cid }) + proxy_management::stop_canister(calls.as_ref(), CanisterIdRecord { canister_id: cid }).await?; + proxy_management::delete_canister(calls.as_ref(), CanisterIdRecord { canister_id: cid }) .await?; // Remove canister ID from the id store if it was referenced by name diff --git a/crates/icp-cli/src/commands/canister/install.rs b/crates/icp-cli/src/commands/canister/install.rs index ed917af29..60e62163a 100644 --- a/crates/icp-cli/src/commands/canister/install.rs +++ b/crates/icp-cli/src/commands/canister/install.rs @@ -91,6 +91,8 @@ pub(crate) async fn exec(ctx: &Context, args: &InstallArgs) -> Result<(), anyhow &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let canister_id = ctx .get_canister_id( &selections.canister, @@ -112,7 +114,7 @@ pub(crate) async fn exec(ctx: &Context, args: &InstallArgs) -> Result<(), anyhow args.mode ); } - if !is_eop_canister(&agent, &canister_id).await { + if !is_eop_canister(calls.as_ref(), &canister_id).await { bail!( "--wasm-memory-persistence only applies to Motoko canisters with enhanced \ orthogonal persistence (EOP). The target canister is not an EOP canister." @@ -148,8 +150,7 @@ pub(crate) async fn exec(ctx: &Context, args: &InstallArgs) -> Result<(), anyhow let canister_display = args.cmd_args.canister.to_string(); let (install_mode, status) = resolve_install_mode_and_status( - &agent, - args.proxy, + calls.as_ref(), &canister_display, &canister_id, &args.mode, @@ -158,7 +159,7 @@ pub(crate) async fn exec(ctx: &Context, args: &InstallArgs) -> Result<(), anyhow // Candid interface compatibility check for upgrades if !args.yes && matches!(install_mode, CanisterInstallMode::Upgrade(_)) { - match check_candid_compatibility(&agent, &canister_id, &wasm).await { + match check_candid_compatibility(calls.as_ref(), &canister_id, &wasm).await { CandidCompatibility::Compatible | CandidCompatibility::Skipped(_) => {} CandidCompatibility::Incompatible(details) => { let warning = format!( @@ -186,8 +187,7 @@ pub(crate) async fn exec(ctx: &Context, args: &InstallArgs) -> Result<(), anyhow } install_canister( - &agent, - args.proxy, + calls.as_ref(), &canister_id, &canister_display, &wasm, diff --git a/crates/icp-cli/src/commands/canister/logs.rs b/crates/icp-cli/src/commands/canister/logs.rs index f51c3986b..cfe89fc5b 100644 --- a/crates/icp-cli/src/commands/canister/logs.rs +++ b/crates/icp-cli/src/commands/canister/logs.rs @@ -1,9 +1,9 @@ +use icp_project::calls::CanisterCalls; use std::io::{ErrorKind, Write as _, stdout}; use anyhow::{Context as _, anyhow}; use candid::Principal; use clap::Args; -use ic_agent::Agent; use ic_management_canister_types::{CanisterLogFilter, CanisterLogRecord, FetchCanisterLogsArgs}; use icp_app::context::Context; use icp_project::signal::stop_signal; @@ -102,6 +102,8 @@ pub(crate) async fn exec(ctx: &Context, args: &LogsArgs) -> Result<(), anyhow::E ) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; + let canister_id = ctx .get_canister_id( &selections.canister, @@ -112,10 +114,10 @@ pub(crate) async fn exec(ctx: &Context, args: &LogsArgs) -> Result<(), anyhow::E if args.follow { // Follow mode: continuously fetch and display new logs - follow_logs(args, &agent, args.proxy, &canister_id, args.interval).await + follow_logs(args, calls.as_ref(), &canister_id, args.interval).await } else { // Single fetch mode: fetch all logs once - fetch_and_display_logs(args, &agent, args.proxy, &canister_id, build_filter(args)?).await + fetch_and_display_logs(args, calls.as_ref(), &canister_id, build_filter(args)?).await } } @@ -166,8 +168,7 @@ fn build_filter(args: &LogsArgs) -> Result, anyhow::Er async fn fetch_and_display_logs( args: &LogsArgs, - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: &candid::Principal, filter: Option, ) -> Result<(), anyhow::Error> { @@ -175,7 +176,7 @@ async fn fetch_and_display_logs( canister_id: *canister_id, filter, }; - let result = proxy_management::fetch_canister_logs(agent, proxy, fetch_args) + let result = proxy_management::fetch_canister_logs(calls, fetch_args) .await .context("Failed to fetch canister logs")?; @@ -216,8 +217,7 @@ const FOLLOW_LOOKBACK_NANOS: u64 = 60 * 60 * 1_000_000_000; // 1 hour async fn follow_logs( args: &LogsArgs, - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: &candid::Principal, interval_seconds: u64, ) -> Result<(), anyhow::Error> { @@ -247,7 +247,7 @@ async fn follow_logs( canister_id: *canister_id, filter, }; - let result = proxy_management::fetch_canister_logs(agent, proxy, fetch_args) + let result = proxy_management::fetch_canister_logs(calls, fetch_args) .await .context("Failed to fetch canister logs")?; diff --git a/crates/icp-cli/src/commands/canister/metadata.rs b/crates/icp-cli/src/commands/canister/metadata.rs index 42226c15c..b4c065426 100644 --- a/crates/icp-cli/src/commands/canister/metadata.rs +++ b/crates/icp-cli/src/commands/canister/metadata.rs @@ -42,9 +42,10 @@ pub(crate) async fn exec(ctx: &Context, args: &MetadataArgs) -> Result<(), anyho &selections.environment, ) .await?; + let calls = icp_app::calls::calls(agent.clone(), None)?; // Fetch the metadata - let metadata = fetch_canister_metadata(&agent, canister_id, &args.metadata_name).await; + let metadata = fetch_canister_metadata(calls.as_ref(), canister_id, &args.metadata_name).await; match metadata { Some(value) => { diff --git a/crates/icp-cli/src/commands/canister/migrate_id.rs b/crates/icp-cli/src/commands/canister/migrate_id.rs index 51bed3850..6055de004 100644 --- a/crates/icp-cli/src/commands/canister/migrate_id.rs +++ b/crates/icp-cli/src/commands/canister/migrate_id.rs @@ -65,6 +65,8 @@ pub(crate) async fn exec(ctx: &Context, args: &MigrateIdArgs) -> Result<(), anyh ) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; + let source_cid = ctx .get_canister_id( &selections.canister, @@ -114,16 +116,14 @@ pub(crate) async fn exec(ctx: &Context, args: &MigrateIdArgs) -> Result<(), anyh // Fetch status of both canisters let source_status = proxy_management::canister_status( - &agent, - args.proxy, + calls.as_ref(), CanisterIdRecord { canister_id: source_cid, }, ) .await?; let target_status = proxy_management::canister_status( - &agent, - args.proxy, + calls.as_ref(), CanisterIdRecord { canister_id: target_cid, }, @@ -176,8 +176,7 @@ pub(crate) async fn exec(ctx: &Context, args: &MigrateIdArgs) -> Result<(), anyh // Check target canister has no snapshots let snapshots = proxy_management::list_canister_snapshots( - &agent, - args.proxy, + calls.as_ref(), CanisterIdRecord { canister_id: target_cid, }, @@ -215,8 +214,7 @@ pub(crate) async fn exec(ctx: &Context, args: &MigrateIdArgs) -> Result<(), anyh let mut new_controllers = source_controllers; new_controllers.push(NNS_MIGRATION_PRINCIPAL); proxy_management::update_settings( - &agent, - args.proxy, + calls.as_ref(), UpdateSettingsArgs { canister_id: source_cid, settings: CanisterSettings { @@ -235,8 +233,7 @@ pub(crate) async fn exec(ctx: &Context, args: &MigrateIdArgs) -> Result<(), anyh let mut new_controllers = target_controllers; new_controllers.push(NNS_MIGRATION_PRINCIPAL); proxy_management::update_settings( - &agent, - args.proxy, + calls.as_ref(), UpdateSettingsArgs { canister_id: target_cid, settings: CanisterSettings { diff --git a/crates/icp-cli/src/commands/canister/settings/show.rs b/crates/icp-cli/src/commands/canister/settings/show.rs index 0d6e85773..fa2fc852f 100644 --- a/crates/icp-cli/src/commands/canister/settings/show.rs +++ b/crates/icp-cli/src/commands/canister/settings/show.rs @@ -49,12 +49,11 @@ pub(crate) async fn exec(ctx: &Context, args: &ShowArgs) -> Result<(), anyhow::E ) .await?; - let result = proxy_management::canister_status( - &agent, - args.proxy, - CanisterIdRecord { canister_id: cid }, - ) - .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; + + let result = + proxy_management::canister_status(calls.as_ref(), CanisterIdRecord { canister_id: cid }) + .await?; let output = if args.json_format { serde_json::to_string(&result.settings).expect("Serializing settings to json failed") diff --git a/crates/icp-cli/src/commands/canister/settings/sync.rs b/crates/icp-cli/src/commands/canister/settings/sync.rs index 392a41553..882c9013d 100644 --- a/crates/icp-cli/src/commands/canister/settings/sync.rs +++ b/crates/icp-cli/src/commands/canister/settings/sync.rs @@ -36,6 +36,8 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), anyhow::E &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -50,7 +52,7 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), anyhow::E .map_err(|e| anyhow::anyhow!(e))?; let unresolved = - icp_project::operations::settings::sync_settings(&agent, args.proxy, &cid, &canister, &ids) + icp_project::operations::settings::sync_settings(calls.as_ref(), &cid, &canister, &ids) .await?; for controller_name in &unresolved { warn!( diff --git a/crates/icp-cli/src/commands/canister/settings/update.rs b/crates/icp-cli/src/commands/canister/settings/update.rs index d1c0d8713..af73992b4 100644 --- a/crates/icp-cli/src/commands/canister/settings/update.rs +++ b/crates/icp-cli/src/commands/canister/settings/update.rs @@ -404,6 +404,8 @@ pub(crate) async fn exec(ctx: &Context, args: &UpdateArgs) -> Result<(), anyhow: &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -426,8 +428,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UpdateArgs) -> Result<(), anyhow: if require_current_settings(args) { current_status = Some( proxy_management::canister_status( - &agent, - args.proxy, + calls.as_ref(), CanisterIdRecord { canister_id: cid }, ) .await?, @@ -589,8 +590,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UpdateArgs) -> Result<(), anyhow: }; proxy_management::update_settings( - &agent, - args.proxy, + calls.as_ref(), UpdateSettingsArgs { canister_id: cid, settings, diff --git a/crates/icp-cli/src/commands/canister/snapshot/create.rs b/crates/icp-cli/src/commands/canister/snapshot/create.rs index ec4c3e9c3..5c30b1ad0 100644 --- a/crates/icp-cli/src/commands/canister/snapshot/create.rs +++ b/crates/icp-cli/src/commands/canister/snapshot/create.rs @@ -48,6 +48,8 @@ pub(crate) async fn exec(ctx: &Context, args: &CreateArgs) -> Result<(), anyhow: &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -58,12 +60,9 @@ pub(crate) async fn exec(ctx: &Context, args: &CreateArgs) -> Result<(), anyhow: // Check canister status - must be stopped to create a snapshot let name = &args.cmd_args.canister; - let status = proxy_management::canister_status( - &agent, - args.proxy, - CanisterIdRecord { canister_id: cid }, - ) - .await?; + let status = + proxy_management::canister_status(calls.as_ref(), CanisterIdRecord { canister_id: cid }) + .await?; match status.status { CanisterStatusType::Running => { bail!( @@ -83,7 +82,7 @@ pub(crate) async fn exec(ctx: &Context, args: &CreateArgs) -> Result<(), anyhow: sender_canister_version: None, }; - let snapshot = proxy_management::take_canister_snapshot(&agent, args.proxy, take_args).await?; + let snapshot = proxy_management::take_canister_snapshot(calls.as_ref(), take_args).await?; if args.json { serde_json::to_writer( stdout(), diff --git a/crates/icp-cli/src/commands/canister/snapshot/delete.rs b/crates/icp-cli/src/commands/canister/snapshot/delete.rs index da34a6e9b..1f39a5165 100644 --- a/crates/icp-cli/src/commands/canister/snapshot/delete.rs +++ b/crates/icp-cli/src/commands/canister/snapshot/delete.rs @@ -32,6 +32,8 @@ pub(crate) async fn exec(ctx: &Context, args: &DeleteArgs) -> Result<(), anyhow: &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -45,7 +47,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DeleteArgs) -> Result<(), anyhow: snapshot_id: args.snapshot_id.0.clone(), }; - proxy_management::delete_canister_snapshot(&agent, args.proxy, delete_args).await?; + proxy_management::delete_canister_snapshot(calls.as_ref(), delete_args).await?; let name = &args.cmd_args.canister; info!( diff --git a/crates/icp-cli/src/commands/canister/snapshot/download.rs b/crates/icp-cli/src/commands/canister/snapshot/download.rs index 9461740cc..14f9eac88 100644 --- a/crates/icp-cli/src/commands/canister/snapshot/download.rs +++ b/crates/icp-cli/src/commands/canister/snapshot/download.rs @@ -49,6 +49,8 @@ pub(crate) async fn exec(ctx: &Context, args: &DownloadArgs) -> Result<(), anyho &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -92,7 +94,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DownloadArgs) -> Result<(), anyho id = hex::encode(snapshot_id), ); - let metadata = read_snapshot_metadata(&agent, args.proxy, cid, snapshot_id).await?; + let metadata = read_snapshot_metadata(calls.as_ref(), cid, snapshot_id).await?; info!( " Timestamp: {}", @@ -134,8 +136,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DownloadArgs) -> Result<(), anyho ), async |task| { download_blob_to_file( - &agent, - args.proxy, + calls.as_ref(), cid, snapshot_id, BlobType::WasmModule, @@ -166,8 +167,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DownloadArgs) -> Result<(), anyho ), async |task| { download_blob_to_file( - &agent, - args.proxy, + calls.as_ref(), cid, snapshot_id, BlobType::WasmMemory, @@ -201,8 +201,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DownloadArgs) -> Result<(), anyho ), async |task| { download_blob_to_file( - &agent, - args.proxy, + calls.as_ref(), cid, snapshot_id, BlobType::StableMemory, @@ -233,15 +232,8 @@ pub(crate) async fn exec(ctx: &Context, args: &DownloadArgs) -> Result<(), anyho for chunk_hash in &metadata.wasm_chunk_store { let chunk_path = paths.wasm_chunk_path(&chunk_hash.hash); if !chunk_path.exists() { - download_wasm_chunk( - &agent, - args.proxy, - cid, - snapshot_id, - chunk_hash, - paths, - ) - .await?; + download_wasm_chunk(calls.as_ref(), cid, snapshot_id, chunk_hash, paths) + .await?; } } info!("WASM chunks: done"); diff --git a/crates/icp-cli/src/commands/canister/snapshot/list.rs b/crates/icp-cli/src/commands/canister/snapshot/list.rs index 9b5e1b781..2f4b98e8c 100644 --- a/crates/icp-cli/src/commands/canister/snapshot/list.rs +++ b/crates/icp-cli/src/commands/canister/snapshot/list.rs @@ -40,6 +40,8 @@ pub(crate) async fn exec(ctx: &Context, args: &ListArgs) -> Result<(), anyhow::E &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -49,8 +51,7 @@ pub(crate) async fn exec(ctx: &Context, args: &ListArgs) -> Result<(), anyhow::E .await?; let snapshots = proxy_management::list_canister_snapshots( - &agent, - args.proxy, + calls.as_ref(), CanisterIdRecord { canister_id: cid }, ) .await?; diff --git a/crates/icp-cli/src/commands/canister/snapshot/restore.rs b/crates/icp-cli/src/commands/canister/snapshot/restore.rs index 2282da795..cde5a1d3e 100644 --- a/crates/icp-cli/src/commands/canister/snapshot/restore.rs +++ b/crates/icp-cli/src/commands/canister/snapshot/restore.rs @@ -35,6 +35,8 @@ pub(crate) async fn exec(ctx: &Context, args: &RestoreArgs) -> Result<(), anyhow &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -45,12 +47,9 @@ pub(crate) async fn exec(ctx: &Context, args: &RestoreArgs) -> Result<(), anyhow // Check canister status - must be stopped to restore a snapshot let name = &args.cmd_args.canister; - let status = proxy_management::canister_status( - &agent, - args.proxy, - CanisterIdRecord { canister_id: cid }, - ) - .await?; + let status = + proxy_management::canister_status(calls.as_ref(), CanisterIdRecord { canister_id: cid }) + .await?; match status.status { CanisterStatusType::Running => { bail!( @@ -69,7 +68,7 @@ pub(crate) async fn exec(ctx: &Context, args: &RestoreArgs) -> Result<(), anyhow sender_canister_version: None, }; - proxy_management::load_canister_snapshot(&agent, args.proxy, load_args).await?; + proxy_management::load_canister_snapshot(calls.as_ref(), load_args).await?; info!( "Restored canister {name} ({cid}) from snapshot {id}", diff --git a/crates/icp-cli/src/commands/canister/snapshot/upload.rs b/crates/icp-cli/src/commands/canister/snapshot/upload.rs index 6f1bb7c63..3a29a7448 100644 --- a/crates/icp-cli/src/commands/canister/snapshot/upload.rs +++ b/crates/icp-cli/src/commands/canister/snapshot/upload.rs @@ -61,6 +61,8 @@ pub(crate) async fn exec(ctx: &Context, args: &UploadArgs) -> Result<(), anyhow: &selections.environment, ) .await?; + + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -111,7 +113,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UploadArgs) -> Result<(), anyhow: // Upload metadata to create a new snapshot let replace_snapshot = args.replace.as_ref().map(|s| s.0.as_slice()); let result = - upload_snapshot_metadata(&agent, args.proxy, cid, &metadata, replace_snapshot) + upload_snapshot_metadata(calls.as_ref(), cid, &metadata, replace_snapshot) .await?; let snapshot_id_hex = hex::encode(&result.snapshot_id); @@ -139,8 +141,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UploadArgs) -> Result<(), anyhow: ), async |task| { upload_blob_from_file( - &agent, - args.proxy, + calls.as_ref(), cid, &snapshot_id_bytes, BlobType::WasmModule, @@ -170,8 +171,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UploadArgs) -> Result<(), anyhow: ), async |task| { upload_blob_from_file( - &agent, - args.proxy, + calls.as_ref(), cid, &snapshot_id_bytes, BlobType::WasmMemory, @@ -201,8 +201,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UploadArgs) -> Result<(), anyhow: ), async |task| { upload_blob_from_file( - &agent, - args.proxy, + calls.as_ref(), cid, &snapshot_id_bytes, BlobType::StableMemory, @@ -230,8 +229,7 @@ pub(crate) async fn exec(ctx: &Context, args: &UploadArgs) -> Result<(), anyhow: let hash_hex = hex::encode(&chunk_hash.hash); if !progress.wasm_chunks_uploaded.contains(&hash_hex) { upload_wasm_chunk( - &agent, - args.proxy, + calls.as_ref(), cid, &snapshot_id_bytes, &chunk_hash.hash, diff --git a/crates/icp-cli/src/commands/canister/start.rs b/crates/icp-cli/src/commands/canister/start.rs index 35a04eaa8..9e18c37ba 100644 --- a/crates/icp-cli/src/commands/canister/start.rs +++ b/crates/icp-cli/src/commands/canister/start.rs @@ -26,6 +26,7 @@ pub(crate) async fn exec(ctx: &Context, args: &StartArgs) -> Result<(), anyhow:: &selections.environment, ) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -34,8 +35,7 @@ pub(crate) async fn exec(ctx: &Context, args: &StartArgs) -> Result<(), anyhow:: ) .await?; - proxy_management::start_canister(&agent, args.proxy, CanisterIdRecord { canister_id: cid }) - .await?; + proxy_management::start_canister(calls.as_ref(), CanisterIdRecord { canister_id: cid }).await?; Ok(()) } diff --git a/crates/icp-cli/src/commands/canister/status.rs b/crates/icp-cli/src/commands/canister/status.rs index 5a36328b7..2113634ab 100644 --- a/crates/icp-cli/src/commands/canister/status.rs +++ b/crates/icp-cli/src/commands/canister/status.rs @@ -1,21 +1,21 @@ use anyhow::{anyhow, bail}; +use candid::Principal; use clap::Args; use clap_complete::ArgValueCandidates; -use ic_agent::{Agent, AgentError, agent::RejectResponse, export::Principal}; use ic_management_canister_types::{CanisterIdRecord, CanisterStatusResult, EnvironmentVariable}; use icp_app::{ context::{Context, NetworkSelection}, identity::IdentitySelection, }; +use icp_project::calls::{CanisterCalls, TypedCallError}; use icp_project::{ canister::Visibility, host::{CanisterSelection, EnvironmentSelection}, }; use serde::Serialize; use std::fmt::Write; -use tracing::debug; -use icp_project::operations::{proxy::UpdateOrProxyError, proxy_management}; +use icp_project::operations::proxy_management; use crate::{ commands::{ @@ -39,13 +39,9 @@ const E_STATUS_ACCESS_DENIED: [&str; 3] = ["IC0512", "IC0541", "IC0542"]; /// The replica checks who may read the status both when accepting the ingress /// message and again during execution, so the same denial arrives uncertified /// from the first and certified from the second. -fn direct_call_reject(err: &UpdateOrProxyError) -> Option<&RejectResponse> { +fn rejection_code(err: &TypedCallError) -> Option<&str> { match err { - UpdateOrProxyError::DirectUpdateCall { - source: - AgentError::CertifiedReject { reject, .. } - | AgentError::UncertifiedReject { reject, .. }, - } => Some(reject), + TypedCallError::Call { source } if source.is_rejection() => source.code(), _ => None, } } @@ -151,61 +147,27 @@ async fn get_principals( Ok(cids) } -async fn read_state_tree_canister_controllers( - agent: &Agent, - cid: Principal, -) -> Result>, anyhow::Error> { - let controllers = match agent.read_state_canister_controllers(cid).await { - Ok(controllers) => controllers, - Err(AgentError::LookupPathAbsent(_)) => { - debug!("Couldn't find a path to the controllers in the state tree for {cid}"); - return Err(anyhow!("Canister {cid} was not found.")); - } - Err(AgentError::InvalidCborData(_)) => { - return Err(anyhow!( - "Invalid cbor data in controllers canister info for canister {cid}" - )); - } - Err(e) => { - return Err(anyhow!( - "Error fetching controllers from the state tree for {cid}: {e}" - )); - } - }; - Ok(Some(controllers)) -} - -/// None can indicate either of these, but we can't tell from here: -/// - the canister doesn't exist -/// - the canister exists but does not have a module installed -async fn read_state_tree_canister_module_hash( - agent: &Agent, - cid: Principal, -) -> Result>, anyhow::Error> { - let module_hash = match agent.read_state_canister_module_hash(cid).await { - Ok(blob) => Some(blob), - Err(AgentError::LookupPathAbsent(_)) => None, - Err(e) => { - return Err(anyhow!( - "Error reading the module hash from the state tree for {cid}: {e}" - )); - } - }; - - Ok(module_hash) -} - +/// The status a canister publishes about itself, for a caller that management +/// access was refused to. +/// +/// Both facts come back certified; whether that took a state-tree read or a +/// management call is the caller implementation's business. async fn build_public_status( - agent: &Agent, + calls: &dyn CanisterCalls, cid: Principal, maybe_name: Option, ) -> Result { - let controllers = match read_state_tree_canister_controllers(agent, cid).await? { - Some(controllers) => controllers.iter().map(|p| p.to_string()).collect(), - None => Vec::new(), - }; - let module_hash = read_state_tree_canister_module_hash(agent, cid) - .await? + let controllers = calls + .controllers(cid) + .await + .map_err(|e| anyhow!("could not read the controllers of canister {cid}: {e}"))? + .iter() + .map(|p| p.to_string()) + .collect(); + let module_hash = calls + .module_hash(cid) + .await + .map_err(|e| anyhow!("could not read the module hash of canister {cid}: {e}"))? .map(|hash| { format!( "0x{}", @@ -257,12 +219,14 @@ pub(crate) async fn exec(ctx: &Context, args: &StatusArgs) -> Result<(), anyhow: ) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.options.proxy)?; + for (i, (maybe_name, cid)) in cids.iter().enumerate() { let output = match args.options.public { true => { // We construct the status out of the state tree let status = - build_public_status(&agent, cid.to_owned(), maybe_name.clone()).await?; + build_public_status(calls.as_ref(), cid.to_owned(), maybe_name.clone()).await?; match args.options.json_format { true => serde_json::to_string(&status) @@ -274,8 +238,7 @@ pub(crate) async fn exec(ctx: &Context, args: &StatusArgs) -> Result<(), anyhow: false => { // Retrieve canister status from management canister match proxy_management::canister_status( - &agent, - args.options.proxy, + calls.as_ref(), CanisterIdRecord { canister_id: *cid }, ) .await @@ -295,29 +258,20 @@ pub(crate) async fn exec(ctx: &Context, args: &StatusArgs) -> Result<(), anyhow: } } Err(e) => { - let Some(reject) = direct_call_reject(&e) else { - bail!("Unknown error fetching canister {cid} status: {e}"); - }; + let code = rejection_code(&e); - if reject.error_code.as_deref() == Some(E_CANISTER_NOT_FOUND) { + if code == Some(E_CANISTER_NOT_FOUND) { bail!("Canister {cid} was not found."); } - if !reject - .error_code - .as_deref() - .is_some_and(|code| E_STATUS_ACCESS_DENIED.contains(&code)) - { - bail!( - "Error looking up canister {cid}: {:?} - {}", - reject.error_code, - reject.reject_message - ); + if !code.is_some_and(|code| E_STATUS_ACCESS_DENIED.contains(&code)) { + bail!("Error looking up canister {cid} status: {e}"); } // Access was denied, so fall back on fetching the public status let status = - build_public_status(&agent, cid.to_owned(), maybe_name.clone()).await?; + build_public_status(calls.as_ref(), cid.to_owned(), maybe_name.clone()) + .await?; match args.options.json_format { true => serde_json::to_string(&status) @@ -614,42 +568,35 @@ fn build_output(result: &SerializableCanisterStatusResult) -> Result Result<(), anyhow::E &selections.environment, ) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; let cid = ctx .get_canister_id( &selections.canister, @@ -34,8 +35,7 @@ pub(crate) async fn exec(ctx: &Context, args: &StopArgs) -> Result<(), anyhow::E ) .await?; - proxy_management::stop_canister(&agent, args.proxy, CanisterIdRecord { canister_id: cid }) - .await?; + proxy_management::stop_canister(calls.as_ref(), CanisterIdRecord { canister_id: cid }).await?; Ok(()) } diff --git a/crates/icp-cli/src/commands/deploy.rs b/crates/icp-cli/src/commands/deploy.rs index 4afbceab9..94288a40f 100644 --- a/crates/icp-cli/src/commands/deploy.rs +++ b/crates/icp-cli/src/commands/deploy.rs @@ -2,13 +2,13 @@ use anyhow::bail; use candid::Principal; use clap::Args; use clap_complete::ArgValueCandidates; -use ic_agent::{Agent, AgentError}; use icp_app::{context::Context, identity::IdentitySelection}; use icp_canister_interfaces::candid_ui::MAINNET_CANDID_UI_CID; +use icp_project::calls::{Call, CallError, CanisterCalls}; use icp_project::operations::deploy::{DeployParams, DeployReport, deploy, resolve_targets}; use icp_project::parsers::CyclesAmount; use icp_project::{ - agent::LazyAgent, + defer::{Deferred, DeferredError}, host::{CanisterSelection, EnvironmentSelection}, network::Configuration as NetworkConfiguration, }; @@ -115,14 +115,18 @@ pub(crate) async fn exec(ctx: &Context, args: &DeployArgs) -> Result<(), anyhow: bail!("--args and --args-file can only be used when deploying a single canister"); } - // One agent for the whole run, including the URLs printed at the end: one - // identity unlock and, for a network whose root key is fetched, one fetch - // rather than one per phase. Deferred rather than made here, because - // unlocking a key and reaching a network are exactly what a deploy that - // fails to build should not do; the first phase that needs the network - // creates it. + // One agent for the whole run, including the calls seam built on it and the + // URLs printed at the end: one identity unlock and, for a network whose + // root key is fetched, one fetch rather than one per phase. Deferred rather + // than made here, because unlocking a key and reaching a network are + // exactly what a deploy that fails to build should not do; the first phase + // that needs the network creates it. let (identity, environment) = (&identity_selection, &environment_selection); - let agent = LazyAgent::new(move || ctx.get_agent_for_env(identity, environment)); + let agent = Deferred::new(move || ctx.get_agent_for_env(identity, environment)); + let calls = Deferred::new(|| async { + let agent = agent.get().await?.clone(); + icp_app::calls::calls(agent, args.proxy).map_err(DeferredError::new) + }); let params = DeployParams { environment: environment_selection.clone(), @@ -141,7 +145,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DeployArgs) -> Result<(), anyhow: // command having to await it phase by phase. let mut report = DeployReport::default(); let result = rendered(ctx.debug, async |reporter| { - deploy(&ctx.host, &agent, ¶ms, reporter, &mut report).await + deploy(&ctx.host, &calls, &agent, ¶ms, reporter, &mut report).await }) .await; @@ -159,7 +163,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DeployArgs) -> Result<(), anyhow: print_canister_urls( ctx, &environment_selection, - agent.get().await?, + calls.get().await?.as_ref(), &canisters, args.json, ) @@ -188,16 +192,14 @@ pub(crate) async fn exec(ctx: &Context, args: &DeployArgs) -> Result<(), anyhow: /// so only the `error_code` string distinguishes them. Transport/other errors /// are inconclusive and, per the false-positive bias, also count as "has /// `http_request`". -async fn has_http_request(agent: &Agent, canister_id: Principal) -> bool { +async fn has_http_request(calls: &dyn CanisterCalls, canister_id: Principal) -> bool { // A valid Candid encoding of zero arguments (`DIDL\0\0`) — *not* raw empty // bytes, which are not a well-formed Candid message. This lets a genuine // zero-argument `http_request` reply, while a single-argument one still // fails to decode and traps; either way the method exists. let empty_args = candid::encode_args(()).expect("encoding () never fails"); - let result = agent - .query(&canister_id, "http_request") - .with_arg(empty_args) - .call() + let result = calls + .query(Call::new(canister_id, "http_request", empty_args)) .await; match result { @@ -216,17 +218,15 @@ async fn has_http_request(agent: &Agent, canister_id: Principal) -> bool { /// `error_code` is absent (older replicas) do we fall back to the message, and /// then only when it names `http_request`, so a nested "no such method" bubbled /// up from an existing handler isn't mistaken for `http_request` being absent. -fn is_method_not_found(err: &AgentError) -> bool { - let reject = match err { - AgentError::CertifiedReject { reject, .. } - | AgentError::UncertifiedReject { reject, .. } => reject, - _ => return false, - }; - match reject.error_code.as_deref() { +fn is_method_not_found(err: &CallError) -> bool { + if !err.is_rejection() { + return false; + } + match err.code() { Some(code) => code == "IC0536", None => { - reject.reject_message.contains("has no query method") - && reject.reject_message.contains("http_request") + let message = err.message().unwrap_or_default(); + message.contains("has no query method") && message.contains("http_request") } } } @@ -235,7 +235,7 @@ fn is_method_not_found(err: &AgentError) -> bool { async fn print_canister_urls( ctx: &Context, environment_selection: &EnvironmentSelection, - agent: &Agent, + calls: &dyn CanisterCalls, canister_names: &[String], json: bool, ) -> Result<(), anyhow::Error> { @@ -289,7 +289,7 @@ async fn print_canister_urls( continue; }; - if has_http_request(agent, canister_id).await { + if has_http_request(calls, canister_id).await { // A canister carries one friendly name normally, or several when // it's a de-duplicated shared dependency canister reached via // multiple alias chains — print one URL for each. Fall back to a @@ -424,22 +424,28 @@ async fn get_candid_ui_id( #[cfg(test)] mod tests { use super::*; - use ic_agent::agent::{RejectCode, RejectResponse}; - - fn reject(error_code: Option<&str>, reject_message: &str) -> AgentError { - AgentError::UncertifiedReject { - reject: RejectResponse { - // Both a missing method and a trap surface as `CanisterError`, - // so the reject code is intentionally the same across cases — - // only `error_code`/message distinguishes them. - reject_code: RejectCode::CanisterError, - reject_message: reject_message.to_string(), - error_code: error_code.map(String::from), - }, - operation: None, + + /// A rejection, which is all the classifier looks at: both a missing + /// method and a trap are rejections, and only the code or the message + /// distinguishes them. + fn reject(code: Option<&str>, message: &str) -> CallError { + CallError::Rejected { + canister: Principal::anonymous(), + method: "http_request".to_owned(), + code: code.map(String::from), + message: message.to_owned(), } } + /// A call that reached no verdict at all. + fn no_verdict() -> CallError { + CallError::failed( + Principal::anonymous(), + "http_request", + std::io::Error::other("connection reset"), + ) + } + #[test] fn method_not_found_detected_by_error_code() { // IC0536 = CanisterMethodNotFound: the method genuinely does not exist. @@ -494,9 +500,9 @@ mod tests { } #[test] - fn non_reject_error_is_inconclusive_not_method_not_found() { - // Transport/other errors are not evidence the method is absent; the - // false-positive bias then treats the canister as a frontend. - assert!(!is_method_not_found(&AgentError::InvalidReplicaStatus)); + fn a_call_with_no_verdict_is_inconclusive_not_method_not_found() { + // A call that reached no verdict is not evidence the method is absent; + // the false-positive bias then treats the canister as a frontend. + assert!(!is_method_not_found(&no_verdict())); } } diff --git a/crates/icp-cli/src/commands/message/send.rs b/crates/icp-cli/src/commands/message/send.rs index 83fdabda8..93ae0bedf 100644 --- a/crates/icp-cli/src/commands/message/send.rs +++ b/crates/icp-cli/src/commands/message/send.rs @@ -258,7 +258,10 @@ async fn resolve_interface( } } match agent { - Some(agent) => Ok(get_candid_type(agent, validated.canister_id).await), + Some(agent) => { + let calls = icp_app::calls::calls(agent.clone(), None)?; + Ok(get_candid_type(calls.as_ref(), validated.canister_id).await) + } None => Ok(None), } } diff --git a/crates/icp-cli/src/commands/sync.rs b/crates/icp-cli/src/commands/sync.rs index a0f694d10..673301854 100644 --- a/crates/icp-cli/src/commands/sync.rs +++ b/crates/icp-cli/src/commands/sync.rs @@ -61,6 +61,7 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), anyhow::E let agent = ctx .get_agent_for_env(&identity_selection, &environment_selection) .await?; + let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; // Prepare list of canisters with their info for syncing let sync_canisters = try_join_all(cnames.iter().map(|name| async { @@ -95,14 +96,12 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), anyhow::E // so detect a non-Running canister up front and abort with an actionable error // instead of letting the plugin's first call fail with a cryptic IC0508 // ("canister is stopped ... does not have a CallContextManager"). - let proxy = args.proxy; try_join_all(sync_canisters.iter().map(|(cid, _, _)| { - let agent = agent.clone(); + let calls = calls.clone(); let cid = *cid; async move { let status = proxy_management::canister_status( - &agent, - proxy, + calls.as_ref(), CanisterIdRecord { canister_id: CanisterId::from(cid), }, diff --git a/crates/icp-project/src/agent.rs b/crates/icp-project/src/agent.rs deleted file mode 100644 index c70a6ac8e..000000000 --- a/crates/icp-project/src/agent.rs +++ /dev/null @@ -1,74 +0,0 @@ -//! The agent an operation speaks through, without the means to make one. -//! -//! Building an agent takes an identity, a key store and a way to unlock it — -//! all of which belong to the surrounding application, not here. So this layer -//! names only what it needs: something it can ask for an agent when it has -//! something to say. - -use std::{error::Error, future::Future}; - -use futures::future::BoxFuture; -use ic_agent::Agent; -use snafu::Snafu; -use tokio::sync::OnceCell; - -/// An [`Agent`] created on first use. -/// -/// Creating an agent unlocks an identity — a password prompt, for an encrypted -/// one — and, on a network whose root key is fetched, costs a round trip. An -/// operation that can fail before it ever speaks to the network, such as a -/// deploy whose build fails, should cost neither: it takes one of these and the -/// first phase that actually needs the network pays for it. -pub struct LazyAgent<'a> { - cell: OnceCell, - create: Box BoxFuture<'a, Result> + Send + Sync + 'a>, -} - -impl<'a> LazyAgent<'a> { - /// Wraps whatever the caller does to produce an agent. - /// - /// `create` runs on the first [`get`](Self::get), and on a later one only - /// while it keeps failing; the first agent it yields is the one every - /// caller sees. - pub fn new(create: F) -> Self - where - F: Fn() -> Fut + Send + Sync + 'a, - Fut: Future> + Send + 'a, - E: Error + Send + Sync + 'static, - { - Self { - cell: OnceCell::new(), - create: Box::new(move || { - let creating = create(); - Box::pin(async move { creating.await.map_err(LazyAgentError::new) }) - }), - } - } - - /// The agent, creating it if this is the first call. - pub async fn get(&self) -> Result<&Agent, LazyAgentError> { - self.cell.get_or_try_init(|| (self.create)()).await - } -} - -/// An agent could not be created. -/// -/// What that took is the caller's business: resolving an identity, unlocking a -/// key, reaching a network for its root key. This layer knows only that it can -/// fail and that whatever went wrong is what the user needs to be told, so the -/// cause is carried whole and displayed as itself rather than being restated -/// here. -#[derive(Debug, Snafu)] -#[snafu(transparent)] -pub struct LazyAgentError { - pub source: Box, -} - -impl LazyAgentError { - /// Wraps a creation function's own error for the boundary. - pub fn new(source: impl Error + Send + Sync + 'static) -> Self { - Self { - source: Box::new(source), - } - } -} diff --git a/crates/icp-project/src/calls.rs b/crates/icp-project/src/calls.rs new file mode 100644 index 000000000..b43bacac8 --- /dev/null +++ b/crates/icp-project/src/calls.rs @@ -0,0 +1,270 @@ +//! Talking to canisters. +//! +//! Modelled on what the sync-plugin interface already exposes to a guest, +//! because that is the irreducible set: submit a call, and read a certified +//! fact about a canister. Everything else in this crate — creating, installing, +//! settings, candid checks, the ledger reads — is built out of those. +//! +//! Certification is not the caller's business. A reader is *assumed* to return +//! certified answers, so verifying whatever proof that took is entirely inside +//! the implementation. That is also why each certified fact gets its own +//! method rather than a general "read the state tree": a caller running inside +//! a canister cannot read the state tree at all, and reaches the same facts +//! through management-canister calls instead. + +use async_trait::async_trait; +use candid::Principal; +use snafu::{ResultExt, Snafu}; + +/// Which canister a call should be routed to, when that differs from the one +/// being called. +/// +/// The management canister has no routing of its own, so a call to it must name +/// the canister it acts on; a subnet-scoped call names a subnet instead. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum RouteTo { + /// Route to the canister being called. The ordinary case. + Callee, + /// Route to some other canister — the target of a management-canister call. + Canister(Principal), + /// Route to a subnet, for a call that acts on the subnet rather than on any + /// canister in it. + Subnet(Principal), +} + +/// One canister call. +#[derive(Clone, Debug)] +pub struct Call { + /// Canister whose method is being called. + pub canister: Principal, + + /// Method name. + pub method: String, + + /// Candid-encoded arguments. + pub arg: Vec, + + /// Where the call should be routed. See [`RouteTo`]. + pub route: RouteTo, + + /// Cycles to attach. Only meaningful for a call an implementation can fund + /// — one made through a proxy canister, or from a canister of its own. + pub cycles: u128, +} + +impl Call { + /// A plain call to `canister`, routed to it, with no cycles attached. + pub fn new(canister: Principal, method: impl Into, arg: Vec) -> Self { + Self { + canister, + method: method.into(), + arg, + route: RouteTo::Callee, + cycles: 0, + } + } + + /// A management-canister call acting on `target`, which is therefore what + /// it must be routed to. + pub fn management(method: impl Into, target: Principal, arg: Vec) -> Self { + Self { + canister: Principal::management_canister(), + method: method.into(), + arg, + route: RouteTo::Canister(target), + cycles: 0, + } + } + + pub fn with_route(mut self, route: RouteTo) -> Self { + self.route = route; + self + } + + pub fn with_cycles(mut self, cycles: u128) -> Self { + self.cycles = cycles; + self + } +} + +/// A call did not produce a reply. +/// +/// The distinction that matters to callers is whether the network reached a +/// verdict: a rejection is an answer, and its code is worth branching on (a +/// canister reported as stopped, or as not found). Anything else means the +/// caller learned nothing and may want to try again. +#[derive(Debug, Snafu)] +pub enum CallError { + #[snafu(display("call to '{method}' on {canister} was rejected: {message}"))] + Rejected { + canister: Principal, + method: String, + /// The replica's error code, e.g. `IC0508`, when it gave one. + code: Option, + message: String, + }, + + /// The call never reached a verdict — a transport failure, a timeout, a + /// malformed reply. The cause is carried whole because what a call travels + /// over is the implementation's business. + #[snafu(display("call to '{method}' on {canister} failed: {source}"))] + Failed { + canister: Principal, + method: String, + source: Box, + }, +} + +impl CallError { + /// Builds a [`CallError::Failed`] for `call` from an implementation's own + /// error. + pub fn failed( + canister: Principal, + method: impl Into, + source: impl std::error::Error + Send + Sync + 'static, + ) -> Self { + Self::Failed { + canister, + method: method.into(), + source: Box::new(source), + } + } + + /// The replica's error code, when this was a rejection that carried one. + /// + /// Callers branch on this rather than on message text: `IC0508` means a + /// canister was observed stopped, `IC0301` that it does not exist. + pub fn code(&self) -> Option<&str> { + match self { + CallError::Rejected { code, .. } => code.as_deref(), + CallError::Failed { .. } => None, + } + } + + /// Whether the network reached a verdict. A rejection did; a transport + /// failure did not, and tells the caller nothing about the canister. + pub fn is_rejection(&self) -> bool { + matches!(self, CallError::Rejected { .. }) + } + + /// The rejection message, for a caller that has to fall back on matching + /// text because no error code was given. + pub fn message(&self) -> Option<&str> { + match self { + CallError::Rejected { message, .. } => Some(message), + CallError::Failed { .. } => None, + } + } +} + +/// How this crate reaches canisters. +/// +/// Every answer is certified. What that takes — verifying a state-tree +/// certificate against a root key, or simply trusting a management-canister +/// reply made from inside the same subnet — belongs to the implementation. +#[async_trait] +pub trait CanisterCalls: Send + Sync { + /// The principal these calls are made as. + fn caller(&self) -> Principal; + + /// Submit an update call and return its reply. + async fn update(&self, call: Call) -> Result, CallError>; + + /// Submit a query call and return its reply. + /// + /// A caller asks for a query when the method is one; how the answer is + /// actually obtained is the implementation's business. One that has to + /// route through something accepting only updates will do that instead, + /// and the reply is the same either way. + async fn query(&self, call: Call) -> Result, CallError>; + + /// A canister's custom-section metadata. + /// + /// `Ok(None)` means the section is certified *absent* from a canister that + /// exists. A canister that does not exist is an error, since the two are + /// otherwise indistinguishable and callers use this to tell them apart. + async fn metadata_section( + &self, + canister: Principal, + path: &str, + ) -> Result>, CallError>; + + /// A canister's controllers. + async fn controllers(&self, canister: Principal) -> Result, CallError>; + + /// The hash of a canister's installed module, or `None` when it has none. + async fn module_hash(&self, canister: Principal) -> Result>, CallError>; + + /// Which subnet `canister` lives on. + async fn subnet_of(&self, canister: Principal) -> Result; + + /// Whether canisters on `subnet` are created through an engine operator + /// rather than through the management canister. + /// + /// Its own question rather than a general topology read, for the same + /// reason the certified reads above are: a caller inside a canister cannot + /// consult the registry, and would have to ask a management canister. + async fn subnet_uses_engine_operator(&self, subnet: Principal) -> Result; +} + +/// A typed call failed, or its arguments or reply would not encode. +#[derive(Debug, Snafu)] +#[snafu(visibility(pub))] +pub enum TypedCallError { + #[snafu(display("failed to encode the arguments for '{method}'"))] + Encode { + source: candid::Error, + method: String, + }, + + #[snafu(transparent)] + Call { source: CallError }, + + #[snafu(display("failed to decode the reply from '{method}'"))] + Decode { + source: candid::Error, + method: String, + }, +} + +/// Make an update call with Candid arguments and decode its reply. +pub async fn update_typed( + calls: &dyn CanisterCalls, + canister: Principal, + method: &str, + args: A, + route: RouteTo, + cycles: u128, +) -> Result +where + A: candid::utils::ArgumentEncoder, + R: for<'a> candid::utils::ArgumentDecoder<'a>, +{ + let arg = candid::encode_args(args).context(EncodeSnafu { method })?; + let reply = calls + .update(Call { + canister, + method: method.to_owned(), + arg, + route, + cycles, + }) + .await?; + candid::decode_args(&reply).context(DecodeSnafu { method }) +} + +/// Make a query call with Candid arguments and decode its reply. +pub async fn query_typed( + calls: &dyn CanisterCalls, + canister: Principal, + method: &str, + args: A, +) -> Result +where + A: candid::utils::ArgumentEncoder, + R: for<'a> candid::utils::ArgumentDecoder<'a>, +{ + let arg = candid::encode_args(args).context(EncodeSnafu { method })?; + let reply = calls.query(Call::new(canister, method, arg)).await?; + candid::decode_args(&reply).context(DecodeSnafu { method }) +} diff --git a/crates/icp-project/src/defer.rs b/crates/icp-project/src/defer.rs new file mode 100644 index 000000000..7ff6e15e0 --- /dev/null +++ b/crates/icp-project/src/defer.rs @@ -0,0 +1,74 @@ +//! Something the caller can produce, produced on first use. +//! +//! What it takes to reach a canister — an identity, a key store and a way to +//! unlock it, an endpoint, a root key — belongs to the surrounding application, +//! not here. So this layer names only what it needs: something it can ask when +//! it has something to say. + +use std::{error::Error, future::Future}; + +use futures::future::BoxFuture; +use snafu::Snafu; +use tokio::sync::OnceCell; + +/// A `T` created on first use. +/// +/// Anything that speaks for an identity costs something to make: unlocking a +/// key, which can mean a password prompt, and on a network whose root key is +/// fetched, a round trip. An operation that can fail before it ever speaks to +/// the network, such as a deploy whose build fails, should cost neither: it +/// takes one of these and the first phase that actually needs the network pays +/// for it. +pub struct Deferred<'a, T> { + cell: OnceCell, + create: Box BoxFuture<'a, Result> + Send + Sync + 'a>, +} + +impl<'a, T> Deferred<'a, T> { + /// Wraps whatever the caller does to produce the value. + /// + /// `create` runs on the first [`get`](Self::get), and on a later one only + /// while it keeps failing; the first value it yields is the one every caller + /// sees. + pub fn new(create: F) -> Self + where + F: Fn() -> Fut + Send + Sync + 'a, + Fut: Future> + Send + 'a, + E: Error + Send + Sync + 'static, + { + Self { + cell: OnceCell::new(), + create: Box::new(move || { + let creating = create(); + Box::pin(async move { creating.await.map_err(DeferredError::new) }) + }), + } + } + + /// The value, creating it if this is the first call. + pub async fn get(&self) -> Result<&T, DeferredError> { + self.cell.get_or_try_init(|| (self.create)()).await + } +} + +/// A [`Deferred`] value could not be created. +/// +/// What that took is the caller's business: resolving an identity, unlocking a +/// key, reaching a network for its root key. This layer knows only that it can +/// fail and that whatever went wrong is what the user needs to be told, so the +/// cause is carried whole and displayed as itself rather than being restated +/// here. +#[derive(Debug, Snafu)] +#[snafu(display("{source}"))] +pub struct DeferredError { + pub source: Box, +} + +impl DeferredError { + /// Wraps a creation function's own error for the boundary. + pub fn new(source: impl Error + Send + Sync + 'static) -> Self { + Self { + source: Box::new(source), + } + } +} diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index 200a44b1b..d38f933ac 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -25,8 +25,9 @@ use crate::{ prelude::*, }; -pub mod agent; +pub mod calls; pub mod canister; +pub mod defer; pub mod files; #[cfg(feature = "host")] pub mod fs; diff --git a/crates/icp-project/src/operations/binding_env_vars.rs b/crates/icp-project/src/operations/binding_env_vars.rs index 8025a0b03..28a598625 100644 --- a/crates/icp-project/src/operations/binding_env_vars.rs +++ b/crates/icp-project/src/operations/binding_env_vars.rs @@ -1,16 +1,17 @@ use std::collections::{BTreeMap, HashSet}; +use std::sync::Arc; use crate::Canister; +use candid::Principal; use futures::{StreamExt, stream::FuturesOrdered}; -use ic_agent::{Agent, export::Principal}; use ic_management_canister_types::{CanisterSettings, EnvironmentVariable, UpdateSettingsArgs}; use icp_events::TaskOutcome; +use crate::calls::{CanisterCalls, TypedCallError}; use crate::operations::task::{Reporter, Task}; use snafu::Snafu; use tracing::error; -use super::proxy::UpdateOrProxyError; use super::proxy_management; #[derive(Debug, Snafu)] @@ -22,7 +23,7 @@ pub enum BindingEnvVarsOperationError { }, #[snafu(transparent)] - UpdateOrProxy { source: UpdateOrProxyError }, + UpdateOrProxy { source: TypedCallError }, } #[derive(Debug, Snafu)] @@ -32,8 +33,7 @@ pub struct SetBindingEnvVarsManyError { } pub async fn set_env_vars_for_canister( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: &Principal, canister_info: &Canister, binding_vars: &[(String, String)], @@ -55,8 +55,7 @@ pub async fn set_env_vars_for_canister( .collect::>(); proxy_management::update_settings( - agent, - proxy, + calls, UpdateSettingsArgs { canister_id: *canister_id, settings: CanisterSettings { @@ -73,8 +72,7 @@ pub async fn set_env_vars_for_canister( /// Orchestrates setting environment variables for multiple canisters concurrently. pub async fn set_binding_env_vars_many( - agent: Agent, - proxy: Option, + calls: Arc, environment_name: &str, target_canisters: Vec<(Principal, Canister)>, canister_list: BTreeMap, @@ -133,9 +131,10 @@ pub async fn set_binding_env_vars_many( }) .collect(); - let agent = agent.clone(); + let calls = calls.clone(); futs.push_back(async move { - let result = set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars).await; + let result = + set_env_vars_for_canister(calls.as_ref(), &cid, &info, &binding_vars).await; match &result { Ok(()) => task.finish(TaskOutcome::succeeded()), diff --git a/crates/icp-project/src/operations/candid_compat.rs b/crates/icp-project/src/operations/candid_compat.rs index e2e0da0ab..270dbb6bd 100644 --- a/crates/icp-project/src/operations/candid_compat.rs +++ b/crates/icp-project/src/operations/candid_compat.rs @@ -6,10 +6,10 @@ use candid::{ }; use candid_parser::utils::CandidSource; use futures::{StreamExt, stream::FuturesOrdered}; -use ic_agent::Agent; use ic_management_canister_types::CanisterInstallMode; use icp_events::TaskOutcome; +use crate::calls::CanisterCalls; use crate::operations::task::{Reporter, Task}; use snafu::Snafu; use tracing::debug; @@ -19,7 +19,7 @@ use crate::operations::{misc::fetch_canister_metadata, wasm::extract_candid_serv /// Checks Candid interface compatibility for all canisters that would be /// upgraded. Aborts if any canister has an incompatible interface. pub async fn check_candid_compatibility_many( - agent: Agent, + calls: Arc, canisters: impl IntoIterator, artifacts: Arc, reporter: &Reporter, @@ -29,7 +29,7 @@ pub async fn check_candid_compatibility_many( for (name, cid, mode) in canisters { let task = reporter.task(Task::candid_check(name, cid)); let is_upgrade = matches!(mode, CanisterInstallMode::Upgrade(_)); - let agent = agent.clone(); + let calls = calls.clone(); let artifacts = artifacts.clone(); check_futs.push_back(async move { @@ -40,7 +40,8 @@ pub async fn check_candid_compatibility_many( return Ok::<_, String>(()); } - let result = check_canister_candid_compat(&agent, &cid, name, &*artifacts).await; + let result = + check_canister_candid_compat(calls.as_ref(), &cid, name, &*artifacts).await; match &result { Ok(()) => task.finish(TaskOutcome::succeeded()), @@ -74,7 +75,7 @@ pub async fn check_candid_compatibility_many( /// Returns `Ok(())` if the check passes or cannot be performed (missing metadata, etc.). /// Returns `Err(CandidCheckFailure)` only when a genuine incompatibility is found. async fn check_canister_candid_compat( - agent: &Agent, + calls: &dyn CanisterCalls, canister_id: &Principal, canister_name: &str, artifacts: &dyn crate::store_artifact::Access, @@ -85,7 +86,7 @@ async fn check_canister_candid_compat( Err(_) => return Ok(()), }; - match check_candid_compatibility(agent, canister_id, &wasm).await { + match check_candid_compatibility(calls, canister_id, &wasm).await { CandidCompatibility::Compatible => Ok(()), CandidCompatibility::Skipped(reason) => { debug!("Candid compatibility check skipped for {canister_name}: {reason}"); @@ -104,7 +105,7 @@ async fn check_canister_candid_compat( /// Returns [`CandidCompatibility::Skipped`] if either side lacks a /// `candid:service` metadata section or if the interfaces cannot be parsed. pub async fn check_candid_compatibility( - agent: &Agent, + calls: &dyn CanisterCalls, canister_id: &Principal, wasm: &[u8], ) -> CandidCompatibility { @@ -119,7 +120,7 @@ pub async fn check_candid_compatibility( }; // Fetch candid:service from the deployed canister - let old_candid = match fetch_canister_metadata(agent, *canister_id, "candid:service").await { + let old_candid = match fetch_canister_metadata(calls, *canister_id, "candid:service").await { Some(s) => s, None => { return CandidCompatibility::Skipped( diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index 2256d0c33..9bdf86b69 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -1,14 +1,11 @@ use std::sync::Arc; use std::time::Duration; +use crate::calls::{Call, CanisterCalls, RouteTo}; use crate::parsers::to_token_unit_amount; use crate::signal::stop_signal; use bigdecimal::{BigDecimal, ToPrimitive}; use candid::{Decode, Encode, IDLArgs, IDLValue, Nat, Principal}; -use ic_agent::{ - Agent, AgentError, - agent::{CallResponse, EffectiveId, RejectCode, SubnetType}, -}; use ic_ledger_types::{ AccountIdentifier, Memo, Subaccount, Tokens, TransferArgs, TransferError, TransferResult, }; @@ -35,7 +32,6 @@ use snafu::{OptionExt, ResultExt, Snafu}; use tokio::{select, sync::OnceCell, time::sleep}; use tracing::{info, warn}; -use super::proxy::UpdateOrProxyError; use super::proxy_management; #[derive(Debug, Snafu)] @@ -46,27 +42,23 @@ pub enum CreateOperationError { #[snafu(display("failed to decode candid"))] CandidDecode { source: candid::Error }, - #[snafu(display("agent error"))] - Agent { source: AgentError }, + #[snafu(display("a canister call failed: {source}"))] + Call { source: crate::calls::CallError }, + + #[snafu(transparent)] + TypedCall { + source: crate::calls::TypedCallError, + }, #[snafu(display("failed to create canister: {message}"))] CreateCanister { message: String }, #[snafu(display("failed to get subnet for canister"))] - GetSubnet { source: AgentError }, - - #[snafu(display("failed to sign the subnet-scoped create_canister call"))] - SignSubnetCreate { source: AgentError }, + GetSubnet { source: crate::calls::CallError }, #[snafu(display("failed to submit create_canister to subnet {subnet}"))] SubmitSubnetCreate { - source: AgentError, - subnet: Principal, - }, - - #[snafu(display("failed to await create_canister on subnet {subnet}"))] - AwaitSubnetCreate { - source: AgentError, + source: crate::calls::CallError, subnet: Principal, }, @@ -74,7 +66,7 @@ pub enum CreateOperationError { EngineCanisterId { message: String }, #[snafu(display("failed to query the engine-canister registry"))] - EngineCanisterQuery { source: AgentError }, + EngineCanisterQuery { source: crate::calls::CallError }, #[snafu(display( "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)" @@ -91,7 +83,7 @@ pub enum CreateOperationError { MissingSubnetId, #[snafu(display("failed to get available subnets"))] - GetAvailableSubnets { source: AgentError }, + GetAvailableSubnets { source: crate::calls::CallError }, #[snafu(display("no available subnets found"))] NoAvailableSubnets, @@ -109,7 +101,7 @@ pub enum CreateOperationError { InvalidIcpAmount { message: String }, #[snafu(display("failed to transfer ICP to the cycles minting canister"))] - TransferIcp { source: AgentError }, + TransferIcp { source: crate::calls::CallError }, #[snafu(display("ICP ledger transfer failed: {message}"))] TransferFailed { message: String }, @@ -130,9 +122,6 @@ pub enum CreateOperationError { Complete the creation by running:\n\n {command}\n" ))] NotifyCreateInterrupted { height: u64, command: String }, - - #[snafu(transparent)] - UpdateOrProxyCall { source: UpdateOrProxyError }, } /// How long to keep retrying `notify_create_canister` before giving up. @@ -179,7 +168,7 @@ pub enum CreateTarget { } struct CreateOperationInner { - agent: Agent, + calls: Arc, target: CreateTarget, funding: CreateFunding, existing_canisters: Vec, @@ -200,14 +189,14 @@ impl Clone for CreateOperation { impl CreateOperation { pub fn new( - agent: Agent, + calls: Arc, target: CreateTarget, funding: CreateFunding, existing_canisters: Vec, ) -> Self { Self { inner: Arc::new(CreateOperationInner { - agent, + calls, target, funding, existing_canisters, @@ -242,13 +231,13 @@ impl CreateOperation { .get_subnet() .await .map_err(|e| CreateOperationError::SubnetResolution { message: e })?; - let subnet_info = self + let uses_engine_operator = self .inner - .agent - .get_subnet_by_id(&selected_subnet) + .calls + .subnet_uses_engine_operator(selected_subnet) .await .context(GetSubnetSnafu)?; - let cid = if let Some(SubnetType::CloudEngine) = subnet_info.subnet_type() { + let cid = if uses_engine_operator { // Resolve the subnet's engine-operator first. Only a definitive // "could not resolve an operator" resolution failure falls back to // the legacy management-canister path — this covers both no operator @@ -309,12 +298,14 @@ impl CreateOperation { // Call cycles ledger create_canister let resp = self .inner - .agent - .update(&CYCLES_LEDGER_PRINCIPAL, "create_canister") - .with_arg(Encode!(&arg).context(CandidEncodeSnafu)?) - .call_and_wait() + .calls + .update(Call::new( + CYCLES_LEDGER_PRINCIPAL, + "create_canister", + Encode!(&arg).context(CandidEncodeSnafu)?, + )) .await - .context(AgentSnafu)?; + .context(CallSnafu)?; let resp: CreateCanisterResponse = Decode!(&resp, CreateCanisterResponse).context(CandidDecodeSnafu)?; let cid = match resp { @@ -342,10 +333,12 @@ impl CreateOperation { }; let resp = match self .inner - .agent - .query(&engine_registry, GET_ENGINE_OPERATOR_BY_SUBNET_METHOD) - .with_arg(Encode!(&arg).context(CandidEncodeSnafu)?) - .call() + .calls + .query(Call::new( + engine_registry, + GET_ENGINE_OPERATOR_BY_SUBNET_METHOD, + Encode!(&arg).context(CandidEncodeSnafu)?, + )) .await { Ok(resp) => resp, @@ -395,12 +388,14 @@ impl CreateOperation { let resp = self .inner - .agent - .update(&operator, "create_canister") - .with_arg(Encode!(&arg).context(CandidEncodeSnafu)?) - .call_and_wait() + .calls + .update(Call::new( + operator, + "create_canister", + Encode!(&arg).context(CandidEncodeSnafu)?, + )) .await - .context(AgentSnafu)?; + .context(CallSnafu)?; let resp: CreateCanisterResponse = Decode!(&resp, CreateCanisterResponse).context(CandidDecodeSnafu)?; let cid = match resp { @@ -427,36 +422,18 @@ impl CreateOperation { ) -> Result { let arg = encode_create_canister_arg(settings).context(CandidEncodeSnafu)?; - // Subnet-scoped routing is its own endpoint rather than an effective - // canister id, so this cannot go through `update_or_proxy`. - let agent = &self.inner.agent; - let effective_id = EffectiveId::Subnet(subnet); - let signed = agent - .update(&Principal::management_canister(), "create_canister") - .with_arg(arg) - .sign() - .context(SignSubnetCreateSnafu)?; - let response = agent - .update_signed(effective_id, signed.signed_update) + // A subnet-scoped call is routed to the subnet rather than to any + // canister on it; what that takes to submit and await is the caller + // implementation's business. + let bytes = self + .inner + .calls + .update( + Call::new(Principal::management_canister(), "create_canister", arg) + .with_route(RouteTo::Subnet(subnet)), + ) .await .context(SubmitSubnetCreateSnafu { subnet })?; - let bytes = match response { - CallResponse::Response(bytes) => bytes, - CallResponse::Poll(request_id) => { - let signed_status = agent - .sign_request_status(effective_id, request_id) - .context(AwaitSubnetCreateSnafu { subnet })?; - agent - .wait_signed( - &request_id, - effective_id, - signed_status.signed_request_status, - ) - .await - .context(AwaitSubnetCreateSnafu { subnet })? - .0 - } - }; let (record,): (CanisterIdRecord,) = candid::decode_args(&bytes).context(CandidDecodeSnafu)?; Ok(record.canister_id) @@ -472,8 +449,12 @@ impl CreateOperation { sender_canister_version: None, }; + // Routing through the proxy is ambient — the caller's `--proxy` is part + // of how every call in the command is made. What the target decides is + // where the cycles come from. + let _ = proxy; let result = - proxy_management::create_canister(&self.inner.agent, Some(proxy), self.cycles(), args) + proxy_management::create_canister(self.inner.calls.as_ref(), self.cycles(), args) .await?; Ok(result.canister_id) @@ -490,11 +471,7 @@ impl CreateOperation { icp: &BigDecimal, recovery_flags: &str, ) -> Result { - let caller = self - .inner - .agent - .get_principal() - .map_err(|message| CreateOperationError::GetPrincipal { message })?; + let caller = self.inner.calls.caller(); // ICP ledger amounts are denominated in e8s (10^-8 ICP). Reject any amount // with more precision than e8s can represent rather than silently @@ -526,10 +503,12 @@ impl CreateOperation { }; let transfer_result = self .inner - .agent - .update(&ICP_LEDGER_PRINCIPAL, "transfer") - .with_arg(Encode!(&transfer_args).context(CandidEncodeSnafu)?) - .call_and_wait() + .calls + .update(Call::new( + ICP_LEDGER_PRINCIPAL, + "transfer", + Encode!(&transfer_args).context(CandidEncodeSnafu)?, + )) .await .context(TransferIcpSnafu)?; let block_index = @@ -606,10 +585,12 @@ impl CreateOperation { async fn notify_create(&self, arg_bytes: &[u8]) -> Result { let resp = match self .inner - .agent - .update(&CYCLES_MINTING_CANISTER_PRINCIPAL, "notify_create_canister") - .with_arg(arg_bytes.to_vec()) - .call_and_wait() + .calls + .update(Call::new( + CYCLES_MINTING_CANISTER_PRINCIPAL, + "notify_create_canister", + arg_bytes.to_vec(), + )) .await { Ok(resp) => resp, @@ -649,16 +630,14 @@ impl CreateOperation { } if let Some(canister) = self.inner.existing_canisters.first() { - let subnet = &self - .inner - .agent - .get_subnet_by_canister(canister) + self.inner + .calls + .subnet_of(*canister) .await - .map_err(|e| e.to_string())?; - Ok(subnet.id()) + .map_err(|e| e.to_string()) } else { // If no canisters exist, pick a random available subnet - let subnets = get_available_subnets(&self.inner.agent) + let subnets = get_available_subnets(self.inner.calls.as_ref()) .await .map_err(|e| e.to_string())?; @@ -709,23 +688,30 @@ pub 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 - ) +/// Whether `err` is a rejection meaning the target canister does not exist on +/// this network (IC0301, "Canister ... not found"). Used to treat a missing +/// engine-canister registry as "no operator registered" so the CloudEngine path +/// can fall back safely. +/// +/// A rejection specifically: a call that never reached a verdict says nothing +/// about whether the canister is there, and must not be read as absence. +fn is_canister_not_found(err: &crate::calls::CallError) -> bool { + err.code() == Some(CANISTER_NOT_FOUND) } -async fn get_available_subnets(agent: &Agent) -> Result, CreateOperationError> { - let bs = agent - .query(&CYCLES_MINTING_CANISTER_PRINCIPAL, "get_default_subnets") - .with_arg(Encode!(&()).context(CandidEncodeSnafu)?) - .call() +/// The replica's error code for a call addressed to a canister that does not +/// exist. +const CANISTER_NOT_FOUND: &str = "IC0301"; + +async fn get_available_subnets( + calls: &dyn CanisterCalls, +) -> Result, CreateOperationError> { + let bs = calls + .query(Call::new( + CYCLES_MINTING_CANISTER_PRINCIPAL, + "get_default_subnets", + Encode!(&()).context(CandidEncodeSnafu)?, + )) .await .context(GetAvailableSubnetsSnafu)?; @@ -822,44 +808,29 @@ mod tests { #[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, - } - } + let rejected = |code: Option<&str>, message: &str| crate::calls::CallError::Rejected { + canister: Principal::anonymous(), + method: "get_engine_operator_by_subnet".to_owned(), + code: code.map(String::from), + message: message.to_owned(), }; - // 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))); + // IC0301 means the registry canister is not deployed here. + assert!(is_canister_not_found(&rejected( + Some("IC0301"), + "Canister q6cfj-fyaaa-aaaar-qb77q-cai not found" + ))); - // Other reject codes (e.g. a canister trap) must NOT be treated as + // Other rejections (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)); + assert!(!is_canister_not_found(&rejected(None, "trapped"))); + + // Neither may a call that reached no verdict at all: it says nothing + // about whether the canister is there. + assert!(!is_canister_not_found(&crate::calls::CallError::failed( + Principal::anonymous(), + "get_engine_operator_by_subnet", + std::io::Error::other("connection reset"), + ))); } } diff --git a/crates/icp-project/src/operations/deploy.rs b/crates/icp-project/src/operations/deploy.rs index 76346866a..61d1f17d5 100644 --- a/crates/icp-project/src/operations/deploy.rs +++ b/crates/icp-project/src/operations/deploy.rs @@ -14,17 +14,18 @@ //! all. use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::sync::Arc; use std::time::Duration; use candid::Principal; use futures::{StreamExt, future::try_join_all, stream::FuturesOrdered}; -use ic_agent::{Agent, AgentError}; use ic_management_canister_types::{CanisterId, CanisterIdRecord, CanisterInstallMode}; use icp_events::TaskOutcome; use itertools::Itertools; use snafu::{OptionExt, ResultExt, Snafu}; -use crate::agent::{LazyAgent, LazyAgentError}; +use crate::calls::{Call, CallError, CanisterCalls}; +use crate::defer::{Deferred, DeferredError}; use crate::host::{ CanisterSelection, EnvironmentSelection, GetCanisterIdForEnvError, GetEnvCanisterError, GetEnvironmentError, GetIdsByEnvironmentError, Host, SetCanisterIdForEnvError, @@ -37,7 +38,6 @@ use crate::operations::{ install::{ InstallManyError, ResolveInstallModeError, install_many, resolve_install_mode_and_status, }, - proxy::UpdateOrProxyError, proxy_management, settings::{ SyncControllerDependentsError, SyncSettingsManyError, sync_controller_dependents, @@ -48,6 +48,7 @@ use crate::operations::{ }; use crate::project::ArgsField; use crate::{CanisterArgsToBytesError, ProjectLoadError}; +use ic_agent::Agent; /// Everything that can stop a deploy. Each phase's failure keeps the typed /// error of the operation that produced it, so a caller can still tell a @@ -64,7 +65,7 @@ pub enum DeployError { Build { source: BuildManyError }, #[snafu(transparent)] - CreateAgent { source: LazyAgentError }, + Deferred { source: DeferredError }, #[snafu(transparent)] GetEnvironment { source: GetEnvironmentError }, @@ -128,7 +129,7 @@ pub enum DeployError { #[snafu(display("Failed to start canister {canister_id} before syncing it"))] StartCanister { canister_id: Principal, - source: UpdateOrProxyError, + source: crate::calls::TypedCallError, }, #[snafu(display( @@ -207,16 +208,19 @@ pub struct DeployReport { /// Run a full deploy, reporting progress as one task tree. /// -/// `agent` speaks for the identity the caller resolved: which identity that is, -/// and how its key was unlocked, is not this layer's business. It is resolved -/// at the first phase that needs the network and not before — a deploy that -/// cannot build, or that `--no-create` refuses, has no business unlocking a key -/// or reaching a network. +/// `calls` and `agent` speak for the identity the caller resolved: which +/// identity that is, and how its key was unlocked, is not this layer's +/// business. Both are resolved at the first phase that needs the network and +/// not before — a deploy that cannot build, or that `--no-create` refuses, has +/// no business unlocking a key or reaching a network. /// /// `report` is written as the run goes; see [`DeployReport`]. pub async fn deploy( host: &Host, - agent: &LazyAgent<'_>, + calls: &Deferred<'_, Arc>, + // Sync steps still run against an agent, because the wasmtime plugin + // runtime does. That goes when the step runners move behind their own seam. + agent: &Deferred<'_, Agent>, params: &DeployParams, reporter: &Reporter, report: &mut DeployReport, @@ -261,8 +265,10 @@ pub async fn deploy( .fail(); } - // Everything from here on talks to the network, so this is where the agent - // gets made — and where the identity it speaks for gets unlocked. + // Everything from here on talks to the network, so this is where the caller + // is asked for the means — and where the identity it speaks for gets + // unlocked. + let calls = calls.get().await?; let agent = agent.get().await?; if canisters_to_create.is_empty() { @@ -272,7 +278,7 @@ pub async fn deploy( let result = create_canisters( host, params, - agent, + calls, &env, &canisters_to_create, existing_canisters.into_values().collect(), @@ -307,8 +313,7 @@ pub async fn deploy( let phase = reporter.task(Task::phase("Setting environment variables:")); let result = set_binding_env_vars_many( - agent.clone(), - params.proxy, + calls.clone(), &env.name, target_canisters.clone(), canister_list.clone(), @@ -319,8 +324,7 @@ pub async fn deploy( let phase = reporter.task(Task::phase("Applying canister settings:")); let result = sync_settings_many( - agent.clone(), - params.proxy, + calls.clone(), target_canisters, canister_list, &env, @@ -331,7 +335,7 @@ pub async fn deploy( // Resolve install plans let canisters = try_join_all(cnames.iter().map(|name| { - let agent = agent.clone(); + let calls = calls.clone(); async move { let cid = host .get_canister_id_for_env( @@ -341,8 +345,7 @@ pub async fn deploy( .await?; let (mode, status) = - resolve_install_mode_and_status(&agent, params.proxy, name, &cid, ¶ms.mode) - .await?; + resolve_install_mode_and_status(calls.as_ref(), name, &cid, ¶ms.mode).await?; let env = host.get_environment(environment_selection).await?; let (_canister_path, canister_info) = env @@ -386,7 +389,7 @@ pub async fn deploy( if !params.yes { let phase = reporter.task(Task::phase("Checking compatibility:")); let result = check_candid_compatibility_many( - agent.clone(), + calls.clone(), canisters .iter() .map(|(name, cid, mode, _, _)| (&**name, *cid, *mode)), @@ -400,8 +403,7 @@ pub async fn deploy( // Install let phase = reporter.task(Task::phase("Installing canisters:")); let result = install_many( - agent.clone(), - params.proxy, + calls.clone(), canisters, host.artifacts.clone(), &phase.reporter(), @@ -409,7 +411,7 @@ pub async fn deploy( .await; finish(&phase, result)?; - sync(host, params, agent, reporter).await?; + sync(host, params, calls, agent, reporter).await?; Ok(()) } @@ -422,7 +424,7 @@ pub async fn deploy( async fn create_canisters( host: &Host, params: &DeployParams, - agent: &Agent, + calls: &Arc, env: &crate::Environment, canisters_to_create: &[&String], existing_ids: Vec, @@ -435,7 +437,7 @@ async fn create_canisters( _ => CreateTarget::None, }; let create_operation = CreateOperation::new( - agent.clone(), + calls.clone(), target, CreateFunding::Cycles(params.cycles), existing_ids, @@ -489,8 +491,7 @@ async fn create_canisters( // store, since that is what a dependent would be looking it up in. sync_controller_dependents( host, - agent, - params.proxy, + calls.as_ref(), canister_name, ¶ms.environment, ) @@ -521,6 +522,7 @@ async fn create_canisters( async fn sync( host: &Host, params: &DeployParams, + calls: &Arc, agent: &Agent, reporter: &Reporter, ) -> Result<(), DeployError> { @@ -559,14 +561,12 @@ async fn sync( // not Running here. Start each canister we're about to sync. Per the IC spec // start_canister is synchronous — its Ok reply means the canister is already // Running, so no status poll is needed — and idempotent (no-op if Running). - let proxy = params.proxy; try_join_all(sync_canisters.iter().map(|(cid, _, _)| { - let agent = agent.clone(); + let calls = calls.clone(); let cid = *cid; async move { proxy_management::start_canister( - &agent, - proxy, + calls.as_ref(), CanisterIdRecord { canister_id: CanisterId::from(cid), }, @@ -585,9 +585,9 @@ async fn sync( // with a transient IC0508 right after a restart. Wait until the query path // consistently sees the canister Running before handing off. try_join_all(sync_canisters.iter().map(|(cid, _, _)| { - let agent = agent.clone(); + let calls = calls.clone(); let cid = *cid; - async move { wait_until_serving_queries(&agent, cid).await } + async move { wait_until_serving_queries(calls.as_ref(), cid).await } })) .await?; @@ -615,7 +615,7 @@ async fn sync( env.network.name.clone(), urls, canister_ids, - proxy, + params.proxy, &phase.reporter(), ) .await; @@ -748,7 +748,7 @@ const READINESS_PROBE_METHOD: &str = ""; /// a hard guarantee — query reads are per-node and boundary nodes load-balance /// across replicas — but it makes the post-restart race rare. async fn wait_until_serving_queries( - agent: &Agent, + calls: &dyn CanisterCalls, canister_id: Principal, ) -> Result<(), DeployError> { const REQUIRED_CONSECUTIVE: u32 = 2; @@ -763,10 +763,7 @@ async fn wait_until_serving_queries( let poll = async { let mut consecutive_ready: u32 = 0; loop { - let probe = agent - .query(&canister_id, READINESS_PROBE_METHOD) - .with_arg(Vec::::new()) - .call(); + let probe = calls.query(Call::new(canister_id, READINESS_PROBE_METHOD, Vec::new())); let ready = match tokio::time::timeout(PROBE_TIMEOUT, probe).await { Ok(Ok(_)) => true, // replied -> Running Ok(Err(err)) => is_serving_reject(&err), // non-stopped reject -> Running @@ -798,39 +795,33 @@ async fn wait_until_serving_queries( /// True when a query error is a *reject from the replica* that indicates the /// canister is Running and serving — i.e. a positive readiness signal. /// -/// A reject means the replica processed the request to a verdict (e.g. "no such -/// query method"), so the canister is up — unless the reject says it is +/// A rejection means the replica processed the request to a verdict (e.g. "no +/// such query method"), so the canister is up — unless the rejection says it is /// stopped/stopping (IC0508/IC0509, with a message-substring fallback), which is -/// a replica still lagging behind the restart. Every other `AgentError` -/// (transport, HTTP, timeout, …) is inconclusive — not evidence the canister is -/// serving — and returns false so the caller retries rather than proceeding. -fn is_serving_reject(err: &AgentError) -> bool { - let reject = match err { - AgentError::CertifiedReject { reject, .. } - | AgentError::UncertifiedReject { reject, .. } => reject, - _ => return false, - }; - let stopped = matches!( - reject.error_code.as_deref(), - Some("IC0508") | Some("IC0509") - ) || reject.reject_message.contains("is stopped") - || reject.reject_message.contains("is stopping"); +/// a replica still lagging behind the restart. A call that reached no verdict at +/// all is inconclusive — not evidence the canister is serving — and returns +/// false so the caller retries rather than proceeding. +fn is_serving_reject(err: &CallError) -> bool { + if !err.is_rejection() { + return false; + } + let message = err.message().unwrap_or_default(); + let stopped = matches!(err.code(), Some("IC0508") | Some("IC0509")) + || message.contains("is stopped") + || message.contains("is stopping"); !stopped } #[cfg(test)] mod tests { use super::*; - use ic_agent::agent::{RejectCode, RejectResponse}; - - fn reject(error_code: Option<&str>, reject_message: &str) -> AgentError { - AgentError::UncertifiedReject { - reject: RejectResponse { - reject_code: RejectCode::CanisterError, - reject_message: reject_message.to_string(), - error_code: error_code.map(String::from), - }, - operation: None, + + fn reject(code: Option<&str>, message: &str) -> CallError { + CallError::Rejected { + canister: Principal::anonymous(), + method: READINESS_PROBE_METHOD.to_owned(), + code: code.map(String::from), + message: message.to_owned(), } } @@ -857,8 +848,12 @@ mod tests { } #[test] - fn a_transport_error_is_inconclusive() { + fn a_call_that_reached_no_verdict_is_inconclusive() { // Not evidence of anything; the caller must retry. - assert!(!is_serving_reject(&AgentError::InvalidReplicaStatus)); + assert!(!is_serving_reject(&CallError::failed( + Principal::anonymous(), + READINESS_PROBE_METHOD, + std::io::Error::other("connection reset"), + ))); } } diff --git a/crates/icp-project/src/operations/install.rs b/crates/icp-project/src/operations/install.rs index 6c68d5646..2637a45ce 100644 --- a/crates/icp-project/src/operations/install.rs +++ b/crates/icp-project/src/operations/install.rs @@ -1,6 +1,6 @@ use candid::Encode; +use candid::Principal; use futures::{StreamExt, stream::FuturesOrdered}; -use ic_agent::{Agent, export::Principal}; use ic_management_canister_types::{ CanisterId, CanisterIdRecord, CanisterInstallMode, CanisterStatusType, ChunkHash, ClearChunkStoreArgs, InstallChunkedCodeArgs, InstallCodeArgs, UpgradeFlags, UploadChunkArgs, @@ -8,6 +8,7 @@ use ic_management_canister_types::{ }; use icp_events::TaskOutcome; +use crate::calls::{CanisterCalls, TypedCallError}; use crate::operations::task::{Reporter, Task}; use sha2::{Digest, Sha256}; use snafu::{ResultExt, Snafu}; @@ -15,7 +16,6 @@ use std::sync::Arc; use tracing::{debug, warn}; use super::misc::fetch_canister_metadata; -use super::proxy::UpdateOrProxyError; use super::proxy_management; /// CLI-facing choice for `wasm_memory_persistence` on EOP upgrades. @@ -40,8 +40,8 @@ impl WasmMemoryPersistenceOpt { /// Returns true if the canister exposes the `enhanced-orthogonal-persistence` /// custom-section metadata (i.e. it is a Motoko EOP canister). -pub async fn is_eop_canister(agent: &Agent, canister_id: &Principal) -> bool { - fetch_canister_metadata(agent, *canister_id, "enhanced-orthogonal-persistence") +pub async fn is_eop_canister(calls: &dyn CanisterCalls, canister_id: &Principal) -> bool { + fetch_canister_metadata(calls, *canister_id, "enhanced-orthogonal-persistence") .await .is_some() } @@ -54,17 +54,17 @@ pub enum InstallOperationError { #[snafu(display("Failed to stop canister '{canister_name}' before upgrade"))] StopCanister { canister_name: String, - source: UpdateOrProxyError, + source: TypedCallError, }, #[snafu(display("Failed to start canister '{canister_name}' after upgrade"))] StartCanister { canister_name: String, - source: UpdateOrProxyError, + source: TypedCallError, }, #[snafu(transparent)] - UpdateOrProxy { source: UpdateOrProxyError }, + UpdateOrProxy { source: TypedCallError }, } #[derive(Debug, Snafu)] @@ -77,15 +77,13 @@ pub struct InstallManyError { /// a [`CanisterInstallMode`]. For "auto", queries `canister_status` to /// determine whether the canister already has code installed. pub async fn resolve_install_mode_and_status( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_name: &str, canister_id: &Principal, mode: &str, ) -> Result<(CanisterInstallMode, CanisterStatusType), ResolveInstallModeError> { let status = proxy_management::canister_status( - agent, - proxy, + calls, CanisterIdRecord { canister_id: CanisterId::from(*canister_id), }, @@ -110,12 +108,11 @@ pub async fn resolve_install_mode_and_status( #[snafu(display("Failed to resolve install mode for canister {canister_name}"))] pub struct ResolveInstallModeError { canister_name: String, - source: UpdateOrProxyError, + source: TypedCallError, } pub async fn install_canister( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: &Principal, canister_name: &str, wasm: &[u8], @@ -132,7 +129,7 @@ pub async fn install_canister( // default to Keep. let persistence = match wasm_memory_persistence { Some(opt) => Some(opt.to_ic()), - None => is_eop_canister(agent, canister_id) + None => is_eop_canister(calls, canister_id) .await .then_some(WasmMemoryPersistence::Keep), }; @@ -154,8 +151,7 @@ pub async fn install_canister( ); do_install_operation( - agent, - proxy, + calls, canister_id, canister_name, wasm, @@ -167,8 +163,7 @@ pub async fn install_canister( } async fn do_install_operation( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: &Principal, canister_name: &str, wasm: &[u8], @@ -206,25 +201,17 @@ async fn do_install_operation( sender_canister_version: None, }; - stop_and_start_if_upgrade( - agent, - proxy, - canister_id, - canister_name, - mode, - status, - async { - proxy_management::install_code(agent, proxy, install_args).await?; - Ok(()) - }, - ) + stop_and_start_if_upgrade(calls, canister_id, canister_name, mode, status, async { + proxy_management::install_code(calls, install_args).await?; + Ok(()) + }) .await?; } else { // Large wasm: use chunked installation debug!("Installing wasm for {canister_name} using chunked installation"); // Clear any existing chunks to ensure a clean state - proxy_management::clear_chunk_store(agent, proxy, ClearChunkStoreArgs { canister_id: cid }) + proxy_management::clear_chunk_store(calls, ClearChunkStoreArgs { canister_id: cid }) .await?; // Split wasm into chunks and upload them @@ -244,7 +231,7 @@ async fn do_install_operation( chunk: chunk.to_vec(), }; - let chunk_hash = proxy_management::upload_chunk(agent, proxy, upload_args).await?; + let chunk_hash = proxy_management::upload_chunk(calls, upload_args).await?; chunk_hashes.push(chunk_hash); } @@ -266,28 +253,18 @@ async fn do_install_operation( sender_canister_version: None, }; - let install_res = stop_and_start_if_upgrade( - agent, - proxy, - canister_id, - canister_name, - mode, - status, - async { - proxy_management::install_chunked_code(agent, proxy, chunked_args).await?; + let install_res = + stop_and_start_if_upgrade(calls, canister_id, canister_name, mode, status, async { + proxy_management::install_chunked_code(calls, chunked_args).await?; Ok(()) - }, - ) - .await; + }) + .await; // Clear chunk store after successful installation to free up storage - let clear_res = proxy_management::clear_chunk_store( - agent, - proxy, - ClearChunkStoreArgs { canister_id: cid }, - ) - .await - .map_err(InstallOperationError::from); + let clear_res = + proxy_management::clear_chunk_store(calls, ClearChunkStoreArgs { canister_id: cid }) + .await + .map_err(InstallOperationError::from); if let Err(clear_error) = clear_res { if let Err(install_error) = install_res { @@ -304,8 +281,7 @@ async fn do_install_operation( } async fn stop_and_start_if_upgrade( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: &Principal, canister_name: &str, mode: CanisterInstallMode, @@ -321,7 +297,7 @@ async fn stop_and_start_if_upgrade( }; // Stop the canister before proceeding if should_guard { - proxy_management::stop_canister(agent, proxy, cid_record.clone()) + proxy_management::stop_canister(calls, cid_record.clone()) .await .context(StopCanisterSnafu { canister_name })?; } @@ -329,7 +305,7 @@ async fn stop_and_start_if_upgrade( let install_result = f.await; // Restart the canister whether or not the installation succeeded if should_guard { - let start_result = proxy_management::start_canister(agent, proxy, cid_record).await; + let start_result = proxy_management::start_canister(calls, cid_record).await; if let Err(start_error) = start_result { // If both install and start failed, report the install error since it's more likely to be the root cause if let Err(install_error) = install_result { @@ -346,8 +322,7 @@ async fn stop_and_start_if_upgrade( /// Installs code to multiple canisters concurrently. pub async fn install_many( - agent: Agent, - proxy: Option, + calls: Arc, canisters: impl IntoIterator< Item = ( String, @@ -364,7 +339,7 @@ pub async fn install_many( for (name, cid, mode, status, init_args) in canisters { let task = reporter.task(Task::install(name.clone(), cid)); - let agent = agent.clone(); + let calls = calls.clone(); let artifacts = artifacts.clone(); futs.push_back(async move { @@ -376,8 +351,7 @@ pub async fn install_many( })?; install_canister( - &agent, - proxy, + calls.as_ref(), &cid, &name, &wasm, diff --git a/crates/icp-project/src/operations/misc.rs b/crates/icp-project/src/operations/misc.rs index 2000548f6..2a2f92813 100644 --- a/crates/icp-project/src/operations/misc.rs +++ b/crates/icp-project/src/operations/misc.rs @@ -2,20 +2,21 @@ use time::{OffsetDateTime, macros::format_description}; +use crate::calls::CanisterCalls; + +/// A canister's custom-section metadata as text, or `None` when it has no such +/// section (or could not be asked). +/// +/// Callers use this to detect a capability — a Motoko EOP canister, a canister +/// that publishes its Candid — so "absent" and "could not tell" lead to the +/// same conservative answer and are not worth distinguishing. pub async fn fetch_canister_metadata( - agent: &ic_agent::Agent, + calls: &dyn CanisterCalls, canister_id: candid::Principal, metadata: &str, ) -> Option { - Some( - String::from_utf8_lossy( - &agent - .read_state_canister_metadata(canister_id, metadata) - .await - .ok()?, - ) - .into(), - ) + let section = calls.metadata_section(canister_id, metadata).await.ok()??; + Some(String::from_utf8_lossy(§ion).into()) } /// Format a nanosecond timestamp as a human-readable UTC datetime string. diff --git a/crates/icp-project/src/operations/mod.rs b/crates/icp-project/src/operations/mod.rs index 0cbeb5f22..0b444a533 100644 --- a/crates/icp-project/src/operations/mod.rs +++ b/crates/icp-project/src/operations/mod.rs @@ -5,7 +5,6 @@ pub mod candid_compat; pub mod create; pub mod deploy; pub mod install; -pub mod proxy; pub mod proxy_management; pub mod recover_cycles; pub mod settings; diff --git a/crates/icp-project/src/operations/proxy.rs b/crates/icp-project/src/operations/proxy.rs deleted file mode 100644 index c9e972f8a..000000000 --- a/crates/icp-project/src/operations/proxy.rs +++ /dev/null @@ -1,114 +0,0 @@ -use candid::utils::{ArgumentDecoder, ArgumentEncoder}; -use candid::{Encode, Nat, Principal}; -use ic_agent::Agent; -use icp_canister_interfaces::proxy::{ProxyArgs, ProxyResult}; -use snafu::{ResultExt, Snafu}; - -#[derive(Debug, Snafu)] -pub enum UpdateOrProxyError { - #[snafu(display("failed to encode proxy call arguments"))] - ProxyEncode { source: candid::Error }, - - #[snafu(display("direct update call failed"))] - DirectUpdateCall { source: ic_agent::AgentError }, - - #[snafu(display("proxy update call failed"))] - ProxyUpdateCall { source: ic_agent::AgentError }, - - #[snafu(display("failed to decode proxy canister response"))] - ProxyDecode { source: candid::Error }, - - #[snafu(display("proxy call failed: {message}"))] - ProxyCall { message: String }, - - #[snafu(display("failed to encode call arguments"))] - CandidEncode { source: candid::Error }, - - #[snafu(display("failed to decode call response"))] - CandidDecode { source: candid::Error }, -} - -/// Dispatches a canister update call, optionally routing through a proxy canister. -/// -/// If `proxy` is `None`, makes a direct update call to the target canister. -/// If `proxy` is `Some`, wraps the call in [`ProxyArgs`] and sends it to the -/// proxy canister's `proxy` method, which forwards it to the target. -/// -/// `effective_canister_id` overrides the effective canister ID used for HTTP -/// routing in direct calls. This is required when calling the management -/// canister, where the effective canister ID must be the target canister -/// rather than `aaaaa-aa`. When `None`, defaults to `canister_id`. -/// -/// The `cycles` parameter is only used for proxied calls. -pub async fn update_or_proxy_raw( - agent: &Agent, - canister_id: Principal, - method: &str, - arg: Vec, - proxy: Option, - effective_canister_id: Option, - cycles: u128, -) -> Result, UpdateOrProxyError> { - if let Some(proxy_cid) = proxy { - let proxy_args = ProxyArgs { - canister_id, - method: method.to_string(), - args: arg, - cycles: Nat::from(cycles), - }; - let proxy_arg_bytes = Encode!(&proxy_args).context(ProxyEncodeSnafu)?; - - let proxy_res = agent - .update(&proxy_cid, "proxy") - .with_arg(proxy_arg_bytes) - .await - .context(ProxyUpdateCallSnafu)?; - - let proxy_result: (ProxyResult,) = - candid::decode_args(&proxy_res).context(ProxyDecodeSnafu)?; - - match proxy_result.0 { - ProxyResult::Ok(ok) => Ok(ok.result), - ProxyResult::Err(err) => ProxyCallSnafu { - message: err.format_error(), - } - .fail(), - } - } else { - let mut builder = agent.update(&canister_id, method).with_arg(arg); - if let Some(eid) = effective_canister_id { - builder = builder.with_effective_canister_id(eid); - } - let res = builder.await.context(DirectUpdateCallSnafu)?; - Ok(res) - } -} - -/// Like [`update_or_proxy_raw`], but accepts typed Candid arguments and decodes the response. -pub async fn update_or_proxy( - agent: &Agent, - canister_id: Principal, - method: &str, - args: A, - proxy: Option, - effective_canister_id: Option, - cycles: u128, -) -> Result -where - A: ArgumentEncoder, - R: for<'a> ArgumentDecoder<'a>, -{ - let arg = candid::encode_args(args).context(CandidEncodeSnafu)?; - let res = update_or_proxy_raw( - agent, - canister_id, - method, - arg, - proxy, - effective_canister_id, - cycles, - ) - .await?; - let decoded: R = candid::decode_args(&res).context(CandidDecodeSnafu)?; - Ok(decoded) -} diff --git a/crates/icp-project/src/operations/proxy_management.rs b/crates/icp-project/src/operations/proxy_management.rs index 1c7662dad..798f58246 100644 --- a/crates/icp-project/src/operations/proxy_management.rs +++ b/crates/icp-project/src/operations/proxy_management.rs @@ -1,5 +1,4 @@ use candid::Principal; -use ic_agent::Agent; use ic_management_canister_types::{ CanisterIdRecord, CanisterStatusResult, ClearChunkStoreArgs, CreateCanisterArgs, DeleteCanisterArgs, DeleteCanisterSnapshotArgs, FetchCanisterLogsArgs, FetchCanisterLogsResult, @@ -12,296 +11,180 @@ use ic_management_canister_types::{ UploadCanisterSnapshotMetadataResult, UploadChunkArgs, UploadChunkResult, }; -use snafu::{ResultExt, Snafu}; +use snafu::ResultExt; -use super::proxy::{UpdateOrProxyError, update_or_proxy}; +use crate::calls::{ + CanisterCalls, DecodeSnafu, EncodeSnafu, RouteTo, TypedCallError, update_typed, +}; -pub async fn create_canister( - agent: &Agent, - proxy: Option, +/// A management-canister call, acting on `target` — which is therefore what it +/// is routed to, since the management canister has no routing of its own. +async fn mgmt( + calls: &dyn CanisterCalls, + method: &str, + target: Option, + args: A, cycles: u128, - args: CreateCanisterArgs, -) -> Result { - let (result,): (CanisterIdRecord,) = update_or_proxy( - agent, +) -> Result +where + A: candid::utils::ArgumentEncoder, + R: for<'a> candid::utils::ArgumentDecoder<'a>, +{ + let route = match target { + Some(t) => RouteTo::Canister(t), + None => RouteTo::Callee, + }; + update_typed( + calls, Principal::management_canister(), - "create_canister", - (args,), - proxy, - None, + method, + args, + route, cycles, ) - .await?; + .await +} + +pub async fn create_canister( + calls: &dyn CanisterCalls, + cycles: u128, + args: CreateCanisterArgs, +) -> Result { + let (result,): (CanisterIdRecord,) = + mgmt(calls, "create_canister", None, (args,), cycles).await?; Ok(result) } pub async fn canister_status( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: CanisterIdRecord, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (CanisterStatusResult,) = update_or_proxy( - agent, - Principal::management_canister(), - "canister_status", - (args,), - proxy, - Some(effective), - 0, - ) - .await?; + let (result,): (CanisterStatusResult,) = + mgmt(calls, "canister_status", Some(effective), (args,), 0).await?; Ok(result) } pub async fn stop_canister( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: StopCanisterArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "stop_canister", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "stop_canister", Some(effective), (args,), 0).await } pub async fn start_canister( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: StartCanisterArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "start_canister", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "start_canister", Some(effective), (args,), 0).await } pub async fn delete_canister( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: DeleteCanisterArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "delete_canister", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "delete_canister", Some(effective), (args,), 0).await } pub async fn update_settings( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: UpdateSettingsArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "update_settings", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "update_settings", Some(effective), (args,), 0).await } pub async fn install_code( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: InstallCodeArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "install_code", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "install_code", Some(effective), (args,), 0).await } pub async fn install_chunked_code( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: InstallChunkedCodeArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.target_canister; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "install_chunked_code", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "install_chunked_code", Some(effective), (args,), 0).await } pub async fn upload_chunk( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: UploadChunkArgs, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (UploadChunkResult,) = update_or_proxy( - agent, - Principal::management_canister(), - "upload_chunk", - (args,), - proxy, - Some(effective), - 0, - ) - .await?; + let (result,): (UploadChunkResult,) = + mgmt(calls, "upload_chunk", Some(effective), (args,), 0).await?; Ok(result) } pub async fn clear_chunk_store( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: ClearChunkStoreArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "clear_chunk_store", - (args,), - proxy, - Some(effective), - 0, - ) - .await -} - -#[derive(Debug, Snafu)] -pub enum FetchCanisterLogsError { - #[snafu(display("failed to encode call arguments"))] - CandidEncode { source: candid::Error }, - - #[snafu(display("failed to decode call response"))] - CandidDecode { source: candid::Error }, - - #[snafu(display("direct query call failed"))] - DirectQueryCall { source: ic_agent::AgentError }, - - #[snafu(display("proxied call failed"))] - ProxiedCall { source: UpdateOrProxyError }, + mgmt::<_, ()>(calls, "clear_chunk_store", Some(effective), (args,), 0).await } /// Fetches canister logs from the management canister. /// -/// Unlike other management canister methods, `fetch_canister_logs` is a -/// **query** call when made directly. When a proxy is provided, the call is -/// routed through the proxy canister as an update call instead. +/// Asked for as a query, which is what it is. An implementation that has to +/// route the call through something that only accepts updates will do so; that +/// is its business, not this caller's. pub async fn fetch_canister_logs( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: FetchCanisterLogsArgs, -) -> Result { - let effective = args.canister_id; - if proxy.is_some() { - let (result,): (FetchCanisterLogsResult,) = update_or_proxy( - agent, - Principal::management_canister(), - "fetch_canister_logs", - (args,), - proxy, - Some(effective), - 0, +) -> Result { + let target = args.canister_id; + let arg = candid::encode_args((args,)).context(EncodeSnafu { + method: "fetch_canister_logs", + })?; + let reply = calls + .query( + crate::calls::Call::new(Principal::management_canister(), "fetch_canister_logs", arg) + .with_route(crate::calls::RouteTo::Canister(target)), ) - .await - .context(ProxiedCallSnafu)?; - Ok(result) - } else { - let arg = candid::encode_args((args,)).context(CandidEncodeSnafu)?; - let res = agent - .query(&Principal::management_canister(), "fetch_canister_logs") - .with_arg(arg) - .with_effective_canister_id(effective) - .await - .context(DirectQueryCallSnafu)?; - let (result,): (FetchCanisterLogsResult,) = - candid::decode_args(&res).context(CandidDecodeSnafu)?; - Ok(result) - } + .await?; + let (result,): (FetchCanisterLogsResult,) = + candid::decode_args(&reply).context(DecodeSnafu { + method: "fetch_canister_logs", + })?; + Ok(result) } pub async fn take_canister_snapshot( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: TakeCanisterSnapshotArgs, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (TakeCanisterSnapshotResult,) = update_or_proxy( - agent, - Principal::management_canister(), - "take_canister_snapshot", - (args,), - proxy, - Some(effective), - 0, - ) - .await?; + let (result,): (TakeCanisterSnapshotResult,) = + mgmt(calls, "take_canister_snapshot", Some(effective), (args,), 0).await?; Ok(result) } pub async fn load_canister_snapshot( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: LoadCanisterSnapshotArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), - "load_canister_snapshot", - (args,), - proxy, - Some(effective), - 0, - ) - .await + mgmt::<_, ()>(calls, "load_canister_snapshot", Some(effective), (args,), 0).await } pub async fn list_canister_snapshots( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: ListCanisterSnapshotsArgs, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (ListCanisterSnapshotsResult,) = update_or_proxy( - agent, - Principal::management_canister(), + let (result,): (ListCanisterSnapshotsResult,) = mgmt( + calls, "list_canister_snapshots", - (args,), - proxy, Some(effective), + (args,), 0, ) .await?; @@ -309,36 +192,30 @@ pub async fn list_canister_snapshots( } pub async fn delete_canister_snapshot( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: DeleteCanisterSnapshotArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), + mgmt::<_, ()>( + calls, "delete_canister_snapshot", - (args,), - proxy, Some(effective), + (args,), 0, ) .await } pub async fn read_canister_snapshot_metadata( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: ReadCanisterSnapshotMetadataArgs, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (ReadCanisterSnapshotMetadataResult,) = update_or_proxy( - agent, - Principal::management_canister(), + let (result,): (ReadCanisterSnapshotMetadataResult,) = mgmt( + calls, "read_canister_snapshot_metadata", - (args,), - proxy, Some(effective), + (args,), 0, ) .await?; @@ -346,18 +223,15 @@ pub async fn read_canister_snapshot_metadata( } pub async fn upload_canister_snapshot_metadata( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: UploadCanisterSnapshotMetadataArgs, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (UploadCanisterSnapshotMetadataResult,) = update_or_proxy( - agent, - Principal::management_canister(), + let (result,): (UploadCanisterSnapshotMetadataResult,) = mgmt( + calls, "upload_canister_snapshot_metadata", - (args,), - proxy, Some(effective), + (args,), 0, ) .await?; @@ -365,18 +239,15 @@ pub async fn upload_canister_snapshot_metadata( } pub async fn read_canister_snapshot_data( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: ReadCanisterSnapshotDataArgs, -) -> Result { +) -> Result { let effective = args.canister_id; - let (result,): (ReadCanisterSnapshotDataResult,) = update_or_proxy( - agent, - Principal::management_canister(), + let (result,): (ReadCanisterSnapshotDataResult,) = mgmt( + calls, "read_canister_snapshot_data", - (args,), - proxy, Some(effective), + (args,), 0, ) .await?; @@ -384,18 +255,15 @@ pub async fn read_canister_snapshot_data( } pub async fn upload_canister_snapshot_data( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, args: UploadCanisterSnapshotDataArgs, -) -> Result<(), UpdateOrProxyError> { +) -> Result<(), TypedCallError> { let effective = args.canister_id; - update_or_proxy::<_, ()>( - agent, - Principal::management_canister(), + mgmt::<_, ()>( + calls, "upload_canister_snapshot_data", - (args,), - proxy, Some(effective), + (args,), 0, ) .await diff --git a/crates/icp-project/src/operations/recover_cycles.rs b/crates/icp-project/src/operations/recover_cycles.rs index f53e01244..954eed6ee 100644 --- a/crates/icp-project/src/operations/recover_cycles.rs +++ b/crates/icp-project/src/operations/recover_cycles.rs @@ -11,8 +11,8 @@ //! failure to move a *meaningful* balance is a hard error so the user can //! retry rather than silently lose cycles. +use crate::calls::{CanisterCalls, TypedCallError}; use candid::{CandidType, Decode, Encode, Principal}; -use ic_agent::Agent; use ic_management_canister_types::{ CanisterId, CanisterIdRecord, CanisterInstallMode, CanisterSettings, InstallCodeArgs, UpdateSettingsArgs, @@ -21,7 +21,6 @@ use serde::Deserialize; use snafu::{ResultExt, Snafu}; use tracing::{info, warn}; -use super::proxy::UpdateOrProxyError; use crate::operations::proxy_management; /// Total cycle balance (from `canister_status`) at or below which recovery is @@ -35,7 +34,7 @@ pub enum RecoverCyclesError { #[snafu(display("failed to query status of canister {canister_id} before cycle recovery"))] QueryStatus { canister_id: Principal, - source: UpdateOrProxyError, + source: TypedCallError, }, #[snafu(display( @@ -43,7 +42,7 @@ pub enum RecoverCyclesError { ))] UpdateSettings { canister_id: Principal, - source: UpdateOrProxyError, + source: TypedCallError, }, #[snafu(display("failed to encode the recovery destination principal"))] @@ -52,13 +51,13 @@ pub enum RecoverCyclesError { #[snafu(display("failed to install the cycle-recovery module on canister {canister_id}"))] InstallModule { canister_id: Principal, - source: UpdateOrProxyError, + source: TypedCallError, }, #[snafu(display("failed to start canister {canister_id} for cycle recovery"))] StartCanister { canister_id: Principal, - source: UpdateOrProxyError, + source: TypedCallError, }, #[snafu(display("failed to encode the recover_cycles arguments"))] @@ -67,7 +66,7 @@ pub enum RecoverCyclesError { #[snafu(display("the recover_cycles call to canister {canister_id} failed"))] CallRecover { canister_id: Principal, - source: ic_agent::AgentError, + source: crate::calls::CallError, }, #[snafu(display("failed to decode the recover_cycles result from canister {canister_id}"))] @@ -107,13 +106,12 @@ enum RecoverResult { /// call; the deposit destination is baked into the install argument, so routing /// does not affect where the cycles land. pub async fn recover_cycles_before_delete( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, canister_id: Principal, destination: Principal, recover_cycles_wasm: &[u8], ) -> Result<(), RecoverCyclesError> { - let status = proxy_management::canister_status(agent, proxy, CanisterIdRecord { canister_id }) + let status = proxy_management::canister_status(calls, CanisterIdRecord { canister_id }) .await .context(QueryStatusSnafu { canister_id })?; let cycles: u128 = status.cycles.0.try_into().unwrap_or(u128::MAX); @@ -130,8 +128,7 @@ pub async fn recover_cycles_before_delete( // Lower the freezing threshold so the recovery canister sees a larger liquid // balance and can deposit close to the full total. proxy_management::update_settings( - agent, - proxy, + calls, UpdateSettingsArgs { canister_id: CanisterId::from(canister_id), settings: CanisterSettings { @@ -153,8 +150,7 @@ pub async fn recover_cycles_before_delete( let arg = Encode!(&destination).context(EncodeDestinationSnafu)?; proxy_management::install_code( - agent, - proxy, + calls, InstallCodeArgs { mode: CanisterInstallMode::Reinstall, canister_id: CanisterId::from(canister_id), @@ -167,16 +163,18 @@ pub async fn recover_cycles_before_delete( .context(InstallModuleSnafu { canister_id })?; // The canister must be running to accept the recovery update call. - proxy_management::start_canister(agent, proxy, CanisterIdRecord { canister_id }) + proxy_management::start_canister(calls, CanisterIdRecord { canister_id }) .await .context(StartCanisterSnafu { canister_id })?; // Direct (non-proxied) call: the destination is the install arg, not the // caller, so proxy routing is irrelevant here. - let bytes = agent - .update(&canister_id, "recover_cycles") - .with_arg(Encode!().context(EncodeArgsSnafu)?) - .call_and_wait() + let bytes = calls + .update(crate::calls::Call::new( + canister_id, + "recover_cycles", + Encode!().context(EncodeArgsSnafu)?, + )) .await .context(CallRecoverSnafu { canister_id })?; let result = Decode!(&bytes, RecoverResult).context(DecodeResultSnafu { canister_id })?; diff --git a/crates/icp-project/src/operations/settings.rs b/crates/icp-project/src/operations/settings.rs index 6049bd783..511ea2af6 100644 --- a/crates/icp-project/src/operations/settings.rs +++ b/crates/icp-project/src/operations/settings.rs @@ -11,19 +11,18 @@ use crate::{ }; use candid::{Nat, Principal}; use futures::{StreamExt, stream::FuturesOrdered}; -use ic_agent::Agent; use ic_management_canister_types::{ CanisterIdRecord, CanisterSettings, EnvironmentVariable, UpdateSettingsArgs, }; use icp_events::TaskOutcome; +use crate::calls::{CanisterCalls, TypedCallError}; use crate::operations::task::{Reporter, Task}; use itertools::Itertools; use num_traits::ToPrimitive; use snafu::{ResultExt, Snafu}; use tracing::warn; -use super::proxy::UpdateOrProxyError; use super::proxy_management; #[derive(Debug, Snafu)] @@ -31,12 +30,12 @@ use super::proxy_management; pub enum SyncSettingsOperationError { #[snafu(display("failed to fetch current canister settings for canister {canister}"))] FetchCurrentSettings { - source: UpdateOrProxyError, + source: TypedCallError, canister: Principal, }, #[snafu(display("failed to update canister settings for canister {canister}"))] UpdateSettings { - source: UpdateOrProxyError, + source: TypedCallError, canister: Principal, }, } @@ -74,16 +73,14 @@ fn environment_variables_eq(a: &[EnvironmentVariable], b: &[EnvironmentVariable] /// references that could not be resolved because the referenced canister has not been created /// yet. Resolved controllers are always applied immediately. pub async fn sync_settings( - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, cid: &Principal, canister: &Canister, ids: &IdMapping, ) -> Result, SyncSettingsOperationError> { - let status = - proxy_management::canister_status(agent, proxy, CanisterIdRecord { canister_id: *cid }) - .await - .context(FetchCurrentSettingsSnafu { canister: *cid })?; + let status = proxy_management::canister_status(calls, CanisterIdRecord { canister_id: *cid }) + .await + .context(FetchCurrentSettingsSnafu { canister: *cid })?; let &Settings { ref log_visibility, ref snapshot_visibility, @@ -213,8 +210,7 @@ pub async fn sync_settings( }; proxy_management::update_settings( - agent, - proxy, + calls, UpdateSettingsArgs { canister_id: *cid, settings, @@ -247,8 +243,7 @@ fn warn_unresolved_controller(controller: &str, canister: &str, environment: &En } pub async fn sync_settings_many( - agent: Agent, - proxy: Option, + calls: Arc, target_canisters: Vec<(Principal, Canister)>, ids: IdMapping, environment: &Environment, @@ -259,12 +254,12 @@ pub async fn sync_settings_many( for (cid, info) in target_canisters { let task = reporter.task(Task::update_settings(info.name.clone(), cid)); - let agent = agent.clone(); + let calls = calls.clone(); let ids = ids.clone(); futs.push_back(async move { let result = async { - let unresolved = sync_settings(&agent, proxy, &cid, &info, &ids).await?; + let unresolved = sync_settings(calls.as_ref(), &cid, &info, &ids).await?; for name in &unresolved { warn_unresolved_controller(name, &info.name, environment); } @@ -315,8 +310,7 @@ pub enum SyncControllerDependentsError { /// `sync_settings` for each so the controller is applied now that it can be resolved. pub async fn sync_controller_dependents( host: &Host, - agent: &Agent, - proxy: Option, + calls: &dyn CanisterCalls, newly_created_name: &str, env: &EnvironmentSelection, ) -> Result<(), SyncControllerDependentsError> { @@ -338,7 +332,7 @@ pub async fn sync_controller_dependents( let Some(&cid) = ids.get(name) else { continue; }; - match sync_settings(agent, proxy, &cid, canister, &ids).await { + match sync_settings(calls, &cid, canister, &ids).await { Ok(unresolved) => { for still_unresolved in &unresolved { warn_unresolved_controller(still_unresolved, name, &env_data); diff --git a/crates/icp-project/src/store_id.rs b/crates/icp-project/src/store_id.rs index 100a45331..f4e438df2 100644 --- a/crates/icp-project/src/store_id.rs +++ b/crates/icp-project/src/store_id.rs @@ -1,6 +1,6 @@ use std::collections::BTreeMap; -use ic_agent::export::Principal; +use candid::Principal; use snafu::Snafu; use crate::manifest::ProjectRootLocateError; From 7bfec18b3362eee2d87f91cf45d62c3f4c8544f6 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 05:54:39 -0700 Subject: [PATCH 02/13] fix: pass a deferred value's cause through instead of restating it `DeferredError` rendered its boxed cause with `#[snafu(display("{source}"))]`, which makes the cause both the wrapper's own message and its reported source, so it prints twice in every chain it reaches. `#[snafu(transparent)]` keeps the message and drops the wrapper from the chain. This is what the hand-written `LazyAgentError` it grew out of did, whose comment said "as `snafu(transparent)` would". --- crates/icp-project/src/defer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/icp-project/src/defer.rs b/crates/icp-project/src/defer.rs index 7ff6e15e0..801450e51 100644 --- a/crates/icp-project/src/defer.rs +++ b/crates/icp-project/src/defer.rs @@ -59,7 +59,7 @@ impl<'a, T> Deferred<'a, T> { /// cause is carried whole and displayed as itself rather than being restated /// here. #[derive(Debug, Snafu)] -#[snafu(display("{source}"))] +#[snafu(transparent)] pub struct DeferredError { pub source: Box, } From 586401ddca3af3e5a24388cdfcd400639961e06c Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 05:56:47 -0700 Subject: [PATCH 03/13] fix: stop appending a call's cause to its own message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Analysis: `CallError::Failed` interpolated `{source}` into its display while also reporting it as a source, so every printed chain carried the cause twice — once inside this message and once beneath it. It was doing that because everything above it passes through: `TypedCallError::Call` is transparent, as is `InstallError::UpdateOrProxy`, so this variant's one line is what `render::rendered_task` shows for a failed task, and dropping the cause from it looked lossy. It is not lossy. `rendered_task` returns the error it rendered, and the concurrent callers short-circuit on it with `try_join_all`, so the error that produced a task line always goes on to `main` and has its full chain printed directly below. The cause therefore still reaches the reader, once, on its own line — and the variant keeps the context the seam errors have none of, naming the method and canister. --- crates/icp-project/src/calls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/icp-project/src/calls.rs b/crates/icp-project/src/calls.rs index b43bacac8..e428336c6 100644 --- a/crates/icp-project/src/calls.rs +++ b/crates/icp-project/src/calls.rs @@ -107,7 +107,7 @@ pub enum CallError { /// The call never reached a verdict — a transport failure, a timeout, a /// malformed reply. The cause is carried whole because what a call travels /// over is the implementation's business. - #[snafu(display("call to '{method}' on {canister} failed: {source}"))] + #[snafu(display("call to '{method}' on {canister} failed"))] Failed { canister: Principal, method: String, From 79ee20fcb8280d76a131105d8de1380ca729d540 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:01:03 -0700 Subject: [PATCH 04/13] fix: stop appending a call's cause to the create error's message `CreateOperationError::Call` interpolated `{source}` into its display while also reporting it as a source, so the cause printed twice in every chain. The message keeps the context it adds and the cause is reported once, beneath it. Same as `CallError::Failed` a commit earlier, for the variant that wraps it. --- crates/icp-project/src/operations/create.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index 9bdf86b69..ada9784f4 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -42,7 +42,7 @@ pub enum CreateOperationError { #[snafu(display("failed to decode candid"))] CandidDecode { source: candid::Error }, - #[snafu(display("a canister call failed: {source}"))] + #[snafu(display("a canister call failed"))] Call { source: crate::calls::CallError }, #[snafu(transparent)] From 21d2f598567a4924ef307fc948d57aa83f4b9ba4 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:14:11 -0700 Subject: [PATCH 05/13] fix: report a canister with nothing installed as having no module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit module_hash mapped every state-tree read failure to an error, so the None the seam documents as "no module installed" was unreachable: an empty canister's absent module_hash path aborted `canister status --public` instead of printing . Read an absent path as absence, and — as metadata_section already does — confirm through the controllers path that there is a canister there at all, so "nothing installed" is never said about nothing. --- crates/icp-app/src/calls.rs | 43 +++++++++----- crates/icp-cli/tests/canister_status_tests.rs | 56 +++++++++++++++++++ crates/icp-project/src/calls.rs | 5 ++ 3 files changed, 90 insertions(+), 14 deletions(-) diff --git a/crates/icp-app/src/calls.rs b/crates/icp-app/src/calls.rs index 67f1526e8..cc56c96c3 100644 --- a/crates/icp-app/src/calls.rs +++ b/crates/icp-app/src/calls.rs @@ -89,6 +89,22 @@ impl AgentCalls { } } + /// Whether `canister` exists, as far as the certified state tree says. + /// + /// The `controllers` path is written when a canister is created, so its + /// presence is what separates a canister with nothing in it from one that + /// was never created at all. + /// + /// A check that could not be made reads as "does not exist": every caller + /// asks this while deciding whether to report a read failure of its own, + /// and an existence check that failed too is no reason to suppress it. + async fn exists(&self, canister: Principal) -> bool { + self.agent + .read_state_canister_controllers(canister) + .await + .is_ok() + } + /// A subnet-scoped update: routed to a subnet rather than to any canister /// on it, which the agent only exposes through a signed submission. async fn to_subnet(&self, subnet: Principal, call: &Call) -> Result, CallError> { @@ -190,15 +206,9 @@ impl CanisterCalls for AgentCalls { Ok(bytes) => Ok(Some(bytes)), Err(err) => { // A path the certificate will not certify looks the same as a - // canister that was never created. `controllers` is written at - // creation, so its presence is what separates "no such section" - // from "no such canister". - if self - .agent - .read_state_canister_controllers(canister) - .await - .is_ok() - { + // canister that was never created, which is the error this + // reports rather than "no such section". + if self.exists(canister).await { Ok(None) } else { Err(Self::wrap(canister, "read_state(metadata)", err)) @@ -215,11 +225,16 @@ impl CanisterCalls for AgentCalls { } async fn module_hash(&self, canister: Principal) -> Result>, CallError> { - self.agent - .read_state_canister_module_hash(canister) - .await - .map(Some) - .map_err(|e| Self::wrap(canister, "read_state(module_hash)", e)) + let err = match self.agent.read_state_canister_module_hash(canister).await { + Ok(hash) => return Ok(Some(hash)), + Err(err) => err, + }; + // No module-hash path means nothing is installed — or that there is no + // canister to install into, which is not an answer this can give. + if matches!(err, AgentError::LookupPathAbsent(_)) && self.exists(canister).await { + return Ok(None); + } + Err(Self::wrap(canister, "read_state(module_hash)", err)) } async fn subnet_of(&self, canister: Principal) -> Result { diff --git a/crates/icp-cli/tests/canister_status_tests.rs b/crates/icp-cli/tests/canister_status_tests.rs index 694d2a4c5..965d66f83 100644 --- a/crates/icp-cli/tests/canister_status_tests.rs +++ b/crates/icp-cli/tests/canister_status_tests.rs @@ -132,6 +132,62 @@ async fn canister_status_through_proxy() { ); } +/// A created-but-empty canister has no module, which the public status says +/// rather than treating the missing hash as a failed read. +#[tokio::test] +async fn public_status_reports_an_empty_canister_as_having_no_module() { + let ctx = TestContext::new(); + + let project_dir = ctx.create_project_dir("icp"); + + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: echo hi + + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} + "#}; + + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(10 * TRILLION); + + // Created, never installed. + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "create", + "my-canister", + "--environment", + "random-environment", + ]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "status", + "my-canister", + "--environment", + "random-environment", + "--public", + ]) + .assert() + .success() + .stdout(contains("Module hash: ").and(contains("controller: 2vxsx-fae"))); +} + /// A caller that may not read the status falls back on the state-tree /// information, rather than failing. #[tokio::test] diff --git a/crates/icp-project/src/calls.rs b/crates/icp-project/src/calls.rs index e428336c6..478dbe307 100644 --- a/crates/icp-project/src/calls.rs +++ b/crates/icp-project/src/calls.rs @@ -193,6 +193,11 @@ pub trait CanisterCalls: Send + Sync { async fn controllers(&self, canister: Principal) -> Result, CallError>; /// The hash of a canister's installed module, or `None` when it has none. + /// + /// A canister that does not exist is an error, as in + /// [`Self::metadata_section`] and for the same reason: nothing installed + /// and nothing there look alike, and separating them is the + /// implementation's job rather than the caller's. async fn module_hash(&self, canister: Principal) -> Result>, CallError>; /// Which subnet `canister` lives on. From 7e0532fb55ed12dc34d60c317c79e94af7407da8 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:16:49 -0700 Subject: [PATCH 06/13] fix: say that a canister does not exist when its controllers are absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the controllers of a canister that is not there reported a failed state-tree read — "" — where it used to say the canister was not found. Controllers exist for every canister that does, so their absence is the canister's: the seam now answers with an Option, which is also how the metadata and module-hash reads already tell one absence from the other. --- crates/icp-app/src/calls.rs | 23 +++-- .../icp-cli/src/commands/canister/status.rs | 14 +-- crates/icp-cli/tests/canister_status_tests.rs | 88 +++++++++++++++++++ crates/icp-project/src/calls.rs | 9 +- 4 files changed, 112 insertions(+), 22 deletions(-) diff --git a/crates/icp-app/src/calls.rs b/crates/icp-app/src/calls.rs index cc56c96c3..770a9f47a 100644 --- a/crates/icp-app/src/calls.rs +++ b/crates/icp-app/src/calls.rs @@ -89,20 +89,16 @@ impl AgentCalls { } } - /// Whether `canister` exists, as far as the certified state tree says. - /// - /// The `controllers` path is written when a canister is created, so its - /// presence is what separates a canister with nothing in it from one that - /// was never created at all. + /// Whether `canister` exists, which its controllers answer — see + /// [`CanisterCalls::controllers`]. /// /// A check that could not be made reads as "does not exist": every caller /// asks this while deciding whether to report a read failure of its own, /// and an existence check that failed too is no reason to suppress it. async fn exists(&self, canister: Principal) -> bool { - self.agent - .read_state_canister_controllers(canister) + self.controllers(canister) .await - .is_ok() + .is_ok_and(|controllers| controllers.is_some()) } /// A subnet-scoped update: routed to a subnet rather than to any canister @@ -217,11 +213,12 @@ impl CanisterCalls for AgentCalls { } } - async fn controllers(&self, canister: Principal) -> Result, CallError> { - self.agent - .read_state_canister_controllers(canister) - .await - .map_err(|e| Self::wrap(canister, "read_state(controllers)", e)) + async fn controllers(&self, canister: Principal) -> Result>, CallError> { + match self.agent.read_state_canister_controllers(canister).await { + Ok(controllers) => Ok(Some(controllers)), + Err(AgentError::LookupPathAbsent(_)) => Ok(None), + Err(err) => Err(Self::wrap(canister, "read_state(controllers)", err)), + } } async fn module_hash(&self, canister: Principal) -> Result>, CallError> { diff --git a/crates/icp-cli/src/commands/canister/status.rs b/crates/icp-cli/src/commands/canister/status.rs index 2113634ab..d6644577e 100644 --- a/crates/icp-cli/src/commands/canister/status.rs +++ b/crates/icp-cli/src/commands/canister/status.rs @@ -157,13 +157,13 @@ async fn build_public_status( cid: Principal, maybe_name: Option, ) -> Result { - let controllers = calls - .controllers(cid) - .await - .map_err(|e| anyhow!("could not read the controllers of canister {cid}: {e}"))? - .iter() - .map(|p| p.to_string()) - .collect(); + let controllers = match calls.controllers(cid).await { + Ok(Some(controllers)) => controllers.iter().map(|p| p.to_string()).collect(), + // No controllers means no canister, which is the more useful thing to + // say than that a read of them came back empty-handed. + Ok(None) => bail!("Canister {cid} was not found."), + Err(e) => bail!("could not read the controllers of canister {cid}: {e}"), + }; let module_hash = calls .module_hash(cid) .await diff --git a/crates/icp-cli/tests/canister_status_tests.rs b/crates/icp-cli/tests/canister_status_tests.rs index 965d66f83..2f7bf1c87 100644 --- a/crates/icp-cli/tests/canister_status_tests.rs +++ b/crates/icp-cli/tests/canister_status_tests.rs @@ -188,6 +188,94 @@ async fn public_status_reports_an_empty_canister_as_having_no_module() { .stdout(contains("Module hash: ").and(contains("controller: 2vxsx-fae"))); } +/// A canister that is not there at all is reported as missing, rather than as +/// a state-tree read that went wrong. +#[tokio::test] +async fn public_status_says_when_the_canister_does_not_exist() { + let ctx = TestContext::new(); + + let project_dir = ctx.create_project_dir("icp"); + + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: echo hi + + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} + "#}; + + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(10 * TRILLION); + + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "create", + "my-canister", + "--environment", + "random-environment", + ]) + .assert() + .success(); + + // Take the id before the canister goes away: deleting it drops the id from + // the store, and the point is to ask about an id in the subnet's range that + // no longer names a canister. + let output = ctx + .icp() + .current_dir(&project_dir) + .args([ + "canister", + "status", + "my-canister", + "--environment", + "random-environment", + "--id-only", + ]) + .output() + .expect("failed to read the canister id"); + let cid = String::from_utf8(output.stdout) + .expect("canister id should be utf-8") + .trim() + .to_owned(); + + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "delete", + "my-canister", + "--environment", + "random-environment", + ]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "status", + &cid, + "--environment", + "random-environment", + "--public", + ]) + .assert() + .failure() + .stderr(contains(format!("Canister {cid} was not found."))); +} + /// A caller that may not read the status falls back on the state-tree /// information, rather than failing. #[tokio::test] diff --git a/crates/icp-project/src/calls.rs b/crates/icp-project/src/calls.rs index 478dbe307..26c193c62 100644 --- a/crates/icp-project/src/calls.rs +++ b/crates/icp-project/src/calls.rs @@ -189,8 +189,13 @@ pub trait CanisterCalls: Send + Sync { path: &str, ) -> Result>, CallError>; - /// A canister's controllers. - async fn controllers(&self, canister: Principal) -> Result, CallError>; + /// A canister's controllers, or `None` when there is no such canister. + /// + /// Controllers are set when a canister is created, so a caller may read + /// their absence as the canister's — and an implementation reads it the + /// same way, to tell a certified absence apart from a canister that was + /// never there. + async fn controllers(&self, canister: Principal) -> Result>, CallError>; /// The hash of a canister's installed module, or `None` when it has none. /// From a81df6e9c6ed0090facb93d153941843f610415b Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:47:44 -0700 Subject: [PATCH 07/13] fix: let a call say it must come from the caller itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--proxy` is a property of the caller, applied to every call it made. That reaches calls whose destination cares who is calling: the deploy readiness probe and the frontend-URL check became proxied updates — where the probe's 2s budget and the check's reject code, both meant for a direct query, no longer hold — and the cycle-recovery call was made by the proxy despite the comment beside it. So let a request name the authority it is made under, which is also the distinction the callee draws, and mark those three. Forwarding stays ambient for everything else, including the call `icp canister call --proxy` exists to forward. --- crates/icp-app/src/calls.rs | 23 +++++++--- crates/icp-cli/src/commands/deploy.rs | 4 +- crates/icp-project/src/calls.rs | 42 +++++++++++++++---- crates/icp-project/src/operations/deploy.rs | 6 ++- .../src/operations/recover_cycles.rs | 15 +++---- 5 files changed, 68 insertions(+), 22 deletions(-) diff --git a/crates/icp-app/src/calls.rs b/crates/icp-app/src/calls.rs index 770a9f47a..d3e59f4b3 100644 --- a/crates/icp-app/src/calls.rs +++ b/crates/icp-app/src/calls.rs @@ -6,9 +6,11 @@ //! //! It also owns *proxy routing*. `--proxy` names a canister that forwards //! management calls on the caller's behalf, and it applies to the whole -//! command, so it is a property of the caller rather than of any one call. A -//! query made through a proxy necessarily becomes an update, which is why the -//! trait leaves how a query is answered to the implementation. +//! command, so it is a property of the caller rather than of any one call — +//! the proxy is the [`Authority::Mediated`] one, and a request that asks for +//! [`Authority::Direct`] skips it. A query made through a proxy necessarily +//! becomes an update, which is why the trait leaves how a query is answered to +//! the implementation. use async_trait::async_trait; use candid::{Encode, Nat, Principal}; @@ -17,7 +19,7 @@ use ic_agent::{ agent::{CallResponse, EffectiveId, SubnetType}, }; use icp_canister_interfaces::proxy::{ProxyArgs, ProxyResult}; -use icp_project::calls::{Call, CallError, CanisterCalls, RouteTo}; +use icp_project::calls::{Authority, Call, CallError, CanisterCalls, RouteTo}; /// [`CanisterCalls`] over an `ic-agent`, optionally forwarding through a proxy /// canister. @@ -44,6 +46,15 @@ impl AgentCalls { }) } + /// The proxy `call` should go through, if any: none when no proxy was + /// configured, and none when the call asked to be made directly. + fn mediator(&self, call: &Call) -> Option { + match call.authority { + Authority::Mediated => self.proxy, + Authority::Direct => None, + } + } + /// Turns an agent error into the shape the trait speaks in: a rejection is /// a verdict and keeps its code, anything else reached none. fn wrap(canister: Principal, method: &str, err: AgentError) -> CallError { @@ -152,7 +163,7 @@ impl CanisterCalls for AgentCalls { } async fn update(&self, call: Call) -> Result, CallError> { - if let Some(proxy) = self.proxy { + if let Some(proxy) = self.mediator(&call) { return self.through_proxy(proxy, &call).await; } if let RouteTo::Subnet(subnet) = call.route { @@ -173,7 +184,7 @@ impl CanisterCalls for AgentCalls { async fn query(&self, call: Call) -> Result, CallError> { // A proxy only accepts updates, so a query through one becomes an // update. The reply is the same either way. - if let Some(proxy) = self.proxy { + if let Some(proxy) = self.mediator(&call) { return self.through_proxy(proxy, &call).await; } let mut builder = self diff --git a/crates/icp-cli/src/commands/deploy.rs b/crates/icp-cli/src/commands/deploy.rs index 94288a40f..eaa5f4de3 100644 --- a/crates/icp-cli/src/commands/deploy.rs +++ b/crates/icp-cli/src/commands/deploy.rs @@ -198,8 +198,10 @@ async fn has_http_request(calls: &dyn CanisterCalls, canister_id: Principal) -> // zero-argument `http_request` reply, while a single-argument one still // fails to decode and traps; either way the method exists. let empty_args = candid::encode_args(()).expect("encoding () never fails"); + // Directly: the probe reads *why* the call failed, and a rejection relayed + // by an intermediary arrives as text with no error code to read. let result = calls - .query(Call::new(canister_id, "http_request", empty_args)) + .query(Call::new(canister_id, "http_request", empty_args).direct()) .await; match result { diff --git a/crates/icp-project/src/calls.rs b/crates/icp-project/src/calls.rs index 26c193c62..bceb7b760 100644 --- a/crates/icp-project/src/calls.rs +++ b/crates/icp-project/src/calls.rs @@ -32,6 +32,24 @@ pub enum RouteTo { Subnet(Principal), } +/// Which of a caller's authorities a request is made under. +/// +/// A caller may reach canisters through an *intermediary* that acts on its +/// behalf — `icp-cli`'s `--proxy` canister, say. The intermediary is what the +/// canister sees as its caller, so its permissions are the ones that apply, +/// and a request can ask to skip it and be made by the caller itself instead. +/// A caller with no intermediary has one authority, and both of these mean the +/// same thing for it. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum Authority { + /// However the caller normally reaches canisters: through its + /// intermediary, if it has one. + #[default] + Mediated, + /// The caller itself, skipping any intermediary. + Direct, +} + /// One canister call. #[derive(Clone, Debug)] pub struct Call { @@ -50,6 +68,9 @@ pub struct Call { /// Cycles to attach. Only meaningful for a call an implementation can fund /// — one made through a proxy canister, or from a canister of its own. pub cycles: u128, + + /// Whose authority to make the call under. See [`Authority`]. + pub authority: Authority, } impl Call { @@ -61,6 +82,7 @@ impl Call { arg, route: RouteTo::Callee, cycles: 0, + authority: Authority::Mediated, } } @@ -73,6 +95,7 @@ impl Call { arg, route: RouteTo::Canister(target), cycles: 0, + authority: Authority::Mediated, } } @@ -85,6 +108,13 @@ impl Call { self.cycles = cycles; self } + + /// Make the call under the caller's own authority, skipping any + /// intermediary it otherwise goes through. + pub fn direct(mut self) -> Self { + self.authority = Authority::Direct; + self + } } /// A call did not produce a reply. @@ -252,13 +282,11 @@ where { let arg = candid::encode_args(args).context(EncodeSnafu { method })?; let reply = calls - .update(Call { - canister, - method: method.to_owned(), - arg, - route, - cycles, - }) + .update( + Call::new(canister, method, arg) + .with_route(route) + .with_cycles(cycles), + ) .await?; candid::decode_args(&reply).context(DecodeSnafu { method }) } diff --git a/crates/icp-project/src/operations/deploy.rs b/crates/icp-project/src/operations/deploy.rs index 61d1f17d5..5ef78e834 100644 --- a/crates/icp-project/src/operations/deploy.rs +++ b/crates/icp-project/src/operations/deploy.rs @@ -763,7 +763,11 @@ async fn wait_until_serving_queries( let poll = async { let mut consecutive_ready: u32 = 0; loop { - let probe = calls.query(Call::new(canister_id, READINESS_PROBE_METHOD, Vec::new())); + // Directly: what this waits for is what a single replica can see, + // and an intermediary would answer from certified state instead — + // as an update, which the probe timeout below has no room for. + let probe = + calls.query(Call::new(canister_id, READINESS_PROBE_METHOD, Vec::new()).direct()); let ready = match tokio::time::timeout(PROBE_TIMEOUT, probe).await { Ok(Ok(_)) => true, // replied -> Running Ok(Err(err)) => is_serving_reject(&err), // non-stopped reject -> Running diff --git a/crates/icp-project/src/operations/recover_cycles.rs b/crates/icp-project/src/operations/recover_cycles.rs index 954eed6ee..9c4620794 100644 --- a/crates/icp-project/src/operations/recover_cycles.rs +++ b/crates/icp-project/src/operations/recover_cycles.rs @@ -167,14 +167,15 @@ pub async fn recover_cycles_before_delete( .await .context(StartCanisterSnafu { canister_id })?; - // Direct (non-proxied) call: the destination is the install arg, not the - // caller, so proxy routing is irrelevant here. let bytes = calls - .update(crate::calls::Call::new( - canister_id, - "recover_cycles", - Encode!().context(EncodeArgsSnafu)?, - )) + .update( + crate::calls::Call::new( + canister_id, + "recover_cycles", + Encode!().context(EncodeArgsSnafu)?, + ) + .direct(), + ) .await .context(CallRecoverSnafu { canister_id })?; let result = Decode!(&bytes, RecoverResult).context(DecodeResultSnafu { canister_id })?; From e8b12bb9d45b25c5cebe8a43425e061edd9e387f Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:48:24 -0700 Subject: [PATCH 08/13] docs: stop describing a proxy parameter cycle recovery no longer takes The doc named a `proxy` argument from when the operation threaded one through by hand; the management hops now go under whatever authority `calls` was built with. --- crates/icp-project/src/operations/recover_cycles.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/icp-project/src/operations/recover_cycles.rs b/crates/icp-project/src/operations/recover_cycles.rs index 9c4620794..7c8bbb83a 100644 --- a/crates/icp-project/src/operations/recover_cycles.rs +++ b/crates/icp-project/src/operations/recover_cycles.rs @@ -101,10 +101,11 @@ enum RecoverResult { /// canister with a meaningful balance failed to have its cycles recovered, so /// the caller can abort the delete. /// -/// `proxy` (the controller, when set) is used for the management hops -/// (install/start). The `recover_cycles` invocation itself is always a direct -/// call; the deposit destination is baked into the install argument, so routing -/// does not affect where the cycles land. +/// The management hops (settings, install, start) need the controller's +/// authority and are made under whichever one `calls` has. The `recover_cycles` +/// invocation itself is always a direct call; the deposit destination is baked +/// into the install argument, so who asks does not affect where the cycles +/// land. pub async fn recover_cycles_before_delete( calls: &dyn CanisterCalls, canister_id: Principal, From a2785f8beed3b8600e02f86e722a9a3cb99fa24b Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:24:01 -0700 Subject: [PATCH 09/13] fix: route a subnet-scoped call to its subnet rather than to a proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proxy branch came first, so a call routed to a subnet lost that routing — and gained a cycles amount — whenever the caller had a proxy. Only clap's refusal to accept `--subnet` with `--proxy` kept a legacy CloudEngine create from being made against the proxy's own subnet. --- crates/icp-app/src/calls.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/icp-app/src/calls.rs b/crates/icp-app/src/calls.rs index d3e59f4b3..fa3db5351 100644 --- a/crates/icp-app/src/calls.rs +++ b/crates/icp-app/src/calls.rs @@ -163,12 +163,16 @@ impl CanisterCalls for AgentCalls { } async fn update(&self, call: Call) -> Result, CallError> { - if let Some(proxy) = self.mediator(&call) { - return self.through_proxy(proxy, &call).await; - } + // A subnet-scoped call names the subnet it acts on, and a proxy could + // only act on the subnet it lives on itself — so this comes first, + // rather than dropping the one thing the call is about. The commands + // that can name a subnet refuse to name a proxy as well. if let RouteTo::Subnet(subnet) = call.route { return self.to_subnet(subnet, &call).await; } + if let Some(proxy) = self.mediator(&call) { + return self.through_proxy(proxy, &call).await; + } let mut builder = self .agent .update(&call.canister, &call.method) From 67df348e4a4513071016fb5602604e3446dd8f5c Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:25:08 -0700 Subject: [PATCH 10/13] fix: read a not-found rejection that carries no error code Matching only IC0301 narrowed what used to be a match on the reject code itself: a replica that populates no error code would take the CloudEngine path's missing-registry fallback away and fail the create outright. Fall back on the rejection message, as the readiness and http_request probes already do. --- crates/icp-project/src/operations/create.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index ada9784f4..adfc0b95a 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -695,8 +695,17 @@ pub fn shell_quote(value: &str) -> String { /// /// A rejection specifically: a call that never reached a verdict says nothing /// about whether the canister is there, and must not be read as absence. +/// +/// A replica that populated no error code leaves only the message to go on, so +/// that is the fallback — and a call with no verdict has no message either, +/// which is what keeps it out of this. fn is_canister_not_found(err: &crate::calls::CallError) -> bool { - err.code() == Some(CANISTER_NOT_FOUND) + match err.code() { + Some(code) => code == CANISTER_NOT_FOUND, + None => err + .message() + .is_some_and(|message| message.contains("Canister") && message.contains("not found")), + } } /// The replica's error code for a call addressed to a canister that does not @@ -821,9 +830,17 @@ mod tests { "Canister q6cfj-fyaaa-aaaar-qb77q-cai not found" ))); + // So does the same rejection from a replica that gave no error code, + // which leaves nothing but the message to read it from. + assert!(is_canister_not_found(&rejected( + None, + "Canister q6cfj-fyaaa-aaaar-qb77q-cai not found" + ))); + // Other rejections (e.g. a canister trap) must NOT be treated as // "not found" — they should propagate rather than fall back. assert!(!is_canister_not_found(&rejected(None, "trapped"))); + assert!(!is_canister_not_found(&rejected(Some("IC0503"), "trapped"))); // Neither may a call that reached no verdict at all: it says nothing // about whether the canister is there. From 3cdba9326e1c9790332c60ffd1285a3316ec9fa0 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 07:25:48 -0700 Subject: [PATCH 11/13] fix: name the engine-operator check in the error it produces The variant this read fails into still said "failed to get subnet for canister" from when it wrapped a canister's subnet lookup. It now wraps a question about an already-chosen subnet, so the user was told about a lookup that never happened; say which subnet and what was asked of it. --- crates/icp-project/src/operations/create.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index adfc0b95a..f8e4d7507 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -53,8 +53,13 @@ pub enum CreateOperationError { #[snafu(display("failed to create canister: {message}"))] CreateCanister { message: String }, - #[snafu(display("failed to get subnet for canister"))] - GetSubnet { source: crate::calls::CallError }, + #[snafu(display( + "failed to check whether subnet {subnet} creates canisters through an engine operator" + ))] + CheckEngineOperator { + source: crate::calls::CallError, + subnet: Principal, + }, #[snafu(display("failed to submit create_canister to subnet {subnet}"))] SubmitSubnetCreate { @@ -236,7 +241,9 @@ impl CreateOperation { .calls .subnet_uses_engine_operator(selected_subnet) .await - .context(GetSubnetSnafu)?; + .context(CheckEngineOperatorSnafu { + subnet: selected_subnet, + })?; let cid = if uses_engine_operator { // Resolve the subnet's engine-operator first. Only a definitive // "could not resolve an operator" resolution failure falls back to From 2533f2ea2caa5f4926cac1bb038a3f7625ba31ee Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 07:18:03 -0700 Subject: [PATCH 12/13] fix: stop a create target from naming a proxy nothing reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routing through a proxy became a property of the caller, so the principal `CreateTarget::Proxy` carried was no longer used for anything. Leaving it in the type let a caller pair a target naming one proxy with calls made through another, with nothing to catch the disagreement — the target now only says that the proxy is what pays. --- .../icp-cli/src/commands/canister/create.rs | 2 +- crates/icp-project/src/operations/create.rs | 33 ++++++++++--------- crates/icp-project/src/operations/deploy.rs | 4 +-- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/crates/icp-cli/src/commands/canister/create.rs b/crates/icp-cli/src/commands/canister/create.rs index efbd41f11..36d814f37 100644 --- a/crates/icp-cli/src/commands/canister/create.rs +++ b/crates/icp-cli/src/commands/canister/create.rs @@ -241,7 +241,7 @@ impl CreateArgs { fn create_target(&self) -> CreateTarget { match (self.subnet, self.proxy) { (Some(subnet), _) => CreateTarget::Subnet(subnet), - (_, Some(proxy)) => CreateTarget::Proxy(proxy), + (_, Some(_)) => CreateTarget::Proxy, _ => CreateTarget::None, } } diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index f8e4d7507..42b065e57 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -163,10 +163,11 @@ pub enum CreateFunding { pub enum CreateTarget { /// Create the canister on a specific subnet, chosen by the caller. Subnet(Principal), - /// Create the canister via a proxy canister. The `create_canister` call is - /// forwarded through the proxy's `proxy` method to the management canister, - /// so the new canister will be placed on the same subnet as the proxy. - Proxy(Principal), + /// Create the canister through the caller's proxy canister, which pays for + /// it and whose subnet the new canister lands on. Which proxy that is is a + /// property of the caller — every call it makes is forwarded through the + /// same one — so the target only says that the cycles come from there. + Proxy, /// No explicit target. The subnet is resolved automatically: either from an /// existing canister in the project or by picking a random available subnet. None, @@ -228,8 +229,8 @@ impl CreateOperation { return self.create_cmc(settings, amount, recovery_flags).await; } - if let CreateTarget::Proxy(proxy) = self.inner.target { - return self.create_proxy(settings, proxy).await; + if let CreateTarget::Proxy = self.inner.target { + return self.create_proxy(settings).await; } let selected_subnet = self @@ -446,20 +447,18 @@ impl CreateOperation { Ok(record.canister_id) } + /// Creation paid for by the proxy the caller already routes through: an + /// ordinary `create_canister` with cycles attached, which only a call made + /// from inside a canister can carry. async fn create_proxy( &self, settings: &CanisterSettings, - proxy: Principal, ) -> Result { let args = MgmtCreateCanisterArgs { settings: Some(settings.clone()), sender_canister_version: None, }; - // Routing through the proxy is ambient — the caller's `--proxy` is part - // of how every call in the command is made. What the target decides is - // where the cycles come from. - let _ = proxy; let result = proxy_management::create_canister(self.inner.calls.as_ref(), self.cycles(), args) .await?; @@ -851,10 +850,12 @@ mod tests { // Neither may a call that reached no verdict at all: it says nothing // about whether the canister is there. - assert!(!is_canister_not_found(&crate::calls::CallError::failed( - Principal::anonymous(), - "get_engine_operator_by_subnet", - std::io::Error::other("connection reset"), - ))); + assert!(!is_canister_not_found( + &crate::calls::CallError::unanswered( + Principal::anonymous(), + "get_engine_operator_by_subnet", + std::io::Error::other("connection reset"), + ) + )); } } diff --git a/crates/icp-project/src/operations/deploy.rs b/crates/icp-project/src/operations/deploy.rs index 5ef78e834..885169c8f 100644 --- a/crates/icp-project/src/operations/deploy.rs +++ b/crates/icp-project/src/operations/deploy.rs @@ -433,7 +433,7 @@ async fn create_canisters( ) -> Result<(), DeployError> { let target = match (params.subnet, params.proxy) { (Some(subnet), _) => CreateTarget::Subnet(subnet), - (_, Some(proxy)) => CreateTarget::Proxy(proxy), + (_, Some(_)) => CreateTarget::Proxy, _ => CreateTarget::None, }; let create_operation = CreateOperation::new( @@ -854,7 +854,7 @@ mod tests { #[test] fn a_call_that_reached_no_verdict_is_inconclusive() { // Not evidence of anything; the caller must retry. - assert!(!is_serving_reject(&CallError::failed( + assert!(!is_serving_reject(&CallError::unanswered( Principal::anonymous(), READINESS_PROBE_METHOD, std::io::Error::other("connection reset"), From b4162831c441aac75faef452277bc69d5e542af6 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 07:18:34 -0700 Subject: [PATCH 13/13] fix: retry only a call the network never answered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A call that reached no verdict was one case, so the snapshot transfers' retry loop had to treat every one of them as worth repeating — including a reply that would not parse or a certificate that would not verify, which fail the same way every time. Those now stall the command for the whole 60-second retry window before reporting what they already knew. Split the no-verdict case in two: a call that went unanswered is the transient one, and is what the retry loop asks for. The agent classifies a timeout or a transport failure as unanswered, which is what it retried before the calls went behind the trait. --- crates/icp-app/src/calls.rs | 50 ++++++++++++++++- .../src/operations/snapshot_transfer.rs | 12 ++--- .../icp-cli/src/commands/canister/status.rs | 2 +- crates/icp-cli/src/commands/deploy.rs | 2 +- crates/icp-project/src/calls.rs | 53 +++++++++++++++---- 5 files changed, 101 insertions(+), 18 deletions(-) diff --git a/crates/icp-app/src/calls.rs b/crates/icp-app/src/calls.rs index fa3db5351..14632cc31 100644 --- a/crates/icp-app/src/calls.rs +++ b/crates/icp-app/src/calls.rs @@ -56,7 +56,9 @@ impl AgentCalls { } /// Turns an agent error into the shape the trait speaks in: a rejection is - /// a verdict and keeps its code, anything else reached none. + /// a verdict and keeps its code, a call that ran out of time or never got + /// down the wire went unanswered, and anything else — a reply that would + /// not parse, a certificate that would not verify — failed for good. fn wrap(canister: Principal, method: &str, err: AgentError) -> CallError { match &err { AgentError::CertifiedReject { reject, .. } @@ -66,6 +68,9 @@ impl AgentCalls { code: reject.error_code.clone(), message: reject.reject_message.clone(), }, + AgentError::TimeoutWaitingForResponse() | AgentError::TransportError(_) => { + CallError::unanswered(canister, method, err) + } _ => CallError::failed(canister, method, err), } } @@ -267,3 +272,46 @@ impl CanisterCalls for AgentCalls { Ok(matches!(info.subnet_type(), Some(SubnetType::CloudEngine))) } } + +#[cfg(test)] +mod tests { + use super::*; + use ic_agent::agent::{RejectCode, RejectResponse}; + + fn wrapped(err: AgentError) -> CallError { + AgentCalls::wrap(Principal::anonymous(), "read_canister_snapshot_data", err) + } + + /// The retry loops in the snapshot transfers branch on this: only a call + /// the network never answered is worth repeating, so a deterministic + /// failure must not be classified as one and stall the command for the + /// whole retry window. + /// + /// `AgentError::TransportError` is transient for the same reason, but the + /// error it carries is private to `ic-agent` and cannot be built here. + #[test] + fn only_an_unanswered_call_is_transient() { + assert!(wrapped(AgentError::TimeoutWaitingForResponse()).is_transient()); + + // Deterministic: the reply was received and could not be understood. + assert!(!wrapped(AgentError::CertificateVerificationFailed()).is_transient()); + assert!(!wrapped(AgentError::MessageError("malformed reply".to_owned())).is_transient()); + } + + /// A rejection is the network's verdict: it keeps its code for callers to + /// branch on, and repeating the call would only earn it again. + #[test] + fn a_rejection_keeps_its_code_and_is_not_transient() { + let err = wrapped(AgentError::CertifiedReject { + reject: RejectResponse { + reject_code: RejectCode::CanisterError, + reject_message: "canister is stopped".to_owned(), + error_code: Some("IC0508".to_owned()), + }, + operation: None, + }); + assert_eq!(err.code(), Some("IC0508")); + assert!(err.is_rejection()); + assert!(!err.is_transient()); + } +} diff --git a/crates/icp-app/src/operations/snapshot_transfer.rs b/crates/icp-app/src/operations/snapshot_transfer.rs index 5009d24a5..bf1c4d45a 100644 --- a/crates/icp-app/src/operations/snapshot_transfer.rs +++ b/crates/icp-app/src/operations/snapshot_transfer.rs @@ -386,14 +386,14 @@ impl BlobType { /// Whether a failed call is worth trying again. /// -/// A rejection is the network's verdict and will be the same next time. Every -/// other failure means the call reached no verdict — a timeout, a dropped -/// connection — and the transfer can pick up where it left off. Encoding and -/// decoding failures fall on this side too; they are deterministic, so the -/// retry budget absorbs one wasted attempt rather than hiding a real problem. +/// Only a call the network never answered — a timeout, a dropped connection — +/// could come out differently, and the transfer picks up where it left off. A +/// rejection is a verdict, and everything else is deterministic: retrying +/// either would only stall the command for the whole retry window before +/// reporting the same thing. fn is_retryable(error: &TypedCallError) -> bool { match error { - TypedCallError::Call { source } => !source.is_rejection(), + TypedCallError::Call { source } => source.is_transient(), TypedCallError::Encode { .. } | TypedCallError::Decode { .. } => false, } } diff --git a/crates/icp-cli/src/commands/canister/status.rs b/crates/icp-cli/src/commands/canister/status.rs index d6644577e..aab97fb66 100644 --- a/crates/icp-cli/src/commands/canister/status.rs +++ b/crates/icp-cli/src/commands/canister/status.rs @@ -590,7 +590,7 @@ mod tests { // A call that reached no verdict has no code to branch on, so the // caller must not read it as an access refusal. let no_verdict = TypedCallError::Call { - source: icp_project::calls::CallError::failed( + source: icp_project::calls::CallError::unanswered( Principal::anonymous(), "canister_status", std::io::Error::other("connection reset"), diff --git a/crates/icp-cli/src/commands/deploy.rs b/crates/icp-cli/src/commands/deploy.rs index eaa5f4de3..225b3d835 100644 --- a/crates/icp-cli/src/commands/deploy.rs +++ b/crates/icp-cli/src/commands/deploy.rs @@ -441,7 +441,7 @@ mod tests { /// A call that reached no verdict at all. fn no_verdict() -> CallError { - CallError::failed( + CallError::unanswered( Principal::anonymous(), "http_request", std::io::Error::other("connection reset"), diff --git a/crates/icp-project/src/calls.rs b/crates/icp-project/src/calls.rs index bceb7b760..13d7a3a7c 100644 --- a/crates/icp-project/src/calls.rs +++ b/crates/icp-project/src/calls.rs @@ -119,10 +119,12 @@ impl Call { /// A call did not produce a reply. /// -/// The distinction that matters to callers is whether the network reached a -/// verdict: a rejection is an answer, and its code is worth branching on (a -/// canister reported as stopped, or as not found). Anything else means the -/// caller learned nothing and may want to try again. +/// Two distinctions matter to callers. The first is whether the network +/// reached a verdict: a rejection is an answer, and its code is worth +/// branching on (a canister reported as stopped, or as not found). The second +/// separates the calls that reached none: one the network never answered may +/// be answered on another attempt, while one whose arguments would not encode +/// or whose reply would not verify will fail the same way every time. #[derive(Debug, Snafu)] pub enum CallError { #[snafu(display("call to '{method}' on {canister} was rejected: {message}"))] @@ -134,9 +136,20 @@ pub enum CallError { message: String, }, - /// The call never reached a verdict — a transport failure, a timeout, a - /// malformed reply. The cause is carried whole because what a call travels - /// over is the implementation's business. + /// The call was submitted and no answer came back — a timeout, a dropped + /// connection. Nothing was learned about the canister, and the same call + /// may well be answered next time. + #[snafu(display("call to '{method}' on {canister} went unanswered"))] + Unanswered { + canister: Principal, + method: String, + source: Box, + }, + + /// The call reached no verdict and would not on another attempt — + /// arguments that would not encode, a reply that would not parse, a + /// certificate that did not verify. The cause is carried whole because + /// what a call travels over is the implementation's business. #[snafu(display("call to '{method}' on {canister} failed"))] Failed { canister: Principal, @@ -160,6 +173,20 @@ impl CallError { } } + /// Builds a [`CallError::Unanswered`] for `call` from an implementation's + /// own error. + pub fn unanswered( + canister: Principal, + method: impl Into, + source: impl std::error::Error + Send + Sync + 'static, + ) -> Self { + Self::Unanswered { + canister, + method: method.into(), + source: Box::new(source), + } + } + /// The replica's error code, when this was a rejection that carried one. /// /// Callers branch on this rather than on message text: `IC0508` means a @@ -167,7 +194,7 @@ impl CallError { pub fn code(&self) -> Option<&str> { match self { CallError::Rejected { code, .. } => code.as_deref(), - CallError::Failed { .. } => None, + CallError::Unanswered { .. } | CallError::Failed { .. } => None, } } @@ -177,12 +204,20 @@ impl CallError { matches!(self, CallError::Rejected { .. }) } + /// Whether making the same call again could come out differently. + /// + /// Only a call the network never answered could: a rejection is a verdict + /// and will be repeated, and every other failure is deterministic. + pub fn is_transient(&self) -> bool { + matches!(self, CallError::Unanswered { .. }) + } + /// The rejection message, for a caller that has to fall back on matching /// text because no error code was given. pub fn message(&self) -> Option<&str> { match self { CallError::Rejected { message, .. } => Some(message), - CallError::Failed { .. } => None, + CallError::Unanswered { .. } | CallError::Failed { .. } => None, } } }