refactor: Abstract out canister calling from icp-project - #774
Open
adamspofford-dfinity wants to merge 13 commits into
Open
adamspofford-dfinity wants to merge 13 commits into
adamspofford-dfinity wants to merge 13 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Metadata failures can be misreported as absence, retry classification delays deterministic failures, and proxy creation routing has conflicting sources of truth.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors canister communication behind a reusable CanisterCalls abstraction while preserving proxy routing and deferred agent creation.
Changes:
- Adds call, routing, authority, and deferred-initialization abstractions.
- Migrates project operations and CLI commands away from direct
ic-agentusage. - Adds public-status coverage for empty and missing canisters.
File summaries
| File | Description |
|---|---|
crates/icp-project/src/store_id.rs |
Uses Candid’s Principal. |
crates/icp-project/src/operations/settings.rs |
Migrates settings calls. |
crates/icp-project/src/operations/recover_cycles.rs |
Migrates cycle recovery calls. |
crates/icp-project/src/operations/proxy.rs |
Removes legacy proxy helpers. |
crates/icp-project/src/operations/proxy_management.rs |
Centralizes typed management calls. |
crates/icp-project/src/operations/mod.rs |
Removes the proxy module. |
crates/icp-project/src/operations/misc.rs |
Abstracts metadata reads. |
crates/icp-project/src/operations/install.rs |
Migrates installation operations. |
crates/icp-project/src/operations/deploy.rs |
Uses deferred abstract callers. |
crates/icp-project/src/operations/create.rs |
Migrates canister creation routing. |
crates/icp-project/src/operations/candid_compat.rs |
Abstracts compatibility metadata reads. |
crates/icp-project/src/operations/binding_env_vars.rs |
Migrates environment updates. |
crates/icp-project/src/lib.rs |
Exports calls and deferred modules. |
crates/icp-project/src/defer.rs |
Adds generic deferred initialization. |
crates/icp-project/src/calls.rs |
Defines the calling abstraction. |
crates/icp-project/src/agent.rs |
Removes LazyAgent. |
crates/icp-cli/tests/canister_status_tests.rs |
Tests public status edge cases. |
crates/icp-cli/src/commands/sync.rs |
Uses abstract calls during sync. |
crates/icp-cli/src/commands/message/send.rs |
Migrates Candid metadata lookup. |
crates/icp-cli/src/commands/deploy.rs |
Constructs deferred call providers. |
crates/icp-cli/src/commands/canister/stop.rs |
Migrates stop calls. |
crates/icp-cli/src/commands/canister/status.rs |
Migrates status and public reads. |
crates/icp-cli/src/commands/canister/start.rs |
Migrates start calls. |
crates/icp-cli/src/commands/canister/snapshot/upload.rs |
Migrates snapshot uploads. |
crates/icp-cli/src/commands/canister/snapshot/restore.rs |
Migrates snapshot restoration. |
crates/icp-cli/src/commands/canister/snapshot/list.rs |
Migrates snapshot listing. |
crates/icp-cli/src/commands/canister/snapshot/download.rs |
Migrates snapshot downloads. |
crates/icp-cli/src/commands/canister/snapshot/delete.rs |
Migrates snapshot deletion. |
crates/icp-cli/src/commands/canister/snapshot/create.rs |
Migrates snapshot creation. |
crates/icp-cli/src/commands/canister/settings/update.rs |
Migrates settings updates. |
crates/icp-cli/src/commands/canister/settings/sync.rs |
Migrates settings synchronization. |
crates/icp-cli/src/commands/canister/settings/show.rs |
Migrates settings retrieval. |
crates/icp-cli/src/commands/canister/migrate_id.rs |
Migrates ID migration calls. |
crates/icp-cli/src/commands/canister/metadata.rs |
Migrates metadata reads. |
crates/icp-cli/src/commands/canister/logs.rs |
Migrates log queries. |
crates/icp-cli/src/commands/canister/install.rs |
Migrates installation command calls. |
crates/icp-cli/src/commands/canister/delete.rs |
Migrates deletion and recovery calls. |
crates/icp-cli/src/commands/canister/create.rs |
Constructs abstract create callers. |
crates/icp-cli/src/commands/canister/call.rs |
Routes raw calls through the abstraction. |
crates/icp-cli/src/call_output.rs |
Abstracts Candid metadata lookup. |
crates/icp-app/src/operations/snapshot_transfer.rs |
Migrates snapshot transfer and retries. |
crates/icp-app/src/lib.rs |
Exports the call implementation. |
crates/icp-app/src/calls.rs |
Implements calls using ic-agent. |
Review details
- Files reviewed: 43/43 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adamspofford-dfinity
force-pushed
the
spofford/abstract-canister-calls
branch
from
September 14, 2026 14:23
9958b8c to
4c3c7c2
Compare
adamspofford-dfinity
force-pushed
the
spofford/abstract-canister-calls
branch
2 times, most recently
from
September 14, 2026 18:34
f6d0fc2 to
90db656
Compare
…face Every operation in `icp-project` talked to canisters through an `ic_agent::Agent`, and threaded an `Option<Principal>` 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.
`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".
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.
`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.
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 <none>. 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.
Reading the controllers of a canister that is not there reported a failed state-tree read — "<lookup path absent>" — 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.
`--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.
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.
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.
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.
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.
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.
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.
adamspofford-dfinity
force-pushed
the
spofford/abstract-canister-calls
branch
from
September 14, 2026 19:14
90db656 to
b416283
Compare
lwshang
reviewed
Sep 18, 2026
Contributor
There was a problem hiding this comment.
586401d drops {source} on the grounds that the error still reaches main for a full chain, "because the concurrent callers short-circuit on it with try_join_all". create_canisters is the one that doesn't: it drains FuturesOrdered and keeps only the first error via error.get_or_insert. So when two canisters fail, the second's cause is printed nowhere — its only trace is the Display-only task line at L458.
TaskOutcome::Failed already carries causes, and crate::error::causes (added in #775) would fill it. sync.rs is the only call site populating it today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack created with GitHub Stacks CLI • Give Feedback 💬