Skip to content

Commit

Permalink
refactor(nns-governance): Delete *_voting_power fields from governanc…
Browse files Browse the repository at this point in the history 
…e.proto. (#3643)

# Rationale/Motivation

These values are derived. Hence, they do not need to be and should not
be stored.

# Overview

Got rid of `into_proto`.

Reintroduced `pb::Neuron::from`.

Added `into_api`.

Depending on the situation, `into_proto` calls were replaced with either
`pb::Neuron::from` or `into_api`. The difference is that the former is
easier (requires less inputs), but the latter provides
(deciding|potential)_voting_power.

# References

Closes [NNS1-3500].

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

[NNS1-3500]:
https://dfinity.atlassian.net/browse/NNS1-3500?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
  • Loading branch information
@daniel-wong-dfinity-org
daniel-wong-dfinity-org authored Jan 30, 2025
1 parent 2eae439 commit 056a8e0
Show file tree
Hide file tree
Showing 17 changed files with 352 additions and 344 deletions.
6 changes: 3 additions & 3 deletions rs/nns/governance/canbench/canbench_results.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ benches:
scopes: {}
compute_ballots_for_new_proposal_with_stable_neurons:
total:
instructions: 2223431
instructions: 2230000
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand Down Expand Up @@ -85,7 +85,7 @@ benches:
scopes: {}
list_neurons_heap:
total:
instructions: 4988540
instructions: 4950000
heap_increase: 9
stable_memory_increase: 0
scopes: {}
Expand All @@ -109,7 +109,7 @@ benches:
scopes: {}
list_proposals:
total:
instructions: 128658
instructions: 126040
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand Down
5 changes: 3 additions & 2 deletions rs/nns/governance/canister/canister.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -711,10 +711,12 @@ async fn manage_neuron(_manage_neuron: ManageNeuronRequest) -> ManageNeuronRespo
#[cfg(feature = "test")]
#[update]
/// Internal method for calling update_neuron.
///
/// *_voting_power fields are ignored, because the value in those fields is derived.
fn update_neuron(neuron: Neuron) -> Option<GovernanceError> {
debug_log("update_neuron");
governance_mut()
.update_neuron(gov_pb::Neuron::from(neuron))
.update_neuron(neuron)
.err()
.map(GovernanceError::from)
}
Expand All @@ -735,7 +737,6 @@ fn get_full_neuron_by_id_or_subaccount(
&(gov_pb::manage_neuron::NeuronIdOrSubaccount::from(by)),
&caller(),
)
.map(Neuron::from)
.map_err(GovernanceError::from)
}

Expand Down
59 changes: 4 additions & 55 deletions rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,62 +412,11 @@ message Neuron {
// to overwrite next.
optional uint32 recent_ballots_next_entry_index = 25;

// The amount of "sway" this neuron has when voting on proposals.
//
// When a proposal is created, each eligible neuron gets a "blank" ballot. The
// amount of voting power in that ballot is set to the neuron's deciding
// voting power at the time of proposal creation. There are two ways that a
// proposal can become decided:
//
// 1. Early: Either more than half of the total voting power in the ballots
// votes in favor (then the proposal is approved), or at least half of the
// votal voting power in the ballots votes against (then, the proposal is
// rejected).
//
// 2. The proposal's voting deadline is reached. At that point, if there is
// more voting power in favor than against, and at least 3% of the total
// voting power voted in favor, then the proposal is approved. Otherwise, it
// is rejected.
//
// If a neuron regularly refreshes its voting power, this has the same value
// as potential_voting_power. Actions that cause a refresh are as follows:
//
// 1. voting directly (not via following)
// 2. set following
// 3. refresh voting power
//
// (All of these actions are performed via the manage_neuron method.)
//
// However, if a neuron has not refreshed in a "long" time, this will be less
// than potential voting power. See VotingPowerEconomics. As a further result
// of less deciding voting power, not only does it have less influence on the
// outcome of proposals, the neuron receives less voting rewards (when it
// votes indirectly via following).
//
// For details, see https://dashboard.internetcomputer.org/proposal/132411.
//
// Per NNS policy, this is opt. Nevertheless, it will never be null.
optional uint64 deciding_voting_power = 26;
reserved "deciding_voting_power";
reserved 26;

// The amount of "sway" this neuron can have if it refreshes its voting power
// frequently enough.
//
// Unlike deciding_voting_power, this does NOT take refreshing into account.
// Rather, this only takes three factors into account:
//
// 1. (Net) staked amount - This is the "base" of a neuron's voting power.
// This primarily consists of the neuron's ICP balance.
//
// 2. Age - Neurons with more age have more voting power (all else being
// equal).
//
// 3. Dissolve delay - Neurons with longer dissolve delay have more voting
// power (all else being equal). Neurons with a dissolve delay of less
// than six months are not eligible to vote. Therefore, such neurons
// are considered to have 0 voting power.
//
// Per NNS policy, this is opt. Nevertheless, it will never be null.
optional uint64 potential_voting_power = 27;
reserved "potential_voting_power";
reserved 27;
}

// Subset of Neuron that has no collections or big fields that might not exist in most neurons, and
Expand Down
57 changes: 0 additions & 57 deletions rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,63 +237,6 @@ pub struct Neuron {
/// to overwrite next.
#[prost(uint32, optional, tag = "25")]
pub recent_ballots_next_entry_index: ::core::option::Option<u32>,
/// The amount of "sway" this neuron has when voting on proposals.
///
/// When a proposal is created, each eligible neuron gets a "blank" ballot. The
/// amount of voting power in that ballot is set to the neuron's deciding
/// voting power at the time of proposal creation. There are two ways that a
/// proposal can become decided:
///
/// 1. Early: Either more than half of the total voting power in the ballots
/// votes in favor (then the proposal is approved), or at least half of the
/// votal voting power in the ballots votes against (then, the proposal is
/// rejected).
///
/// 2. The proposal's voting deadline is reached. At that point, if there is
/// more voting power in favor than against, and at least 3% of the total
/// voting power voted in favor, then the proposal is approved. Otherwise, it
/// is rejected.
///
/// If a neuron regularly refreshes its voting power, this has the same value
/// as potential_voting_power. Actions that cause a refresh are as follows:
///
/// 1. voting directly (not via following)
/// 2. set following
/// 3. refresh voting power
///
/// (All of these actions are performed via the manage_neuron method.)
///
/// However, if a neuron has not refreshed in a "long" time, this will be less
/// than potential voting power. See VotingPowerEconomics. As a further result
/// of less deciding voting power, not only does it have less influence on the
/// outcome of proposals, the neuron receives less voting rewards (when it
/// votes indirectly via following).
///
/// For details, see <https://dashboard.internetcomputer.org/proposal/132411.>
///
/// Per NNS policy, this is opt. Nevertheless, it will never be null.
#[prost(uint64, optional, tag = "26")]
pub deciding_voting_power: ::core::option::Option<u64>,
/// The amount of "sway" this neuron can have if it refreshes its voting power
/// frequently enough.
///
/// Unlike deciding_voting_power, this does NOT take refreshing into account.
/// Rather, this only takes three factors into account:
///
/// 1. (Net) staked amount - This is the "base" of a neuron's voting power.
/// This primarily consists of the neuron's ICP balance.
///
/// 2. Age - Neurons with more age have more voting power (all else being
/// equal).
///
/// 3. Dissolve delay - Neurons with longer dissolve delay have more voting
/// power (all else being equal). Neurons with a dissolve delay of less
/// than six months are not eligible to vote. Therefore, such neurons
/// are considered to have 0 voting power.
///
/// Per NNS policy, this is opt. Nevertheless, it will never be null.
#[prost(uint64, optional, tag = "27")]
pub potential_voting_power: ::core::option::Option<u64>,
/// At any time, at most one of `when_dissolved` and
/// `dissolve_delay` are specified.
///
Expand Down
30 changes: 13 additions & 17 deletions rs/nns/governance/src/governance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ use crate::{
CreateServiceNervousSystem, ExecuteNnsFunction, GetNeuronsFundAuditInfoRequest,
GetNeuronsFundAuditInfoResponse, Governance as GovernanceProto, GovernanceError,
InstallCode, KnownNeuron, ListKnownNeuronsResponse, ListProposalInfo, ManageNeuron,
MonthlyNodeProviderRewards, Motion, NetworkEconomics, Neuron as NeuronProto,
NeuronState, NeuronsFundAuditInfo, NeuronsFundData,
MonthlyNodeProviderRewards, Motion, NetworkEconomics, NeuronState,
NeuronsFundAuditInfo, NeuronsFundData,
NeuronsFundEconomics as NeuronsFundNetworkEconomicsPb,
NeuronsFundParticipation as NeuronsFundParticipationPb,
NeuronsFundSnapshot as NeuronsFundSnapshotPb, NnsFunction, NodeProvider, Proposal,
Expand Down Expand Up @@ -2082,7 +2082,7 @@ impl Governance {
/// Preconditions:
/// - the given `neuron` already exists in `self.neuron_store.neurons`
#[cfg(feature = "test")]
pub fn update_neuron(&mut self, neuron: NeuronProto) -> Result<(), GovernanceError> {
pub fn update_neuron(&mut self, neuron: api::Neuron) -> Result<(), GovernanceError> {
// Converting from API type to internal type.
let new_neuron = Neuron::try_from(neuron).expect("Neuron must be valid");

Expand Down Expand Up @@ -2306,13 +2306,7 @@ impl Governance {
&& neuron.visibility() == Some(Visibility::Public)
);
if let_caller_read_full_neuron {
let mut proto = neuron.clone().into_proto(self.voting_power_economics(), now);
// We get the recent_ballots from the neuron itself, because
// we are using a circular buffer to store them. This solution is not ideal, but
// 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(api::Neuron::from(proto));
full_neurons.push(neuron.clone().into_api(now, self.voting_power_economics()));
}
});
}
Expand Down Expand Up @@ -3674,7 +3668,7 @@ impl Governance {
&self,
by: &NeuronIdOrSubaccount,
caller: &PrincipalId,
) -> Result<NeuronProto, GovernanceError> {
) -> Result<api::Neuron, GovernanceError> {
let neuron_id = self.find_neuron_id(by)?;
self.get_full_neuron(&neuron_id, caller)
}
Expand All @@ -3688,13 +3682,15 @@ impl Governance {
&self,
id: &NeuronId,
caller: &PrincipalId,
) -> Result<NeuronProto, GovernanceError> {
let now_seconds = self.env.now();

self.neuron_store
) -> Result<api::Neuron, GovernanceError> {
let native_neuron = self
.neuron_store
.get_full_neuron(*id, *caller)
.map(|neuron| neuron.into_proto(self.voting_power_economics(), now_seconds))
.map_err(GovernanceError::from)
.map_err(GovernanceError::from)?;

let now_seconds = self.env.now();
let voting_power_economics = self.voting_power_economics();
Ok(native_neuron.into_api(now_seconds, voting_power_economics))
}

// Returns the set of currently registered node providers.
Expand Down
22 changes: 10 additions & 12 deletions rs/nns/governance/src/governance/benches.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
install_code::CanisterInstallMode, neuron::Followees, proposal::Action, Ballot, BallotInfo,
CreateServiceNervousSystem, ExecuteNnsFunction, Governance as GovernanceProto, InstallCode,
KnownNeuron, ListProposalInfo, NetworkEconomics, Neuron as NeuronProto, NnsFunction,
Proposal, ProposalData, Topic, Vote, VotingPowerEconomics,
Proposal, ProposalData, Topic, Vote,
},
temporarily_disable_allow_active_neurons_in_stable_memory,
temporarily_disable_migrate_active_neurons_to_stable_memory,
Expand Down Expand Up @@ -494,13 +494,12 @@ fn compute_ballots_for_new_proposal_with_stable_neurons() -> BenchResult {
.map(|id| {
(
id,
make_neuron(
NeuronProto::from(make_neuron(
id,
PrincipalId::new_user_test_id(id),
1_000_000_000,
hashmap! {}, // get the default followees
)
.into_proto(&VotingPowerEconomics::DEFAULT, now_seconds),
)),
)
})
.collect::<BTreeMap<u64, NeuronProto>>();
Expand Down Expand Up @@ -535,13 +534,12 @@ fn list_neurons_benchmark() -> BenchResult {
let neurons = (0..100)
.map(|id| {
(id, {
let mut neuron: NeuronProto = make_neuron(
let mut neuron = NeuronProto::from(make_neuron(
id,
PrincipalId::new_user_test_id(id),
1_000_000_000,
hashmap! {}, // get the default followees
)
.into_proto(&VotingPowerEconomics::DEFAULT, 123_456_789);
));
neuron.hot_keys = vec![PrincipalId::new_user_test_id(1)];
neuron
})
Expand Down Expand Up @@ -606,15 +604,15 @@ fn create_service_nervous_system_action_with_large_payload() -> CreateServiceNer
fn list_proposals_benchmark() -> BenchResult {
let neurons = (1..=100)
.map(|id| {
(id, {
make_neuron(
(
id,
NeuronProto::from(make_neuron(
id,
PrincipalId::new_user_test_id(id),
1_000_000_000,
hashmap! {}, // get the default followees
)
.into_proto(&VotingPowerEconomics::DEFAULT, 123_456_789)
})
)),
)
})
.collect::<BTreeMap<u64, NeuronProto>>();

Expand Down
14 changes: 3 additions & 11 deletions rs/nns/governance/src/governance/merge_neurons.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
};
use ic_base_types::PrincipalId;
use ic_nns_common::pb::v1::NeuronId;
use ic_nns_governance_api::pb::v1::{self as api, manage_neuron_response::MergeResponse};
use ic_nns_governance_api::pb::v1::manage_neuron_response::MergeResponse;
use icp_ledger::Subaccount;
use std::collections::BTreeMap;

Expand Down Expand Up @@ -349,16 +349,8 @@ pub fn build_merge_neurons_response(
now_seconds: u64,
requester: PrincipalId,
) -> MergeResponse {
let source_neuron = Some(api::Neuron::from(
source
.clone()
.into_proto(voting_power_economics, now_seconds),
));
let target_neuron = Some(api::Neuron::from(
target
.clone()
.into_proto(voting_power_economics, now_seconds),
));
let source_neuron = Some(source.clone().into_api(now_seconds, voting_power_economics));
let target_neuron = Some(target.clone().into_api(now_seconds, voting_power_economics));

let source_neuron_info =
Some(source.get_neuron_info(voting_power_economics, now_seconds, requester));
Expand Down
22 changes: 10 additions & 12 deletions rs/nns/governance/src/governance/tests/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1517,25 +1517,25 @@ fn topic_min_max_test() {
#[cfg(feature = "test")]
#[test]
fn test_update_neuron_errors_out_expectedly() {
fn build_neuron_proto(account: Vec<u8>) -> NeuronProto {
NeuronProto {
fn new_neuron(account: Vec<u8>) -> api::Neuron {
api::Neuron {
account,
id: Some(NeuronId { id: 1 }),
controller: Some(PrincipalId::new_user_test_id(1)),
followees: hashmap! {
2 => Followees {
2 => api::neuron::Followees {
followees: vec![NeuronId { id : 3}]
}
},
aging_since_timestamp_seconds: 1,
dissolve_state: Some(DissolveState::DissolveDelaySeconds(42)),
dissolve_state: Some(api::neuron::DissolveState::DissolveDelaySeconds(42)),
..Default::default()
}
}

let neuron1_subaccount_blob = vec![1; 32];
let neuron1_subaccount = Subaccount::try_from(neuron1_subaccount_blob.as_slice()).unwrap();
let neuron1 = build_neuron_proto(neuron1_subaccount_blob.clone());
let neuron1 = NeuronProto::from(new_neuron(neuron1_subaccount_blob.clone()));
let neurons = btreemap! { 1 => neuron1 };
let governance_proto = GovernanceProto {
neurons,
Expand All @@ -1549,7 +1549,7 @@ fn test_update_neuron_errors_out_expectedly() {
);

assert_eq!(
governance.update_neuron(build_neuron_proto(vec![0; 32])),
governance.update_neuron(new_neuron(vec![0; 32])),
Err(GovernanceError::new_with_message(
ErrorType::PreconditionFailed,
format!(
Expand All @@ -1569,7 +1569,7 @@ fn test_compute_ballots_for_new_proposal() {
let controller = PrincipalId::new_user_test_id(i);
let d = i / 10_u64.pow(i.ilog10());

NeuronBuilder::new(
let neuron = NeuronBuilder::new(
NeuronId { id: i },
Subaccount::try_from([d as u8; 32].as_slice()).unwrap(),
controller,
Expand All @@ -1580,11 +1580,9 @@ fn test_compute_ballots_for_new_proposal() {
CREATED_TIMESTAMP_SECONDS,
)
.with_cached_neuron_stake_e8s(i * E8)
.build()
.into_proto(
&VotingPowerEconomics::DEFAULT,
CREATED_TIMESTAMP_SECONDS + 999,
)
.build();

NeuronProto::from(neuron)
}

let mut neuron_10 = new_neuron(10);
Expand Down
Loading

0 comments on commit 056a8e0

Please sign in to comment.