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.

Sign up for GitHub

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): Refresh voting power. #2320

Merged
merged 3 commits into from
Oct 31, 2024
Merged

feat(nns): Refresh voting power. #2320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3dd4e7f
Refresh voting power when vote directly. Also, when change following.
daniel-wong-dfinity-org Oct 29, 2024
445eca4
Test that following causes voting power to refresh.
daniel-wong-dfinity-org Oct 30, 2024
820fc51
wording
daniel-wong-dfinity-org Oct 31, 2024
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
27 changes: 25 additions & 2 deletions rs/nns/governance/src/governance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5584,6 +5584,8 @@ impl Governance {
// Finally, add this proposal as an open proposal.
self.insert_proposal(proposal_num, proposal_data);

self.refresh_voting_power(proposer_id);

Ok(proposal_id)
}

Expand Down Expand Up @@ -5966,9 +5968,27 @@ impl Governance {

self.process_proposal(proposal_id.id);

self.refresh_voting_power(neuron_id);

Ok(())
}

fn refresh_voting_power(&mut self, neuron_id: &NeuronId) {
let now_seconds = self.env.now();

let result = self.with_neuron_mut(neuron_id, |neuron| {
neuron.refresh_voting_power(now_seconds);
});

if let Err(err) = result {
println!(
"{}WARNING: Tried to refresh the voting power of neuron {}, \
but was unable to find it: {}",
LOG_PREFIX, neuron_id.id, err,
);
}
}

/// Add or remove followees for this neuron for a specified topic.
///
/// If the list of followees is empty, remove the followees for
Expand Down Expand Up @@ -6031,17 +6051,20 @@ impl Governance {
)
})?;

let now_seconds = self.env.now();
self.with_neuron_mut(id, |neuron| {
if follow_request.followees.is_empty() {
neuron.followees.remove(&(topic as i32))
neuron.followees.remove(&(topic as i32));
} else {
neuron.followees.insert(
topic as i32,
Followees {
followees: follow_request.followees.clone(),
},
)
);
}

neuron.refresh_voting_power(now_seconds);
})?;

Ok(())
Expand Down
12 changes: 12 additions & 0 deletions rs/nns/governance/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,18 @@ mod subaccount_index;
/// The value of 10_000 follows the Candid recommendation.
const DEFAULT_SKIPPING_QUOTA: usize = 10_000;

/// Value: one second after midnight, 2024-11-05 (UTC).
///
/// How this value was chosen: This is around the earliest time when
/// "refreshing" a neuron's voting power might be released, (assuming the usual
/// NNS release cycle). Significantly different values could also work, but this
/// seems like a nice "neutral" value.
///
/// How this value is used: when a neuron does not have a value in the
/// voting_power_refreshed_timestamp_seconds field (because it was created before
/// this feature), we pretend as though this value is in that field.
pub const DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS: u64 = 1731628801;

daniel-wong-dfinity-org marked this conversation as resolved.
Show resolved Hide resolved
// TODO(NNS1-3248): Delete this once the feature has made it through the
// probation period. At that point, we will not need this "kill switch". We can
// leave this here indefinitely, but it will just be clutter after a modest
Expand Down
17 changes: 5 additions & 12 deletions rs/nns/governance/src/neuron/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::{
Neuron as NeuronProto, NeuronInfo, NeuronStakeTransfer, NeuronState, NeuronType, Topic,
Visibility, Vote,
},
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
};
use ic_base_types::PrincipalId;
use ic_cdk::println;
Expand All @@ -23,18 +24,6 @@ use ic_nns_common::pb::v1::{NeuronId, ProposalId};
use icp_ledger::Subaccount;
use std::collections::{BTreeSet, HashMap};

/// Value: one second after midnight, 2024-11-05 (UTC).
///
/// How this value was chosen: This is around the earliest time when
/// "refreshing" a neuron's voting power might be released, (assuming the usual
/// NNS release cycle). Significantly different values could also work, but this
/// seems like a nice "neutral" value.
///
/// How this value is used: when a neuron does not have a value in the
/// voting_power_refreshed_timestamp_seconds field (because it was created before
/// this feature), we pretend as though this value is in that field.
const DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS: u64 = 1731628801;

/// A neuron type internal to the governance crate. Currently, this type is identical to the
/// prost-generated Neuron type (except for derivations for prost). Gradually, this type will evolve
/// towards having all private fields while exposing methods for mutations, which allows it to hold
Expand Down Expand Up @@ -480,6 +469,10 @@ impl Neuron {
}
}

pub(crate) fn refresh_voting_power(&mut self, now_seconds: u64) {
self.voting_power_refreshed_timestamp_seconds = now_seconds;
}
daniel-wong-dfinity-org marked this conversation as resolved.
Show resolved Hide resolved

pub(crate) fn ready_to_unstake_maturity(&self, now_seconds: u64) -> bool {
self.state(now_seconds) == NeuronState::Dissolved
&& self.staked_maturity_e8s_equivalent.unwrap_or(0) > 0
Expand Down
129 changes: 99 additions & 30 deletions rs/nns/governance/tests/governance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ use ic_nns_governance::{
},
temporarily_disable_private_neuron_enforcement, temporarily_disable_set_visibility_proposals,
temporarily_enable_private_neuron_enforcement, temporarily_enable_set_visibility_proposals,
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
};
use ic_nns_governance_api::{
pb::v1::CreateServiceNervousSystem as ApiCreateServiceNervousSystem,
Expand Down Expand Up @@ -265,16 +266,30 @@ fn test_single_neuron_proposal_new() {
NNSStateChange::GovernanceProto(vec![
GovernanceChange::Neurons(vec![MapChange::Changed(
0,
vec![NeuronChange::RecentBallots(vec![VecChange::Added(
0,
vec![
BallotInfoChange::ProposalId(OptionChange::Different(
None,
Some(ProposalId { id: 1 }),
)),
BallotInfoChange::Vote(I32Change(0, 1)),
],
)])],
vec![
NeuronChange::RecentBallots(vec![VecChange::Added(
0,
vec![
BallotInfoChange::ProposalId(OptionChange::Different(
None,
Some(ProposalId { id: 1 }),
)),
BallotInfoChange::Vote(I32Change(0, 1)),
],
)]),
// Neuron's voting power was refreshed, because it directly votes (i.e. not
// as a result of following), but implicitly voted (by virtue of being the
// proposer). (Normally, the amount would increase, but for testing
// purposes, it is enough to show that the value changed.)
NeuronChange::VotingPowerRefreshedTimestampSeconds(
OptionChange::BothSome(
U64Change(
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
DEFAULT_TEST_START_TIMESTAMP_SECONDS,
),
),
),
],
)]),
GovernanceChange::Proposals(vec![MapChange::Added(
1,
Expand Down Expand Up @@ -932,6 +947,13 @@ async fn test_cascade_following_new() {
BallotInfoChange::Vote(I32Change(0, 1)),
],
)]),
// As in an earlier test, the proposer's voting power gets refreshed.
NeuronChange::VotingPowerRefreshedTimestampSeconds(OptionChange::BothSome(
U64Change(
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
DEFAULT_TEST_START_TIMESTAMP_SECONDS,
),
),),
],
)]),
GovernanceChange::Proposals(vec![MapChange::Added(
Expand Down Expand Up @@ -1064,16 +1086,25 @@ async fn test_cascade_following_new() {
Changed::Changed(vec![NNSStateChange::GovernanceProto(vec![
GovernanceChange::Neurons(vec![MapChange::Changed(
5,
vec![NeuronChange::RecentBallots(vec![VecChange::Added(
0,
vec![
BallotInfoChange::ProposalId(OptionChange::Different(
None,
Some(ProposalId { id: 1 }),
)),
BallotInfoChange::Vote(I32Change(0, 1)),
],
)])],
vec![
NeuronChange::RecentBallots(vec![VecChange::Added(
0,
vec![
BallotInfoChange::ProposalId(OptionChange::Different(
None,
Some(ProposalId { id: 1 }),
)),
BallotInfoChange::Vote(I32Change(0, 1)),
],
)]),
// Neuron's voting power was refreshed, because it voted directly.
NeuronChange::VotingPowerRefreshedTimestampSeconds(OptionChange::BothSome(
U64Change(
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
DEFAULT_TEST_START_TIMESTAMP_SECONDS,
),
),),
],
)]),
GovernanceChange::Proposals(vec![MapChange::Changed(
1,
Expand Down Expand Up @@ -1167,16 +1198,26 @@ async fn test_cascade_following_new() {
),
MapChange::Changed(
6,
vec![NeuronChange::RecentBallots(vec![VecChange::Added(
0,
vec![
BallotInfoChange::ProposalId(OptionChange::Different(
None,
Some(ProposalId { id: 1 }),
)),
BallotInfoChange::Vote(I32Change(0, 1)),
],
)])],
vec![
NeuronChange::RecentBallots(vec![VecChange::Added(
0,
vec![
BallotInfoChange::ProposalId(OptionChange::Different(
None,
Some(ProposalId { id: 1 }),
)),
BallotInfoChange::Vote(I32Change(0, 1)),
],
)]),
// Only this neuron voted directly; therefore, even though other neurons
// voted (via following) only this neuron's voting power was refreshed.
NeuronChange::VotingPowerRefreshedTimestampSeconds(OptionChange::BothSome(
U64Change(
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
DEFAULT_TEST_START_TIMESTAMP_SECONDS,
),
),),
],
),
]),
GovernanceChange::Proposals(vec![MapChange::Changed(
Expand Down Expand Up @@ -1699,6 +1740,18 @@ async fn test_all_follow_proposer() {
driver.get_fake_ledger(),
driver.get_fake_cmc(),
);

// Later, we'll inspect the same field again to verify that voting power
// refresh actually took place. This is just to make sure that the later
// value is different from this earlier one.
assert_eq!(
gov.with_neuron(&NeuronId { id: 5 }, |neuron| {
neuron.voting_power_refreshed_timestamp_seconds()
})
.unwrap(),
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
);

// Add following for 5 and 6 for 1.
gov.manage_neuron(
// Must match neuron 5's serialized_id.
Expand All @@ -1716,6 +1769,22 @@ async fn test_all_follow_proposer() {
.unwrap()
.panic_if_error("Manage neuron failed");

// Assert that neuron 5's voting power was refreshed. More concretely,
// verifying that neuron 5's voting_power_refreshed_timestamp_seconds
// changed from before. (Earlier, we saw that this field had a different
// value, to wit, DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS)
assert_ne!(
DEFAULT_TEST_START_TIMESTAMP_SECONDS,
DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS,
);
assert_eq!(
gov.with_neuron(&NeuronId { id: 5 }, |neuron| {
neuron.voting_power_refreshed_timestamp_seconds()
})
.unwrap(),
DEFAULT_TEST_START_TIMESTAMP_SECONDS,
);

gov.manage_neuron(
// Must match neuron 6's serialized_id.
&principal(6),
Expand Down