Skip to content

Commit 955833d

Browse files
lwshangclaude
andauthored
refactor: adopt ic-utils with_canister_settings in create_mgmt (#661)
* refactor: adopt ic-utils with_canister_settings in create_mgmt Bump the agent-rs crates (ic-agent, ic-utils, ic-identity-hsm) to 0.49.1 and ic-management-canister-types to 0.8.0 so create_mgmt can use the new CreateCanisterBuilder API from dfinity/agent-rs#737. create_mgmt now builds the call via ManagementCanister::create_canister().with_canister_settings(...), and routes to the target subnet with with_effective_subnet_id(subnet) instead of resolving a canister id from the subnet's ranges. This drops the manual Encode!/update/Decode! dance and the canister-range lookup; the function now takes the subnet Principal directly. Validated end-to-end by the canister_create_cloud_engine integration test. Also handle the breaking changes the bump introduces: - CanisterSettings/DefiniteCanisterSettings gained snapshot_visibility; add it to the two exhaustive settings literals (left None with a TODO to make it configurable like log_visibility in a follow-up PR). - ic_agent::identity::Delegation gained permissions; add permissions: None to the three literal constructions. - ManagementCanister::canister_status now returns QueryOrUpdateCall; use .call() in the canister top-up test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: explain why imported delegations use permissions: None The wire DelegationChain from the cli-backend canister has no permissions field and the CLI only ever signs unrestricted delegations, so every delegation reconstructed here was signed with permissions absent. Because the field is skip_serializing_if'd out of the signable hash when None, reproducing it as None yields the exact signed bytes and the chain still verifies in DelegatedIdentity::new. Documents this to preempt the concern that None drops a signed field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 95c24d4 commit 955833d

9 files changed

Lines changed: 77 additions & 51 deletions

File tree

‎Cargo.lock‎

Lines changed: 25 additions & 16 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,15 @@ hex = { version = "0.4.3", features = ["serde"] }
5353
hmac = { version = "0.13", features = ["zeroize"] }
5454
hybrid-array = { version = "0.4.10", features = ["zeroize"] }
5555
httptest = "0.16.3"
56-
ic-agent = { version = "0.47.0" }
56+
ic-agent = { version = "0.49.1" }
5757
ic-ed25519 = "0.6.0"
5858
ic-ledger-types = "0.16.0"
59-
ic-management-canister-types = { version = "0.7.1" }
60-
ic-utils = { version = "0.47.0" }
59+
ic-management-canister-types = { version = "0.8.0" }
60+
ic-utils = { version = "0.49.1" }
6161
icp = { path = "crates/icp" }
6262
icp-canister-interfaces = { path = "crates/icp-canister-interfaces" }
6363
icp-sync-plugin = { path = "crates/icp-sync-plugin" }
64-
ic-identity-hsm = "0.47.0"
64+
ic-identity-hsm = "0.49.1"
6565
icrc-ledger-types = "0.1.10"
6666
indexmap = { version = "2", features = ["serde"] }
6767
indicatif = "0.18.0"

‎crates/icp-cli/src/commands/canister/settings/update.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,10 @@ pub(crate) async fn exec(ctx: &Context, args: &UpdateArgs) -> Result<(), anyhow:
305305
log_memory_limit: args.log_memory_limit.as_ref().map(|m| Nat::from(m.get())),
306306
log_visibility,
307307
environment_variables,
308+
// TODO: expose snapshot_visibility as a `settings update` flag (with
309+
// set/add/remove-viewer sub-flags), mirroring log_visibility. Tracked for
310+
// a follow-up PR; until then, leave it unchanged.
311+
snapshot_visibility: None,
308312
};
309313

310314
proxy_management::update_settings(

‎crates/icp-cli/src/commands/identity/delegation/sign.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ pub(crate) async fn exec(ctx: &Context, args: &SignArgs) -> Result<(), SignError
5757
pubkey: target_pubkey.clone(),
5858
expiration,
5959
targets: args.canisters.clone(),
60+
permissions: None,
6061
};
6162

6263
let sig = identity

‎crates/icp-cli/src/operations/create.rs‎

Lines changed: 17 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,14 @@ use std::time::Duration;
33

44
use bigdecimal::{BigDecimal, ToPrimitive};
55
use candid::{Decode, Encode, IDLArgs, IDLValue, Nat, Principal};
6-
use ic_agent::{
7-
Agent, AgentError,
8-
agent::{Subnet, SubnetType},
9-
};
6+
use ic_agent::{Agent, AgentError, agent::SubnetType};
107
use ic_ledger_types::{
118
AccountIdentifier, Memo, Subaccount, Tokens, TransferArgs, TransferError, TransferResult,
129
};
1310
use ic_management_canister_types::{
14-
CanisterIdRecord, CanisterSettings, CreateCanisterArgs as MgmtCreateCanisterArgs,
11+
CanisterSettings, CreateCanisterArgs as MgmtCreateCanisterArgs,
1512
};
13+
use ic_utils::interfaces::ManagementCanister;
1614
use icp::parsers::to_token_unit_amount;
1715
use icp::signal::stop_signal;
1816
use icp_canister_interfaces::{
@@ -216,7 +214,7 @@ impl CreateOperation {
216214
.await
217215
.context(GetSubnetSnafu)?;
218216
let cid = if let Some(SubnetType::CloudEngine) = subnet_info.subnet_type() {
219-
self.create_mgmt(settings, &subnet_info).await?
217+
self.create_mgmt(settings, selected_subnet).await?
220218
} else {
221219
self.create_ledger(settings, selected_subnet).await?
222220
};
@@ -278,32 +276,22 @@ impl CreateOperation {
278276
async fn create_mgmt(
279277
&self,
280278
settings: &CanisterSettings,
281-
selected_subnet: &Subnet,
279+
subnet: Principal,
282280
) -> Result<Principal, CreateOperationError> {
283-
let arg = MgmtCreateCanisterArgs {
284-
settings: Some(settings.clone()),
285-
sender_canister_version: None,
286-
};
287-
288-
// Call management canister create_canister
289-
let resp = self
290-
.inner
291-
.agent
292-
.update(&Principal::management_canister(), "create_canister")
293-
.with_arg(Encode!(&arg).context(CandidEncodeSnafu)?)
294-
.with_effective_canister_id(
295-
*selected_subnet
296-
.iter_canister_ranges()
297-
.next()
298-
.context(CreateCanisterSnafu {
299-
message: "subnet did not contain canister ranges",
300-
})?
301-
.start(),
302-
)
281+
// Call the management canister's create_canister, routing the request to
282+
// the target subnet by its id (subnet-scoped, so no canister on that
283+
// subnet is needed to derive an effective canister id) and forwarding the
284+
// settings as a whole via `with_canister_settings`. Subnet-scoped routing
285+
// requires subnet-administrator permissions, which the CloudEngine path has.
286+
let mgmt = ManagementCanister::create(&self.inner.agent);
287+
let (canister_id,) = mgmt
288+
.create_canister()
289+
.with_effective_subnet_id(subnet)
290+
.with_canister_settings(settings.clone())
291+
.call_and_wait()
303292
.await
304293
.context(AgentSnafu)?;
305-
let resp: CanisterIdRecord = Decode!(&resp, CanisterIdRecord).context(CandidDecodeSnafu)?;
306-
Ok(resp.canister_id)
294+
Ok(canister_id)
307295
}
308296

309297
async fn create_proxy(

‎crates/icp-cli/src/operations/settings.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,10 @@ pub(crate) async fn sync_settings(
200200
log_memory_limit: log_memory_limit.as_ref().map(|m| Nat::from(m.get())),
201201
environment_variables: environment_variable_setting,
202202
controllers: controllers_setting,
203+
// TODO: make snapshot_visibility configurable from the manifest and synced
204+
// here, mirroring log_visibility (Controllers/Public/AllowedViewers).
205+
// Tracked for a follow-up PR; until then, leave it unchanged.
206+
snapshot_visibility: None,
203207
};
204208

205209
proxy_management::update_settings(

‎crates/icp-cli/tests/canister_top_up_tests.rs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,13 @@ async fn canister_top_up() {
5656

5757
let agent = ctx.agent();
5858
let mgmt = ManagementCanister::create(&agent);
59-
let canister_balance = mgmt.canister_status(&canister_id).await.unwrap().0.cycles;
59+
let canister_balance = mgmt
60+
.canister_status(&canister_id)
61+
.call()
62+
.await
63+
.unwrap()
64+
.0
65+
.cycles;
6066

6167
ctx.icp()
6268
.current_dir(&project_dir)
@@ -102,6 +108,12 @@ async fn canister_top_up() {
102108
))
103109
.success();
104110

105-
let new_canister_balance = mgmt.canister_status(&canister_id).await.unwrap().0.cycles;
111+
let new_canister_balance = mgmt
112+
.canister_status(&canister_id)
113+
.call()
114+
.await
115+
.unwrap()
116+
.0
117+
.cycles;
106118
assert!((canister_balance + 10 * TRILLION) - new_canister_balance < 100_000_000_u128);
107119
}

‎crates/icp/src/identity/delegation.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,13 @@ pub fn to_agent_types(
7676
pubkey,
7777
expiration,
7878
targets,
79+
// The wire `DelegationChain` carries no request-kind
80+
// permissions, and the delegations the CLI signs are always
81+
// unrestricted, so every delegation here was signed with
82+
// `permissions` absent. `None` reproduces the exact signed
83+
// bytes (it is `skip_serializing_if`'d out of the hash), so
84+
// the chain still verifies in `DelegatedIdentity::new`.
85+
permissions: None,
7986
},
8087
signature,
8188
})

‎crates/icp/src/identity/key.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,7 @@ fn create_pem_session_and_build_identity(
477477
pubkey: session_pubkey.clone(),
478478
expiration,
479479
targets: None,
480+
permissions: None,
480481
};
481482

482483
let sig = identity

0 commit comments

Comments
 (0)