Fix API ordering of recent ballots to match current behavior

rs/nns/governance/src/governance.rs

@@ -2313,7 +2313,13 @@ impl Governance {
                             && neuron.visibility() == Some(Visibility::Public)
                         );
                 if let_caller_read_full_neuron {
-                    full_neurons.push(NeuronProto::from(neuron.clone()));
+                    let mut proto = NeuronProto::from(neuron.clone());
+                    // 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.recent_ballots();
+                    full_neurons.push(proto);
                 }
             });
         }

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

@@ -77,8 +77,6 @@ pub struct Neuron {
     /// Map `Topic` to followees. The key is represented by an integer as
     /// Protobuf does not support enum keys in maps.
     pub followees: HashMap<i32, Followees>,
-    // TODO DO NOT MERGE - we will need to add an accessor for this field
-    // because the order is different than before...
     /// Information about how this neuron voted in the recent past. It
     /// only contains proposals that the neuron voted yes or no on.
     pub recent_ballots: Vec<BallotInfo>,
@@ -438,6 +436,21 @@ impl Neuron {
         std::cmp::min(ad_stake, u64::MAX as u128) as u64
     }
 
+    pub(crate) fn recent_ballots(&self) -> Vec<BallotInfo> {
+        if self.recent_ballots_next_entry_index.is_none() {
+            self.recent_ballots.clone()
+        } else {
+            let recent_ballots = &self.recent_ballots;
+            let index = self.recent_ballots_next_entry_index.unwrap();
+            recent_ballots[index..]
+                .iter()
+                .chain(recent_ballots[..index].iter())
+                .rev()
+                .cloned()
+                .collect()
+        }
+    }
+
     /// Given the specified `ballots`: determine how this neuron would
     /// vote on a proposal of `topic` based on which neurons this
     /// neuron follows on this topic (or on the default topic if this
@@ -498,6 +511,7 @@ impl Neuron {
     /// Register that this neuron has cast a ballot for a
     /// proposal. Don't include votes on "real time" topics (such as
     /// setting the ICP/SDR exchange rate).
+    /// TODO(NNS1-3479) delete this method after all neurons are migrated to stable memory
     pub(crate) fn register_recent_ballot(
         &mut self,
         topic: Topic,
@@ -892,7 +906,7 @@ impl Neuron {
             || self.visibility() == Some(Visibility::Public)
             || self.is_hotkey_or_controller(&requester);
         if show_full {
-            recent_ballots.append(&mut self.recent_ballots.clone());
+            recent_ballots.append(&mut self.recent_ballots());
             joined_community_fund_timestamp_seconds = self.joined_community_fund_timestamp_seconds;
         }
 
@@ -1115,7 +1129,6 @@ impl Neuron {
 impl From<Neuron> for NeuronProto {
     fn from(neuron: Neuron) -> Self {
         let visibility = neuron.visibility().map(|visibility| visibility as i32);
-
         let Neuron {
             id,
             subaccount,

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

@@ -721,3 +721,50 @@ fn test_conversion_from_old_ballot_storage_not_full() {
 
     assert_eq!(neuron.recent_ballots, expected_updated_ballots);
 }
+
+#[test]
+fn test_recent_ballots_accessor_pre_and_post_migration() {
+    let principal_id = PrincipalId::new_user_test_id(42);
+    let created_timestamp_seconds = 1729791574;
+
+    let recent_ballots: Vec<_> = (0..100)
+        .map(|id| BallotInfo {
+            proposal_id: Some(ProposalId { id }),
+            vote: Vote::Yes as i32,
+        })
+        .collect();
+
+    let mut neuron = NeuronBuilder::new(
+        NeuronId { id: 42 },
+        Subaccount::try_from(vec![42u8; 32].as_slice()).unwrap(),
+        principal_id,
+        DissolveStateAndAge::NotDissolving {
+            dissolve_delay_seconds: 12 * ONE_MONTH_SECONDS,
+            aging_since_timestamp_seconds: created_timestamp_seconds + 42,
+        },
+        created_timestamp_seconds, // created
+    )
+    .with_recent_ballots(recent_ballots.clone())
+    .build();
+    neuron.recent_ballots_next_entry_index = None;
+
+    assert_eq!(neuron.recent_ballots(), recent_ballots);
+
+    neuron.register_recent_ballot(Topic::NetworkEconomics, &ProposalId { id: 100 }, Vote::No);
+    assert_eq!(neuron.recent_ballots_next_entry_index, Some(1));
+
+    let expected_updated_ballots = {
+        let mut recent_ballots = recent_ballots.clone();
+        recent_ballots.insert(
+            0,
+            BallotInfo {
+                proposal_id: Some(ProposalId { id: 100 }),
+                vote: Vote::No as i32,
+            },
+        );
+        recent_ballots.pop();
+        recent_ballots
+    };
+
+    assert_eq!(neuron.recent_ballots(), expected_updated_ballots);
+}

rs/nns/governance/src/storage/neurons.rs

@@ -249,14 +249,14 @@ where
 
         let recent_entry_index = main_neuron_part.recent_ballots_next_entry_index;
 
-        let next_entry_index = if recent_entry_index.is_none() {
+        let next_entry_index = if let Some(recent_entry_index) = recent_entry_index {
+            recent_entry_index as usize
+        } else {
             let mut ballots = read_repeated_field(neuron_id, &self.recent_ballots_map);
             ballots.reverse();
             let next_entry = ballots.len() % MAX_NEURON_RECENT_BALLOTS;
             update_repeated_field(neuron_id, ballots, &mut self.recent_ballots_map);
             next_entry
-        } else {
-            recent_entry_index.unwrap() as usize
         };
         // We cannot error after this, or we risk creating some chaos with the ordering
         // of the ballots b/c of the migration code above.