Skip to content

refactor: Abstract out canister calling from icp-project - #774

Open
adamspofford-dfinity wants to merge 13 commits into
spofford/abstract-filesystemfrom
spofford/abstract-canister-calls
Open

refactor: Abstract out canister calling from icp-project#774
adamspofford-dfinity wants to merge 13 commits into
spofford/abstract-filesystemfrom
spofford/abstract-canister-calls

Conversation

@adamspofford-dfinity

@adamspofford-dfinity adamspofford-dfinity commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Stack created with GitHub Stacks CLI • Give Feedback 💬

@adamspofford-dfinity
adamspofford-dfinity requested a review from a team as a code owner September 11, 2026 15:05
@adamspofford-dfinity
adamspofford-dfinity added this pull request to stack #777 September 11, 2026 15:05
@adamspofford-dfinity
adamspofford-dfinity requested a balanced review from Copilot September 14, 2026 12:48
Copilot AI reviewed Sep 14, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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-agent usage.
  • 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.

Comment thread crates/icp-app/src/calls.rs
Comment thread crates/icp-project/src/operations/create.rs Outdated
Comment thread crates/icp-app/src/operations/snapshot_transfer.rs
@adamspofford-dfinity
adamspofford-dfinity force-pushed the spofford/abstract-canister-calls branch from 9958b8c to 4c3c7c2 Compare September 14, 2026 14:23
@adamspofford-dfinity
adamspofford-dfinity force-pushed the spofford/abstract-canister-calls branch 2 times, most recently from f6d0fc2 to 90db656 Compare September 14, 2026 18:34
@adamspofford-dfinity
refactor: reach canisters through a seam shaped like the plugin inter…
7a63f45 
…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.
@adamspofford-dfinity
fix: pass a deferred value's cause through instead of restating it
7bfec18 
`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".
@adamspofford-dfinity
fix: stop appending a call's cause to its own message
586401d 
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.
@adamspofford-dfinity
fix: stop appending a call's cause to the create error's message
79ee20f 
`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.
@adamspofford-dfinity
fix: report a canister with nothing installed as having no module
21d2f59 
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.
@adamspofford-dfinity
fix: say that a canister does not exist when its controllers are absent
7e0532f 
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.
@adamspofford-dfinity
fix: let a call say it must come from the caller itself
a81df6e 
`--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.
@adamspofford-dfinity
docs: stop describing a proxy parameter cycle recovery no longer takes
e8b12bb 
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.
@adamspofford-dfinity
fix: route a subnet-scoped call to its subnet rather than to a proxy
a2785f8 
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.
@adamspofford-dfinity
fix: read a not-found rejection that carries no error code
67df348 
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.
@adamspofford-dfinity
fix: name the engine-operator check in the error it produces
3cdba93 
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.
@adamspofford-dfinity
fix: stop a create target from naming a proxy nothing reads
2533f2e 
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.
@adamspofford-dfinity
fix: retry only a call the network never answered
b416283 
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
adamspofford-dfinity force-pushed the spofford/abstract-canister-calls branch from 90db656 to b416283 Compare September 14, 2026 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adamspofford-dfinity