Address PR feedback

rs/nervous_system/integration_tests/tests/advance_target_version.rs

@@ -1,3 +1,4 @@
+use ic_nervous_system_agent::sns::governance::GovernanceCanister;
 use ic_nervous_system_integration_tests::pocket_ic_helpers::sns;
 use ic_nervous_system_integration_tests::{
     create_service_nervous_system_builder::CreateServiceNervousSystemBuilder,
@@ -6,6 +7,7 @@ use ic_nervous_system_integration_tests::{
     },
 };
 use ic_nns_test_utils::sns_wasm::create_modified_sns_wasm;
+use ic_sns_governance::pb::v1::governance::Versions;
 use ic_sns_governance::{
     governance::UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS, pb::v1 as sns_pb,
 };
@@ -15,6 +17,34 @@ use pocket_ic::nonblocking::PocketIc;
 use pocket_ic::PocketIcBuilder;
 use std::time::Duration;
 
+/// Verifies that the upgrade journal has the expected entries.
+async fn assert_upgrade_journal(
+    pocket_ic: &PocketIc,
+    governance: GovernanceCanister,
+    expected_entries: &[sns_pb::upgrade_journal_entry::Event],
+) {
+    let sns_pb::GetUpgradeJournalResponse {
+        upgrade_journal, ..
+    } = sns::governance::get_upgrade_journal(pocket_ic, governance.canister_id).await;
+
+    let upgrade_journal = upgrade_journal.unwrap().entries;
+    assert_eq!(upgrade_journal.len(), expected_entries.len());
+
+    for (index, (actual, expected)) in upgrade_journal
+        .iter()
+        .zip(expected_entries.iter())
+        .enumerate()
+    {
+        assert!(actual.timestamp_seconds.is_some());
+        assert_eq!(
+            &actual.event,
+            &Some(expected.clone()),
+            "Upgrade journal entry at index {} does not match",
+            index
+        );
+    }
+}
+
 /// Advances time by up to `timeout_seconds` seconds and `timeout_seconds` tickets (1 tick = 1 second).
 /// Each tick, it observes the state using the provided `observe` function.
 /// If the observed state matches the `expected` state, it returns `Ok(())`.
@@ -96,20 +126,28 @@ async fn test_get_upgrade_journal() {
         sns
     };
 
-    // State A: right after SNS creation.
+    // Step 1: right after SNS creation.
+    let mut expected_upgrade_journal_entries = vec![];
     {
-        let sns_pb::GetUpgradeJournalResponse {
-            upgrade_steps,
-            response_timestamp_seconds,
-            ..
-        } = sns::governance::get_upgrade_journal(&pocket_ic, sns.governance.canister_id).await;
-        let upgrade_steps = upgrade_steps
-            .expect("upgrade_steps should be Some")
-            .versions;
-        assert_eq!(upgrade_steps, vec![initial_sns_version.clone()]);
-        assert_eq!(response_timestamp_seconds, Some(1620501459));
+        expected_upgrade_journal_entries.push(
+            sns_pb::upgrade_journal_entry::Event::UpgradeStepsRefreshed(
+                sns_pb::upgrade_journal_entry::UpgradeStepsRefreshed {
+                    upgrade_steps: Some(Versions {
+                        versions: vec![initial_sns_version.clone()],
+                    }),
+                },
+            ),
+        );
+
+        assert_upgrade_journal(
+            &pocket_ic,
+            sns.governance,
+            &expected_upgrade_journal_entries,
+        )
+        .await;
     }
 
+    // Step 1.1: wait for the upgrade steps to be refreshed.
     await_with_timeout(
         &pocket_ic,
         UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS,
@@ -125,7 +163,7 @@ async fn test_get_upgrade_journal() {
     .await
     .unwrap();
 
-    // Publish a new SNS version.
+    // Step 2: Publish new SNS versions.
     let (new_sns_version_1, new_sns_version_2) = {
         let (_, original_root_wasm) = deployed_sns_starting_info
             .get(&SnsCanisterType::Root)
@@ -156,11 +194,12 @@ async fn test_get_upgrade_journal() {
         };
 
         let sns_version = nns::sns_wasm::get_latest_sns_version(&pocket_ic).await;
-        assert_ne!(sns_version, initial_sns_version);
+        assert_ne!(sns_version, initial_sns_version.clone());
 
         (new_sns_version_1, new_sns_version_2)
     };
 
+    // Step 2.1: wait for the upgrade steps to be refreshed.
     await_with_timeout(
         &pocket_ic,
         UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS,
@@ -172,27 +211,56 @@ async fn test_get_upgrade_journal() {
                 .versions
         },
         &vec![
-            initial_sns_version,
-            new_sns_version_1,
+            initial_sns_version.clone(),
+            new_sns_version_1.clone(),
             new_sns_version_2.clone(),
         ],
     )
     .await
     .unwrap();
 
-    // Advance the target version.
     {
-        sns::governance::advance_target_version(
+        expected_upgrade_journal_entries.push(
+            sns_pb::upgrade_journal_entry::Event::UpgradeStepsRefreshed(
+                sns_pb::upgrade_journal_entry::UpgradeStepsRefreshed {
+                    upgrade_steps: Some(Versions {
+                        versions: vec![
+                            initial_sns_version.clone(),
+                            new_sns_version_1.clone(),
+                            new_sns_version_2.clone(),
+                        ],
+                    }),
+                },
+            ),
+        );
+
+        assert_upgrade_journal(
             &pocket_ic,
-            sns.governance.canister_id,
-            new_sns_version_2.clone(),
+            sns.governance,
+            &expected_upgrade_journal_entries,
         )
         .await;
+    }
+
+    // State 3: Advance the target version.
+    sns::governance::advance_target_version(
+        &pocket_ic,
+        sns.governance.canister_id,
+        new_sns_version_2.clone(),
+    )
+    .await;
 
-        // Check that the target version is set to the new version.
-        let sns_pb::GetUpgradeJournalResponse { target_version, .. } =
-            sns::governance::get_upgrade_journal(&pocket_ic, sns.governance.canister_id).await;
+    expected_upgrade_journal_entries.push(sns_pb::upgrade_journal_entry::Event::TargetVersionSet(
+        sns_pb::upgrade_journal_entry::TargetVersionSet {
+            old_target_version: None,
+            new_target_version: Some(new_sns_version_2),
+        },
+    ));
 
-        assert_eq!(target_version, Some(new_sns_version_2.clone()));
-    }
+    assert_upgrade_journal(
+        &pocket_ic,
+        sns.governance,
+        &expected_upgrade_journal_entries,
+    )
+    .await;
 }

rs/sns/governance/canister/canister.rs

@@ -677,8 +677,7 @@ async fn mint_tokens(request: MintTokensRequest) -> MintTokensResponse {
 #[cfg(feature = "test")]
 #[update]
 fn advance_target_version(request: AdvanceTargetVersionRequest) -> AdvanceTargetVersionResponse {
-    governance_mut().proto.target_version = request.target_version;
-    AdvanceTargetVersionResponse {}
+    governance_mut().advance_target_version(request)
 }
 
 fn main() {

rs/sns/governance/canister/governance.did

@@ -724,7 +724,7 @@ type UpgradeJournalEntry = record {
   event : opt variant {
     UpgradeStepsRefreshed : UpgradeStepsRefreshed;
     TargetVersionSet : TargetVersionSet;
-    TargetVersionReset : TargetVersionSet;
+    TargetVersionReset : TargetVersionReset;
     UpgradeStarted : UpgradeStarted;
     UpgradeOutcome : UpgradeOutcome;
   };
@@ -740,6 +740,11 @@ type TargetVersionSet = record {
   old_target_version : opt Version;
 };
 
+type TargetVersionReset = record {
+  new_target_version : opt Version;
+  old_target_version : opt Version;
+};
+
 type UpgradeStarted = record {
   current_version : opt Version;
   expected_version : opt Version;

rs/sns/governance/canister/governance_test.did

@@ -738,7 +738,7 @@ type UpgradeJournalEntry = record {
   event : opt variant {
     UpgradeStepsRefreshed : UpgradeStepsRefreshed;
     TargetVersionSet : TargetVersionSet;
-    TargetVersionReset : TargetVersionSet;
+    TargetVersionReset : TargetVersionReset;
     UpgradeStarted : UpgradeStarted;
     UpgradeOutcome : UpgradeOutcome;
   };
@@ -754,6 +754,11 @@ type TargetVersionSet = record {
   old_target_version : opt Version;
 };
 
+type TargetVersionReset = record {
+  new_target_version : opt Version;
+  old_target_version : opt Version;
+};
+
 type UpgradeStarted = record {
   current_version : opt Version;
   expected_version : opt Version;

rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto

@@ -2147,7 +2147,7 @@ message UpgradeJournalEntry {
   oneof event {
     UpgradeStepsRefreshed upgrade_steps_refreshed = 1;
     TargetVersionSet target_version_set = 2;
-    TargetVersionSet target_version_reset = 3;
+    TargetVersionReset target_version_reset = 3;
     UpgradeStarted upgrade_started = 4;
     UpgradeOutcome upgrade_outcome = 5;
   }
@@ -2162,6 +2162,11 @@ message UpgradeJournalEntry {
     optional Governance.Version new_target_version = 2;
   }
 
+  message TargetVersionReset {
+    optional Governance.Version old_target_version = 1;
+    optional Governance.Version new_target_version = 2;
+  }
+
   message UpgradeStarted {
     optional Governance.Version current_version = 1;
     optional Governance.Version expected_version = 2;

rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs

@@ -3387,6 +3387,20 @@ pub mod upgrade_journal_entry {
         PartialEq,
         ::prost::Message,
     )]
+    pub struct TargetVersionReset {
+        #[prost(message, optional, tag = "1")]
+        pub old_target_version: ::core::option::Option<super::governance::Version>,
+        #[prost(message, optional, tag = "2")]
+        pub new_target_version: ::core::option::Option<super::governance::Version>,
+    }
+    #[derive(
+        candid::CandidType,
+        candid::Deserialize,
+        comparable::Comparable,
+        Clone,
+        PartialEq,
+        ::prost::Message,
+    )]
     pub struct UpgradeStarted {
         #[prost(message, optional, tag = "1")]
         pub current_version: ::core::option::Option<super::governance::Version>,
@@ -3475,7 +3489,7 @@ pub mod upgrade_journal_entry {
         #[prost(message, tag = "2")]
         TargetVersionSet(TargetVersionSet),
         #[prost(message, tag = "3")]
-        TargetVersionReset(TargetVersionSet),
+        TargetVersionReset(TargetVersionReset),
         #[prost(message, tag = "4")]
         UpgradeStarted(UpgradeStarted),
         #[prost(message, tag = "5")]

rs/sns/governance/src/governance.rs

@@ -38,24 +38,24 @@ use crate::{
             proposal_data::ActionAuxiliary as ActionAuxiliaryPb,
             transfer_sns_treasury_funds::TransferFrom,
             upgrade_journal_entry, Account as AccountProto, AddMaturityRequest,
-            AddMaturityResponse, Ballot, ClaimSwapNeuronsError, ClaimSwapNeuronsRequest,
-            ClaimSwapNeuronsResponse, ClaimedSwapNeuronStatus, DefaultFollowees,
-            DeregisterDappCanisters, DisburseMaturityInProgress, Empty,
-            ExecuteGenericNervousSystemFunction, FailStuckUpgradeInProgressRequest,
-            FailStuckUpgradeInProgressResponse, GetMaturityModulationRequest,
-            GetMaturityModulationResponse, GetMetadataRequest, GetMetadataResponse, GetMode,
-            GetModeResponse, GetNeuron, GetNeuronResponse, GetProposal, GetProposalResponse,
-            GetSnsInitializationParametersRequest, GetSnsInitializationParametersResponse,
-            GetUpgradeJournalResponse, Governance as GovernanceProto, GovernanceError,
-            ListNervousSystemFunctionsResponse, ListNeurons, ListNeuronsResponse, ListProposals,
-            ListProposalsResponse, ManageDappCanisterSettings, ManageLedgerParameters,
-            ManageNeuron, ManageNeuronResponse, ManageSnsMetadata, MintSnsTokens,
-            MintTokensRequest, MintTokensResponse, NervousSystemFunction, NervousSystemParameters,
-            Neuron, NeuronId, NeuronPermission, NeuronPermissionList, NeuronPermissionType,
-            Proposal, ProposalData, ProposalDecisionStatus, ProposalId, ProposalRewardStatus,
-            RegisterDappCanisters, RewardEvent, Tally, TransferSnsTreasuryFunds, UpgradeJournal,
-            UpgradeJournalEntry, UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Vote,
-            WaitForQuietState,
+            AddMaturityResponse, AdvanceTargetVersionRequest, AdvanceTargetVersionResponse, Ballot,
+            ClaimSwapNeuronsError, ClaimSwapNeuronsRequest, ClaimSwapNeuronsResponse,
+            ClaimedSwapNeuronStatus, DefaultFollowees, DeregisterDappCanisters,
+            DisburseMaturityInProgress, Empty, ExecuteGenericNervousSystemFunction,
+            FailStuckUpgradeInProgressRequest, FailStuckUpgradeInProgressResponse,
+            GetMaturityModulationRequest, GetMaturityModulationResponse, GetMetadataRequest,
+            GetMetadataResponse, GetMode, GetModeResponse, GetNeuron, GetNeuronResponse,
+            GetProposal, GetProposalResponse, GetSnsInitializationParametersRequest,
+            GetSnsInitializationParametersResponse, GetUpgradeJournalResponse,
+            Governance as GovernanceProto, GovernanceError, ListNervousSystemFunctionsResponse,
+            ListNeurons, ListNeuronsResponse, ListProposals, ListProposalsResponse,
+            ManageDappCanisterSettings, ManageLedgerParameters, ManageNeuron, ManageNeuronResponse,
+            ManageSnsMetadata, MintSnsTokens, MintTokensRequest, MintTokensResponse,
+            NervousSystemFunction, NervousSystemParameters, Neuron, NeuronId, NeuronPermission,
+            NeuronPermissionList, NeuronPermissionType, Proposal, ProposalData,
+            ProposalDecisionStatus, ProposalId, ProposalRewardStatus, RegisterDappCanisters,
+            RewardEvent, Tally, TransferSnsTreasuryFunds, UpgradeJournal, UpgradeJournalEntry,
+            UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Vote, WaitForQuietState,
         },
     },
     proposal::{
@@ -4805,9 +4805,13 @@ impl Governance {
         }
     }
 
-    pub fn push_to_upgrade_journal(&mut self, event: impl Into<upgrade_journal_entry::Event>) {
+    pub fn push_to_upgrade_journal<Event>(&mut self, event: Event)
+    where
+        upgrade_journal_entry::Event: From<Event>,
+    {
+        let event = upgrade_journal_entry::Event::from(event);
         let upgrade_journal_entry = UpgradeJournalEntry {
-            event: Some(event.into()),
+            event: Some(event),
             timestamp_seconds: Some(self.env.now()),
         };
         match self.proto.upgrade_journal {
@@ -4842,6 +4846,22 @@ impl Governance {
         }
     }
 
+    pub fn advance_target_version(
+        &mut self,
+        request: AdvanceTargetVersionRequest,
+    ) -> AdvanceTargetVersionResponse {
+        self.push_to_upgrade_journal(upgrade_journal_entry::Event::TargetVersionSet(
+            upgrade_journal_entry::TargetVersionSet {
+                old_target_version: self.proto.target_version.clone(),
+                new_target_version: request.target_version.clone(),
+            },
+        ));
+
+        self.proto.target_version = request.target_version;
+
+        AdvanceTargetVersionResponse {}
+    }
+
     fn should_update_maturity_modulation(&self) -> bool {
         // Check if we're already updating the neuron maturity modulation.
         let updated_at_timestamp_seconds = self
@@ -5382,7 +5402,7 @@ impl Governance {
 
                 if self.env.now() > mark_failed_at {
                     let error = format!(
-                        "Upgrade marked as failed at {} seconds from genesis. \
+                        "Upgrade marked as failed at {} seconds from unix epoch. \
                              Governance could not determine running version from root: {}. \
                              Setting upgrade to failed to unblock retry.",
                         self.env.now(),
@@ -5423,7 +5443,9 @@ impl Governance {
 
                 self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome {
                     status: Some(
-                        upgrade_journal_entry::upgrade_outcome::Status::ExternalFailure(Empty {}),
+                        upgrade_journal_entry::upgrade_outcome::Status::InvalidState(
+                            upgrade_journal_entry::upgrade_outcome::InvalidState { version: None },
+                        ),
                     ),
                     human_readable: Some(error.to_string()),
                 });
@@ -8465,8 +8487,10 @@ mod tests {
                     upgrade_journal_entry::UpgradeOutcome {
                         human_readable: Some(_),
                         status: Some(
-                            upgrade_journal_entry::upgrade_outcome::Status::ExternalFailure(
-                                Empty {}
+                            upgrade_journal_entry::upgrade_outcome::Status::InvalidState(
+                                upgrade_journal_entry::upgrade_outcome::InvalidState {
+                                    version: None,
+                                }
                             )
                         ),
                     }