From 787ad8b88db70671db7e68909d73f41117b77aad Mon Sep 17 00:00:00 2001 From: Daniel Thurau Date: Mon, 17 Apr 2023 10:09:10 +0000 Subject: [PATCH] NNS1-1925 - Add the `controllers` field to the CanisterStatusResult served by NNS Root `canister_status` API --- Cargo.lock | 6 +- rs/nervous_system/root/BUILD.bazel | 1 + rs/nervous_system/root/Cargo.toml | 1 + rs/nervous_system/root/src/lib.rs | 156 +++++++++++++++- rs/nns/handlers/lifeline/tests/test.rs | 3 +- rs/nns/handlers/root/BUILD.bazel | 4 + rs/nns/handlers/root/Cargo.toml | 4 + rs/nns/handlers/root/canister/canister.rs | 58 ++++-- rs/nns/handlers/root/canister/root.did | 5 + rs/nns/handlers/root/src/lib.rs | 166 ++++++++++++++++++ rs/nns/handlers/root/tests/test.rs | 39 ++++ rs/nns/integration_tests/src/lifeline.rs | 20 ++- rs/nns/test_utils/src/governance.rs | 6 +- .../canister_test/src/canister.rs | 8 +- rs/sns/root/BUILD.bazel | 1 - rs/sns/root/Cargo.toml | 1 - rs/sns/root/canister/canister.rs | 2 +- rs/sns/root/canister/root.did | 2 + rs/sns/root/src/lib.rs | 14 +- 19 files changed, 449 insertions(+), 48 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a8ac3970f70..48dc1d03777 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7310,6 +7310,7 @@ dependencies = [ "ic-crypto-sha", "ic-ic00-types", "ic-nervous-system-common", + "lazy_static", "serde", "serde_bytes", ] @@ -7480,8 +7481,11 @@ dependencies = [ "dfn_macro", "hex", "ic-base-types", + "ic-canisters-http-types", "ic-crypto-sha", "ic-ic00-types", + "ic-metrics-encoder", + "ic-nervous-system-common", "ic-nervous-system-common-build-metadata", "ic-nervous-system-root", "ic-nns-common", @@ -7497,6 +7501,7 @@ dependencies = [ "ic-test-utilities-compare-dirs", "ic-types", "lazy_static", + "maplit", "on_wire", "prost", "registry-canister", @@ -8977,7 +8982,6 @@ dependencies = [ "ic-sns-swap", "ic-test-utilities-compare-dirs", "icrc-ledger-types", - "lazy_static", "num-traits", "prost", "serde", diff --git a/rs/nervous_system/root/BUILD.bazel b/rs/nervous_system/root/BUILD.bazel index dc33ee210c7..d5ef156a2e6 100644 --- a/rs/nervous_system/root/BUILD.bazel +++ b/rs/nervous_system/root/BUILD.bazel @@ -11,6 +11,7 @@ DEPENDENCIES = [ "//rs/types/ic00_types", "@crate_index//:candid", "@crate_index//:ic-cdk", + "@crate_index//:lazy_static", "@crate_index//:serde", "@crate_index//:serde_bytes", ] diff --git a/rs/nervous_system/root/Cargo.toml b/rs/nervous_system/root/Cargo.toml index e8d4d51a0ad..090631373ac 100644 --- a/rs/nervous_system/root/Cargo.toml +++ b/rs/nervous_system/root/Cargo.toml @@ -14,5 +14,6 @@ ic-cdk = { version = "0.6.0", default-features = false } ic-crypto-sha = { path = "../../crypto/sha/" } ic-nervous-system-common = { path="../common" } ic-ic00-types = { path = "../../types/ic00_types" } +lazy_static = "1.4.0" serde = { version = "1.0.99", features = ["derive"] } serde_bytes = "0.11" diff --git a/rs/nervous_system/root/src/lib.rs b/rs/nervous_system/root/src/lib.rs index 48779e20836..1b486ba4219 100644 --- a/rs/nervous_system/root/src/lib.rs +++ b/rs/nervous_system/root/src/lib.rs @@ -4,13 +4,24 @@ use ic_base_types::PrincipalId; use ic_crypto_sha::Sha256; use ic_ic00_types::{CanisterInstallMode, InstallCodeArgs, IC_00}; use ic_nervous_system_common::MethodAuthzChange; - +use lazy_static::lazy_static; use serde::Serialize; +use std::str::FromStr; pub const LOG_PREFIX: &str = "[Root Canister] "; +// Copied from /rs/replicated_state/src/canister_state/system_state.rs because the +// fields are not exported from there. These values may be present in the response +// from the replica and as such should be overridden. +lazy_static! { + pub static ref DEFAULT_PRINCIPAL_MULTIPLE_CONTROLLERS: PrincipalId = + PrincipalId::from_str("ifxlm-aqaaa-multi-pleco-ntrol-lersa-h3ae").unwrap(); + pub static ref DEFAULT_PRINCIPAL_ZERO_CONTROLLERS: PrincipalId = + PrincipalId::from_str("zrl4w-cqaaa-nocon-troll-eraaa-d5qc").unwrap(); +} + /// Copied from ic-types::ic_00::CanisterIdRecord. -#[derive(CandidType, Deserialize, Debug)] +#[derive(CandidType, Deserialize, Debug, Clone, Copy)] pub struct CanisterIdRecord { canister_id: CanisterId, } @@ -51,23 +62,86 @@ impl std::fmt::Display for CanisterStatusType { } } -/// Partial copy-paste of ic-types::ic_00::CanisterStatusResult. +/// Partial copy-paste of ic-types::ic_00::DefiniteCanisterSettings. /// /// Only the fields that we need are copied. /// Candid deserialization is supposed to be tolerant to having data for unknown /// fields (which is simply discarded). #[derive(CandidType, Debug, Deserialize, Eq, PartialEq)] +pub struct DefiniteCanisterSettings { + pub controllers: Vec, +} + +/// Partial copy-paste of ic-types::ic_00::CanisterStatusResult. +/// +/// Only the fields that we need are copied. +/// Candid deserialization is supposed to be tolerant to having data for unknown +/// fields (which are simply discarded). +#[derive(CandidType, Debug, Deserialize, Eq, PartialEq)] pub struct CanisterStatusResult { pub status: CanisterStatusType, pub module_hash: Option>, + // TODO NNS1-2170 - Remove this field when our clients no longer depend on it. pub controller: PrincipalId, pub memory_size: candid::Nat, + pub settings: DefiniteCanisterSettings, +} + +/// Partial copy-paste of ic-types::ic_00::CanisterStatusResult. +/// +/// Only the fields we need and are supported from the replica are copied. +/// Notice that `controller` is not present. Candid deserialization is tolerant +/// to having data for unknown fields (which are simply discarded). +#[derive(CandidType, Debug, Deserialize, Eq, PartialEq)] +struct CanisterStatusResultFromManagementCanister { + // no controller. This is fine regardless of whether it sends us controller. + pub status: CanisterStatusType, + pub module_hash: Option>, + pub memory_size: candid::Nat, + pub settings: DefiniteCanisterSettings, } impl CanisterStatusResult { pub fn controller(&self) -> PrincipalId { self.controller } + + /// Overrides any value returned in the non-standard and deprecated field `controller`. + /// This field can be deprecated from the CanisterStatusResult after downstream clients + /// have moved from its use. For now, the method severs the tie between the response + /// from the IC Interface and the response served to clients of NNS Root. + /// + /// If the controllers field is empty, this method follows the convention set by the + /// IC Interface and fills in the Default Principal for the required controller field. + fn fill_controller_field(self) -> Self { + let controllers = self.settings.controllers.clone(); + + // Let's set `controller` to be the first principal in `controllers`. + return if let Some(controller) = controllers.first() { + Self { + controller: *controller, + ..self + } + } else { + Self { + controller: *DEFAULT_PRINCIPAL_ZERO_CONTROLLERS, + ..self + } + }; + } +} + +impl From for CanisterStatusResult { + fn from(value: CanisterStatusResultFromManagementCanister) -> Self { + CanisterStatusResult { + controller: PrincipalId::new_anonymous(), + status: value.status, + module_hash: value.module_hash, + memory_size: value.memory_size, + settings: value.settings, + } + .fill_controller_field() + } } /// The payload to a proposal to upgrade a canister. @@ -390,13 +464,83 @@ where } } -pub async fn canister_status(canister_id_record: CanisterIdRecord) -> CanisterStatusResult { +pub async fn canister_status( + canister_id_record: CanisterIdRecord, +) -> Result, String)> { call( IC_00, "canister_status", - dfn_candid::candid, + dfn_candid::candid::, (canister_id_record,), ) .await - .unwrap() + .map(CanisterStatusResult::from) +} + +#[cfg(test)] +mod tests { + use crate::{ + CanisterStatusResult, CanisterStatusResultFromManagementCanister, CanisterStatusType, + DefiniteCanisterSettings, DEFAULT_PRINCIPAL_ZERO_CONTROLLERS, + }; + use ic_base_types::PrincipalId; + + #[test] + fn test_canister_status_result_from_management_sets_controller_when_multiple_are_present() { + let test_principal_1 = PrincipalId::new_user_test_id(1); + let test_principal_2 = PrincipalId::new_user_test_id(2); + let status = CanisterStatusResult::from(CanisterStatusResultFromManagementCanister { + status: CanisterStatusType::Running, + module_hash: None, + memory_size: Default::default(), + settings: DefiniteCanisterSettings { + controllers: vec![test_principal_1, test_principal_2], + }, + }); + assert_eq!(status.controller(), test_principal_1); + } + + #[test] + fn test_canister_status_result_from_management_sets_controller_when_none_are_present() { + let status = CanisterStatusResult::from(CanisterStatusResultFromManagementCanister { + memory_size: Default::default(), + settings: DefiniteCanisterSettings { + controllers: vec![], + }, + status: CanisterStatusType::Running, + module_hash: None, + }); + assert_eq!(status.controller(), *DEFAULT_PRINCIPAL_ZERO_CONTROLLERS); + } + + #[test] + fn test_canister_status_result_from_trait_for_canister_status_result_from_management_canister() + { + let test_principal = PrincipalId::new_user_test_id(1); + let m = CanisterStatusResultFromManagementCanister { + status: CanisterStatusType::Running, + module_hash: Some(vec![1, 2, 3]), + memory_size: candid::Nat::from(100), + settings: DefiniteCanisterSettings { + controllers: vec![test_principal], + }, + }; + + let expected_canister_status_result = CanisterStatusResult { + status: CanisterStatusType::Running, + module_hash: Some(vec![1, 2, 3]), + controller: test_principal, + memory_size: candid::Nat::from(100), + settings: DefiniteCanisterSettings { + controllers: vec![test_principal], + }, + }; + + let actual_canister_status_result = CanisterStatusResult::from(m); + + assert_eq!( + actual_canister_status_result, + expected_canister_status_result + ); + } } diff --git a/rs/nns/handlers/lifeline/tests/test.rs b/rs/nns/handlers/lifeline/tests/test.rs index f4b8272c87e..39b7824c0d0 100644 --- a/rs/nns/handlers/lifeline/tests/test.rs +++ b/rs/nns/handlers/lifeline/tests/test.rs @@ -1,6 +1,5 @@ use dfn_candid::candid; -use ic_ic00_types::CanisterIdRecord; -use ic_nervous_system_root::CanisterStatusResult; +use ic_ic00_types::{CanisterIdRecord, CanisterStatusResult}; use ic_nns_common::init::LifelineCanisterInitPayloadBuilder; use ic_nns_test_utils::itest_helpers::{ local_test_on_nns_subnet, set_up_lifeline_canister, set_up_universal_canister, diff --git a/rs/nns/handlers/root/BUILD.bazel b/rs/nns/handlers/root/BUILD.bazel index 0a388947bce..c66559fd1ef 100644 --- a/rs/nns/handlers/root/BUILD.bazel +++ b/rs/nns/handlers/root/BUILD.bazel @@ -15,6 +15,7 @@ filegroup( DEPENDENCIES = [ "//rs/crypto/sha", "//rs/nervous_system/root", + "//rs/nervous_system/common", "//rs/nns/common", "//rs/nns/constants", "//rs/nns/governance", @@ -25,10 +26,12 @@ DEPENDENCIES = [ "//rs/rust_canisters/dfn_candid", "//rs/rust_canisters/dfn_core", "//rs/rust_canisters/on_wire", + "//rs/rust_canisters/http_types", "//rs/types/base_types", "//rs/types/ic00_types", "@crate_index//:build-info", "@crate_index//:candid", + "@crate_index//:ic-metrics-encoder", "@crate_index//:prost", "@crate_index//:serde", "@crate_index//:serde_bytes", @@ -59,6 +62,7 @@ DEV_DEPENDENCIES = select({ "@crate_index//:assert_matches", "@crate_index//:hex", "@crate_index//:lazy_static", + "@crate_index//:maplit", ], }) diff --git a/rs/nns/handlers/root/Cargo.toml b/rs/nns/handlers/root/Cargo.toml index d0fa621b076..681c500724b 100644 --- a/rs/nns/handlers/root/Cargo.toml +++ b/rs/nns/handlers/root/Cargo.toml @@ -23,9 +23,12 @@ dfn_candid = {path="../../../rust_canisters/dfn_candid"} dfn_core = { path = "../../../rust_canisters/dfn_core" } dfn_macro = { path = "../../../rust_canisters/dfn_macro" } ic-base-types = {path="../../../types/base_types"} +ic-canisters-http-types = { path = "../../../rust_canisters/http_types" } ic-crypto-sha = {path = "../../../crypto/sha/"} ic-ic00-types = {path="../../../types/ic00_types"} +ic-metrics-encoder = "1" ic-nervous-system-common-build-metadata = { path = "../../../nervous_system/common/build_metadata" } +ic-nervous-system-common = { path = "../../../nervous_system/common" } ic-nervous-system-root = { path = "../../../nervous_system/root" } ic-nns-common ={path="../../common"} ic-nns-constants ={path="../../constants"} @@ -52,6 +55,7 @@ on_wire = { path = "../../../rust_canisters/on_wire" } assert_matches = "1.3.0" hex = "0.4.2" lazy_static = "1.4.0" +maplit = "1.0.2" registry-canister = { path = "../../../registry/canister" } [build-dependencies] diff --git a/rs/nns/handlers/root/canister/canister.rs b/rs/nns/handlers/root/canister/canister.rs index 25ee828f8ce..24ad156d903 100644 --- a/rs/nns/handlers/root/canister/canister.rs +++ b/rs/nns/handlers/root/canister/canister.rs @@ -1,17 +1,21 @@ -use dfn_candid::candid; +use candid::candid_method; +use dfn_candid::{candid, candid_one}; use dfn_core::{ api::caller, endpoint::{over, over_async}, stable, }; use ic_base_types::PrincipalId; +use ic_canisters_http_types::{HttpRequest, HttpResponse, HttpResponseBuilder}; +use ic_nervous_system_common::serve_metrics; use ic_nervous_system_root::{ - change_canister, AddCanisterProposal, CanisterIdRecord, ChangeCanisterProposal, - StopOrStartCanisterProposal, LOG_PREFIX, + change_canister, AddCanisterProposal, CanisterIdRecord, CanisterStatusResult, + ChangeCanisterProposal, StopOrStartCanisterProposal, LOG_PREFIX, }; use ic_nns_common::{access_control::check_caller_is_governance, types::CallCanisterProposal}; +use ic_nns_constants::ROOT_CANISTER_ID; use ic_nns_handler_root::{ - canister_management, + canister_management, encode_metrics, root_proposals::{GovernanceUpgradeRootProposal, RootProposalBallot}, }; @@ -19,7 +23,6 @@ fn main() {} #[cfg(target_arch = "wasm32")] use dfn_core::println; -use ic_nns_handler_root::canister_management::do_add_nns_canister; // canister_init and canister_post_upgrade are needed here // to ensure that printer hook is set up, otherwise error @@ -51,18 +54,27 @@ ic_nervous_system_common_build_metadata::define_get_build_metadata_candid_method #[export_name = "canister_update canister_status"] fn canister_status() { println!("{}canister_status", LOG_PREFIX); - over_async(candid, |(canister_id_record,): (CanisterIdRecord,)| { - ic_nervous_system_root::canister_status(canister_id_record) - }) + over_async(candid_one, canister_status_) } -#[export_name = "canister_update submit_change_nns_canister_proposal"] -fn submit_change_nns_canister_proposal() { - panic!( - "This method was removed in PR 11215. \ - Use instead function `manage_neuron` on the Governance canister \ - to submit a proposal to change an NNS canister." - ); +#[candid_method(update, rename = "canister_status")] +async fn canister_status_(canister_id_record: CanisterIdRecord) -> CanisterStatusResult { + ic_nns_handler_root::increment_open_canister_status_calls(canister_id_record.get_canister_id()); + + let canister_status_response = + ic_nervous_system_root::canister_status(canister_id_record).await; + + ic_nns_handler_root::decrement_open_canister_status_calls(canister_id_record.get_canister_id()); + + /* + TODO NNS1-2197 - Remove this un-needed call to get the canister_status of NNS Root(this canister) + when this call stack does not rely on panics to indicate errors. This call is made to commit the + open status call counter to canister memory. + */ + let _unused_canister_status_response = + ic_nervous_system_root::canister_status(CanisterIdRecord::from(ROOT_CANISTER_ID)).await; + + canister_status_response.unwrap() } #[export_name = "canister_update submit_root_proposal_to_upgrade_governance_canister"] @@ -130,7 +142,7 @@ fn change_nns_canister() { fn add_nns_canister() { check_caller_is_governance(); over_async(candid, |(proposal,): (AddCanisterProposal,)| async move { - do_add_nns_canister(proposal).await; + canister_management::do_add_nns_canister(proposal).await; }); } @@ -165,3 +177,17 @@ fn call_canister() { dfn_core::api::futures::spawn(future); }); } + +/// Resources to serve for a given http_request +#[export_name = "canister_query http_request"] +fn http_request() { + over(candid_one, serve_http) +} + +/// Serve an HttpRequest made to this canister +pub fn serve_http(request: HttpRequest) -> HttpResponse { + match request.path() { + "/metrics" => serve_metrics(encode_metrics), + _ => HttpResponseBuilder::not_found().build(), + } +} diff --git a/rs/nns/handlers/root/canister/root.did b/rs/nns/handlers/root/canister/root.did index 47b588a30c7..9d5216c5c25 100644 --- a/rs/nns/handlers/root/canister/root.did +++ b/rs/nns/handlers/root/canister/root.did @@ -1,10 +1,15 @@ type CanisterIdRecord = record { canister_id : principal }; +type definite_canister_settings = record { + controllers : vec principal; +}; + type CanisterStatusResult = record { controller : principal; status : variant { stopped; stopping; running }; memory_size : nat; module_hash : opt vec nat8; + settings: definite_canister_settings; }; service : { diff --git a/rs/nns/handlers/root/src/lib.rs b/rs/nns/handlers/root/src/lib.rs index f3d69ec4717..a290fae17f2 100644 --- a/rs/nns/handlers/root/src/lib.rs +++ b/rs/nns/handlers/root/src/lib.rs @@ -1,4 +1,170 @@ +use ic_base_types::CanisterId; +use std::{cell::RefCell, collections::BTreeMap}; + pub mod canister_management; pub mod init; pub mod pb; pub mod root_proposals; + +thread_local! { + /// The map of CanisterId to count of open `CanisterStatus` calls to that canister. This serve + /// as a data source for the metrics endpoint. This is stored in a local RefCell and is not persisted + /// across upgrades. This state need not be persisted because the canister will not upgrade if + /// there are any open call contexts, which is tracked by this map. + pub static OPEN_CANISTER_STATUS_CALLS: RefCell> = RefCell::new(BTreeMap::new()); +} + +pub fn increment_open_canister_status_calls(canister_id: CanisterId) { + OPEN_CANISTER_STATUS_CALLS.with(|open_calls| { + open_calls.borrow_mut().entry(canister_id).or_insert(0); + if let Some(counter) = open_calls.borrow_mut().get_mut(&canister_id) { + *counter = counter.saturating_add(1_u64); + } + }); +} + +pub fn decrement_open_canister_status_calls(canister_id: CanisterId) { + // TODO - remove entries in the map once their counter is decremented to + // zero. + OPEN_CANISTER_STATUS_CALLS.with(|open_calls| { + open_calls.borrow_mut().entry(canister_id).or_insert(0); + if let Some(counter) = open_calls.borrow_mut().get_mut(&canister_id) { + *counter = counter.saturating_sub(1_u64); + } + }); +} + +/// Encode the metrics in a format that can be understood by Prometheus. +pub fn encode_metrics(w: &mut ic_metrics_encoder::MetricsEncoder>) -> std::io::Result<()> { + let open_canister_status_calls = OPEN_CANISTER_STATUS_CALLS + .with(|open_canister_status_calls| open_canister_status_calls.borrow().clone()); + + let open_canister_status_calls_count: u64 = open_canister_status_calls.values().sum(); + + w.encode_gauge( + "nns_root_open_canister_status_calls_count", + open_canister_status_calls_count as f64, + "Count of open CanisterStatusCalls.", + )?; + + let mut metrics = w.gauge_vec( + "nns_root_open_canister_status_calls", + "The list of counters and canister_ids with open canister_status calls.", + )?; + + for (canister_id, call_count) in open_canister_status_calls.iter() { + metrics = metrics.value( + &[("canister_id", &format!("{}", canister_id))], + (*call_count) as f64, + )?; + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use crate::{ + decrement_open_canister_status_calls, increment_open_canister_status_calls, + OPEN_CANISTER_STATUS_CALLS, + }; + use ic_base_types::{CanisterId, PrincipalId}; + + fn get_canister_open_canister_status_calls(canister_id: CanisterId) -> Option { + OPEN_CANISTER_STATUS_CALLS.with(|open_calls| open_calls.borrow().get(&canister_id).cloned()) + } + + #[test] + pub fn test_increment_open_canister_status_calls() { + // Set up some test data + let canister_id_1 = CanisterId::try_from(PrincipalId::new_user_test_id(1)).unwrap(); + let canister_id_2 = CanisterId::try_from(PrincipalId::new_user_test_id(2)).unwrap(); + + // Assert that the value is None if a canister has never been incremented + assert_eq!(get_canister_open_canister_status_calls(canister_id_1), None); + + // Increment and check the value has increased + increment_open_canister_status_calls(canister_id_1); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_1), + Some(1) + ); + + // Assert that the value is None if a canister has never been incremented. + assert_eq!(get_canister_open_canister_status_calls(canister_id_2), None); + + // Increment a different canister and make sure it's count is incremented + increment_open_canister_status_calls(canister_id_2); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_2), + Some(1) + ); + + // Increment both keys + increment_open_canister_status_calls(canister_id_1); + increment_open_canister_status_calls(canister_id_2); + + // Assert the values have both been incremented again. + assert_eq!( + get_canister_open_canister_status_calls(canister_id_1), + Some(2) + ); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_2), + Some(2) + ); + } + + #[test] + pub fn test_decrement_open_canister_status_calls() { + // Set up some test data + let canister_id_1 = CanisterId::try_from(PrincipalId::new_user_test_id(1)).unwrap(); + let canister_id_2 = CanisterId::try_from(PrincipalId::new_user_test_id(2)).unwrap(); + let canister_id_3 = CanisterId::try_from(PrincipalId::new_user_test_id(3)).unwrap(); + + // Populate some keys with counts by incrementing + for _ in 0..3 { + increment_open_canister_status_calls(canister_id_1); + increment_open_canister_status_calls(canister_id_2); + } + + // Assert that both keys have expected values + assert_eq!( + get_canister_open_canister_status_calls(canister_id_1), + Some(3) + ); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_2), + Some(3) + ); + + // Decrement a key and check its value and the other value in the map + decrement_open_canister_status_calls(canister_id_1); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_1), + Some(2) + ); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_2), + Some(3) + ); + + // Decrement a different key past zero. This should result in 0 instead of a panic + for _ in 0..4 { + decrement_open_canister_status_calls(canister_id_2); + } + assert_eq!( + get_canister_open_canister_status_calls(canister_id_1), + Some(2) + ); + assert_eq!( + get_canister_open_canister_status_calls(canister_id_2), + Some(0) + ); + + // Decrement a non-existent key. This should result in 0 and not a panic. + assert_eq!(get_canister_open_canister_status_calls(canister_id_3), None); + decrement_open_canister_status_calls(canister_id_3); + assert_eq!(get_canister_open_canister_status_calls(canister_id_3), None); + } +} diff --git a/rs/nns/handlers/root/tests/test.rs b/rs/nns/handlers/root/tests/test.rs index a877c1d66ea..bf00bd558cf 100644 --- a/rs/nns/handlers/root/tests/test.rs +++ b/rs/nns/handlers/root/tests/test.rs @@ -1,6 +1,7 @@ use assert_matches::assert_matches; use candid::Encode; use dfn_candid::candid; +use ic_base_types::PrincipalId; use ic_ic00_types::CanisterInstallMode::Upgrade; use ic_nervous_system_root::{CanisterIdRecord, CanisterStatusResult, ChangeCanisterProposal}; use ic_nns_handler_root::init::RootCanisterInitPayloadBuilder; @@ -9,6 +10,8 @@ use ic_nns_test_utils::itest_helpers::{ set_up_universal_canister, }; use ic_test_utilities::universal_canister::UNIVERSAL_CANISTER_WASM; +use maplit::btreeset; +use std::collections::BTreeSet; /// Verifies that an anonymous user can get the status of any NNS canister /// through the root handler. @@ -35,6 +38,42 @@ fn test_get_status() { .await .unwrap(); assert_eq!(status.controller(), root.canister_id().get()); + assert_eq!(status.settings.controllers, vec![root.canister_id().get()]); + + Ok(()) + }); +} + +/// Verifies that an anonymous user can get the status of any canister controlled by root, and +/// this supports multiple controllers. +#[test] +fn test_get_status_multiple_controllers() { + local_test_on_nns_subnet(|runtime| async move { + let root = + set_up_root_canister(&runtime, RootCanisterInitPayloadBuilder::new().build()).await; + let other_controller = PrincipalId::new_user_test_id(1000); + + // Create some NNS canister to be own by the root and another controller + let universal = set_up_universal_canister(&runtime).await; + universal + .set_controllers(vec![root.canister_id().get(), other_controller]) + .await + .unwrap(); + + // Get the status of an NNS canister + let status: CanisterStatusResult = root + .update_( + "canister_status", + candid, + (CanisterIdRecord::from(universal.canister_id()),), + ) + .await + .unwrap(); + let actual_controllers: BTreeSet = + status.settings.controllers.iter().cloned().collect(); + let expected_controllers = btreeset! {other_controller, root.canister_id().get()}; + + assert_eq!(actual_controllers, expected_controllers); Ok(()) }); diff --git a/rs/nns/integration_tests/src/lifeline.rs b/rs/nns/integration_tests/src/lifeline.rs index 0474828c03d..40825e5b485 100644 --- a/rs/nns/integration_tests/src/lifeline.rs +++ b/rs/nns/integration_tests/src/lifeline.rs @@ -1,10 +1,10 @@ use dfn_candid::{candid, candid_one}; use ic_canister_client_sender::Sender; +use ic_ic00_types::{CanisterIdRecord, CanisterStatusResult}; use ic_nervous_system_common_test_keys::{ TEST_NEURON_1_OWNER_KEYPAIR, TEST_NEURON_2_OWNER_KEYPAIR, }; -use ic_nervous_system_root::{CanisterIdRecord, CanisterStatusResult}; use ic_nns_common::pb::v1::NeuronId; use ic_nns_common::types::ProposalId; use ic_nns_governance::pb::v1::manage_neuron::Command; @@ -64,7 +64,9 @@ fn test_submit_and_accept_root_canister_upgrade_proposal() { .await .expect("getting root canister status failed"); - let root_checksum = root_status.module_hash.expect("root canister has no hash"); + let root_checksum = root_status + .module_hash() + .expect("root canister has no hash"); assert_ne!( root_checksum, ic_crypto_sha::Sha256::hash(wasm_module.clone().as_slice()) @@ -74,7 +76,7 @@ fn test_submit_and_accept_root_canister_upgrade_proposal() { let magic = funny.to_le_bytes(); let proposal = create_external_update_proposal_candid( - "Proposal to ugprade the root canister", + "Proposal to upgrade the root canister", "", "", NnsFunction::NnsRootUpgrade, @@ -153,7 +155,9 @@ fn test_submit_and_accept_root_canister_upgrade_proposal() { .await .expect("getting root canister status failed"); - let root_checksum = root_status.module_hash.expect("root canister has no hash"); + let root_checksum = root_status + .module_hash() + .expect("root canister has no hash"); assert_eq!( root_checksum, ic_crypto_sha::Sha256::hash(wasm_module.as_slice()) @@ -190,7 +194,9 @@ fn test_submit_and_accept_forced_root_canister_upgrade_proposal() { .await .expect("getting root canister status failed"); - let root_checksum = root_status.module_hash.expect("root canister has no hash"); + let root_checksum = root_status + .module_hash() + .expect("root canister has no hash"); assert_ne!(root_checksum, ic_crypto_sha::Sha256::hash(empty_wasm)); let init_arg: &[u8] = &[]; @@ -275,7 +281,9 @@ fn test_submit_and_accept_forced_root_canister_upgrade_proposal() { .await .expect("getting root canister status failed"); - let root_checksum = root_status.module_hash.expect("root canister has no hash"); + let root_checksum = root_status + .module_hash() + .expect("root canister has no hash"); assert_eq!(root_checksum, ic_crypto_sha::Sha256::hash(empty_wasm)); Ok(()) diff --git a/rs/nns/test_utils/src/governance.rs b/rs/nns/test_utils/src/governance.rs index 5da97dd1858..682e38d0c3a 100644 --- a/rs/nns/test_utils/src/governance.rs +++ b/rs/nns/test_utils/src/governance.rs @@ -372,7 +372,7 @@ pub async fn upgrade_root_canister_by_proposal( ); loop { - let status: CanisterStatusResult = lifeline + let status: ic_ic00_types::CanisterStatusResult = lifeline .update_( "canister_status", candid_one, @@ -380,7 +380,9 @@ pub async fn upgrade_root_canister_by_proposal( ) .await .unwrap(); - if status.module_hash.unwrap().as_slice() == new_module_hash && status.status == Running { + if status.module_hash().unwrap().as_slice() == new_module_hash + && status.status() == ic_ic00_types::CanisterStatusType::Running + { break; } } diff --git a/rs/rust_canisters/canister_test/src/canister.rs b/rs/rust_canisters/canister_test/src/canister.rs index 1d2284126b9..fad87f9c808 100644 --- a/rs/rust_canisters/canister_test/src/canister.rs +++ b/rs/rust_canisters/canister_test/src/canister.rs @@ -735,14 +735,18 @@ impl<'a> Canister<'a> { } pub async fn set_controller(&self, new_controller: PrincipalId) -> Result<(), String> { + self.set_controllers(vec![new_controller]).await + } + + pub async fn set_controllers(&self, new_controllers: Vec) -> Result<(), String> { self.runtime .get_management_canister_with_effective_canister_id(self.canister_id().into()) .update_( ic00::Method::UpdateSettings.to_string(), - dfn_candid::candid_multi_arity, + candid_multi_arity, (UpdateSettingsArgs::new( self.canister_id, - CanisterSettingsArgs::new(Some(vec![new_controller]), None, None, None), + CanisterSettingsArgs::new(Some(new_controllers), None, None, None), ),), ) .await diff --git a/rs/sns/root/BUILD.bazel b/rs/sns/root/BUILD.bazel index dd119c9a108..5b2247da1c0 100644 --- a/rs/sns/root/BUILD.bazel +++ b/rs/sns/root/BUILD.bazel @@ -27,7 +27,6 @@ DEPENDENCIES = [ "@crate_index//:comparable", "@crate_index//:futures", "@crate_index//:ic-metrics-encoder", - "@crate_index//:lazy_static", "@crate_index//:num-traits", "@crate_index//:prost", "@crate_index//:serde", diff --git a/rs/sns/root/Cargo.toml b/rs/sns/root/Cargo.toml index 8407797a2d2..5402f31525b 100644 --- a/rs/sns/root/Cargo.toml +++ b/rs/sns/root/Cargo.toml @@ -29,7 +29,6 @@ ic-nervous-system-common-build-metadata = { path = "../../nervous_system/common/ ic-nervous-system-root = { path = "../../nervous_system/root" } ic-sns-swap = { path = "../swap" } icrc-ledger-types = { path = "../../../packages/icrc-ledger-types" } -lazy_static = "1.4.0" num-traits = "0.2.12" prost = "0.11.0" serde = { version = "1.0", features = ["derive"] } diff --git a/rs/sns/root/canister/canister.rs b/rs/sns/root/canister/canister.rs index 208b2744167..068469b0eff 100644 --- a/rs/sns/root/canister/canister.rs +++ b/rs/sns/root/canister/canister.rs @@ -191,7 +191,7 @@ fn canister_status() { async fn canister_status_( id: ic_nervous_system_root::CanisterIdRecord, ) -> ic_nervous_system_root::CanisterStatusResult { - ic_nervous_system_root::canister_status(id).await + ic_nervous_system_root::canister_status(id).await.unwrap() } /// Return the canister status of all SNS canisters that this root canister diff --git a/rs/sns/root/canister/root.did b/rs/sns/root/canister/root.did index 04985e9d945..500f4616129 100644 --- a/rs/sns/root/canister/root.did +++ b/rs/sns/root/canister/root.did @@ -4,6 +4,7 @@ type CanisterStatusResult = record { controller : principal; status : CanisterStatusType; memory_size : nat; + settings : DefiniteCanisterSettings; module_hash : opt vec nat8; }; type CanisterStatusResultV2 = record { @@ -23,6 +24,7 @@ type CanisterSummary = record { status : opt CanisterStatusResultV2; canister_id : opt principal; }; +type DefiniteCanisterSettings = record { controllers : vec principal }; type DefiniteCanisterSettingsArgs = record { controller : principal; freezing_threshold : nat; diff --git a/rs/sns/root/src/lib.rs b/rs/sns/root/src/lib.rs index 1250a6474cc..3b7722a761d 100644 --- a/rs/sns/root/src/lib.rs +++ b/rs/sns/root/src/lib.rs @@ -15,11 +15,10 @@ use dfn_core::CanisterId; use futures::{future::join_all, join}; use ic_base_types::{NumBytes, PrincipalId}; use ic_canister_log::log; +use ic_nervous_system_root::DEFAULT_PRINCIPAL_MULTIPLE_CONTROLLERS; use ic_sns_swap::pb::v1::GetCanisterStatusRequest; use icrc_ledger_types::icrc3::archive::ArchiveInfo; -use lazy_static::lazy_static; use num_traits::cast::ToPrimitive; -use std::str::FromStr; use std::{cell::RefCell, collections::BTreeSet, thread::LocalKey}; #[cfg(target_arch = "wasm32")] @@ -29,14 +28,6 @@ const ONE_DAY_SECONDS: u64 = 24 * 60 * 60; // The number of dapp canisters that can be registered with the SNS Root const DAPP_CANISTER_REGISTRATION_LIMIT: usize = 100; -// Copied from /rs/replicated_state/src/canister_state/system_state.rs -lazy_static! { - static ref DEFAULT_PRINCIPAL_MULTIPLE_CONTROLLERS: PrincipalId = - PrincipalId::from_str("ifxlm-aqaaa-multi-pleco-ntrol-lersa-h3ae").unwrap(); - static ref DEFAULT_PRINCIPAL_ZERO_CONTROLLERS: PrincipalId = - PrincipalId::from_str("zrl4w-cqaaa-nocon-troll-eraaa-d5qc").unwrap(); -} - /// Begin Local Copy of Various Candid Type definitions from ic00_types /// /// This is the standard practice; this allows the Candid interface to evolve @@ -1143,6 +1134,9 @@ mod tests { use candid::Principal; use dfn_core::api::now; use futures::FutureExt; + use ic_nervous_system_root::{ + DEFAULT_PRINCIPAL_MULTIPLE_CONTROLLERS, DEFAULT_PRINCIPAL_ZERO_CONTROLLERS, + }; use std::collections::VecDeque; use std::sync::{Arc, Mutex}; use std::time::SystemTime;