From 367ab73788ed95af643e057a79bd2f98e1f3eee2 Mon Sep 17 00:00:00 2001 From: Daniel Wong <97631336+daniel-wong-dfinity-org@users.noreply.github.com> Date: Wed, 22 Jan 2025 13:50:05 +0100 Subject: [PATCH] refactor(nns): Deleted ListNeurons from NNS governance.proto. (#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:
  1. Delete the definitions from governance.proto.
  2. Delete the conversions in rs/nns/governance/src/pb/conversions.rs.
  3. Run bazel build //rs/nns/governance. This will reveal all the places that need to be updated in the main lib.
  4. In every file that used the types that were deleted in step 1, replace them with the analogous API types. E.g. the following will probably get you more than 90% of the way there: 
    -use crate::pb::v1::{FooRequest, FooResponse};
+use ic_nns_governance_api::pb::v1::{FooRequest, FooResponse};
    The reason this is pretty effective is that the definitions of the API types were simply copied from the Prost-generated type.
  5. 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.
  6. Fix canister.rs. It might well be that the only thing you need to do here is remove some .into(). 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 .into() even though the object is already of the desired type.
  7. Fix tests. Again, we apply steps 3-5, but this time, the goal is to make //rs/nns/governance/... build.
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 --- Cargo.lock | 1 - bazel/canisters.bzl | 2 + rs/nns/governance/BUILD.bazel | 19 +++++- .../governance/canbench/canbench_results.yml | 4 +- rs/nns/governance/canister/canister.rs | 2 +- .../ic_nns_governance/pb/v1/governance.proto | 45 ------------- .../src/gen/ic_nns_governance.pb.v1.rs | 66 ------------------- rs/nns/governance/src/governance.rs | 25 +++++-- rs/nns/governance/src/governance/benches.rs | 5 +- rs/nns/governance/src/pb/conversions.rs | 46 ------------- rs/nns/governance/tests/governance.rs | 51 ++++++++++---- rs/rosetta-api/icp/BUILD.bazel | 1 - rs/rosetta-api/icp/Cargo.toml | 1 - .../icp/tests/system_tests/common/utils.rs | 4 +- .../test_cases/neuron_management.rs | 17 +++-- 15 files changed, 92 insertions(+), 197 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f2e4877645c..66492550925 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11668,7 +11668,6 @@ dependencies = [ "ic-limits", "ic-nns-common", "ic-nns-constants", - "ic-nns-governance", "ic-nns-governance-api", "ic-nns-governance-init", "ic-nns-handler-root", diff --git a/bazel/canisters.bzl b/bazel/canisters.bzl index 2308dc7147e..21d899fbb3e 100644 --- a/bazel/canisters.bzl +++ b/bazel/canisters.bzl @@ -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", diff --git a/rs/nns/governance/BUILD.bazel b/rs/nns/governance/BUILD.bazel index 23cabdab270..337614a9104 100644 --- a/rs/nns/governance/BUILD.bazel +++ b/rs/nns/governance/BUILD.bazel @@ -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", diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index 7fc2d61ada9..e4dc1b66b4f 100644 --- a/rs/nns/governance/canbench/canbench_results.yml +++ b/rs/nns/governance/canbench/canbench_results.yml @@ -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: diff --git a/rs/nns/governance/canister/canister.rs b/rs/nns/governance/canister/canister.rs index 2f54dda2027..81c42986e18 100644 --- a/rs/nns/governance/canister/canister.rs +++ b/rs/nns/governance/canister/canister.rs @@ -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] diff --git a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto index 7db94271fc0..c4bcdf1ddd4 100644 --- a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto +++ b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto @@ -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 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. diff --git a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs index bfc6f5d5de3..ec3199b12ce 100644 --- a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs +++ b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs @@ -3927,72 +3927,6 @@ pub struct ListProposalInfoResponse { #[prost(message, repeated, tag = "1")] pub proposal_info: ::prost::alloc::vec::Vec, } -/// 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, - /// 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, - /// 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, -} -/// 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, - /// 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, -} /// A response to "ListKnownNeurons" #[derive( candid::CandidType, diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index 52c74392d00..3ad7782bfe5 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.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)); } }); } diff --git a/rs/nns/governance/src/governance/benches.rs b/rs/nns/governance/src/governance/benches.rs index 9952e9fee1a..4636c07899d 100644 --- a/rs/nns/governance/src/governance/benches.rs +++ b/rs/nns/governance/src/governance/benches.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}; diff --git a/rs/nns/governance/src/pb/conversions.rs b/rs/nns/governance/src/pb/conversions.rs index e25e507ab52..5ab2efdaccf 100644 --- a/rs/nns/governance/src/pb/conversions.rs +++ b/rs/nns/governance/src/pb/conversions.rs @@ -3759,52 +3759,6 @@ impl From for pb::ListProposalInfoResponse { } } -impl From 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 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 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 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 for pb_api::ListKnownNeuronsResponse { fn from(item: pb::ListKnownNeuronsResponse) -> Self { Self { diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index 534115d2941..d8b7495c04e 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -84,11 +84,11 @@ use ic_nns_governance::{ AddOrRemoveNodeProvider, Ballot, BallotChange, BallotInfo, BallotInfoChange, CreateServiceNervousSystem, Empty, ExecuteNnsFunction, Governance as GovernanceProto, GovernanceChange, GovernanceError, IdealMatchedParticipationFunction, InstallCode, - KnownNeuron, KnownNeuronData, ListNeurons, ListNeuronsResponse, ListProposalInfo, - ListProposalInfoResponse, ManageNeuron, ManageNeuronResponse, MonthlyNodeProviderRewards, - Motion, NetworkEconomics, Neuron, NeuronChange, NeuronState, NeuronType, NeuronsFundData, - NeuronsFundParticipation, NeuronsFundSnapshot, NnsFunction, NodeProvider, Proposal, - ProposalChange, ProposalData, ProposalDataChange, + KnownNeuron, KnownNeuronData, ListProposalInfo, ListProposalInfoResponse, ManageNeuron, + ManageNeuronResponse, MonthlyNodeProviderRewards, Motion, NetworkEconomics, Neuron, + NeuronChange, NeuronState, NeuronType, NeuronsFundData, NeuronsFundParticipation, + NeuronsFundSnapshot, NnsFunction, NodeProvider, Proposal, ProposalChange, ProposalData, + ProposalDataChange, ProposalRewardStatus::{self, AcceptVotes, ReadyToSettle}, ProposalStatus::{self, Rejected}, RewardEvent, RewardNodeProvider, RewardNodeProviders, @@ -101,7 +101,10 @@ use ic_nns_governance::{ DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, }; use ic_nns_governance_api::{ - pb::v1::CreateServiceNervousSystem as ApiCreateServiceNervousSystem, + pb::v1::{ + self as api, CreateServiceNervousSystem as ApiCreateServiceNervousSystem, ListNeurons, + ListNeuronsResponse, + }, proposal_validation::validate_proposal_title, }; use ic_nns_governance_init::GovernanceCanisterInitPayloadBuilder; @@ -2329,13 +2332,17 @@ fn test_get_neuron_when_private_neuron_enforcement_disabled() { assert_eq!(neuron_info.visibility, None); assert_eq!(full_neuron.visibility, None); + // This conversion will not be needed if/when we get rid of the definition + // of NeuronInfo from governance.proto. + let neuron_info = api::NeuronInfo::from(neuron_info); + assert_eq!( list_neurons_response, ListNeuronsResponse { neuron_infos: hashmap! { 1 => neuron_info, }, - full_neurons: vec![full_neuron], + full_neurons: vec![api::Neuron::from(full_neuron)], }, ); } @@ -2384,13 +2391,17 @@ fn test_get_neuron_when_private_neuron_enforcement_enabled() { assert_eq!(neuron_info.visibility, Some(Visibility::Private as i32)); assert_eq!(full_neuron.visibility, Some(Visibility::Private as i32)); + // This conversion will not be needed if/when we get rid of the definition + // of NeuronInfo from governance.proto. + let neuron_info = api::NeuronInfo::from(neuron_info); + assert_eq!( list_neurons_response, ListNeuronsResponse { neuron_infos: hashmap! { 1 => neuron_info, }, - full_neurons: vec![full_neuron], + full_neurons: vec![api::Neuron::from(full_neuron)], }, ); } @@ -10816,6 +10827,10 @@ fn test_include_public_neurons_in_full_neurons() { } } + let expected_full_neurons = expected_full_neurons + .into_iter() + .map(api::Neuron::from) + .collect::>(); assert_eq!(list_neurons_response.full_neurons, expected_full_neurons); } @@ -14756,7 +14771,7 @@ fn test_neuron_info_private_enforcement() { let hot_key = PrincipalId::new_user_test_id(random.gen()); let proposal_id = random.gen(); - let recent_ballots = vec![BallotInfo { + let recent_ballots = vec![api::BallotInfo { proposal_id: Some(ProposalId { id: proposal_id }), vote: Vote::Yes as i32, }]; @@ -14767,7 +14782,11 @@ fn test_neuron_info_private_enforcement() { let base_neuron = { let controller = Some(controller); let hot_keys = vec![hot_key]; - let recent_ballots = recent_ballots.clone(); + let recent_ballots = recent_ballots + .iter() + .cloned() + .map(BallotInfo::from) + .collect(); let dissolve_state = Some(DissolveState::DissolveDelaySeconds(random.gen())); let cached_neuron_stake_e8s = random.gen(); @@ -14838,8 +14857,16 @@ fn test_neuron_info_private_enforcement() { let random_principal_id = PrincipalId::new_user_test_id(617_157_922); // Step 2.1: Call get_neuron_info. - let get_neuron_info = - |requester| governance.get_neuron_info(&neuron_id, requester).unwrap(); + let get_neuron_info = |requester| { + let neuron_info = governance.get_neuron_info(&neuron_id, requester).unwrap(); + + // We would like to get rid of the definition of NeuronInfo + // in governance.proto. When we get rid of that definition, + // this conversion will no longer be needed. Getting rid of + // the definition should be possible, since we do not store + // NeuronInfo in stable memory (AFAIK). + api::NeuronInfo::from(neuron_info) + }; let controller_get_result = get_neuron_info(controller); let hot_key_get_result = get_neuron_info(hot_key); let random_principal_get_result = get_neuron_info(random_principal_id); diff --git a/rs/rosetta-api/icp/BUILD.bazel b/rs/rosetta-api/icp/BUILD.bazel index 199d511bd7e..e1fa7c56348 100644 --- a/rs/rosetta-api/icp/BUILD.bazel +++ b/rs/rosetta-api/icp/BUILD.bazel @@ -74,7 +74,6 @@ DEV_DEPENDENCIES = [ "//rs/ledger_suite/icrc1", "//rs/ledger_suite/icrc1/test_utils", "//rs/ledger_suite/icrc1/tokens_u256", - "//rs/nns/governance", "//rs/nns/governance/init", "//rs/nns/handlers/root/impl:root", "//rs/nns/test_utils", diff --git a/rs/rosetta-api/icp/Cargo.toml b/rs/rosetta-api/icp/Cargo.toml index b0f4855169a..2f9d00b6d75 100644 --- a/rs/rosetta-api/icp/Cargo.toml +++ b/rs/rosetta-api/icp/Cargo.toml @@ -59,7 +59,6 @@ futures = { workspace = true } ic-base-types = { path = "../../types/base_types" } ic-crypto-ed25519 = { path = "../../crypto/ed25519" } ic-ledger-canister-blocks-synchronizer-test-utils = { path = "ledger_canister_blocks_synchronizer/test_utils" } -ic-nns-governance = { path = "../../nns/governance" } ic-rosetta-test-utils = { path = "test_utils" } ic-types = { path = "../../types/types" } proptest = { workspace = true } diff --git a/rs/rosetta-api/icp/tests/system_tests/common/utils.rs b/rs/rosetta-api/icp/tests/system_tests/common/utils.rs index 4dfcd549f52..4e39acd64f2 100644 --- a/rs/rosetta-api/icp/tests/system_tests/common/utils.rs +++ b/rs/rosetta-api/icp/tests/system_tests/common/utils.rs @@ -7,9 +7,9 @@ use ic_icp_rosetta_client::RosettaClient; use ic_ledger_core::block::BlockType; use ic_nns_constants::GOVERNANCE_CANISTER_ID; use ic_nns_constants::LEDGER_CANISTER_ID; -use ic_nns_governance::pb::v1::ListNeurons; -use ic_nns_governance::pb::v1::ListNeuronsResponse; use ic_nns_governance_api::pb::v1::GovernanceError; +use ic_nns_governance_api::pb::v1::ListNeurons; +use ic_nns_governance_api::pb::v1::ListNeuronsResponse; use ic_rosetta_api::convert::to_hash; use icp_ledger::GetBlocksArgs; use icp_ledger::QueryEncodedBlocksResponse; diff --git a/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs b/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs index 71d9d0a2d5a..7252733304b 100644 --- a/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs +++ b/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs @@ -15,8 +15,8 @@ use ic_icp_rosetta_client::{ RosettaStakeMaturityArgs, }; use ic_icrc1_test_utils::basic_identity_strategy; -use ic_nns_governance::pb::v1::neuron::DissolveState; -use ic_nns_governance::pb::v1::KnownNeuronData; +use ic_nns_governance_api::pb::v1::neuron::DissolveState; +use ic_nns_governance_api::pb::v1::KnownNeuronData; use ic_rosetta_api::ledger_client::list_known_neurons_response::ListKnownNeuronsResponse; use ic_rosetta_api::ledger_client::list_neurons_response::ListNeuronsResponse; use ic_rosetta_api::ledger_client::neuron_response::NeuronResponse; @@ -388,7 +388,7 @@ fn test_start_and_stop_neuron_dissolve() { let neuron = list_neurons(&agent).await.full_neurons[0].to_owned(); assert!( matches!( - neuron.dissolve_state.unwrap(), + neuron.dissolve_state.clone().unwrap(), DissolveState::WhenDissolvedTimestampSeconds(_) ), "Neuron should be in WhenDissolvedTimestampSeconds state, but is instead: {:?}", @@ -611,9 +611,9 @@ fn test_disburse_neuron() { // We now update the neuron so it is in state DISSOLVED let now = env.pocket_ic.get_time().await.duration_since(UNIX_EPOCH).unwrap().as_secs(); neuron.dissolve_state = Some(DissolveState::WhenDissolvedTimestampSeconds(now - 1)); - update_neuron(&agent, neuron.into()).await; + update_neuron(&agent, neuron).await; - match list_neurons(&agent).await.full_neurons[0].dissolve_state.unwrap() { + match list_neurons(&agent).await.full_neurons[0].dissolve_state.clone().unwrap() { DissolveState::WhenDissolvedTimestampSeconds (d) => { // The neuron should now be in DISSOLVED state assert!(d