fix: set correct controller if no controller is specified in canister…

… settings (#85)
dfinity · Dec 6, 2023 · 6d535ff · 6d535ff
1 parent a4292c5
commit 6d535ff
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 18 deletions.
diff --git a/cycles-ledger/src/storage.rs b/cycles-ledger/src/storage.rs
@@ -1579,13 +1579,25 @@ pub async fn create_canister(
         let _ = reimburse();
     });
 
-    let argument = argument.unwrap_or_else(|| CmcCreateCanisterArgs {
-        settings: Some(CanisterSettings {
-            controllers: Some(vec![ic_cdk::api::caller()]),
-            ..Default::default()
-        }),
-        subnet_selection: None,
-    });
+    let argument = argument
+        .map(|arg| CmcCreateCanisterArgs {
+            settings: arg.settings.map(|settings| CanisterSettings {
+                controllers: Some(
+                    settings
+                        .controllers
+                        .unwrap_or_else(|| vec![ic_cdk::api::caller()]),
+                ),
+                ..settings
+            }),
+            ..arg
+        })
+        .unwrap_or_else(|| CmcCreateCanisterArgs {
+            settings: Some(CanisterSettings {
+                controllers: Some(vec![ic_cdk::api::caller()]),
+                ..Default::default()
+            }),
+            subnet_selection: None,
+        });
     let create_canister_result: Result<
         (Result<Principal, CmcCreateCanisterError>,),
         (RejectionCode, String),

diff --git a/cycles-ledger/tests/tests.rs b/cycles-ledger/tests/tests.rs
@@ -1791,6 +1791,34 @@ fn test_create_canister() {
         }
     );
 
+    // If `CanisterSettings` do not specify a controller, the caller should still control the resulting canister
+    let canister_settings = CanisterSettings {
+        controllers: None,
+        compute_allocation: Some(Nat::from(7)),
+        memory_allocation: Some(Nat::from(8)),
+        freezing_threshold: Some(Nat::from(9)),
+    };
+    let CreateCanisterSuccess { canister_id, .. } = create_canister(
+        &env,
+        ledger_id,
+        user,
+        CreateCanisterArgs {
+            from_subaccount: user.subaccount,
+            created_at_time: None,
+            amount: CREATE_CANISTER_CYCLES.into(),
+            creation_args: Some(CmcCreateCanisterArgs {
+                subnet_selection: None,
+                settings: Some(canister_settings),
+            }),
+        },
+    )
+    .unwrap();
+    expected_balance -= CREATE_CANISTER_CYCLES + FEE;
+    let status = canister_status(&env, canister_id, user.owner);
+    assert_eq!(expected_balance, balance_of(&env, ledger_id, user));
+    assert_eq!(CREATE_CANISTER_CYCLES, status.cycles);
+    assert_eq!(status.settings.controllers, vec![user.owner]);
+
     // reject before `await`
     if let CreateCanisterError::InsufficientFunds { balance } = create_canister(
         &env,

diff --git a/fake-cmc/src/main.rs b/fake-cmc/src/main.rs
@@ -1,4 +1,5 @@
 use candid::{candid_method, Principal};
+use core::panic;
 use cycles_ledger::endpoints::{CmcCreateCanisterArgs, CmcCreateCanisterError};
 use fake_cmc::State;
 use ic_cdk::{
@@ -45,18 +46,26 @@ async fn create_canister(arg: CmcCreateCanisterArgs) -> Result<Principal, CmcCre
         };
         return Err(error);
     };
-
     ic_cdk::api::call::msg_cycles_accept128(cycles);
-    match ic_cdk::api::management_canister::main::create_canister(
-        CreateCanisterArgument {
-            settings: arg.settings,
-        },
-        cycles,
-    )
-    .await
-    {
-        Ok((record,)) => Ok(record.canister_id),
-        Err(error) => panic!("create_canister failed: {:?}", error),
+
+    // "Canister <id> is already installed" happens because the canister id counter doesn't take into account that a canister with that id
+    // was already created using `provisional_create_canister_with_id`. Simply loop to try the next canister id.
+    loop {
+        match ic_cdk::api::management_canister::main::create_canister(
+            CreateCanisterArgument {
+                settings: arg.settings.clone(),
+            },
+            cycles,
+        )
+        .await
+        {
+            Ok((record,)) => return Ok(record.canister_id),
+            Err(error) => {
+                if !error.1.contains("is already installed") {
+                    panic!("create_canister failed: {:?}", error)
+                }
+            }
+        }
     }
 }