Merge branch '@anchpop/nns-owns-dapp-canisters-for-longer' into 'master'

feat(sns): NNS1-3116: NNS root co-controls dapp canisters until the swap is finished

# How to read this MR

The first commit modifies NNS root to not ever hand sole control of the dapp canisters over to SNS root. Instead, NNS root adds SNS Root as a controller but also leaves itself as a co-controller.

The second commit modifies SNS Swap to make SNS root the sole controller after a successful swap. 

# Description

!19430 will disallow dangerous proposals from being submitted until the swap is done.

One of these dangerous proposals is upgrading SNS-controlled canisters. But there may be an issue where an SNS-controlled canister requires an emergency upgrade during a swap. If NNS root is a co-controller, the canister will still be upgradable through an NNS proposal during an emergency. NNS root is removed as a co-controller after the swap is done, leaving only SNS root.

Once this MR is merged, NNS root should not be upgraded without also upgrading SNS Swap, although nothing that terrible would happen if it were. (The consequence would be that NNS Root would remain a co-controller after the swap was done, as the new version of Swap is responsible for removing it. This would be a recoverable issue though, as all we would have to do is upgrade Swap via an NNS proposal and then call finalize.)

Closes NNS1-3116 

Closes NNS1-3116

See merge request dfinity-lab/public/ic!19527

rs/nervous_system/integration_tests/src/pocket_ic_helpers.rs

@@ -2337,7 +2337,7 @@ pub mod sns {
         ///        or `auto_finalize_swap_response`.
         ///     2. `auto_finalize_swap_response` does not match the expected pattern for a *committed* SNS
         ///        Swap's `auto_finalize_swap_response`. In particular:
-        ///        - `set_dapp_controllers_call_result` must be `None`,
+        ///        - After NNS1-3117 `set_dapp_controllers_call_result` must be `Some` and not have any errors, but for now it can take any value
         ///        - `sweep_sns_result` must be `Some`.
         /// * `Err` if `auto_finalize_swap_response` contains any errors.
         pub fn is_auto_finalization_status_committed_or_err(
@@ -2359,7 +2359,8 @@ pub mod sns {
                     sweep_sns_result: Some(_),
                     claim_neuron_result: Some(_),
                     set_mode_call_result: Some(_),
-                    set_dapp_controllers_call_result: None,
+                    // TODO(NNS1-3117): set_dapp_controllers_call_result should be required to be Some and not have any errors
+                    set_dapp_controllers_call_result: _,
                     settle_community_fund_participation_result: None,
                     error_message: None,
                 }
@@ -2478,8 +2479,7 @@ pub mod sns {
                 last_auto_finalization_status = Some(auto_finalization_status);
             }
             Err(format!(
-                "Looks like the expected SNS auto-finalization status is never going to be reached: {:#?}",
-                last_auto_finalization_status,
+                "Looks like the expected SNS auto-finalization status of {status:?} is never going to be reached: {last_auto_finalization_status:#?}",
             ))
         }
 

rs/nervous_system/integration_tests/tests/sns_lifecycle.rs

@@ -848,18 +848,13 @@ fn test_sns_lifecycle(
             })
         };
 
-        let expected_set_dapp_controllers_call_result =
-            if swap_finalization_status == SwapFinalizationStatus::Aborted {
-                Some(SetDappControllersCallResult {
-                    possibility: Some(set_dapp_controllers_call_result::Possibility::Ok(
-                        SetDappControllersResponse {
-                            failed_updates: vec![],
-                        },
-                    )),
-                })
-            } else {
-                None
-            };
+        let expected_set_dapp_controllers_call_result = Some(SetDappControllersCallResult {
+            possibility: Some(set_dapp_controllers_call_result::Possibility::Ok(
+                SetDappControllersResponse {
+                    failed_updates: vec![],
+                },
+            )),
+        });
 
         assert_eq!(
             sns::swap::finalize_swap(&pocket_ic, swap_canister_id),

rs/nns/handlers/root/interface/src/client.rs

@@ -153,6 +153,7 @@ impl SpyNnsRootCanisterClient {
         self.observed_calls.lock().unwrap().clone().into()
     }
 
+    #[track_caller]
     pub fn assert_all_replies_consumed(&self) {
         assert_eq!(
             self.replies.lock().unwrap().clone(),

rs/nns/sns-wasm/src/sns_wasm.rs

@@ -874,12 +874,17 @@ where
                 non_controlled_dapp_canisters: vec![],
             })
         })?;
-
-        // Request that NNS Root make the newly created SNS Root canister the sole controller
-        // of the dapp_canisters, removing itself.
+        // Request that NNS Root add the newly created SNS Root canister as a controller
+        // of the dapp_canisters.
+        // (NNS Root will be removed as a controller after the swap is done)
         let dapp_canister_to_new_controllers: BTreeMap<Canister, Vec<PrincipalId>> = dapp_canisters
             .iter()
-            .map(|canister| (*canister, vec![sns_init_canister_ids.root]))
+            .map(|canister| {
+                (
+                    *canister,
+                    vec![sns_init_canister_ids.root, ROOT_CANISTER_ID.get()],
+                )
+            })
             .collect();
         Self::change_controllers_of_nns_root_owned_canisters(
             nns_root_canister_client,
@@ -4019,6 +4024,7 @@ mod test {
         );
     }
 
+    #[track_caller]
     fn assert_nns_root_calls(
         nns_root_canister_client: &SpyNnsRootCanisterClient,
         expected_change_canister_controllers_calls: Vec<(PrincipalId, Vec<PrincipalId>)>,
@@ -4484,9 +4490,9 @@ mod test {
                 (dapp_ids[0], vec![ROOT_CANISTER_ID.get()]),
                 (dapp_ids[1], vec![ROOT_CANISTER_ID.get()]),
                 (dapp_ids[2], vec![ROOT_CANISTER_ID.get()]),
-                (dapp_ids[0], vec![root_id.get()]),
-                (dapp_ids[1], vec![root_id.get()]),
-                (dapp_ids[2], vec![root_id.get()]),
+                (dapp_ids[0], vec![root_id.get(), ROOT_CANISTER_ID.get()]),
+                (dapp_ids[1], vec![root_id.get(), ROOT_CANISTER_ID.get()]),
+                (dapp_ids[2], vec![root_id.get(), ROOT_CANISTER_ID.get()]),
                 (
                     dapp_ids[1],
                     vec![original_dapp_controller, ROOT_CANISTER_ID.get()],
@@ -4865,9 +4871,9 @@ mod test {
                 (dapp_ids[0], vec![ROOT_CANISTER_ID.get()]),
                 (dapp_ids[1], vec![ROOT_CANISTER_ID.get()]),
                 (dapp_ids[2], vec![ROOT_CANISTER_ID.get()]),
-                (dapp_ids[0], vec![root_id.get()]),
-                (dapp_ids[1], vec![root_id.get()]),
-                (dapp_ids[2], vec![root_id.get()]),
+                (dapp_ids[0], vec![root_id.get(), ROOT_CANISTER_ID.get()]),
+                (dapp_ids[1], vec![root_id.get(), ROOT_CANISTER_ID.get()]),
+                (dapp_ids[2], vec![root_id.get(), ROOT_CANISTER_ID.get()]),
                 (dapp_ids[1], vec![original_dapp_controller, ROOT_CANISTER_ID.get()]),
                 (dapp_ids[2], vec![original_dapp_controller, ROOT_CANISTER_ID.get()]),
             ],

rs/sns/cli/src/propose.rs

@@ -176,26 +176,34 @@ pub fn exec(args: ProposeArgs) {
 }
 
 fn confirmation_messages(proposal: &Proposal) -> Vec<String> {
-    let canisters = match &proposal.action {
-        Some(Action::CreateServiceNervousSystem(csns)) => csns
-            .dapp_canisters
-            .iter()
-            .filter_map(|canister| canister.id.as_ref())
-            .map(|id| format!("  - {}", id))
-            .join("\n"),
+    let csns = match &proposal.action {
+        Some(Action::CreateServiceNervousSystem(csns)) => csns,
         _ => {
             eprintln!(
                 "Internal error: Somehow a proposal was made not of type CreateServiceNervousSystem",
             );
             std::process::exit(1);
         }
     };
+    let canisters = csns
+        .dapp_canisters
+        .iter()
+        .filter_map(|canister| canister.id.as_ref())
+        .map(|id| format!("  - {}", id))
+        .join("\n");
+    let fallback_controllers = csns
+        .fallback_controller_principal_ids
+        .iter()
+        .map(|id| format!("  - {}", id))
+        .join("\n");
     let dapp_canister_controllers = format!(
         r#"A CreateServiceNervousSystem proposal will be submitted.
 If adopted, this proposal will create an SNS, which will control these canisters:
 {canisters}
-These canisters must have NNS root as a co-controller when the proposal is adopted, otherwise the SNS launch will be aborted.
-Otherwise, when the proposal is adopted, the SNS will be created and the SNS will take sole control over those canisters."#
+If these canisters do not have NNS root as a co-controller when the proposal is adopted, the SNS launch will be aborted.
+Otherwise, when the proposal is adopted, the SNS will be created and the SNS and NNS will have sole control over those canisters.
+Then, if the swap completes successfully, the SNS will take sole control. If the swap fails, control will be given to the fallback controllers:
+{fallback_controllers}"#
     );
 
     let disallowed_types = Mode::proposal_types_disallowed_in_pre_initialization_swap()
@@ -506,8 +514,10 @@ mod test {
 If adopted, this proposal will create an SNS, which will control these canisters:
   - c2n4r-wni5m-dqaaa-aaaap-4ai
   - ucm27-3lxwy-faaaa-aaaap-4ai
-These canisters must have NNS root as a co-controller when the proposal is adopted, otherwise the SNS launch will be aborted.
-Otherwise, when the proposal is adopted, the SNS will be created and the SNS will take sole control over those canisters."#,
+If these canisters do not have NNS root as a co-controller when the proposal is adopted, the SNS launch will be aborted.
+Otherwise, when the proposal is adopted, the SNS will be created and the SNS and NNS will have sole control over those canisters.
+Then, if the swap completes successfully, the SNS will take sole control. If the swap fails, control will be given to the fallback controllers:
+  - 5zxxw-63ouu-faaaa-aaaap-4ai"#,
             r#"After the proposal is adopted, a swap is started. While the swap is running, the SNS will be in a restricted mode.
 Within this restricted mode, some proposal actions will not be allowed:
   - Manage nervous system parameters

rs/sns/integration_tests/src/initialization_flow.rs

@@ -46,6 +46,7 @@ use ic_state_machine_tests::StateMachine;
 use icp_ledger::DEFAULT_TRANSFER_FEE;
 use lazy_static::lazy_static;
 use maplit::hashmap;
+use std::collections::BTreeSet;
 use std::{collections::HashMap, time::UNIX_EPOCH};
 
 // Valid images to be used in the CreateServiceNervousSystem proposal.
@@ -389,8 +390,13 @@ fn test_one_proposal_sns_initialization_success_with_neurons_fund_participation(
                 .as_ref()
                 .unwrap()
                 .settings
-                .controllers,
-            vec![test_sns.root_canister_id.unwrap()]
+                .controllers
+                .clone()
+                .into_iter()
+                .collect::<BTreeSet<_>>(),
+            vec![test_sns.root_canister_id.unwrap(), ROOT_CANISTER_ID.get()]
+                .into_iter()
+                .collect::<BTreeSet<_>>()
         );
     }
 
@@ -695,8 +701,13 @@ fn test_one_proposal_sns_initialization_success_without_neurons_fund_participati
                 .as_ref()
                 .unwrap()
                 .settings
-                .controllers,
-            vec![test_sns.root_canister_id.unwrap()]
+                .controllers
+                .clone()
+                .into_iter()
+                .collect::<BTreeSet<_>>(),
+            vec![test_sns.root_canister_id.unwrap(), ROOT_CANISTER_ID.get()]
+                .into_iter()
+                .collect::<BTreeSet<_>>()
         );
     }
 
@@ -1007,8 +1018,13 @@ fn test_one_proposal_sns_initialization_failed_swap_returns_neurons_fund_and_dap
                 .as_ref()
                 .unwrap()
                 .settings
-                .controllers,
-            vec![test_sns.root_canister_id.unwrap()]
+                .controllers
+                .clone()
+                .into_iter()
+                .collect::<BTreeSet<_>>(),
+            vec![test_sns.root_canister_id.unwrap(), ROOT_CANISTER_ID.get()]
+                .into_iter()
+                .collect::<BTreeSet<_>>(),
         );
     }
 

rs/sns/swap/src/swap.rs

@@ -1345,6 +1345,21 @@ impl Swap {
             .await)
     }
 
+    /// Calls SNS Root's set_dapp_controllers with SNS Root's principal id,
+    /// giving SNS Root sole control.
+    pub async fn take_sole_control_of_dapp_controllers(
+        &self,
+        sns_root_client: &mut impl SnsRootClient,
+    ) -> Result<Result<SetDappControllersResponse, CanisterCallError>, String> {
+        let sns_root_principal_id = self.init()?.sns_root()?.get();
+        Ok(sns_root_client
+            .set_dapp_controllers(SetDappControllersRequest {
+                canister_ids: None,
+                controller_principal_ids: vec![sns_root_principal_id],
+            })
+            .await)
+    }
+
     /// Calls restore_dapp_controllers() and handles errors for finalize
     async fn restore_dapp_controllers_for_finalize(
         &self,
@@ -1361,6 +1376,24 @@ impl Swap {
         }
     }
 
+    /// Calls take_sole_control_of_dapp_controllers() and handles errors for finalize
+    async fn take_sole_control_of_dapp_controllers_for_finalize(
+        &self,
+        sns_root_client: &mut impl SnsRootClient,
+    ) -> SetDappControllersCallResult {
+        let result = self
+            .take_sole_control_of_dapp_controllers(sns_root_client)
+            .await;
+
+        match result {
+            Ok(result) => result.into(),
+            Err(err_message) => {
+                log!(ERROR, "Halting set_dapp_controllers(), {:?}", err_message);
+                SetDappControllersCallResult { possibility: None }
+            }
+        }
+    }
+
     /// Acquires the lock on `finalize_swap`.
     pub fn lock_finalize_swap(&mut self) -> Result<(), String> {
         match self.is_finalize_swap_locked() {
@@ -1532,6 +1565,15 @@ impl Swap {
             Self::set_sns_governance_to_normal_mode(environment.sns_governance_mut()).await,
         );
 
+        // The following step is non-critical, so we'll do it after we set
+        // governance to normal mode, but only if there were no errors.
+        if !finalize_swap_response.has_error_message() {
+            finalize_swap_response.set_set_dapp_controllers_result(
+                self.take_sole_control_of_dapp_controllers_for_finalize(environment.sns_root_mut())
+                    .await,
+            );
+        }
+
         finalize_swap_response
     }
 

rs/sns/swap/src/swap_builder.rs

@@ -105,6 +105,11 @@ impl SwapBuilder {
         self
     }
 
+    pub fn with_sns_root_canister_id(mut self, sns_root_canister_id: CanisterId) -> Self {
+        self.sns_root_canister_id = sns_root_canister_id;
+        self
+    }
+
     pub fn with_lifecycle(mut self, lifecycle: Lifecycle) -> Self {
         self.lifecycle = lifecycle;
         self

rs/sns/swap/tests/common/doubles.rs

@@ -1,5 +1,5 @@
 use async_trait::async_trait;
-use ic_base_types::CanisterId;
+use ic_base_types::{CanisterId, PrincipalId};
 use ic_ledger_core::Tokens;
 use ic_nervous_system_common::{ledger::ICRC1Ledger, NervousSystemError};
 use ic_nervous_system_common_test_utils::SpyLedger;
@@ -12,8 +12,9 @@ use ic_sns_swap::{
     clients::{NnsGovernanceClient, SnsGovernanceClient, SnsRootClient},
     environment::CanisterClients,
     pb::v1::{
-        CanisterCallError, SetDappControllersRequest, SetDappControllersResponse,
-        SettleNeuronsFundParticipationRequest, SettleNeuronsFundParticipationResponse,
+        set_dapp_controllers_request::CanisterIds, CanisterCallError, SetDappControllersRequest,
+        SetDappControllersResponse, SettleNeuronsFundParticipationRequest,
+        SettleNeuronsFundParticipationResponse,
     },
 };
 use icrc_ledger_types::icrc1::account::{Account, Subaccount};
@@ -42,6 +43,21 @@ pub enum SnsRootClientCall {
     SetDappControllers(SetDappControllersRequest),
 }
 
+impl SnsRootClientCall {
+    pub fn set_dapp_controllers(
+        canisters: Option<Vec<CanisterId>>,
+        controllers: Vec<PrincipalId>,
+    ) -> Self {
+        let request = SetDappControllersRequest {
+            canister_ids: canisters.map(|canisters| CanisterIds {
+                canister_ids: canisters.into_iter().map(|x| x.get()).collect(),
+            }),
+            controller_principal_ids: controllers,
+        };
+        SnsRootClientCall::SetDappControllers(request)
+    }
+}
+
 #[allow(clippy::large_enum_variant)]
 #[derive(Debug, PartialEq)]
 pub enum SnsRootClientReply {