refactor: Abstract out canister calling from icp-project - #774
Open
adamspofford-dfinity wants to merge 11 commits into
Open
refactor: Abstract out canister calling from icp-project#774adamspofford-dfinity wants to merge 11 commits into
adamspofford-dfinity wants to merge 11 commits into
Conversation
…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.
adamspofford-dfinity
added this pull request to stack #777
September 11, 2026 15:05
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
1 participant
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 💬