refactor(nns): Deleted ListNeurons from NNS governance.proto. (dfinity#3546)

Also ListNeuronsResponse.

# Motivation/Inspiration

I started working on deleting `(deciding|potential)_voting_power` fields
from `governance.proto` ([NNS1-3500]). Some of those fields appear in
`NeuronInfo`. I looked into that type, and it seems like we could delete
it, because AFAICT, it is only used in a couple of response types, such
as `ListNeuronsResponse`. Therefore, this indirectly helps with getting
rid of `*_voting_power` fields in `governance.proto`.

[NNS1-3500]: https://dfinity.atlassian.net/browse/NNS1-3500

We should be able to delete most request and response types from
`governance.proto`, because we do not store those in stable memory. This
shows how we would delete other request and response types. This also
shows that such deletions can be fairly straightforward.

# Bonus Changes

Switch rosetta from using `//rs/nns/governance` to
`//rs/nns/governance/api`. This made it possible to "lock down"
//rs/nns/governance, i.e. reduce its visibility. Because [min visibility
good].

[min visibility good]:
https://bazel.build/concepts/visibility#rule-target-visibility

Make aliases generated by rust_canister inherit visibility. This was
needed to lock down //rs/nns/governance.

# How to Delete Request & Response Types from governance.proto

Here is an overview of how this PR was constructed; the same steps can
(very likely) be used to delete other request and response types:

<ol>
<li>Delete the definitions from <code>governance.proto</code>.

<li>Delete the conversions in
<code>rs/nns/governance/src/pb/conversions.rs</code>.

<li>Run <code>bazel build //rs/nns/governance</code>. This will reveal
all the places that need to be updated in the main lib.

<li>In every file that used the types that were deleted in step 1,
<i>replace them with the analogous API types</i>. E.g. the following
will probably get you more than 90% of the way there:
<pre><code>-use crate::pb::v1::{FooRequest, FooResponse};
+use ic_nns_governance_api::pb::v1::{FooRequest, FooResponse};
</code></pre>
The reason this is pretty effective is that the definitions of the API
types were simply copied from the Prost-generated type.

<li>Fix problems that arise from this replacement. E.g. there may cases
where you need to convert from an API type "back" to a Prost-generated
type, or the other way around. This is likely the most difficult step.

<li>Fix <code>canister.rs</code>. It might well be that the only thing
you need to do here is remove some <code>.into()</code>. Those are no
longer necessary, because now, they just convert T into T. Ofc, this
change is not strictly necessary, but clippy might yell at you, and in
any case, it confuses humans when you do <code>.into()</code> even
though the object is already of the desired type.

<li>Fix tests. Again, we apply steps 3-5, but this time, the goal is to
make <code>//rs/nns/governance/...</code> build.

</ol>

The only "insight" here is that it is fairly easy to replace
Prost-generated request and response types with API types. Other than
that, these steps are more or less corollaries of "delete request and
response types".

# Why Min Visibility Good

When you want to change something, you won't need to change the whole
world. I.e. encapsulation, modularity, loose coupling, non-entanglement.
Being able to make changes in the future is extrem imperative, because
change is the only constant.

This is the exact same rationale that is used to justify [private ALL
the things].

[private ALL the things]:
https://google.github.io/styleguide/cppguide.html#Access_Control

Author: daniel-wong-dfinity-org

Cargo.lock

@@ -135,12 +135,14 @@ def rust_canister(name, service_file, visibility = ["//visibility:public"], test
     native.alias(
         name = name,
         actual = name + ".wasm",
+        visibility = visibility,
     )
 
     # DID service related targets
     native.alias(
         name = name + ".didfile",
         actual = service_file,
+        visibility = visibility,
     )
     did_git_test(
         name = name + "_did_git_test",

bazel/canisters.bzl

@@ -6,9 +6,21 @@ load("//bazel:defs.bzl", "rust_bench", "rust_test_suite_with_extra_srcs")
 load("//bazel:prost.bzl", "generated_files_check")
 load(":feature_flags.bzl", "test_with_tla")
 
-package(default_visibility = ["//visibility:public"])
+package(default_visibility = ["//rs/nervous_system:default_visibility"])
 
-exports_files(["canister/governance.did"])
+DID_FILES = glob([
+    "**/*.did",
+])
+
+exports_files(
+    # Notice that governance.proto is NOT included. This is very intentional. We
+    # only use that for storing things to stable memory, not for communication
+    # with clients.
+    DID_FILES,
+    visibility = [
+        "//visibility:public",
+    ],
+)
 
 # Allows temporarily disabling TLA checks from the command line;
 # just pass `--define tla_disabled=true` to your Bazel command
@@ -190,6 +202,7 @@ rust_library(
     crate_name = "ic_nns_governance",
     proc_macro_deps = MACRO_DEPENDENCIES,
     version = "0.9.0",
+    visibility = ["//visibility:private"],
     deps = DEPENDENCIES + [
         "@crate_index//:canbench-rs",
     ],
@@ -225,6 +238,7 @@ rust_canister(
     compile_data = ["canister/governance.did"],
     proc_macro_deps = MACRO_DEPENDENCIES,
     service_file = ":canister/governance.did",
+    visibility = ["//visibility:public"],
     deps = DEPENDENCIES + [
         ":build_script",
         ":governance",
@@ -244,6 +258,7 @@ rust_canister(
     crate_root = "canister/canister.rs",
     proc_macro_deps = MACRO_DEPENDENCIES,
     service_file = ":canister/governance_test.did",
+    visibility = ["//visibility:public"],
     deps = DEPENDENCIES + [
         ":build_script",
         ":governance--test_feature",

rs/nns/governance/BUILD.bazel

@@ -85,7 +85,7 @@ benches:
     scopes: {}
   list_neurons_heap:
     total:
-      instructions: 4763239
+      instructions: 4880000
       heap_increase: 9
       stable_memory_increase: 0
     scopes: {}
@@ -104,7 +104,7 @@ benches:
   list_neurons_stable:
     total:
       instructions: 113374022
-      heap_increase: 5
+      heap_increase: 4
       stable_memory_increase: 0
     scopes: {}
   list_proposals:

rs/nns/governance/canbench/canbench_results.yml

@@ -812,7 +812,7 @@ fn list_proposals(req: ListProposalInfo) -> ListProposalInfoResponse {
 #[query]
 fn list_neurons(req: ListNeurons) -> ListNeuronsResponse {
     debug_log("list_neurons");
-    governance().list_neurons(&(req.into()), caller()).into()
+    governance().list_neurons(&req, caller())
 }
 
 #[query]

rs/nns/governance/canister/canister.rs

@@ -2630,51 +2630,6 @@ message ListProposalInfoResponse {
   repeated ProposalInfo proposal_info = 1;
 }
 
-// A request to list neurons. The "requested list", i.e., the list of
-// neuron IDs to retrieve information about, is the union of the list
-// of neurons listed in `neuron_ids` and, if `caller_neurons` is true,
-// the list of neuron IDs of neurons for which the caller is the
-// controller or one of the hot keys.
-message ListNeurons {
-  option (ic_base_types.pb.v1.tui_signed_message) = true;
-  // The neurons to get information about. The "requested list"
-  // contains all of these neuron IDs.
-  repeated fixed64 neuron_ids = 1 [(ic_base_types.pb.v1.tui_signed_display_q2_2021) = true];
-  // If true, the "requested list" also contains the neuron ID of the
-  // neurons that the calling principal is authorized to read.
-  bool include_neurons_readable_by_caller = 2 [(ic_base_types.pb.v1.tui_signed_display_q2_2021) = true];
-  // Whether to also include empty neurons readable by the caller. This field only has an effect
-  // when `include_neurons_readable_by_caller` is true. If a neuron's id already exists in the
-  // `neuron_ids` field, then the neuron will be included in the response regardless of the value of
-  // this field. Since the previous behavior was to always include empty neurons readable by caller,
-  // if this field is not provided, it defaults to true, in order to maintain backwards
-  // compatibility. Here, being "empty" means 0 stake, 0 maturity and 0 staked maturity.
-  optional bool include_empty_neurons_readable_by_caller = 3;
-  // If this is set to true, and a neuron in the "requested list" has its
-  // visibility set to public, then, it will (also) be included in the
-  // full_neurons field in the response (which is of type ListNeuronsResponse).
-  // Note that this has no effect on which neurons are in the "requested list".
-  // In particular, this does not cause all public neurons to become part of the
-  // requested list. In general, you probably want to set this to true, but
-  // since this feature was added later, it is opt in to avoid confusing
-  // existing (unmigrated) callers.
-  optional bool include_public_neurons_in_full_neurons = 4;
-}
-
-// A response to a `ListNeurons` request.
-//
-// The "requested list" is described in `ListNeurons`.
-message ListNeuronsResponse {
-  // For each neuron ID in the "requested list", if this neuron exists,
-  // its `NeuronInfo` at the time of the call will be in this map.
-  map<fixed64, NeuronInfo> neuron_infos = 1;
-  // For each neuron ID in the "requested list", if the neuron exists,
-  // and the caller is authorized to read the full neuron (controller,
-  // hot key, or controller or hot key of some followee on the
-  // `ManageNeuron` topic).
-  repeated Neuron full_neurons = 2;
-}
-
 // A response to "ListKnownNeurons"
 message ListKnownNeuronsResponse {
   // List of known neurons.

rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto

@@ -3927,72 +3927,6 @@ pub struct ListProposalInfoResponse {
     #[prost(message, repeated, tag = "1")]
     pub proposal_info: ::prost::alloc::vec::Vec<ProposalInfo>,
 }
-/// A request to list neurons. The "requested list", i.e., the list of
-/// neuron IDs to retrieve information about, is the union of the list
-/// of neurons listed in `neuron_ids` and, if `caller_neurons` is true,
-/// the list of neuron IDs of neurons for which the caller is the
-/// controller or one of the hot keys.
-#[derive(
-    candid::CandidType,
-    candid::Deserialize,
-    serde::Serialize,
-    comparable::Comparable,
-    Clone,
-    PartialEq,
-    ::prost::Message,
-)]
-pub struct ListNeurons {
-    /// The neurons to get information about. The "requested list"
-    /// contains all of these neuron IDs.
-    #[prost(fixed64, repeated, packed = "false", tag = "1")]
-    pub neuron_ids: ::prost::alloc::vec::Vec<u64>,
-    /// If true, the "requested list" also contains the neuron ID of the
-    /// neurons that the calling principal is authorized to read.
-    #[prost(bool, tag = "2")]
-    pub include_neurons_readable_by_caller: bool,
-    /// Whether to also include empty neurons readable by the caller. This field only has an effect
-    /// when `include_neurons_readable_by_caller` is true. If a neuron's id already exists in the
-    /// `neuron_ids` field, then the neuron will be included in the response regardless of the value of
-    /// this field. Since the previous behavior was to always include empty neurons readable by caller,
-    /// if this field is not provided, it defaults to true, in order to maintain backwards
-    /// compatibility. Here, being "empty" means 0 stake, 0 maturity and 0 staked maturity.
-    #[prost(bool, optional, tag = "3")]
-    pub include_empty_neurons_readable_by_caller: ::core::option::Option<bool>,
-    /// If this is set to true, and a neuron in the "requested list" has its
-    /// visibility set to public, then, it will (also) be included in the
-    /// full_neurons field in the response (which is of type ListNeuronsResponse).
-    /// Note that this has no effect on which neurons are in the "requested list".
-    /// In particular, this does not cause all public neurons to become part of the
-    /// requested list. In general, you probably want to set this to true, but
-    /// since this feature was added later, it is opt in to avoid confusing
-    /// existing (unmigrated) callers.
-    #[prost(bool, optional, tag = "4")]
-    pub include_public_neurons_in_full_neurons: ::core::option::Option<bool>,
-}
-/// A response to a `ListNeurons` request.
-///
-/// The "requested list" is described in `ListNeurons`.
-#[derive(
-    candid::CandidType,
-    candid::Deserialize,
-    serde::Serialize,
-    comparable::Comparable,
-    Clone,
-    PartialEq,
-    ::prost::Message,
-)]
-pub struct ListNeuronsResponse {
-    /// For each neuron ID in the "requested list", if this neuron exists,
-    /// its `NeuronInfo` at the time of the call will be in this map.
-    #[prost(map = "fixed64, message", tag = "1")]
-    pub neuron_infos: ::std::collections::HashMap<u64, NeuronInfo>,
-    /// For each neuron ID in the "requested list", if the neuron exists,
-    /// and the caller is authorized to read the full neuron (controller,
-    /// hot key, or controller or hot key of some followee on the
-    /// `ManageNeuron` topic).
-    #[prost(message, repeated, tag = "2")]
-    pub full_neurons: ::prost::alloc::vec::Vec<Neuron>,
-}
 /// A response to "ListKnownNeurons"
 #[derive(
     candid::CandidType,

rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs

@@ -53,10 +53,10 @@ use crate::{
         swap_background_information, ArchivedMonthlyNodeProviderRewards, Ballot,
         CreateServiceNervousSystem, ExecuteNnsFunction, GetNeuronsFundAuditInfoRequest,
         GetNeuronsFundAuditInfoResponse, Governance as GovernanceProto, GovernanceError,
-        InstallCode, KnownNeuron, ListKnownNeuronsResponse, ListNeurons, ListNeuronsResponse,
-        ListProposalInfo, ListProposalInfoResponse, ManageNeuron, ManageNeuronResponse,
-        MonthlyNodeProviderRewards, Motion, NetworkEconomics, Neuron as NeuronProto, NeuronInfo,
-        NeuronState, NeuronsFundAuditInfo, NeuronsFundData,
+        InstallCode, KnownNeuron, ListKnownNeuronsResponse, ListProposalInfo,
+        ListProposalInfoResponse, ManageNeuron, ManageNeuronResponse, MonthlyNodeProviderRewards,
+        Motion, NetworkEconomics, Neuron as NeuronProto, NeuronInfo, NeuronState,
+        NeuronsFundAuditInfo, NeuronsFundData,
         NeuronsFundEconomics as NeuronsFundNetworkEconomicsPb,
         NeuronsFundParticipation as NeuronsFundParticipationPb,
         NeuronsFundSnapshot as NeuronsFundSnapshotPb, NnsFunction, NodeProvider, Proposal,
@@ -94,7 +94,11 @@ use ic_nns_constants::{
     SUBNET_RENTAL_CANISTER_ID,
 };
 use ic_nns_governance_api::{
-    pb::v1::CreateServiceNervousSystem as ApiCreateServiceNervousSystem, proposal_validation,
+    pb::v1::{
+        self as api, CreateServiceNervousSystem as ApiCreateServiceNervousSystem, ListNeurons,
+        ListNeuronsResponse,
+    },
+    proposal_validation,
     subnet_rental::SubnetRentalRequest,
 };
 use ic_protobuf::registry::dc::v1::AddOrRemoveDataCentersProposalPayload;
@@ -2470,7 +2474,14 @@ impl Governance {
             // requested_neuron_ids are supplied by the caller.
             let _ignore_when_neuron_not_found = self.with_neuron(&neuron_id, |neuron| {
                 // Populate neuron_infos.
-                neuron_infos.insert(neuron_id.id, neuron.get_neuron_info(self.voting_power_economics(), now, caller));
+                let neuron_info = neuron.get_neuron_info(self.voting_power_economics(), now, caller);
+
+                // We will be able to get rid of this conversion once we delete
+                // NeuronInfo from governance.proto. That should be possible,
+                // since we do not store NeuronInfo in stable memory.
+                let neuron_info = api::NeuronInfo::from(neuron_info);
+
+                neuron_infos.insert(neuron_id.id, neuron_info);
 
                 // Populate full_neurons.
                 let let_caller_read_full_neuron =
@@ -2489,7 +2500,7 @@ impl Governance {
                     // we need to do a larger refactoring to use the correct API types instead of the internal
                     // governance proto at this level.
                     proto.recent_ballots = neuron.sorted_recent_ballots();
-                    full_neurons.push(proto);
+                    full_neurons.push(api::Neuron::from(proto));
                 }
             });
         }

rs/nns/governance/src/governance.rs

@@ -8,8 +8,8 @@ use crate::{
     pb::v1::{
         install_code::CanisterInstallMode, neuron::Followees, proposal::Action, Ballot, BallotInfo,
         CreateServiceNervousSystem, ExecuteNnsFunction, Governance as GovernanceProto, InstallCode,
-        KnownNeuron, ListNeurons, ListProposalInfo, NetworkEconomics, Neuron as NeuronProto,
-        NnsFunction, Proposal, ProposalData, Topic, Vote, VotingPowerEconomics,
+        KnownNeuron, ListProposalInfo, NetworkEconomics, Neuron as NeuronProto, NnsFunction,
+        Proposal, ProposalData, Topic, Vote, VotingPowerEconomics,
     },
     temporarily_disable_allow_active_neurons_in_stable_memory,
     temporarily_disable_migrate_active_neurons_to_stable_memory,
@@ -28,6 +28,7 @@ use ic_nns_common::{
     types::NeuronId,
 };
 use ic_nns_constants::GOVERNANCE_CANISTER_ID;
+use ic_nns_governance_api::pb::v1::ListNeurons;
 use icp_ledger::Subaccount;
 use maplit::hashmap;
 use rand::{Rng, SeedableRng};

rs/nns/governance/src/governance/benches.rs

@@ -3759,52 +3759,6 @@ impl From<pb_api::ListProposalInfoResponse> for pb::ListProposalInfoResponse {
     }
 }
 
-impl From<pb::ListNeurons> for pb_api::ListNeurons {
-    fn from(item: pb::ListNeurons) -> Self {
-        Self {
-            neuron_ids: item.neuron_ids,
-            include_neurons_readable_by_caller: item.include_neurons_readable_by_caller,
-            include_empty_neurons_readable_by_caller: item.include_empty_neurons_readable_by_caller,
-            include_public_neurons_in_full_neurons: item.include_public_neurons_in_full_neurons,
-        }
-    }
-}
-impl From<pb_api::ListNeurons> for pb::ListNeurons {
-    fn from(item: pb_api::ListNeurons) -> Self {
-        Self {
-            neuron_ids: item.neuron_ids,
-            include_neurons_readable_by_caller: item.include_neurons_readable_by_caller,
-            include_empty_neurons_readable_by_caller: item.include_empty_neurons_readable_by_caller,
-            include_public_neurons_in_full_neurons: item.include_public_neurons_in_full_neurons,
-        }
-    }
-}
-
-impl From<pb::ListNeuronsResponse> for pb_api::ListNeuronsResponse {
-    fn from(item: pb::ListNeuronsResponse) -> Self {
-        Self {
-            neuron_infos: item
-                .neuron_infos
-                .into_iter()
-                .map(|(k, v)| (k, v.into()))
-                .collect(),
-            full_neurons: item.full_neurons.into_iter().map(|x| x.into()).collect(),
-        }
-    }
-}
-impl From<pb_api::ListNeuronsResponse> for pb::ListNeuronsResponse {
-    fn from(item: pb_api::ListNeuronsResponse) -> Self {
-        Self {
-            neuron_infos: item
-                .neuron_infos
-                .into_iter()
-                .map(|(k, v)| (k, v.into()))
-                .collect(),
-            full_neurons: item.full_neurons.into_iter().map(|x| x.into()).collect(),
-        }
-    }
-}
-
 impl From<pb::ListKnownNeuronsResponse> for pb_api::ListKnownNeuronsResponse {
     fn from(item: pb::ListKnownNeuronsResponse) -> Self {
         Self {