feat: Allow specifying other canisters as controllers in the manifest#546
feat: Allow specifying other canisters as controllers in the manifest#546adamspofford-dfinity wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for configuring canister controllers via manifests (icp.yaml, environment.yaml, canister.yaml), allowing controllers to be specified as either explicit principals or references to other canisters in the same project, with deferred resolution for canisters that don’t exist yet.
Changes:
- Extend JSON schemas to include a new
controllerssetting and aControllerRefdefinition. - Introduce
ControllerRefplus controller-resolution utilities in theicpcrate. - Update
icp-clisettings sync and create/deploy flows to apply controllers append-only, warn on unresolved canister-name refs, and sync dependents once a referenced controller canister is created.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/schemas/icp-yaml-schema.json | Adds controllers to canister settings plus ControllerRef schema definition. |
| docs/schemas/environment-yaml-schema.json | Adds controllers support and ControllerRef definition for environment manifests. |
| docs/schemas/canister-yaml-schema.json | Adds controllers support and ControllerRef definition for per-canister manifests. |
| crates/icp/src/canister/mod.rs | Introduces ControllerRef, resolution helpers, schema impl, and unit tests. |
| crates/icp-cli/src/operations/settings.rs | Applies controller syncing (append-only) with unresolved-name warnings and adds dependent syncing helper. |
| crates/icp-cli/src/commands/deploy.rs | Triggers dependent controller sync after new canisters are created; passes ID mapping into settings sync. |
| crates/icp-cli/src/commands/canister/settings/sync.rs | Plumbs ID mapping into settings sync and warns on unresolved controller refs. |
| crates/icp-cli/src/commands/canister/create.rs | Resolves manifest controller refs on create, injects caller for manifest-derived controllers, warns on unresolved refs, and triggers dependent syncing. |
| crates/icp-cli/tests/deploy_tests.rs | Adds integration tests for fixed-principal controllers and canister-name controller references during deploy. |
| crates/icp-cli/tests/canister_create_tests.rs | Adds integration tests for controller behavior during canister create, including deferred resolution + warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lwshang
left a comment
There was a problem hiding this comment.
ControllerRef::CanisterName entries are validated lazily — an unknown name (e.g. a typo) silently generates a warning on every create/deploy/sync run, forever, with no way to distinguish "not created yet" from "doesn't exist at all."
consolidate_manifest in crates/icp/src/project.rs is where all other cross-reference checks live. After the canister loop (line ~301), all canister names are known — a pass over each canister's settings.controllers that returns an error for any ControllerRef::CanisterName not in the map would catch typos at load time and fit the existing validation pattern.
| args.canister_settings_with_default(&canister_info, &ids, caller); | ||
| for name in &unresolved { | ||
| warn!( | ||
| "Controller canister '{name}' for '{canister}' does not exist yet; \ |
There was a problem hiding this comment.
Two nearly identical warnings use different phrasing for the same situation:
create.rs: "does not exist yet"settings/sync.rs: "has not been created yet"
Pick one and use it consistently.
Adds the ability to specify controllers in icp.yaml, including fixed principals and names of other canisters. When it is another canister, if the canister does not yet exist, then the controlled canister will be updated by the command that creates the controlling canister.
This implementation treats controller lists as append-only, since manual modifications are much more likely than other canister settings and more damaging to get wrong.