Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(sns): Separate UpgradeInProgress and PendingVersion #2469

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+101 −36
Open

refactor(sns): Separate UpgradeInProgress and PendingVersion #2469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions rs/sns/governance/canister/canister.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}
}

Expand Down
9 changes: 8 additions & 1 deletion rs/sns/governance/canister/governance.did
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 8 additions & 1 deletion rs/sns/governance/canister/governance_test.did
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 17 additions & 3 deletions rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
35 changes: 32 additions & 3 deletions rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down
44 changes: 22 additions & 22 deletions rs/sns/governance/src/governance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Loading
Loading