refactor: Use Principal in RemoveNodeOperatorsPayload, instead of Vec…

…<u8> (#3386)

The objective in this PR is to make the ic-admin (and potentially other)
output prettier by showing the list of operators in
`RemoveNodeOperatorsPayload` as principals (strings) instead of Vec[u8].
In result:

before:
```bash
ic-admin propose-to-remove-node-operators --nns-urls https://ic0.app xla4b-4vmw4-db4cm-qg63h-6jvj6-zm2nj-von5y-7dx2k-calku-e6hke-wae --dry-run --summary "Remove obsolete node operator"
[...]
Title: Remove node operators with principal ids: ["xla4b"]

Summary: Remove obsolete node operator

Payload: RemoveNodeOperatorsPayload {
    node_operators_to_remove: [
        [
            172,
            183,
            6,
            30,
            9,
            144,
            55,
            182,
            127,
            38,
            169,
            246,
            89,
            166,
            166,
            174,
            111,
            113,
            241,
            223,
            74,
            16,
            22,
            170,
            19,
            199,
            81,
            44,
            2,
        ],
    ],
}
```

With this change:
```
ic-admin propose-to-remove-node-operators --nns-urls https://ic0.app xla4b-4vmw4-db4cm-qg63h-6jvj6-zm2nj-von5y-7dx2k-calku-e6hke-wae --dry-run --summary "Remove obsolete node operator"
[...]
Title: Remove node operators with principal ids: ["xla4b"]

Summary: Remove obsolete node operator

Payload: RemoveNodeOperatorsPayload {
    node_operators_to_remove: [],
    node_operator_principals_to_remove: Some(
        NodeOperatorPrincipals {
            principals: [
                xla4b-4vmw4-db4cm-qg63h-6jvj6-zm2nj-von5y-7dx2k-calku-e6hke-wae,
            ],
        },
    ),
}


```

### Changes
- Removed `RemoveNodeOperatorsPayload` from `node_operator.proto`.
- Moved `RemoveNodeOperatorsPayload` definition into
`do_remove_node_operators.rs`.

---------

Co-authored-by: Max Summe <maximilian.summe@dfinity.org>

rs/nns/integration_tests/src/node_operator_handler.rs

@@ -20,14 +20,17 @@ use ic_nns_test_utils::{
     itest_helpers::{state_machine_test_on_nns_subnet, NnsCanisters},
     registry::{get_value_or_panic, prepare_add_node_payload},
 };
-use ic_protobuf::registry::node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload};
+use ic_protobuf::registry::node_operator::v1::NodeOperatorRecord;
 use ic_registry_keys::make_node_operator_record_key;
 use ic_registry_transport::{
     deserialize_get_value_response, serialize_get_value_request, Error::KeyNotPresent,
 };
 use ic_types::PrincipalId;
 use maplit::btreemap;
-use registry_canister::mutations::do_add_node_operator::AddNodeOperatorPayload;
+use registry_canister::mutations::{
+    do_add_node_operator::AddNodeOperatorPayload,
+    do_remove_node_operators::RemoveNodeOperatorsPayload,
+};
 use std::time::Duration;
 
 /// Test that new Node Operator records can be added and removed to/from the
@@ -123,11 +126,10 @@ fn test_node_operator_records_can_be_added_and_removed() {
             .await
             .unwrap();
 
-        let node_operator_id_1: Vec<u8> = (*TEST_NEURON_1_OWNER_PRINCIPAL.into_vec()).to_vec();
-        let node_operator_id_2: Vec<u8> = (*TEST_NEURON_2_OWNER_PRINCIPAL.into_vec()).to_vec();
-        let proposal_payload = RemoveNodeOperatorsPayload {
-            node_operators_to_remove: vec![node_operator_id_1, node_operator_id_2],
-        };
+        let proposal_payload = RemoveNodeOperatorsPayload::new(vec![
+            *TEST_NEURON_1_OWNER_PRINCIPAL,
+            *TEST_NEURON_2_OWNER_PRINCIPAL,
+        ]);
 
         let node_operator_record_key_1 =
             make_node_operator_record_key(*TEST_NEURON_1_OWNER_PRINCIPAL).into_bytes();

rs/protobuf/def/registry/node_operator/v1/node_operator.proto

@@ -29,8 +29,3 @@ message NodeOperatorRecord {
 
   optional string ipv6 = 6;
 }
-
-// The payload of a request to remove Node Operator records from the Registry
-message RemoveNodeOperatorsPayload {
-  repeated bytes node_operators_to_remove = 1;
-}

rs/protobuf/src/gen/registry/registry.node_operator.v1.rs

@@ -39,18 +39,3 @@ pub struct NodeOperatorRecord {
     #[prost(string, optional, tag = "6")]
     pub ipv6: ::core::option::Option<::prost::alloc::string::String>,
 }
-/// The payload of a request to remove Node Operator records from the Registry
-#[derive(
-    candid::CandidType,
-    serde::Serialize,
-    candid::Deserialize,
-    Eq,
-    Hash,
-    Clone,
-    PartialEq,
-    ::prost::Message,
-)]
-pub struct RemoveNodeOperatorsPayload {
-    #[prost(bytes = "vec", repeated, tag = "1")]
-    pub node_operators_to_remove: ::prost::alloc::vec::Vec<::prost::alloc::vec::Vec<u8>>,
-}

rs/registry/admin/src/main.rs

@@ -77,7 +77,7 @@ use ic_protobuf::registry::{
     dc::v1::{AddOrRemoveDataCentersProposalPayload, DataCenterRecord},
     firewall::v1::{FirewallConfig, FirewallRule, FirewallRuleSet},
     node::v1::{NodeRecord, NodeRewardType},
-    node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload},
+    node_operator::v1::NodeOperatorRecord,
     node_rewards::v2::{NodeRewardRate, UpdateNodeRewardsTableProposalPayload},
     provisional_whitelist::v1::ProvisionalWhitelist as ProvisionalWhitelistProto,
     replica_version::v1::{BlessedReplicaVersions, ReplicaVersionRecord},
@@ -134,6 +134,7 @@ use registry_canister::mutations::{
     do_deploy_guestos_to_all_subnet_nodes::DeployGuestosToAllSubnetNodesPayload,
     do_deploy_guestos_to_all_unassigned_nodes::DeployGuestosToAllUnassignedNodesPayload,
     do_remove_api_boundary_nodes::RemoveApiBoundaryNodesPayload,
+    do_remove_node_operators::RemoveNodeOperatorsPayload,
     do_revise_elected_replica_versions::ReviseElectedGuestosVersionsPayload,
     do_set_firewall_config::SetFirewallConfigPayload,
     do_update_api_boundary_nodes_version::DeployGuestosToSomeApiBoundaryNodes,
@@ -701,14 +702,7 @@ impl ProposalTitle for ProposeToRemoveNodeOperatorsCmd {
 #[async_trait]
 impl ProposalPayload<RemoveNodeOperatorsPayload> for ProposeToRemoveNodeOperatorsCmd {
     async fn payload(&self, _: &Agent) -> RemoveNodeOperatorsPayload {
-        RemoveNodeOperatorsPayload {
-            node_operators_to_remove: self
-                .node_operators_to_remove
-                .clone()
-                .iter()
-                .map(|x| x.to_vec())
-                .collect(),
-        }
+        RemoveNodeOperatorsPayload::new(self.node_operators_to_remove.clone())
     }
 }
 

rs/registry/canister/canister/canister.rs

@@ -10,7 +10,7 @@ use ic_nervous_system_string::clamp_debug_len;
 use ic_nns_constants::{GOVERNANCE_CANISTER_ID, ROOT_CANISTER_ID};
 use ic_protobuf::registry::{
     dc::v1::{AddOrRemoveDataCentersProposalPayload, DataCenterRecord},
-    node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload},
+    node_operator::v1::NodeOperatorRecord,
     node_rewards::v2::UpdateNodeRewardsTableProposalPayload,
 };
 use ic_registry_canister_api::{
@@ -43,6 +43,7 @@ use registry_canister::{
         do_deploy_guestos_to_all_unassigned_nodes::DeployGuestosToAllUnassignedNodesPayload,
         do_recover_subnet::RecoverSubnetPayload,
         do_remove_api_boundary_nodes::RemoveApiBoundaryNodesPayload,
+        do_remove_node_operators::RemoveNodeOperatorsPayload,
         do_remove_nodes_from_subnet::RemoveNodesFromSubnetPayload,
         do_revise_elected_replica_versions::ReviseElectedGuestosVersionsPayload,
         do_set_firewall_config::SetFirewallConfigPayload,

rs/registry/canister/canister/registry.did

@@ -273,6 +273,11 @@ type RemoveNodeDirectlyPayload = record { node_id : principal };
 
 type RemoveNodeOperatorsPayload = record {
   node_operators_to_remove : vec blob;
+  node_operator_principals_to_remove : opt NodeOperatorPrincipals;
+};
+
+type NodeOperatorPrincipals = record {
+  principals : vec principal;
 };
 
 type RemoveNodesPayload = record { node_ids : vec principal };

rs/registry/canister/src/mutations/do_remove_node_operators.rs

@@ -3,12 +3,11 @@ use crate::{common::LOG_PREFIX, registry::Registry};
 #[cfg(target_arch = "wasm32")]
 use dfn_core::println;
 
+use candid::CandidType;
 use ic_base_types::PrincipalId;
-use ic_protobuf::registry::node_operator::v1::RemoveNodeOperatorsPayload;
 use ic_registry_keys::{make_node_operator_record_key, NODE_RECORD_KEY_PREFIX};
 use ic_registry_transport::pb::v1::{registry_mutation, RegistryMutation};
-
-use std::convert::TryFrom;
+use serde::{Deserialize, Serialize};
 
 use ic_protobuf::registry::node::v1::NodeRecord;
 use prost::Message;
@@ -20,26 +19,21 @@ impl Registry {
 
         let mut mutations = vec![];
 
-        // Node Operator IDs that are parsable as PrincipalIds and have an associated
-        // NodeOperatorRecord in the Registry
-        let mut valid_node_operator_ids = payload
-            .node_operators_to_remove
+        // Filter Node Operator IDs that have a NodeOperatorRecord in the Registry
+        let mut valid_node_operator_ids_to_remove: Vec<PrincipalId> = payload
+            .principal_ids_to_remove()
             .into_iter()
-            .filter_map(|bytes| {
-                PrincipalId::try_from(bytes)
-                    .ok()
-                    .filter(|node_operator_id| {
-                        let node_operator_record_key =
-                            make_node_operator_record_key(*node_operator_id).into_bytes();
-                        self.get(&node_operator_record_key, self.latest_version())
-                            .is_some()
-                    })
+            .filter(|node_operator_id| {
+                let node_operator_record_key =
+                    make_node_operator_record_key(*node_operator_id).into_bytes();
+                self.get(&node_operator_record_key, self.latest_version())
+                    .is_some()
             })
             .collect();
 
-        self.filter_node_operators_that_have_nodes(&mut valid_node_operator_ids);
+        self.filter_node_operators_that_have_nodes(&mut valid_node_operator_ids_to_remove);
 
-        for node_operator_id in valid_node_operator_ids {
+        for node_operator_id in valid_node_operator_ids_to_remove {
             let node_operator_record_key =
                 make_node_operator_record_key(node_operator_id).into_bytes();
             mutations.push(RegistryMutation {
@@ -73,3 +67,50 @@ impl Registry {
         }
     }
 }
+
+/// The payload of a request to remove Node Operator records from the Registry
+#[derive(Clone, Debug, Eq, PartialEq, CandidType, Deserialize, Serialize, Hash)]
+pub struct RemoveNodeOperatorsPayload {
+    // Old compatibility field, required for Candid, to be removed in the future
+    pub node_operators_to_remove: Vec<Vec<u8>>,
+
+    // New field, where the Node Operator IDs are passed as PrincipalIds instead of Vec<u8>
+    pub node_operator_principals_to_remove: Option<NodeOperatorPrincipals>,
+}
+
+/// Wrapper message for the optional repeated field
+#[derive(Clone, Debug, Eq, PartialEq, CandidType, Deserialize, Serialize, Hash)]
+pub struct NodeOperatorPrincipals {
+    pub principals: Vec<PrincipalId>,
+}
+
+impl RemoveNodeOperatorsPayload {
+    pub fn new(node_operators_to_remove: Vec<PrincipalId>) -> Self {
+        Self {
+            node_operators_to_remove: vec![],
+            node_operator_principals_to_remove: Some(NodeOperatorPrincipals {
+                principals: node_operators_to_remove,
+            }),
+        }
+    }
+
+    pub fn principal_ids_to_remove(&self) -> Vec<PrincipalId> {
+        // Ensure only one of the fields is set to avoid confusing semantics.
+        // If the new field is present, panic if the old field is also set.
+        // This approach encourages clients to use the new field and allows for
+        // eventual deprecation of the old field.
+        match &self.node_operator_principals_to_remove {
+            Some(principals) if self.node_operators_to_remove.is_empty() => {
+                principals.principals.clone()
+            }
+            Some(_) => {
+                panic!("Cannot specify both node_operators_to_remove and node_operator_principals_to_remove");
+            }
+            None => self
+                .node_operators_to_remove
+                .iter()
+                .filter_map(|bytes| PrincipalId::try_from(bytes.clone()).ok())
+                .collect(),
+        }
+    }
+}

rs/registry/canister/tests/remove_node_operators.rs

@@ -8,8 +8,10 @@ use ic_nns_test_utils::{
     },
     registry::invariant_compliant_mutation_as_atomic_req,
 };
-use ic_protobuf::registry::node_operator::v1::RemoveNodeOperatorsPayload;
-use registry_canister::init::RegistryCanisterInitPayloadBuilder;
+use registry_canister::{
+    init::RegistryCanisterInitPayloadBuilder,
+    mutations::do_remove_node_operators::RemoveNodeOperatorsPayload,
+};
 
 #[test]
 fn test_the_anonymous_user_cannot_remove_node_operators() {
@@ -22,9 +24,7 @@ fn test_the_anonymous_user_cannot_remove_node_operators() {
         )
         .await;
 
-        let payload = RemoveNodeOperatorsPayload {
-            node_operators_to_remove: vec![],
-        };
+        let payload = RemoveNodeOperatorsPayload::new(vec![]);
 
         // The anonymous end-user tries to remove node operators, bypassing
         // the Governance canister. This should be rejected.
@@ -72,9 +72,7 @@ fn test_a_canister_other_than_the_governance_canister_cannot_remove_node_operato
         )
         .await;
 
-        let payload = RemoveNodeOperatorsPayload {
-            node_operators_to_remove: vec![],
-        };
+        let payload = RemoveNodeOperatorsPayload::new(vec![]);
 
         // The attacker canister tries to remove node operators, pretending
         // to be the Governance canister. This should have no effect.