feat(nns): Implement proposal execution for updating settings for root (

#866)

# Why

The `update_canister_settings` proposal type should work on cansiters
controlled by root (already implemented) as well as root itself (this
PR)

Only a controller of a canister can change its settings. lifeline is the
controller of root. Therefore, execution of update settings proposals
that target root are implemented by calling a (new) method of lifeline.
Whereas, for all other NNS canisters, update settings proposals are
implemented via a call to root.

# What

* Add an endpoint on the lifeline (update_root_settings)
* Call `Lifeline::update_root_settings` from Governance for proposal
execution
* Minor changes
  * Fix the error messages for some times
  * Fix the lifeline.did which was out-dated

rs/nns/governance/src/proposals/install_code.rs

@@ -202,9 +202,9 @@ mod tests {
         };
 
         let is_invalid_proposal_with_keywords = |install_code: InstallCode, keywords: Vec<&str>| {
-            let error = install_code
-                .validate()
-                .expect_err("Expecting validation error for {install_code:?} but got Ok(())");
+            let error = install_code.validate().expect_err(&format!(
+                "Expecting validation error for {install_code:?} but got Ok(())"
+            ));
             assert_eq!(error.error_type, ErrorType::InvalidProposal as i32);
             for keyword in keywords {
                 let error_message = error.error_message.to_lowercase();

rs/nns/governance/src/proposals/stop_or_start_canister.rs

@@ -133,9 +133,9 @@ mod tests {
 
         let is_invalid_proposal_with_keywords =
             |stop_or_start_canister: StopOrStartCanister, keywords: Vec<&str>| {
-                let error = stop_or_start_canister.validate().expect_err(
-                    "Expecting validation error for {stop_or_start_canister:?} but got Ok(())",
-                );
+                let error = stop_or_start_canister.validate().expect_err(&format!(
+                    "Expecting validation error for {stop_or_start_canister:?} but got Ok(())"
+                ));
                 assert_eq!(error.error_type, ErrorType::InvalidProposal as i32);
                 for keyword in keywords {
                     let error_message = error.error_message.to_lowercase();

rs/nns/governance/src/proposals/update_canister_settings.rs

@@ -12,7 +12,7 @@ use ic_base_types::CanisterId;
 use ic_nervous_system_clients::update_settings::{
     CanisterSettings as RootCanisterSettings, LogVisibility as RootLogVisibility,
 };
-use ic_nns_constants::ROOT_CANISTER_ID;
+use ic_nns_constants::{LIFELINE_CANISTER_ID, ROOT_CANISTER_ID};
 use ic_nns_handler_root_interface::UpdateCanisterSettingsRequest;
 
 impl UpdateCanisterSettings {
@@ -37,11 +37,6 @@ impl UpdateCanisterSettings {
             .ok_or(invalid_proposal_error("Canister ID is required"))?;
         let canister_id = CanisterId::try_from(canister_principal_id)
             .map_err(|_| invalid_proposal_error("Invalid canister ID"))?;
-        if canister_id == ROOT_CANISTER_ID {
-            return Err(invalid_proposal_error(
-                "Updating root canister settings is not supported yet.",
-            ));
-        }
         Ok(canister_id)
     }
 
@@ -107,18 +102,29 @@ impl UpdateCanisterSettings {
 
 impl CallCanister for UpdateCanisterSettings {
     fn canister_and_function(&self) -> Result<(CanisterId, &str), GovernanceError> {
-        Ok((ROOT_CANISTER_ID, "update_canister_settings"))
+        let canister_id = self.valid_canister_id()?;
+        if canister_id == ROOT_CANISTER_ID {
+            Ok((LIFELINE_CANISTER_ID, "update_root_settings"))
+        } else {
+            Ok((ROOT_CANISTER_ID, "update_canister_settings"))
+        }
     }
 
     fn payload(&self) -> Result<Vec<u8>, GovernanceError> {
-        let canister_id = self.valid_canister_id()?.get();
+        let canister_id = self.valid_canister_id()?;
         let settings = self.valid_canister_settings()?;
-        let update_settings = UpdateCanisterSettingsRequest {
-            canister_id,
-            settings,
-        };
-        Encode!(&update_settings)
-            .map_err(|err| invalid_proposal_error(&format!("Failed to encode payload: {err}")))
+
+        if canister_id == ROOT_CANISTER_ID {
+            Encode!(&settings)
+                .map_err(|err| invalid_proposal_error(&format!("Failed to encode payload: {err}")))
+        } else {
+            let update_settings = UpdateCanisterSettingsRequest {
+                canister_id: canister_id.get(),
+                settings,
+            };
+            Encode!(&update_settings)
+                .map_err(|err| invalid_proposal_error(&format!("Failed to encode payload: {err}")))
+        }
     }
 }
 
@@ -129,8 +135,13 @@ mod tests {
     use crate::pb::v1::governance_error::ErrorType;
     #[cfg(feature = "test")]
     use crate::pb::v1::update_canister_settings::{CanisterSettings, Controllers};
-
+    #[cfg(feature = "test")]
+    use candid::Decode;
+    #[cfg(feature = "test")]
+    use ic_base_types::CanisterId;
     use ic_nns_constants::LEDGER_CANISTER_ID;
+    #[cfg(feature = "test")]
+    use ic_nns_constants::{GOVERNANCE_CANISTER_ID, SNS_WASM_CANISTER_ID};
 
     #[cfg(not(feature = "test"))]
     #[test]
@@ -163,9 +174,9 @@ mod tests {
 
         let is_invalid_proposal_with_keywords =
             |update_canister_settings: UpdateCanisterSettings, keywords: Vec<&str>| {
-                let error = update_canister_settings
-                    .validate()
-                    .expect_err("Expecting validation error for {install_code:?} but got Ok(())");
+                let error = update_canister_settings.validate().expect_err(&format!(
+                    "Expecting validation error for {update_canister_settings:?} but got Ok(())"
+                ));
                 assert_eq!(error.error_type, ErrorType::InvalidProposal as i32);
                 for keyword in keywords {
                     let error_message = error.error_message.to_lowercase();
@@ -223,22 +234,11 @@ mod tests {
             },
             vec!["invalid log visibility", "0"],
         );
-
-        is_invalid_proposal_with_keywords(
-            UpdateCanisterSettings {
-                canister_id: Some(ROOT_CANISTER_ID.get()),
-                ..valid_update_canister_settings.clone()
-            },
-            vec!["root canister", "not supported"],
-        );
     }
 
     #[cfg(feature = "test")]
     #[test]
     fn test_update_ledger_canister_settings() {
-        use candid::Decode;
-        use ic_nns_constants::GOVERNANCE_CANISTER_ID;
-
         let update_ledger_canister_settings = UpdateCanisterSettings {
             canister_id: Some(LEDGER_CANISTER_ID.get()),
             // The value of the settings are arbitrary and do not have any meaning.
@@ -288,10 +288,54 @@ mod tests {
 
     #[cfg(feature = "test")]
     #[test]
-    fn test_update_canister_settings_topic_mapping() {
-        use ic_base_types::CanisterId;
-        use ic_nns_constants::SNS_WASM_CANISTER_ID;
+    fn test_update_root_canister_settings() {
+        let update_root_canister_settings = UpdateCanisterSettings {
+            canister_id: Some(ROOT_CANISTER_ID.get()),
+            // The value of the settings are arbitrary and do not have any meaning.
+            settings: Some(CanisterSettings {
+                controllers: Some(Controllers {
+                    controllers: vec![LIFELINE_CANISTER_ID.get()],
+                }),
+                memory_allocation: Some(1 << 32),
+                wasm_memory_limit: Some(1 << 31),
+                compute_allocation: Some(10),
+                freezing_threshold: Some(100),
+                log_visibility: Some(LogVisibility::Public as i32),
+            }),
+        };
 
+        assert_eq!(update_root_canister_settings.validate(), Ok(()));
+        assert_eq!(
+            update_root_canister_settings.valid_topic(),
+            Ok(Topic::ProtocolCanisterManagement)
+        );
+        assert_eq!(
+            update_root_canister_settings.canister_and_function(),
+            Ok((LIFELINE_CANISTER_ID, "update_root_settings"))
+        );
+
+        let decoded_payload = Decode!(
+            &update_root_canister_settings.payload().unwrap(),
+            RootCanisterSettings
+        )
+        .unwrap();
+        assert_eq!(
+            decoded_payload,
+            RootCanisterSettings {
+                controllers: Some(vec![LIFELINE_CANISTER_ID.get()]),
+                memory_allocation: Some(Nat::from(1u64 << 32)),
+                wasm_memory_limit: Some(Nat::from(1u64 << 31)),
+                compute_allocation: Some(Nat::from(10u64)),
+                freezing_threshold: Some(Nat::from(100u64)),
+                log_visibility: Some(RootLogVisibility::Public),
+                reserved_cycles_limit: None,
+            }
+        );
+    }
+
+    #[cfg(feature = "test")]
+    #[test]
+    fn test_update_canister_settings_topic_mapping() {
         let test_cases = vec![
             (LEDGER_CANISTER_ID, Topic::ProtocolCanisterManagement),
             (SNS_WASM_CANISTER_ID, Topic::ServiceNervousSystemManagement),

rs/nns/handlers/lifeline/impl/lifeline.did

@@ -1,17 +1,25 @@
-type CanisterStatusResult = record {
-  status : CanisterStatusType;
-  memory_size : nat;
-  cycles : nat;
-  settings : DefiniteCanisterSettings;
-  module_hash : opt vec nat8;
+type UpgradeRootProposalPayload = record {
+  module_arg: blob;
+  stop_upgrade_start: bool;
+  wasm_module: blob;
 };
-type CanisterStatusType = variant { stopped; stopping; running };
-type DefiniteCanisterSettings = record { controllers : vec principal };
+type HardResetRootToVersionPayload = record {
+  wasm_module: blob;
+  init_arg: blob;
+};
+type CanisterSettings = record {
+  controllers: opt vec principal;
+  compute_allocation: opt nat;
+  memory_allocation: opt nat;
+  freezing_threshold: opt nat;
+  reserved_cycles_limit: opt nat;
+  wasm_memory_limit: opt nat;
+  log_visibility: opt LogVisibility;
+};
+type LogVisibility = variant { controllers; public };
+
 service : {
-  upgrade_root:
-   (record {
-      module_arg: blob;
-      stop_upgrade_start: bool;
-      wasm_module: blob;
-    }) -> ();
+  upgrade_root: (UpgradeRootProposalPayload) -> ();
+  hard_reset_root_to_version: (HardResetRootToVersionPayload) -> ();
+  upgrade_root_settings: (CanisterSettings) -> ();
 }

rs/nns/handlers/lifeline/impl/lifeline.mo

@@ -8,8 +8,30 @@ actor {
     private let governanceCanister : Principal = Prim.principalOfActor Governance;
     private let root : Principal = Prim.principalOfActor Root;
 
-    type UpgradeRootProposalPayload = { wasm_module : Blob; module_arg : Blob; stop_upgrade_start : Bool };
-    type HardResetRootToVersionPayload = { wasm_module : Blob; init_arg : Blob; };
+    type UpgradeRootProposalPayload = {
+      wasm_module : Blob;
+      module_arg : Blob;
+      stop_upgrade_start : Bool;
+    };
+
+    type HardResetRootToVersionPayload = {
+      wasm_module : Blob;
+      init_arg : Blob;
+    };
+
+    type CanisterIdRecord = { canister_id : Principal };
+
+    type LogVisibility = {#controllers; #public_};
+
+    type CanisterSettings = {
+      controllers : ?[Principal];
+      compute_allocation: ?Nat;
+      memory_allocation: ?Nat;
+      freezing_threshold: ?Nat;
+      reserved_cycles_limit: ?Nat;
+      wasm_memory_limit: ?Nat;
+      log_visibility: ?LogVisibility;
+    };
 
     // IC00 is the management canister. We rely on it for the four
     // fundamental methods as listed below.
@@ -22,7 +44,11 @@ actor {
       } -> async ();
       start_canister : CanisterIdRecord -> async ();
       stop_canister : CanisterIdRecord -> async ();
-      uninstall_code : CanisterIdRecord -> async ()
+      uninstall_code : CanisterIdRecord -> async ();
+      update_settings: {
+        canister_id: Principal;
+        settings: CanisterSettings;
+      } -> async ();
     };
 
     public shared ({caller}) func upgrade_root(pl : UpgradeRootProposalPayload) : async () {
@@ -72,10 +98,16 @@ actor {
       debug { Prim.debugPrint "hard_reset_root: finished installing" };
     };
 
-    type CanisterIdRecord = { canister_id : Principal };
+    public shared ({caller}) func update_root_settings(settings: CanisterSettings) : async () {
+      assert caller == governanceCanister;
 
-    type DefiniteCanisterSettings = {
-        controllers : [Principal];
+      debug { Prim.debugPrint ("update_root_settings: about to update settings") };
+
+      await ic00.update_settings({
+        canister_id = root;
+        settings = settings;
+      });
 
+      debug { Prim.debugPrint ("update_root_settings: finished updating settings") };
     };
 }

rs/nns/integration_tests/src/update_canister_settings.rs

@@ -1,7 +1,7 @@
 use candid::Nat;
 use ic_base_types::{CanisterId, PrincipalId};
 use ic_nervous_system_clients::canister_status::{DefiniteCanisterSettings, LogVisibility};
-use ic_nns_constants::{REGISTRY_CANISTER_ID, ROOT_CANISTER_ID};
+use ic_nns_constants::{LIFELINE_CANISTER_ID, REGISTRY_CANISTER_ID, ROOT_CANISTER_ID};
 use ic_nns_governance_api::pb::v1::{
     manage_neuron_response::Command,
     proposal::Action,
@@ -19,16 +19,21 @@ use ic_nns_test_utils::{
     },
 };
 
-#[test]
-fn test_update_canister_settings() {
+fn test_update_canister_settings_proposal(
+    target_canister_id: CanisterId,
+    controller_canister_id: CanisterId,
+) {
     // Step 1: Set up the NNS canisters and get the neuron.
     let state_machine = state_machine_builder_for_nns_tests().build();
     let nns_init_payloads = NnsInitPayloadsBuilder::new().with_test_neurons().build();
     setup_nns_canisters(&state_machine, nns_init_payloads);
     let n1 = get_neuron_1();
 
     // Step 2: Define the target settings and make sure they are different from the current ones.
-    let target_controllers = vec![ROOT_CANISTER_ID.get(), PrincipalId::new_user_test_id(1)];
+    let target_controllers = vec![
+        controller_canister_id.get(),
+        PrincipalId::new_user_test_id(1),
+    ];
     let target_memory_allocation = 1u64 << 33;
     let target_compute_allocation = 10u64;
     let target_freezing_threshold = 100_000u64;
@@ -37,8 +42,8 @@ fn test_update_canister_settings() {
     let canister_settings = || -> DefiniteCanisterSettings {
         get_canister_status(
             &state_machine,
-            ROOT_CANISTER_ID.get(),
-            REGISTRY_CANISTER_ID,
+            controller_canister_id.get(),
+            target_canister_id,
             CanisterId::ic_00(),
         )
         .unwrap()
@@ -73,7 +78,7 @@ fn test_update_canister_settings() {
         &Proposal {
             title: Some("Update canister settings".to_string()),
             action: Some(Action::UpdateCanisterSettings(UpdateCanisterSettings {
-                canister_id: Some(REGISTRY_CANISTER_ID.get()),
+                canister_id: Some(target_canister_id.get()),
                 settings: Some(CanisterSettings {
                     controllers: Some(Controllers {
                         controllers: target_controllers.clone(),
@@ -115,3 +120,13 @@ fn test_update_canister_settings() {
     );
     assert_eq!(updated_settings.log_visibility, target_log_visibility);
 }
+
+#[test]
+fn test_update_canister_settings_proposal_non_root() {
+    test_update_canister_settings_proposal(REGISTRY_CANISTER_ID, ROOT_CANISTER_ID);
+}
+
+#[test]
+fn test_update_canister_settings_proposal_root() {
+    test_update_canister_settings_proposal(ROOT_CANISTER_ID, LIFELINE_CANISTER_ID);
+}