chore(defi): remove deprecated ic-cdk imports in ic-ledger-suite-orchestrator#10291
chore(defi): remove deprecated ic-cdk imports in ic-ledger-suite-orchestrator#10291gregorydemay wants to merge 2 commits into
Conversation
…orchestrator
Migrate the orchestrator off the deprecated `ic_cdk::api::management_canister::main::*`,
`ic_cdk::api::call::{RejectionCode, call, call_with_payment, notify_with_payment128}`,
`ic_cdk::id`, `ic_cdk::api::canister_balance128` and `ic_cdk::api::stable::stable_size`
surfaces introduced in #6264, and drop the file-level `#![allow(deprecated)]`
attributes. `IcCanisterRuntime` now uses the modern `ic_cdk::management_canister`
functions and the `ic_cdk::call::Call` builder; the `Reason::from_reject`
helper is rewritten on top of the new `CallFailed` / `OnewayError` enums while
keeping the same `Reason` variants. `get_canister_status` now returns the modern
`CanisterStatusResult`; `ledger_suite_orchestrator.did` is updated to mirror
the new fields (matching the shape already adopted by the ckBTC and ckETH minters).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Removes reliance on deprecated ic-cdk APIs in ic-ledger-suite-orchestrator by migrating management-canister calls and call patterns to the modern ic_cdk::management_canister surface and the ic_cdk::call::Call builder, and updates the public Candid interface accordingly.
Changes:
- Replaced deprecated management canister imports/types and updated
get_canister_statusto return the modern status shape. - Rewrote canister-to-canister calls and cycle deposit logic to use
ic_cdk::call::Call(including oneway notifications). - Updated
ledger_suite_orchestrator.didto reflect the modernCanisterStatusResultfields; removed#![allow(deprecated)]from affected test crates.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rs/ethereum/ledger-suite-orchestrator/tests/tests.rs | Removes #![allow(deprecated)] after migrating off deprecated APIs. |
| rs/ethereum/ledger-suite-orchestrator/test_utils/src/lib.rs | Updates test utility decoding/types to use CanisterStatusResult. |
| rs/ethereum/ledger-suite-orchestrator/src/management/mod.rs | Migrates runtime management calls to ic_cdk::management_canister + Call builder; refactors rejection/error mapping. |
| rs/ethereum/ledger-suite-orchestrator/src/main.rs | Updates management-canister status endpoint, stable memory, and cycle balance calls to non-deprecated APIs. |
| rs/ethereum/ledger-suite-orchestrator/ledger_suite_orchestrator.did | Updates Candid types for the new canister status response shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks for the cleanup @gregorydemay!
| controllers.len() | ||
| ); | ||
| let payment = u128::from(cycles_for_canister_creation); | ||
| let balance = ic_cdk::api::canister_cycle_balance(); |
There was a problem hiding this comment.
nit: I think it would be more accurate to call canister_liquid_cycle_balance here, but we would anyway return OutOfCycles from the create_canister call - the only difference would be that in case canister_cycle_balance is enough, but the liquid balance is too low, we'd end up making a call to create_canister that we could otherwise have avoided. Perhaps a test for this case could make sense (setting a high freezing threshold would reserve a lot of cycles)?
There was a problem hiding this comment.
🤖 Good catch — 824644f switches the pre-flight to ic_cdk::api::canister_liquid_cycle_balance() so it matches what Call::await will check internally (the variant that becomes CallFailed::InsufficientLiquidCycleBalance). Same Reason::OutOfCycles end result, but no longer wastes a Call::await when liquid balance is below payment and the raw cycle balance is above it.
I left the high-freezing-threshold integration test as a follow-up for now: it would need a new LedgerSuiteOrchestrator::with_high_freezing_threshold helper in test_utils and a dedicated test against create_canister, and the unit tests I added in src/management/tests.rs already lock in the InsufficientLiquidCycleBalance → OutOfCycles mapping. Happy to add the integration test in a follow-up PR if you think it would be valuable.
| rejected.raw_reject_code(), | ||
| rejected.reject_message().to_string(), | ||
| ), | ||
| Error::InsufficientLiquidCycleBalance(_) => Self::OutOfCycles, |
There was a problem hiding this comment.
Same comment as in the other PR - this seems like new behavior, and it could make sense to add a test.
There was a problem hiding this comment.
🤖 Same context as on #10290 (comment): the mapping is intentional new behaviour from the cdk 0.18 surface — Reason::OutOfCycles was a defined-but-never-produced variant before, and routing InsufficientLiquidCycleBalance to it yields a more accurate error message than the previous InternalError fallback.
824644f adds unit tests in src/management/tests.rs covering this for all three mapping helpers (from_call_error, from_call_failed, from_oneway_error) plus the reject-code and CallPerformFailed paths to lock in the existing shape.
Per review on #10291: 1. `IcCanisterRuntime::create_canister` pre-flight now reads `ic_cdk::api::canister_liquid_cycle_balance` instead of `canister_cycle_balance`. This matches the cdk's own pre-flight check (which produces `CallFailed::InsufficientLiquidCycleBalance`) and avoids issuing a `Call::await` we know would fail when the cycle balance is high but the liquid balance is below the freezing-threshold reserve. 2. Add unit tests in `src/management/tests.rs` covering the three error mapping helpers (`Reason::from_call_error`, `from_call_failed`, `from_oneway_error`) — including the new `InsufficientLiquidCycleBalance → OutOfCycles` mapping the reviewer flagged, plus companion tests for the reject-code and `CallPerformFailed` paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes the last of the three remaining crates listed in DEFI-2304 — removing the
#![allow(deprecated)]attributes introduced in #6264 whenic-cdkwas upgraded to 0.18, by migrating the affected files off the deprecatedic_cdksurface.This PR handles
ic-ledger-suite-orchestrator:ic_cdk::api::management_canister::main::{CanisterIdRecord, CanisterStatusResponse, canister_status, start_canister, stop_canister}→ic_cdk::management_canister::{CanisterStatusArgs, CanisterStatusResult, StartCanisterArgs, StopCanisterArgs, canister_status, start_canister, stop_canister}.ic_cdk::api::call::{call, call_with_payment}andnotify_with_payment128inIcCanisterRuntimeare rewritten on top of theic_cdk::call::Callbuilder (Call::unbounded_wait(...).with_arg(...).with_cycles(...)and.oneway()for deposit_cycles).ic_cdk::api::call::RejectionCode→ic_cdk::call::{CallFailed, OnewayError, RejectCode}; the existingReason::from_rejectmapping is split intofrom_call_error/from_call_failed/from_oneway_errorhelpers that share a commonfrom_reject_code_messagecore, preserving the sameReasonvariants.ic_cdk::id()→ic_cdk::api::canister_self().ic_cdk::api::canister_balance128()→ic_cdk::api::canister_cycle_balance().ic_cdk::api::stable::stable_size()→ic_cdk::stable::stable_size().get_canister_statusnow returns the modernCanisterStatusResult;ledger_suite_orchestrator.didis updated to mirror the new fields (the same shape already adopted by ckBTC minter in chore(XC): remove deprecation fromic-cdkinic-ckbtc-minter#6761 and ckETH minter in chore(defi): remove deprecated ic-cdk imports in ic-cketh-minter #10290).create_canisterstill pays the exactcycles_for_canister_creationamount the caller supplies (rather than letting the cdk auto-compute the cost viacreate_canister_with_extra_cycles), so the orchestrator's existing semantics around cycle accounting are unchanged.Follows the pattern set by #6755 (
ic-btc-checker) and #6761 (ic-ckbtc-minter). No behavior change.