refactor(sns): Separate UpgradeInProgress and PendingVersion (#2469)

As of this PR, UpgradeInProgress is used in the API, while
PendingVersion is used internally. This will allow us to evolve them
separately.

This is not actually a breaking change as far as proto is concerned

rs/sns/governance/canister/canister.rs

@@ -40,7 +40,8 @@ use ic_sns_governance::{
     },
     logs::{ERROR, INFO},
     pb::v1::{
-        ClaimSwapNeuronsRequest, ClaimSwapNeuronsResponse, FailStuckUpgradeInProgressRequest,
+        get_running_sns_version_response::UpgradeInProgress, ClaimSwapNeuronsRequest,
+        ClaimSwapNeuronsResponse, FailStuckUpgradeInProgressRequest,
         FailStuckUpgradeInProgressResponse, GetMaturityModulationRequest,
         GetMaturityModulationResponse, GetMetadataRequest, GetMetadataResponse, GetMode,
         GetModeResponse, GetNeuron, GetNeuronResponse, GetProposal, GetProposalResponse,
@@ -467,9 +468,16 @@ async fn get_root_canister_status(_: ()) -> CanisterStatusResultV2 {
 #[query]
 fn get_running_sns_version(_: GetRunningSnsVersionRequest) -> GetRunningSnsVersionResponse {
     log!(INFO, "get_running_sns_version");
+    let pending_version = governance().proto.pending_version.clone();
+    let upgrade_in_progress = pending_version.map(|upgrade_in_progress| UpgradeInProgress {
+        target_version: upgrade_in_progress.target_version.clone(),
+        mark_failed_at_seconds: upgrade_in_progress.mark_failed_at_seconds,
+        checking_upgrade_lock: upgrade_in_progress.checking_upgrade_lock,
+        proposal_id: upgrade_in_progress.proposal_id,
+    });
     GetRunningSnsVersionResponse {
         deployed_version: governance().proto.deployed_version.clone(),
-        pending_version: governance().proto.pending_version.clone(),
+        pending_version: upgrade_in_progress,
     }
 }
 

rs/sns/governance/canister/governance.did

@@ -282,7 +282,7 @@ type Governance = record {
   cached_upgrade_steps : opt CachedUpgradeSteps;
   sns_initialization_parameters : text;
   latest_reward_event : opt RewardEvent;
-  pending_version : opt UpgradeInProgress;
+  pending_version : opt PendingVersion;
   swap_canister_id : opt principal;
   ledger_canister_id : opt principal;
   proposals : vec record { nat64; ProposalData };
@@ -678,6 +678,13 @@ type UpgradeInProgress = record {
   target_version : opt Version;
 };
 
+type PendingVersion = record {
+  mark_failed_at_seconds : nat64;
+  checking_upgrade_lock : nat64;
+  proposal_id : nat64;
+  target_version : opt Version;
+};
+
 type UpgradeSnsControlledCanister = record {
   new_canister_wasm : blob;
   mode : opt int32;

rs/sns/governance/canister/governance_test.did

@@ -291,7 +291,7 @@ type Governance = record {
   cached_upgrade_steps : opt CachedUpgradeSteps;
   sns_initialization_parameters : text;
   latest_reward_event : opt RewardEvent;
-  pending_version : opt UpgradeInProgress;
+  pending_version : opt PendingVersion;
   swap_canister_id : opt principal;
   ledger_canister_id : opt principal;
   proposals : vec record { nat64; ProposalData };
@@ -692,6 +692,13 @@ type UpgradeInProgress = record {
   target_version : opt Version;
 };
 
+type PendingVersion = record {
+  mark_failed_at_seconds : nat64;
+  checking_upgrade_lock : nat64;
+  proposal_id : nat64;
+  target_version : opt Version;
+};
+
 type UpgradeSnsControlledCanister = record {
   new_canister_wasm : blob;
   mode : opt int32;

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

@@ -1415,7 +1415,7 @@ message Governance {
   Version deployed_version = 23;
 
   // An upgrade in progress, defined as a version target and a time at which it is considered failed.
-  message UpgradeInProgress {
+  message PendingVersion {
     // Version to  be upgraded to
     Version target_version = 1;
     // Seconds since UNIX epoch to mark this as a failed version if not in sync with current version
@@ -1428,7 +1428,7 @@ message Governance {
   }
 
   // Version SNS is in process of upgrading to.
-  UpgradeInProgress pending_version = 24;
+  PendingVersion pending_version = 24;
   Version target_version = 30;
 
   // True if the run_periodic_tasks function is currently finalizing disburse maturity, meaning
@@ -1501,11 +1501,25 @@ message GetRunningSnsVersionRequest {}
 
 // Response with the SNS's currently running version and any upgrades
 // that are in progress.
+// GetUpgradeJournal is a superior API to this one that should
 message GetRunningSnsVersionResponse {
   // The currently deployed version of the SNS.
   Governance.Version deployed_version = 1;
   // The upgrade in progress, if any.
-  Governance.UpgradeInProgress pending_version = 2;
+  UpgradeInProgress pending_version = 2;
+
+  // The same as PendingVersion (stored in the governance proto). They are separated to make it easy to change one without changing the other.
+  message UpgradeInProgress {
+    // Version to  be upgraded to
+    Governance.Version target_version = 1;
+    // Seconds since UNIX epoch to mark this as a failed version if not in sync with current version
+    uint64 mark_failed_at_seconds = 2;
+    // Lock to avoid checking over and over again.  Also, it is a counter for how many times we have attempted to check,
+    // allowing us to fail in case we otherwise have gotten stuck.
+    uint64 checking_upgrade_lock = 3;
+    // The proposal that initiated this upgrade
+    uint64 proposal_id = 4;
+  }
 }
 
 // Request to fail an upgrade proposal that is Adopted but not Executed or

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

@@ -1636,7 +1636,7 @@ pub struct Governance {
     pub deployed_version: ::core::option::Option<governance::Version>,
     /// Version SNS is in process of upgrading to.
     #[prost(message, optional, tag = "24")]
-    pub pending_version: ::core::option::Option<governance::UpgradeInProgress>,
+    pub pending_version: ::core::option::Option<governance::PendingVersion>,
     #[prost(message, optional, tag = "30")]
     pub target_version: ::core::option::Option<governance::Version>,
     /// True if the run_periodic_tasks function is currently finalizing disburse maturity, meaning
@@ -1886,7 +1886,7 @@ pub mod governance {
         PartialEq,
         ::prost::Message,
     )]
-    pub struct UpgradeInProgress {
+    pub struct PendingVersion {
         /// Version to  be upgraded to
         #[prost(message, optional, tag = "1")]
         pub target_version: ::core::option::Option<Version>,
@@ -2065,6 +2065,7 @@ pub struct GetSnsInitializationParametersResponse {
 pub struct GetRunningSnsVersionRequest {}
 /// Response with the SNS's currently running version and any upgrades
 /// that are in progress.
+/// GetUpgradeJournal is a superior API to this one that should
 #[derive(
     candid::CandidType,
     candid::Deserialize,
@@ -2079,7 +2080,35 @@ pub struct GetRunningSnsVersionResponse {
     pub deployed_version: ::core::option::Option<governance::Version>,
     /// The upgrade in progress, if any.
     #[prost(message, optional, tag = "2")]
-    pub pending_version: ::core::option::Option<governance::UpgradeInProgress>,
+    pub pending_version:
+        ::core::option::Option<get_running_sns_version_response::UpgradeInProgress>,
+}
+/// Nested message and enum types in `GetRunningSnsVersionResponse`.
+pub mod get_running_sns_version_response {
+    /// The same as PendingVersion (stored in the governance proto). They are separated to make it easy to change one without changing the other.
+    #[derive(
+        candid::CandidType,
+        candid::Deserialize,
+        comparable::Comparable,
+        Clone,
+        PartialEq,
+        ::prost::Message,
+    )]
+    pub struct UpgradeInProgress {
+        /// Version to  be upgraded to
+        #[prost(message, optional, tag = "1")]
+        pub target_version: ::core::option::Option<super::governance::Version>,
+        /// Seconds since UNIX epoch to mark this as a failed version if not in sync with current version
+        #[prost(uint64, tag = "2")]
+        pub mark_failed_at_seconds: u64,
+        /// Lock to avoid checking over and over again.  Also, it is a counter for how many times we have attempted to check,
+        /// allowing us to fail in case we otherwise have gotten stuck.
+        #[prost(uint64, tag = "3")]
+        pub checking_upgrade_lock: u64,
+        /// The proposal that initiated this upgrade
+        #[prost(uint64, tag = "4")]
+        pub proposal_id: u64,
+    }
 }
 /// Request to fail an upgrade proposal that is Adopted but not Executed or
 /// Failed if it is past the time when it should have been marked as failed.

rs/sns/governance/src/governance.rs

@@ -20,8 +20,8 @@ use crate::{
             governance::{
                 self,
                 neuron_in_flight_command::{self, Command as InFlightCommand},
-                CachedUpgradeSteps, MaturityModulation, NeuronInFlightCommand, SnsMetadata,
-                UpgradeInProgress, Version, Versions,
+                CachedUpgradeSteps, MaturityModulation, NeuronInFlightCommand, PendingVersion,
+                SnsMetadata, Version, Versions,
             },
             governance_error::ErrorType,
             manage_neuron::{
@@ -2645,7 +2645,7 @@ impl Governance {
         // A canister upgrade has been successfully kicked-off. Set the pending upgrade-in-progress
         // field so that Governance's run_periodic_tasks logic can check on the status of
         // this upgrade.
-        self.proto.pending_version = Some(UpgradeInProgress {
+        self.proto.pending_version = Some(PendingVersion {
             target_version: Some(next_version),
             mark_failed_at_seconds: self.env.now() + 5 * 60,
             checking_upgrade_lock: 0,
@@ -7321,7 +7321,7 @@ mod tests {
         assert_required_calls();
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.into()),
                 mark_failed_at_seconds: now + 5 * 60,
                 checking_upgrade_lock: 0,
@@ -7636,7 +7636,7 @@ mod tests {
         // Now set the pending_version in Governance such that the period_task to check upgrade
         // status is triggered.
         let mark_failed_at_seconds = governance.env.now() + ONE_DAY_SECONDS;
-        governance.proto.pending_version = Some(UpgradeInProgress {
+        governance.proto.pending_version = Some(PendingVersion {
             target_version: Some(next_version.clone().into()),
             mark_failed_at_seconds,
             checking_upgrade_lock: 0,
@@ -7646,7 +7646,7 @@ mod tests {
         // Make sure Governance state is correctly set
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.clone().into()),
                 mark_failed_at_seconds,
                 checking_upgrade_lock: 0,
@@ -7734,7 +7734,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.clone().into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now - 1,
                     checking_upgrade_lock: 0,
@@ -7752,7 +7752,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.into()),
                 mark_failed_at_seconds: now - 1,
                 checking_upgrade_lock: 0,
@@ -7829,7 +7829,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.clone().into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now + 5 * 60,
                     checking_upgrade_lock: 0,
@@ -7875,7 +7875,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.clone().into()),
                 mark_failed_at_seconds: now + 5 * 60,
                 checking_upgrade_lock: 0,
@@ -7970,7 +7970,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.clone().into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now + 1,
                     checking_upgrade_lock: 0,
@@ -8016,7 +8016,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.clone().into()),
                 mark_failed_at_seconds: now + 1,
                 checking_upgrade_lock: 0,
@@ -8033,7 +8033,7 @@ mod tests {
         // We still have pending version
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.into()),
                 mark_failed_at_seconds: now + 1,
                 checking_upgrade_lock: 0,
@@ -8106,7 +8106,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.clone().into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now - 1,
                     checking_upgrade_lock: 0,
@@ -8152,7 +8152,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.clone().into()),
                 mark_failed_at_seconds: now - 1,
                 checking_upgrade_lock: 0,
@@ -8253,7 +8253,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     // This should be impossible due to how it's set, but is the condition of this test
                     target_version: None,
                     mark_failed_at_seconds: now - 1,
@@ -8389,7 +8389,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: None,
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now + 5 * 60,
                     checking_upgrade_lock: 0,
@@ -8435,7 +8435,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.into()),
                 mark_failed_at_seconds: now + 5 * 60,
                 checking_upgrade_lock: 0,
@@ -8596,7 +8596,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.clone().into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now + 5 * 60,
                     checking_upgrade_lock: 0,
@@ -8614,7 +8614,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.clone().into()),
                 mark_failed_at_seconds: now + 5 * 60,
                 checking_upgrade_lock: 0,
@@ -8674,7 +8674,7 @@ mod tests {
             GovernanceProto {
                 root_canister_id: Some(root_canister_id.get()),
                 deployed_version: Some(current_version.clone().into()),
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     target_version: Some(next_version.clone().into()),
                     mark_failed_at_seconds: now + 5 * 60,
                     checking_upgrade_lock: 0,
@@ -8692,7 +8692,7 @@ mod tests {
 
         assert_eq!(
             governance.proto.pending_version.clone().unwrap(),
-            UpgradeInProgress {
+            PendingVersion {
                 target_version: Some(next_version.clone().into()),
                 mark_failed_at_seconds: now + 5 * 60,
                 checking_upgrade_lock: 0,
@@ -9103,7 +9103,7 @@ mod tests {
                     proposal_id => proposal,
                 },
                 // There's already an upgrade pending
-                pending_version: Some(UpgradeInProgress {
+                pending_version: Some(PendingVersion {
                     ..Default::default()
                 }),
                 ..basic_governance_proto()