Skip to content

Commit

Permalink
Merge branch 'jason/NNS1-2935-6' into 'master'
Browse files Browse the repository at this point in the history
refactor(nns): NNS1-2935 Refactor Neuron methods related to dissolve state and age into methods on the enum

The main goal is to make the Neuron type have a dissolve_state_and_age member, instead of dissolve_state and aging_since_timestamp_seconds. Therefore the methods that mostly operate on the existing 2 fields are refactored into methods on the dissolve_state_and_age enum. In the next MRs, more methods will be moved in a similar way, and after no Neuron methods directly use the existing 2 fields, those 2 fields can be replaced by a single member dissolve_state_and_age easily.

Other notes:

1. Some "deprecated\_..." methods are introduced to provide a temporary way of operating directly on those 2 fields, which will be no longer needed once https://gitlab.com/dfinity-lab/public/ic/-/merge_requests/18705 is merged.
2. The logic in increase_dissolve_delay is pretty complicated. Other than the tests added in this MR, the existing tests (in neuron/mod.rs) also helps make sure there is no regression. The new tests (against DissolveStateAndAge) are added based on the existing tests, and the existing tests will be deleted in the next MR. They are not deleted in this MR so that we can be more certain that the refactoring still pass those tests. 

See merge request dfinity-lab/public/ic!18732
  • Loading branch information
jasonz-dfinity committed Apr 24, 2024
2 parents fe27f12 + 0ea83fb commit 80e0363
Show file tree
Hide file tree
Showing 16 changed files with 713 additions and 133 deletions.
2 changes: 1 addition & 1 deletion rs/nns/governance/src/governance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::{
reassemble_governance_proto, split_governance_proto, HeapGovernanceData, XdrConversionRate,
},
migrations::maybe_run_migrations,
neuron::types::{DissolveStateAndAge, Neuron, NeuronBuilder},
neuron::{DissolveStateAndAge, Neuron, NeuronBuilder},
neuron_data_validation::{NeuronDataValidationSummary, NeuronDataValidator},
neuron_store::{NeuronMetrics, NeuronStore},
neurons_fund::{
Expand Down
2 changes: 1 addition & 1 deletion rs/nns/governance/src/governance/ledger_helper.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
governance::{governance_minting_account, neuron_subaccount},
neuron::types::Neuron,
neuron::Neuron,
neuron_store::NeuronStore,
pb::v1::{governance_error::ErrorType, GovernanceError},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl GovernanceNeuronMutation for MergeNeuronMutation {

let new_aging_since_timestamp_seconds = now.saturating_sub(new_age_seconds);
let aging_timestamp_seconds_delta = (new_aging_since_timestamp_seconds as i128)
.saturating_sub(target_neuron.aging_since_timestamp_seconds as i128);
.saturating_sub(target_neuron.deprecated_aging_since_timestamp_seconds() as i128);

Ok(btreemap! {
source_neuron.id() => NeuronDeltas {
Expand All @@ -106,7 +106,7 @@ impl GovernanceNeuronMutation for MergeNeuronMutation {
.neg(),
// Reset aging
aging_since_timestamp_seconds: if source_stake_less_transaction_fee_e8s > 0 {
now.saturating_sub(source_neuron.aging_since_timestamp_seconds) as i128
now.saturating_sub(source_neuron.deprecated_aging_since_timestamp_seconds()) as i128
} else {
0
},
Expand Down Expand Up @@ -212,12 +212,13 @@ impl GovernanceNeuronMutation for MergeNeuronMutation {
// here, since we do not change the dissolve state in any other
// way.
// let source_age_timestamp_seconds = source_neuron_mut.aging_since_timestamp_seconds;
if source_neuron_mut.aging_since_timestamp_seconds != u64::MAX {
source_neuron_mut.aging_since_timestamp_seconds =
if source_neuron_mut.deprecated_aging_since_timestamp_seconds() != u64::MAX {
source_neuron_mut.deprecated_set_aging_since_timestamp_seconds(
saturating_add_or_subtract_u64_i128(
source_neuron_mut.aging_since_timestamp_seconds,
source_neuron_mut.deprecated_aging_since_timestamp_seconds(),
source_delta_mut.aging_since_timestamp_seconds,
);
),
);
// Record that the delta was partially applied
source_delta_mut.aging_since_timestamp_seconds = 0;
}
Expand All @@ -241,11 +242,12 @@ impl GovernanceNeuronMutation for MergeNeuronMutation {
source_neuron_mut.cached_neuron_stake_e8s,
original_delta_cached_neuron_stake_e8s.saturating_neg(),
);
source_neuron_mut.aging_since_timestamp_seconds =
source_neuron_mut.deprecated_set_aging_since_timestamp_seconds(
saturating_add_or_subtract_u64_i128(
source_neuron_mut.aging_since_timestamp_seconds,
source_neuron_mut.deprecated_aging_since_timestamp_seconds(),
original_delta_aging_since_timestamp_seconds.saturating_neg(),
);
),
);
})
.expect("Expected the source neuron to exist");

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{governance::Governance, neuron::types::Neuron, pb::v1::GovernanceError};
use crate::{governance::Governance, neuron::Neuron, pb::v1::GovernanceError};
use async_trait::async_trait;
use ic_nns_common::pb::v1::NeuronId;
use std::{
Expand Down Expand Up @@ -236,10 +236,10 @@ impl NeuronDeltas {
neuron.staked_maturity_e8s_equivalent = Some(staked_maturity_e8s_equivalent_new_value);
}

neuron.aging_since_timestamp_seconds = saturating_add_or_subtract_u64_i128(
neuron.aging_since_timestamp_seconds,
neuron.deprecated_set_aging_since_timestamp_seconds(saturating_add_or_subtract_u64_i128(
neuron.deprecated_aging_since_timestamp_seconds(),
self.aging_since_timestamp_seconds,
);
));
}
}

Expand Down
8 changes: 4 additions & 4 deletions rs/nns/governance/src/governance/merge_neurons.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
combine_aged_stakes,
ledger_helper::{BurnNeuronFeesOperation, NeuronStakeTransferOperation},
},
neuron::types::{DissolveStateAndAge, Neuron},
neuron::{DissolveStateAndAge, Neuron},
neuron_store::NeuronStore,
pb::v1::{
governance_error::ErrorType, manage_neuron::Merge, manage_neuron::NeuronIdOrSubaccount,
Expand Down Expand Up @@ -68,15 +68,15 @@ impl MergeNeuronsEffect {

pub fn source_effect(&self) -> MergeNeuronsSourceEffect {
MergeNeuronsSourceEffect {
dissolve_state_and_age: self.source_neuron_dissolve_state_and_age.clone(),
dissolve_state_and_age: self.source_neuron_dissolve_state_and_age,
subtract_maturity: self.transfer_maturity_e8s,
subtract_staked_maturity: self.transfer_staked_maturity_e8s,
}
}

pub fn target_effect(&self) -> MergeNeuronsTargetEffect {
MergeNeuronsTargetEffect {
dissolve_state_and_age: self.target_neuron_dissolve_state_and_age.clone(),
dissolve_state_and_age: self.target_neuron_dissolve_state_and_age,
add_maturity: self.transfer_maturity_e8s,
add_staked_maturity: self.transfer_staked_maturity_e8s,
}
Expand Down Expand Up @@ -620,7 +620,7 @@ fn is_neuron_involved_with_open_proposals(
mod tests {
use super::*;
use crate::{
neuron::types::{DissolveStateAndAge, NeuronBuilder},
neuron::{DissolveStateAndAge, NeuronBuilder},
pb::v1::{neuron::Followees, proposal::Action, ManageNeuron, NeuronType, Proposal, Topic},
};
use assert_matches::assert_matches;
Expand Down
6 changes: 3 additions & 3 deletions rs/nns/governance/src/governance/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::*;
use crate::{
neuron::types::{DissolveStateAndAge, NeuronBuilder},
neuron::{DissolveStateAndAge, NeuronBuilder},
pb::v1::{
governance::{followers_map::Followers, FollowersMap},
Neuron as NeuronProto,
Expand Down Expand Up @@ -887,7 +887,7 @@ mod metrics_tests {
}

mod neuron_archiving_tests {
use crate::neuron::types::{DissolveStateAndAge, NeuronBuilder};
use crate::neuron::{DissolveStateAndAge, NeuronBuilder};
use ic_base_types::PrincipalId;
use ic_nns_common::pb::v1::NeuronId;
use icp_ledger::Subaccount;
Expand Down Expand Up @@ -1040,7 +1040,7 @@ mod neuron_archiving_tests {
mod cast_vote_and_cascade_follow {
use crate::{
governance::{Governance, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS},
neuron::types::{DissolveStateAndAge, Neuron, NeuronBuilder},
neuron::{DissolveStateAndAge, Neuron, NeuronBuilder},
neuron_store::NeuronStore,
pb::v1::{neuron::Followees, Ballot, Topic, Vote},
};
Expand Down
Loading

0 comments on commit 80e0363

Please sign in to comment.