fix(nns): minor validation fix for dissolve state and age

rs/nns/governance/src/governance.rs

@@ -3441,6 +3441,17 @@ impl Governance {
             MAX_DISSOLVE_DELAY_SECONDS,
         );
 
+        let dissolve_state_and_age = if dissolve_delay_seconds > 0 {
+            DissolveStateAndAge::NotDissolving {
+                dissolve_delay_seconds,
+                aging_since_timestamp_seconds: created_timestamp_seconds,
+            }
+        } else {
+            DissolveStateAndAge::DissolvingOrDissolved {
+                when_dissolved_timestamp_seconds: created_timestamp_seconds,
+            }
+        };
+
         // Before we do the transfer, we need to save the neuron in the map
         // otherwise a trap after the transfer is successful but before this
         // method finishes would cause the funds to be lost.
@@ -3451,10 +3462,7 @@ impl Governance {
             child_nid,
             to_subaccount,
             child_controller,
-            DissolveStateAndAge::NotDissolving {
-                dissolve_delay_seconds,
-                aging_since_timestamp_seconds: created_timestamp_seconds,
-            },
+            dissolve_state_and_age,
             created_timestamp_seconds,
         )
         .with_followees(self.heap_data.default_followees.clone())
@@ -4205,15 +4213,24 @@ impl Governance {
                     reward_to_neuron.dissolve_delay_seconds,
                     MAX_DISSOLVE_DELAY_SECONDS,
                 );
+
+                let dissolve_state_and_age = if dissolve_delay_seconds > 0 {
+                    DissolveStateAndAge::NotDissolving {
+                        dissolve_delay_seconds,
+                        aging_since_timestamp_seconds: now,
+                    }
+                } else {
+                    DissolveStateAndAge::DissolvingOrDissolved {
+                        when_dissolved_timestamp_seconds: now,
+                    }
+                };
+
                 // Transfer successful.
                 let neuron = NeuronBuilder::new(
                     nid,
                     to_subaccount,
                     *np_principal,
-                    DissolveStateAndAge::NotDissolving {
-                        dissolve_delay_seconds,
-                        aging_since_timestamp_seconds: now,
-                    },
+                    dissolve_state_and_age,
                     now,
                 )
                 .with_followees(self.heap_data.default_followees.clone())

rs/nns/governance/src/neuron/dissolve_state_and_age.rs

@@ -1,4 +1,6 @@
-use crate::{governance::MAX_DISSOLVE_DELAY_SECONDS, pb::v1::NeuronState};
+use crate::{
+    governance::MAX_DISSOLVE_DELAY_SECONDS, neuron::StoredDissolveStateAndAge, pb::v1::NeuronState,
+};
 
 /// An enum to represent different combinations of a neurons dissolve_state and
 /// aging_since_timestamp_seconds. Currently, the back-and-forth conversions should make sure the
@@ -20,6 +22,22 @@ pub enum DissolveStateAndAge {
 }
 
 impl DissolveStateAndAge {
+    pub fn validate(self) -> Result<Self, String> {
+        let original = self;
+        let stored_dissolve_state_and_age = StoredDissolveStateAndAge::from(self);
+
+        let validated_dissolve_state_and_age = Self::try_from(stored_dissolve_state_and_age)
+            .map_err(|e| format!("Invalid dissolve state and age: {}", e))?;
+
+        if validated_dissolve_state_and_age != original {
+            return Err( format!(
+                    "Dissolve state and age is not valid, as it does not serialize/deserialize symmetrically. In: {:?}, Out: {:?}",
+                    original, validated_dissolve_state_and_age
+                ));
+        }
+
+        Ok(self)
+    }
     /// Returns the current state given the current time. Mainly for differentiating between
     /// dissolving and dissolved neurons.
     pub fn current_state(self, now_seconds: u64) -> NeuronState {

rs/nns/governance/src/neuron/types.rs

@@ -23,8 +23,7 @@ use ic_nns_common::pb::v1::{NeuronId, ProposalId};
 use icp_ledger::Subaccount;
 use std::collections::{BTreeSet, HashMap};
 
-/// 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
+/// A neuron type internal to the governance crate. Gradually, this type will evolve
 /// towards having all private fields while exposing methods for mutations, which allows it to hold
 /// invariants.
 #[derive(Clone, Debug)]
@@ -1681,9 +1680,13 @@ impl TryFrom<StoredDissolveStateAndAge> for DissolveStateAndAge {
                 when_dissolved_timestamp_seconds,
             ) => {
                 if aging_since_timestamp_seconds == u64::MAX {
-                    Ok(DissolveStateAndAge::DissolvingOrDissolved {
-                        when_dissolved_timestamp_seconds,
-                    })
+                    if when_dissolved_timestamp_seconds > 0 {
+                        Ok(DissolveStateAndAge::DissolvingOrDissolved {
+                            when_dissolved_timestamp_seconds,
+                        })
+                    } else {
+                        Err("Dissolve delay must be greater than 0".to_string())
+                    }
                 } else {
                     Err("Aging since timestamp must be u64::MAX for dissolving or dissolved neurons".to_string())
                 }

rs/nns/governance/src/neuron/types/tests.rs

@@ -6,6 +6,7 @@ use crate::{
 };
 use ic_cdk::println;
 
+use crate::pb::v1::neuron::DissolveState;
 use ic_nervous_system_common::{E8, ONE_YEAR_SECONDS};
 use ic_stable_structures::Storable;
 use icp_ledger::Subaccount;
@@ -88,6 +89,13 @@ fn test_dissolve_state_and_age_conversion_failure() {
             },
             "Dissolve delay must be greater than 0",
         ),
+        (
+            StoredDissolveStateAndAge {
+                dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(0)),
+                aging_since_timestamp_seconds: u64::MAX,
+            },
+            "Dissolve delay must be greater than 0",
+        ),
     ];
 
     for (invalid_stored_dissolve_state_and_age, error) in test_cases {

rs/nns/governance/src/neuron_store.rs

@@ -451,6 +451,8 @@ impl NeuronStore {
     pub fn add_neuron(&mut self, neuron: Neuron) -> Result<NeuronId, NeuronStoreError> {
         let neuron_id = neuron.id();
 
+        self.validate_neuron(&neuron)?;
+
         if self.contains(neuron_id) {
             return Err(NeuronStoreError::NeuronAlreadyExists(neuron_id));
         }
@@ -472,6 +474,17 @@ impl NeuronStore {
         Ok(neuron_id)
     }
 
+    fn validate_neuron(&self, neuron: &Neuron) -> Result<(), NeuronStoreError> {
+        neuron
+            .dissolve_state_and_age()
+            .validate()
+            .map_err(|reason| NeuronStoreError::InvalidData {
+                reason: format!("Neuron cannot be saved: {}", reason),
+            })?;
+
+        Ok(())
+    }
+
     fn add_neuron_to_indexes(&mut self, neuron: &Neuron) {
         if let Err(error) = with_stable_neuron_indexes_mut(|indexes| indexes.add_neuron(neuron)) {
             println!(
@@ -619,6 +632,8 @@ impl NeuronStore {
         };
         let is_neuron_changed = *old_neuron != new_neuron;
 
+        self.validate_neuron(&new_neuron)?;
+
         // Perform transition between 2 storage if necessary.
         //
         // Note: