Merge branch 'abk/run-810-make-canisterid-new-infallable' into 'master'

RUN-810: Switch `CanisterId::new` to infallable `CanisterId::unchecked_from_principal`

`CanisterId::new` returns a `Result` type, but the value is always `Ok`
so it would be better to not return a `Result`. 

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

rs/canonical_state/src/encoding/old_types.rs

@@ -117,8 +117,12 @@ impl TryFrom<RequestV13> for ic_types::messages::Request {
         .try_into()?;
 
         Ok(Self {
-            receiver: ic_types::CanisterId::new(request.receiver.as_slice().try_into()?)?,
-            sender: ic_types::CanisterId::new(request.sender.as_slice().try_into()?)?,
+            receiver: ic_types::CanisterId::unchecked_from_principal(
+                request.receiver.as_slice().try_into()?,
+            ),
+            sender: ic_types::CanisterId::unchecked_from_principal(
+                request.sender.as_slice().try_into()?,
+            ),
             sender_reply_callback: request.sender_reply_callback.into(),
             payment,
             method_name: request.method_name,
@@ -167,8 +171,12 @@ impl TryFrom<RequestV3> for ic_types::messages::Request {
 
     fn try_from(request: RequestV3) -> Result<Self, Self::Error> {
         Ok(Self {
-            receiver: ic_types::CanisterId::new(request.receiver.as_slice().try_into()?)?,
-            sender: ic_types::CanisterId::new(request.sender.as_slice().try_into()?)?,
+            receiver: ic_types::CanisterId::unchecked_from_principal(
+                request.receiver.as_slice().try_into()?,
+            ),
+            sender: ic_types::CanisterId::unchecked_from_principal(
+                request.sender.as_slice().try_into()?,
+            ),
             sender_reply_callback: request.sender_reply_callback.into(),
             payment: request.payment.cycles.try_into()?,
             method_name: request.method_name,
@@ -214,8 +222,12 @@ impl TryFrom<ResponseV3> for ic_types::messages::Response {
 
     fn try_from(response: ResponseV3) -> Result<Self, Self::Error> {
         Ok(Self {
-            originator: ic_types::CanisterId::new(response.originator.as_slice().try_into()?)?,
-            respondent: ic_types::CanisterId::new(response.respondent.as_slice().try_into()?)?,
+            originator: ic_types::CanisterId::unchecked_from_principal(
+                response.originator.as_slice().try_into()?,
+            ),
+            respondent: ic_types::CanisterId::unchecked_from_principal(
+                response.respondent.as_slice().try_into()?,
+            ),
             originator_reply_callback: response.originator_reply_callback.into(),
             refund: response.refund.cycles.try_into()?,
             response_payload: response.response_payload.try_into()?,

rs/canonical_state/src/encoding/types.rs

@@ -321,8 +321,12 @@ impl TryFrom<Request> for ic_types::messages::Request {
         .try_into()?;
 
         Ok(Self {
-            receiver: ic_types::CanisterId::new(request.receiver.as_slice().try_into()?)?,
-            sender: ic_types::CanisterId::new(request.sender.as_slice().try_into()?)?,
+            receiver: ic_types::CanisterId::unchecked_from_principal(
+                request.receiver.as_slice().try_into()?,
+            ),
+            sender: ic_types::CanisterId::unchecked_from_principal(
+                request.sender.as_slice().try_into()?,
+            ),
             sender_reply_callback: request.sender_reply_callback.into(),
             payment,
             method_name: request.method_name,
@@ -362,8 +366,12 @@ impl TryFrom<Response> for ic_types::messages::Response {
         .try_into()?;
 
         Ok(Self {
-            originator: ic_types::CanisterId::new(response.originator.as_slice().try_into()?)?,
-            respondent: ic_types::CanisterId::new(response.respondent.as_slice().try_into()?)?,
+            originator: ic_types::CanisterId::unchecked_from_principal(
+                response.originator.as_slice().try_into()?,
+            ),
+            respondent: ic_types::CanisterId::unchecked_from_principal(
+                response.respondent.as_slice().try_into()?,
+            ),
             originator_reply_callback: response.originator_reply_callback.into(),
             refund,
             response_payload: response.response_payload.try_into()?,

rs/canonical_state/src/lazy_tree_conversion.rs

@@ -164,7 +164,7 @@ impl LabelLike for CanisterId {
     }
 
     fn from_label(label: &[u8]) -> Option<Self> {
-        PrincipalId::from_label(label).map(|principal| Self::new(principal).unwrap())
+        PrincipalId::from_label(label).map(|principal| Self::unchecked_from_principal(principal))
     }
 }
 

rs/drun/src/message.rs

@@ -195,13 +195,7 @@ fn parse_message(s: &str, nonce: u64) -> Result<Message, String> {
 fn parse_canister_id(canister_id: &str) -> Result<CanisterId, String> {
     use std::str::FromStr;
     match PrincipalId::from_str(canister_id) {
-        Ok(id) => match CanisterId::new(id) {
-            Ok(id) => Ok(id),
-            Err(err) => Err(format!(
-                "Failed to convert {} to canister id with {}",
-                canister_id, err
-            )),
-        },
+        Ok(id) => Ok(CanisterId::unchecked_from_principal(id)),
         Err(err) => Err(format!(
             "Failed to convert {} to principal id with {}",
             canister_id, err

rs/execution_environment/src/bitcoin.rs

@@ -120,8 +120,9 @@ mod tests {
 
     #[test]
     fn clears_state_of_former_bitcoin_canisters() {
-        let bitcoin_canister_id =
-            CanisterId::new(PrincipalId::from_str("rwlgt-iiaaa-aaaaa-aaaaa-cai").unwrap()).unwrap();
+        let bitcoin_canister_id = CanisterId::unchecked_from_principal(
+            PrincipalId::from_str("rwlgt-iiaaa-aaaaa-aaaaa-cai").unwrap(),
+        );
 
         let mut test = ExecutionTestBuilder::new()
             // Set the bitcoin canister to be the ID of the canister about to be created.

rs/execution_environment/src/canister_manager.rs

@@ -265,12 +265,7 @@ impl TryFrom<(CanisterChangeOrigin, InstallCodeArgsV2)> for InstallCodeContext {
 
     fn try_from(input: (CanisterChangeOrigin, InstallCodeArgsV2)) -> Result<Self, Self::Error> {
         let (origin, args) = input;
-        let canister_id = CanisterId::new(args.canister_id).map_err(|err| {
-            InstallCodeContextError::InvalidCanisterId(format!(
-                "Converting canister id {} failed with {}",
-                args.canister_id, err
-            ))
-        })?;
+        let canister_id = CanisterId::unchecked_from_principal(args.canister_id);
         let compute_allocation = match args.compute_allocation {
             Some(ca) => Some(ComputeAllocation::try_from(ca.0.to_u64().ok_or_else(
                 || {
@@ -1247,7 +1242,7 @@ impl CanisterManager {
         state: &mut ReplicatedState,
         specified_id: PrincipalId,
     ) -> Result<CanisterId, CanisterManagerError> {
-        let new_canister_id = CanisterId::new(specified_id).unwrap();
+        let new_canister_id = CanisterId::unchecked_from_principal(specified_id);
 
         if state.canister_states.get(&new_canister_id).is_some() {
             return Err(CanisterManagerError::CanisterAlreadyExists(new_canister_id));

rs/execution_environment/src/execution_environment.rs

@@ -1955,12 +1955,6 @@ impl ExecutionEnvironment {
         // TODO EXC-1060: get the right public key.
         _key_id: &EcdsaKeyId,
     ) -> Result<ECDSAPublicKeyResponse, UserError> {
-        let _ = CanisterId::new(principal_id).map_err(|err| {
-            UserError::new(
-                ErrorCode::CanisterContractViolation,
-                format!("Not a canister id: {}", err),
-            )
-        })?;
         let path = ExtendedDerivationPath {
             caller: principal_id,
             derivation_path,

rs/http_endpoints/public/src/call.rs

@@ -179,16 +179,7 @@ impl Service<Request<Bytes>> for CallService {
             }
         };
 
-        let effective_canister_id = match CanisterId::new(effective_principal_id) {
-            Ok(canister_id) => canister_id,
-            Err(_) => {
-                let res = make_plaintext_response(
-                    StatusCode::BAD_REQUEST,
-                    format!("Invalid canister id: {}", effective_principal_id),
-                );
-                return Box::pin(async move { Ok(res) });
-            }
-        };
+        let effective_canister_id = CanisterId::unchecked_from_principal(effective_principal_id);
 
         // Reject requests where `canister_id` != `effective_canister_id` for non mgmt canister calls.
         // This needs to be enforced because boundary nodes block access based on the `effective_canister_id`

rs/http_endpoints/public/src/query.rs

@@ -146,16 +146,7 @@ impl Service<Request<Bytes>> for QueryService {
             }
         };
 
-        let effective_canister_id = match CanisterId::new(effective_principal_id) {
-            Ok(canister_id) => canister_id,
-            Err(_) => {
-                let res = make_plaintext_response(
-                    StatusCode::BAD_REQUEST,
-                    format!("Invalid canister id: {}", effective_principal_id),
-                );
-                return Box::pin(async move { Ok(res) });
-            }
-        };
+        let effective_canister_id = CanisterId::unchecked_from_principal(effective_principal_id);
 
         // Reject requests where `canister_id` != `effective_canister_id`. In comparison to update
         // requests we don't need to check for the mgmt canister since all mgmt canister calls are updated calls.

rs/http_endpoints/public/src/read_state/canister.rs

@@ -260,10 +260,7 @@ fn verify_paths(
                 verify_principal_ids(&principal_id, &effective_principal_id)?;
                 can_read_canister_metadata(
                     user,
-                    &CanisterId::new(principal_id).map_err(|_| HttpError {
-                        status: StatusCode::BAD_REQUEST,
-                        message: format!("Invalid canister id {}.", principal_id),
-                    })?,
+                    &CanisterId::unchecked_from_principal(principal_id),
                     &name,
                     state,
                 )?

rs/ingress_manager/benches/build_payload.rs

@@ -199,7 +199,9 @@ fn build_payload(criterion: &mut Criterion) {
         // canister ids iterator
         let mut rng = rand::thread_rng();
         let canisters: Vec<CanisterId> = (0..(size / 10))
-            .map(|_| CanisterId::new(PrincipalId::new_user_test_id(rng.next_u64())).unwrap())
+            .map(|_| {
+                CanisterId::unchecked_from_principal(PrincipalId::new_user_test_id(rng.next_u64()))
+            })
             .collect();
 
         run_test(

rs/nns/cmc/src/main.rs

@@ -329,7 +329,7 @@ fn set_authorized_subnetwork_list(who: Option<PrincipalId>, subnets: Vec<SubnetI
     with_state_mut(|state| {
         let governance_canister_id = state.governance_canister_id;
 
-        if CanisterId::new(caller()) != Ok(governance_canister_id) {
+        if CanisterId::unchecked_from_principal(caller()) != governance_canister_id {
             panic!("Only the governance canister can set authorized subnetwork lists.");
         }
 
@@ -384,7 +384,7 @@ fn update_subnet_type_() {
 fn update_subnet_type(args: UpdateSubnetTypeArgs) -> UpdateSubnetTypeResult {
     let governance_canister_id = with_state(|state| state.governance_canister_id);
 
-    if CanisterId::new(caller()) != Ok(governance_canister_id) {
+    if CanisterId::unchecked_from_principal(caller()) != governance_canister_id {
         panic!("Only the governance canister can update the available subnet types.");
     }
 
@@ -456,7 +456,7 @@ fn change_subnet_type_assignment(
 ) -> ChangeSubnetTypeAssignmentResult {
     let governance_canister_id = with_state(|state| state.governance_canister_id);
 
-    if CanisterId::new(caller()) != Ok(governance_canister_id) {
+    if CanisterId::unchecked_from_principal(caller()) != governance_canister_id {
         panic!(
             "Only the governance canister can change the assignment of subnets to subnet types."
         );
@@ -1316,7 +1316,7 @@ async fn transaction_notification(tn: TransactionNotification) -> Result<CyclesR
 
     let ledger_canister_id = with_state(|state| state.ledger_canister_id);
 
-    if CanisterId::new(caller) != Ok(ledger_canister_id) {
+    if CanisterId::unchecked_from_principal(caller) != ledger_canister_id {
         return Err(format!(
             "This canister can only be notified by the ledger canister ({}), not by {}.",
             ledger_canister_id, caller
@@ -2047,10 +2047,9 @@ mod tests {
         }
         blocks_notified.insert(
             60,
-            NotificationStatus::NotifiedCreateCanister(Ok(CanisterId::new(
+            NotificationStatus::NotifiedCreateCanister(Ok(CanisterId::unchecked_from_principal(
                 PrincipalId::new_user_test_id(4),
-            )
-            .unwrap())),
+            ))),
         );
         state.blocks_notified = Some(blocks_notified);
 

rs/nns/handlers/root/impl/src/lib.rs

@@ -134,8 +134,7 @@ fn principal_name(principal_id: PrincipalId) -> String {
         };
     }
 
-    CanisterId::new(principal_id)
-        .ok()
+    Some(CanisterId::unchecked_from_principal(principal_id))
         .and_then(|canister_id| CANISTER_ID_TO_NAME.get(&canister_id))
         .map(|name| format!("{}_canister", name))
         .unwrap_or_else(|| principal_id.to_string())

rs/nns/sns-wasm/src/pb/mod.rs

@@ -133,24 +133,24 @@ impl Display for SnsVersion {
 impl SnsCanisterIds {
     /// Get Root CanisterId
     pub fn root(&self) -> CanisterId {
-        CanisterId::new(self.root.unwrap()).unwrap()
+        CanisterId::unchecked_from_principal(self.root.unwrap())
     }
     /// Get Governance CanisterId
     pub fn governance(&self) -> CanisterId {
-        CanisterId::new(self.governance.unwrap()).unwrap()
+        CanisterId::unchecked_from_principal(self.governance.unwrap())
     }
     /// Get Ledger CanisterId
     pub fn ledger(&self) -> CanisterId {
-        CanisterId::new(self.ledger.unwrap()).unwrap()
+        CanisterId::unchecked_from_principal(self.ledger.unwrap())
     }
     /// Get Swap CanisterId
     pub fn swap(&self) -> CanisterId {
-        CanisterId::new(self.swap.unwrap()).unwrap()
+        CanisterId::unchecked_from_principal(self.swap.unwrap())
     }
 
     /// Get Index CanisterId
     pub fn index(&self) -> CanisterId {
-        CanisterId::new(self.index.unwrap()).unwrap()
+        CanisterId::unchecked_from_principal(self.index.unwrap())
     }
 }
 
@@ -312,7 +312,8 @@ impl SnsCanisterIds {
         ]
         .into_iter()
         .flat_map(|(label, principal_id)| {
-            principal_id.map(|principal_id| (label, CanisterId::new(principal_id).unwrap()))
+            principal_id
+                .map(|principal_id| (label, CanisterId::unchecked_from_principal(principal_id)))
         })
         .collect()
     }