Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nns): Add neuron_minimum_dissolve_delay_to_vote_seconds to VotingPowerEconomics #4180

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2294,6 +2294,17 @@ pub struct VotingPowerEconomics {
/// Initially, set to 1/12 years.
#[prost(uint64, optional, tag = "2")]
pub clear_following_after_seconds: ::core::option::Option<u64>,

/// The minimum dissolve delay a neuron must have in order to be eligible to vote or
/// make proposals.
///
/// Neurons with a dissolve delay lower than this threshold will not have
/// voting power, even if they are otherwise active.
///
/// This value is an essential part of the staking mechanism, promoting
/// long-term alignment with the network's governance.
#[prost(uint64, optional, tag = "3")]
pub neuron_minimum_dissolve_delay_to_vote_seconds: ::core::option::Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my main question is - if we're exposing this as an API parameter, does that mean that the value is actually being used to see if a neuron can vote? It looks like not yet. Is there a followup PR planned to actually use the value on VotingPowerEconomics to determine neuron voting eligibility?

}

/// The thresholds specify the shape of the ideal matching function used by the Neurons' Fund to
Expand Down
4 changes: 4 additions & 0 deletions rs/nns/governance/api/src/pb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,12 @@ impl VotingPowerEconomics {
Self::DEFAULT_START_REDUCING_VOTING_POWER_AFTER_SECONDS,
),
clear_following_after_seconds: Some(Self::DEFAULT_CLEAR_FOLLOWING_AFTER_SECONDS),
neuron_minimum_dissolve_delay_to_vote_seconds: Some(
Self::MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS,
),
};

pub const MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS: u64 = 6 * ONE_MONTH_SECONDS;
pub const DEFAULT_START_REDUCING_VOTING_POWER_AFTER_SECONDS: u64 = 6 * ONE_MONTH_SECONDS;
pub const DEFAULT_CLEAR_FOLLOWING_AFTER_SECONDS: u64 = ONE_MONTH_SECONDS;

Expand Down
9 changes: 9 additions & 0 deletions rs/nns/governance/canister/governance.did
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,15 @@ type VotingPowerEconomics = record {
//
// Initially, set to 1/12 years.
clear_following_after_seconds : opt nat64;

// The minimum dissolve delay a neuron must have in order to be eligible to vote.
//
// Neurons with a dissolve delay lower than this threshold will not have
// voting power, even if they are otherwise active.
//
// This value is an essential part of the staking mechanism, promoting
// long-term alignment with the network's governance.
neuron_minimum_dissolve_delay_to_vote_seconds : opt nat64;
};

type Neuron = record {
Expand Down
8 changes: 8 additions & 0 deletions rs/nns/governance/canister/governance_test.did
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,14 @@ type VotingPowerEconomics = record {
//
// Initially, set to 1/12 years.
clear_following_after_seconds : opt nat64;
// The minimum dissolve delay a neuron must have in order to be eligible to vote.
//
// Neurons with a dissolve delay lower than this threshold will not have
// voting power, even if they are otherwise active.
//
// This value is an essential part of the staking mechanism, promoting
// long-term alignment with the network's governance.
neuron_minimum_dissolve_delay_to_vote_seconds : opt nat64;
};

type Neuron = record {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1672,6 +1672,15 @@ message VotingPowerEconomics {
//
// Initially, set to 1/12 years.
optional uint64 clear_following_after_seconds = 2;

// The minimum dissolve delay a neuron must have in order to be eligible to vote.
//
// Neurons with a dissolve delay lower than this threshold will not have
// voting power, even if they are otherwise active.
//
// This value is an essential part of the staking mechanism, promoting
// long-term alignment with the network's governance.
optional uint64 neuron_minimum_dissolve_delay_to_vote_seconds = 3;
}

// The thresholds specify the shape of the ideal matching function used by the Neurons' Fund to
Expand Down
9 changes: 9 additions & 0 deletions rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2124,6 +2124,15 @@ pub struct VotingPowerEconomics {
/// Initially, set to 1/12 years.
#[prost(uint64, optional, tag = "2")]
pub clear_following_after_seconds: ::core::option::Option<u64>,
/// The minimum dissolve delay a neuron must have in order to be eligible to vote.
///
/// Neurons with a dissolve delay lower than this threshold will not have
/// voting power, even if they are otherwise active.
///
/// This value is an essential part of the staking mechanism, promoting
/// long-term alignment with the network's governance.
#[prost(uint64, optional, tag = "3")]
pub neuron_minimum_dissolve_delay_to_vote_seconds: ::core::option::Option<u64>,
}
/// The thresholds specify the shape of the ideal matching function used by the Neurons' Fund to
/// determine how much to contribute for a given direct participation amount. Note that the actual
Expand Down
1 change: 1 addition & 0 deletions rs/nns/governance/src/governance/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1524,6 +1524,7 @@ fn test_deciding_voting_power_adjustment_factor() {
let voting_power_economics = VotingPowerEconomics {
start_reducing_voting_power_after_seconds: Some(60),
clear_following_after_seconds: Some(30),
neuron_minimum_dissolve_delay_to_vote_seconds: Some(60),
};

let deciding_voting_power = |seconds_since_refresh| {
Expand Down
27 changes: 27 additions & 0 deletions rs/nns/governance/src/network_economics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use ic_nervous_system_linear_map::LinearMap;
use ic_nervous_system_proto::pb::v1::{Decimal as DecimalProto, Percentage};
use icp_ledger::DEFAULT_TRANSFER_FEE;
use rust_decimal::Decimal;
use std::ops::RangeInclusive;
use std::time::Duration;

impl NetworkEconomics {
Expand Down Expand Up @@ -273,8 +274,16 @@ impl VotingPowerEconomics {
Self::DEFAULT_START_REDUCING_VOTING_POWER_AFTER_SECONDS,
),
clear_following_after_seconds: Some(Self::DEFAULT_CLEAR_FOLLOWING_AFTER_SECONDS),
neuron_minimum_dissolve_delay_to_vote_seconds: Some(
Self::MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS,
),
};

// [MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS] represents the default time threshold (in seconds)
// after which voting power begins to decrease in the network economics configuration. This is a preset
// value for the system, but it should be updated to align with [MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS].
pub const MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS: u64 =
crate::governance::MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS;
pub const DEFAULT_START_REDUCING_VOTING_POWER_AFTER_SECONDS: u64 = 6 * ONE_MONTH_SECONDS;
pub const DEFAULT_CLEAR_FOLLOWING_AFTER_SECONDS: u64 = ONE_MONTH_SECONDS;

Expand Down Expand Up @@ -351,6 +360,21 @@ impl VotingPowerEconomics {
defects.push("clear_following_after_seconds must be set.".to_string());
}

if let Some(delay) = self.neuron_minimum_dissolve_delay_to_vote_seconds {
pub const MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS_BOUNDS: RangeInclusive<u64> =
(3 * ONE_MONTH_SECONDS)..=(6 * ONE_MONTH_SECONDS);

if !MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS_BOUNDS.contains(&delay) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this - why not just do >= and <= instead of constructing a range and seeing if the value is inside of it?

let defect = format!(
"neuron_minimum_dissolve_delay_to_vote_seconds ({:?}) must be between three and six months.",
self.neuron_minimum_dissolve_delay_to_vote_seconds
);
defects.push(defect);
}
} else {
defects.push("neuron_minimum_dissolve_delay_to_vote_seconds must be set.".to_string());
}

if !defects.is_empty() {
return Err(defects);
}
Expand Down Expand Up @@ -522,6 +546,9 @@ impl InheritFrom for VotingPowerEconomics {
clear_following_after_seconds: self
.clear_following_after_seconds
.inherit_from(&base.clear_following_after_seconds),
neuron_minimum_dissolve_delay_to_vote_seconds: self
.neuron_minimum_dissolve_delay_to_vote_seconds
.inherit_from(&base.neuron_minimum_dissolve_delay_to_vote_seconds),
}
}
}
Expand Down
57 changes: 57 additions & 0 deletions rs/nns/governance/src/network_economics_tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::*;
use crate::governance::MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS;
use ic_nervous_system_proto::pb::v1::Decimal as ProtoDecimal;
use pretty_assertions::assert_eq;

Expand Down Expand Up @@ -87,3 +88,59 @@ fn test_inherit_from_recursively() {
fn test_network_economics_with_default_values_is_valid() {
assert_eq!(NetworkEconomics::with_default_values().validate(), Ok(()));
}

#[test]
fn test_neuron_minimum_dissolve_delay_to_vote_seconds_bounds() {
// Define constants for better readability and maintainability
const LOWER_BOUND_SECONDS: u64 = MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS / 2; // 3 months
const UPPER_BOUND_SECONDS: u64 = MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS; // 6 months
const DEFAULT_SECONDS: u64 = LOWER_BOUND_SECONDS; // Assuming default is the minimum

// Test cases: (delay in seconds, expected result)
let test_cases = [
(
None,
Err(vec!["neuron_minimum_dissolve_delay_to_vote_seconds must be set.".to_string()]),
),
(
Some(LOWER_BOUND_SECONDS - 1),
Err(vec![
format!("neuron_minimum_dissolve_delay_to_vote_seconds (Some({})) must be between three and six months.", LOWER_BOUND_SECONDS -1)
]),
),
(
Some(UPPER_BOUND_SECONDS + 1),
Err(vec![
format!("neuron_minimum_dissolve_delay_to_vote_seconds (Some({})) must be between three and six months.", UPPER_BOUND_SECONDS + 1)
]),
),
(
Some(DEFAULT_SECONDS),
Ok(()),
),
(
Some(LOWER_BOUND_SECONDS),
Ok(()),
),
(
Some(UPPER_BOUND_SECONDS),
Ok(()),
),
];

for (delay_seconds, expected_result) in test_cases {
let mut economics = NetworkEconomics::with_default_values();
economics
.voting_power_economics
.as_mut()
.expect("bug: voting_power_economics missing")
.neuron_minimum_dissolve_delay_to_vote_seconds = delay_seconds;

assert_eq!(
economics.validate(),
expected_result,
"Failed for delay: {:?}",
delay_seconds
);
}
}
1 change: 1 addition & 0 deletions rs/nns/governance/src/neuron_store/neuron_store_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,7 @@ fn test_prune_some_following_super_strict_voting_power_refresh() {
// supposed to be cleared.
start_reducing_voting_power_after_seconds: Some(42),
clear_following_after_seconds: Some(58),
neuron_minimum_dissolve_delay_to_vote_seconds: Some(42)
},
&mut neuron_store,
Bound::Unbounded, // Start new cycle.
Expand Down
4 changes: 4 additions & 0 deletions rs/nns/governance/src/pb/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1886,6 +1886,8 @@ impl From<pb_api::VotingPowerEconomics> for pb::VotingPowerEconomics {
start_reducing_voting_power_after_seconds: item
.start_reducing_voting_power_after_seconds,
clear_following_after_seconds: item.clear_following_after_seconds,
neuron_minimum_dissolve_delay_to_vote_seconds: item
.neuron_minimum_dissolve_delay_to_vote_seconds,
}
}
}
Expand All @@ -1896,6 +1898,8 @@ impl From<pb::VotingPowerEconomics> for pb_api::VotingPowerEconomics {
start_reducing_voting_power_after_seconds: item
.start_reducing_voting_power_after_seconds,
clear_following_after_seconds: item.clear_following_after_seconds,
neuron_minimum_dissolve_delay_to_vote_seconds: item
.neuron_minimum_dissolve_delay_to_vote_seconds,
}
}
}
Expand Down
22 changes: 22 additions & 0 deletions rs/nns/governance/tests/governance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8388,6 +8388,22 @@ fn test_network_economics_proposal() {
.unwrap()
.neuron_minimum_stake_e8s = 1234;

// Verify that neuron_minimum_dissolve_delay_to_vote_seconds is set to 6 months by default.
assert_eq!(
gov.heap_data
.economics
.as_ref()
.unwrap()
.voting_power_economics
.unwrap()
.neuron_minimum_dissolve_delay_to_vote_seconds,
Some(MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS),
);

// A value that differs from the default, useful for testing that the proposal has the intended effect.
let non_default_neuron_minimum_dissolve_delay_to_vote_seconds =
MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS / 2;

// Propose to change some, NetworkEconomics parameters.
let pid = match gov
.manage_neuron(
Expand All @@ -8404,6 +8420,9 @@ fn test_network_economics_proposal() {
voting_power_economics: Some(VotingPowerEconomics {
start_reducing_voting_power_after_seconds: Some(42),
clear_following_after_seconds: Some(4242),
neuron_minimum_dissolve_delay_to_vote_seconds: Some(
non_default_neuron_minimum_dissolve_delay_to_vote_seconds,
),
}),
..Default::default()
})),
Expand Down Expand Up @@ -8434,6 +8453,9 @@ fn test_network_economics_proposal() {
voting_power_economics: Some(VotingPowerEconomics {
start_reducing_voting_power_after_seconds: Some(42),
clear_following_after_seconds: Some(4242),
neuron_minimum_dissolve_delay_to_vote_seconds: Some(
non_default_neuron_minimum_dissolve_delay_to_vote_seconds
)
}),

// No changes to the rest.
Expand Down