From 140a46b308fd25c86e38265d2eccc38bcb437620 Mon Sep 17 00:00:00 2001 From: Andre Popovitch Date: Wed, 13 Nov 2024 00:53:54 -0600 Subject: [PATCH] refactor(sns): Add helpers for the upgrade journal (#2527) The upgrade journal event types were very verbose to construct previously. This PR hopes to make them a litlte nicer Addresses this: https://github.com/dfinity/ic/pull/2034#discussion_r1831401522 --- .../tests/advance_target_version.rs | 47 +++--- rs/sns/governance/src/governance.rs | 116 ++++---------- rs/sns/governance/src/lib.rs | 1 + rs/sns/governance/src/types.rs | 35 +---- rs/sns/governance/src/upgrade_journal.rs | 142 ++++++++++++++++++ 5 files changed, 201 insertions(+), 140 deletions(-) create mode 100644 rs/sns/governance/src/upgrade_journal.rs diff --git a/rs/nervous_system/integration_tests/tests/advance_target_version.rs b/rs/nervous_system/integration_tests/tests/advance_target_version.rs index 4c2feb9b3a8..0735af5305d 100644 --- a/rs/nervous_system/integration_tests/tests/advance_target_version.rs +++ b/rs/nervous_system/integration_tests/tests/advance_target_version.rs @@ -7,9 +7,10 @@ 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, + governance::UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS, + pb::v1 as sns_pb, + pb::v1::upgrade_journal_entry::{Event, TargetVersionSet, UpgradeStepsRefreshed}, }; use ic_sns_swap::pb::v1::Lifecycle; use ic_sns_wasm::pb::v1::SnsCanisterType; @@ -129,15 +130,9 @@ async fn test_get_upgrade_journal() { // Step 1: right after SNS creation. let mut expected_upgrade_journal_entries = vec![]; { - 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()], - }), - }, - ), - ); + expected_upgrade_journal_entries.push(Event::UpgradeStepsRefreshed( + UpgradeStepsRefreshed::new(vec![initial_sns_version.clone()]), + )); assert_upgrade_journal( &pocket_ic, @@ -220,19 +215,13 @@ async fn test_get_upgrade_journal() { .unwrap(); { - 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(), - ], - }), - }, - ), - ); + expected_upgrade_journal_entries.push(Event::UpgradeStepsRefreshed( + UpgradeStepsRefreshed::new(vec![ + initial_sns_version.clone(), + new_sns_version_1.clone(), + new_sns_version_2.clone(), + ]), + )); assert_upgrade_journal( &pocket_ic, @@ -250,12 +239,10 @@ async fn test_get_upgrade_journal() { ) .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), - }, - )); + expected_upgrade_journal_entries.push(Event::TargetVersionSet(TargetVersionSet::new( + None, + Some(new_sns_version_2.clone()), + ))); assert_upgrade_journal( &pocket_ic, diff --git a/rs/sns/governance/src/governance.rs b/rs/sns/governance/src/governance.rs index 9d7f0ce4590..3d342a0513a 100644 --- a/rs/sns/governance/src/governance.rs +++ b/rs/sns/governance/src/governance.rs @@ -54,8 +54,8 @@ use crate::{ NervousSystemFunction, NervousSystemParameters, Neuron, NeuronId, NeuronPermission, NeuronPermissionList, NeuronPermissionType, Proposal, ProposalData, ProposalDecisionStatus, ProposalId, ProposalRewardStatus, RegisterDappCanisters, - RewardEvent, Tally, TransferSnsTreasuryFunds, UpgradeJournal, UpgradeJournalEntry, - UpgradeSnsControlledCanister, Vote, WaitForQuietState, + RewardEvent, Tally, TransferSnsTreasuryFunds, UpgradeSnsControlledCanister, Vote, + WaitForQuietState, }, }, proposal::{ @@ -2600,15 +2600,11 @@ impl Governance { ) })?; - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeStarted { - current_version: Some(current_version.clone()), - expected_version: Some(next_version.clone()), - reason: Some( - upgrade_journal_entry::upgrade_started::Reason::UpgradeSnsToNextVersionProposal( - ProposalId { id: proposal_id }, - ), - ), - }); + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeStarted::from_proposal( + current_version.clone(), + next_version.clone(), + ProposalId { id: proposal_id }, + )); let target_wasm = get_wasm(&*self.env, new_wasm_hash.to_vec(), canister_type_to_upgrade) .await @@ -4828,30 +4824,9 @@ impl Governance { // Refresh the upgrade steps if they have changed if cached_upgrade_steps.upgrade_steps != Some(upgrade_steps.clone()) { cached_upgrade_steps.upgrade_steps = Some(upgrade_steps.clone()); - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeStepsRefreshed { - upgrade_steps: Some(upgrade_steps), - }); - } - } - - pub fn push_to_upgrade_journal(&mut self, event: Event) - where - upgrade_journal_entry::Event: From, - { - let event = upgrade_journal_entry::Event::from(event); - let upgrade_journal_entry = UpgradeJournalEntry { - event: Some(event), - timestamp_seconds: Some(self.env.now()), - }; - match self.proto.upgrade_journal { - None => { - self.proto.upgrade_journal = Some(UpgradeJournal { - entries: vec![upgrade_journal_entry], - }); - } - Some(ref mut journal) => { - journal.entries.push(upgrade_journal_entry); - } + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeStepsRefreshed::new( + upgrade_steps.versions, + )); } } @@ -4879,11 +4854,9 @@ impl Governance { &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.push_to_upgrade_journal(upgrade_journal_entry::TargetVersionSet::new( + self.proto.target_version.clone(), + request.target_version.clone(), )); self.proto.target_version = request.target_version; @@ -5355,14 +5328,10 @@ impl Governance { let msg = "No target_version set for upgrade_in_progress. This should be impossible. \ Clearing upgrade_in_progress state and marking proposal failed to unblock further upgrades."; - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { - status: Some( - upgrade_journal_entry::upgrade_outcome::Status::InvalidState( - upgrade_journal_entry::upgrade_outcome::InvalidState { version: None }, - ), - ), - human_readable: Some(msg.to_string()), - }); + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome::invalid_state( + msg.to_string(), + None, + )); self.fail_sns_upgrade_to_next_version_proposal( upgrade_in_progress.proposal_id, GovernanceError::new_with_message(ErrorType::PreconditionFailed, msg), @@ -5394,12 +5363,9 @@ impl Governance { let error = "Too many attempts to check upgrade without success. Marking upgrade failed."; - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { - status: Some(upgrade_journal_entry::upgrade_outcome::Status::Timeout( - Empty {}, - )), - human_readable: Some(error.to_string()), - }); + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome::timeout( + error.to_string(), + )); self.fail_sns_upgrade_to_next_version_proposal( proposal_id, @@ -5438,12 +5404,9 @@ impl Governance { message, ); - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { - status: Some(upgrade_journal_entry::upgrade_outcome::Status::Timeout( - Empty {}, - )), - human_readable: Some(error.to_string()), - }); + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome::timeout( + error.to_string(), + )); self.fail_sns_upgrade_to_next_version_proposal( proposal_id, GovernanceError::new_with_message(ErrorType::External, error), @@ -5470,14 +5433,10 @@ impl Governance { self.env.now(), ); - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { - status: Some( - upgrade_journal_entry::upgrade_outcome::Status::InvalidState( - upgrade_journal_entry::upgrade_outcome::InvalidState { version: None }, - ), - ), - human_readable: Some(error.to_string()), - }); + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome::invalid_state( + error.to_string(), + None, + )); self.proto.deployed_version = Some(running_version); self.fail_sns_upgrade_to_next_version_proposal( @@ -5499,12 +5458,7 @@ impl Governance { self.env.now(), target_version ); - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { - status: Some(upgrade_journal_entry::upgrade_outcome::Status::Success( - Empty {}, - )), - human_readable: None, - }); + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome::success(None)); self.set_proposal_execution_status(proposal_id, Ok(())); self.proto.deployed_version = Some(target_version); self.proto.pending_version = None; @@ -5519,13 +5473,9 @@ impl Governance { errors ); - self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome { - status: Some(upgrade_journal_entry::upgrade_outcome::Status::Timeout( - Empty {}, - )), - human_readable: Some(error.to_string()), - }); - + self.push_to_upgrade_journal(upgrade_journal_entry::UpgradeOutcome::timeout( + error.to_string(), + )); self.fail_sns_upgrade_to_next_version_proposal( proposal_id, GovernanceError::new_with_message(ErrorType::External, error), @@ -5848,8 +5798,8 @@ mod tests { manage_neuron_response, nervous_system_function::{FunctionType, GenericNervousSystemFunction}, neuron, Account as AccountProto, Motion, NeuronPermissionType, ProposalData, - ProposalId, Tally, UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, - VotingRewardsParameters, WaitForQuietState, + ProposalId, Tally, UpgradeJournalEntry, UpgradeSnsControlledCanister, + UpgradeSnsToNextVersion, VotingRewardsParameters, WaitForQuietState, }, reward, sns_upgrade::{ diff --git a/rs/sns/governance/src/lib.rs b/rs/sns/governance/src/lib.rs index ee44401d906..87be99438ce 100644 --- a/rs/sns/governance/src/lib.rs +++ b/rs/sns/governance/src/lib.rs @@ -12,6 +12,7 @@ mod request_impls; pub mod reward; pub mod sns_upgrade; pub mod types; +pub mod upgrade_journal; mod treasury; diff --git a/rs/sns/governance/src/types.rs b/rs/sns/governance/src/types.rs index 819481b4d83..6c6f9a937e2 100644 --- a/rs/sns/governance/src/types.rs +++ b/rs/sns/governance/src/types.rs @@ -26,14 +26,14 @@ use crate::{ nervous_system_function::FunctionType, neuron::Followees, proposal::Action, - upgrade_journal_entry, ClaimSwapNeuronsError, ClaimSwapNeuronsResponse, - ClaimedSwapNeuronStatus, DefaultFollowees, DeregisterDappCanisters, Empty, - ExecuteGenericNervousSystemFunction, GovernanceError, ManageDappCanisterSettings, - ManageLedgerParameters, ManageNeuronResponse, ManageSnsMetadata, MintSnsTokens, Motion, - NervousSystemFunction, NervousSystemParameters, Neuron, NeuronId, NeuronIds, - NeuronPermission, NeuronPermissionList, NeuronPermissionType, ProposalId, - RegisterDappCanisters, RewardEvent, TransferSnsTreasuryFunds, - UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Vote, VotingRewardsParameters, + ClaimSwapNeuronsError, ClaimSwapNeuronsResponse, ClaimedSwapNeuronStatus, + DefaultFollowees, DeregisterDappCanisters, Empty, ExecuteGenericNervousSystemFunction, + GovernanceError, ManageDappCanisterSettings, ManageLedgerParameters, + ManageNeuronResponse, ManageSnsMetadata, MintSnsTokens, Motion, NervousSystemFunction, + NervousSystemParameters, Neuron, NeuronId, NeuronIds, NeuronPermission, + NeuronPermissionList, NeuronPermissionType, ProposalId, RegisterDappCanisters, + RewardEvent, TransferSnsTreasuryFunds, UpgradeSnsControlledCanister, + UpgradeSnsToNextVersion, Vote, VotingRewardsParameters, }, }, proposal::ValidGenericNervousSystemFunction, @@ -2749,25 +2749,6 @@ pub mod test_helpers { } } } - -impl From for upgrade_journal_entry::Event { - fn from(event: upgrade_journal_entry::UpgradeStepsRefreshed) -> Self { - upgrade_journal_entry::Event::UpgradeStepsRefreshed(event) - } -} -impl From for upgrade_journal_entry::Event { - fn from(event: upgrade_journal_entry::UpgradeStarted) -> Self { - upgrade_journal_entry::Event::UpgradeStarted(event) - } -} -impl From for upgrade_journal_entry::Event { - fn from(event: upgrade_journal_entry::UpgradeOutcome) -> Self { - upgrade_journal_entry::Event::UpgradeOutcome(event) - } -} -// Note, we do not implement From for upgrade_journal_entry::Event -// because it is ambiguous which event variant to convert it to (TargetVersionSet vs TargetVersionReset). - #[cfg(test)] pub(crate) mod tests { use super::*; diff --git a/rs/sns/governance/src/upgrade_journal.rs b/rs/sns/governance/src/upgrade_journal.rs new file mode 100644 index 00000000000..5ca22adf572 --- /dev/null +++ b/rs/sns/governance/src/upgrade_journal.rs @@ -0,0 +1,142 @@ +use crate::governance::Governance; +use crate::pb::v1::{ + governance::{Version, Versions}, + upgrade_journal_entry::{self, upgrade_outcome, upgrade_started}, + Empty, ProposalId, UpgradeJournal, UpgradeJournalEntry, +}; + +impl upgrade_journal_entry::UpgradeStepsRefreshed { + /// Creates a new UpgradeStepsRefreshed event with the given versions + pub fn new(versions: Vec) -> Self { + Self { + upgrade_steps: Some(Versions { versions }), + } + } +} + +impl upgrade_journal_entry::TargetVersionSet { + /// Creates a new TargetVersionSet event with old and new versions + pub fn new(old_version: Option, new_version: Option) -> Self { + Self { + old_target_version: old_version, + new_target_version: new_version, + } + } +} + +impl upgrade_journal_entry::TargetVersionReset { + /// Creates a new TargetVersionReset event with old and new versions + pub fn new(old_version: Option, new_version: Option) -> Self { + Self { + old_target_version: old_version, + new_target_version: new_version, + } + } +} + +impl upgrade_journal_entry::UpgradeStarted { + /// Creates a new UpgradeStarted event triggered by a proposal + pub fn from_proposal(current: Version, expected: Version, proposal_id: ProposalId) -> Self { + Self { + current_version: Some(current), + expected_version: Some(expected), + reason: Some(upgrade_started::Reason::UpgradeSnsToNextVersionProposal( + proposal_id, + )), + } + } + + /// Creates a new UpgradeStarted event triggered by being behind target version + pub fn from_behind_target(current: Version, expected: Version) -> Self { + Self { + current_version: Some(current), + expected_version: Some(expected), + reason: Some(upgrade_started::Reason::BehindTargetVersion(Empty {})), + } + } +} + +impl upgrade_journal_entry::UpgradeOutcome { + /// Creates a new successful upgrade outcome + pub fn success(message: Option) -> Self { + Self { + human_readable: message, + status: Some(upgrade_outcome::Status::Success(Empty {})), + } + } + + /// Creates a new timeout upgrade outcome + pub fn timeout(message: String) -> Self { + Self { + human_readable: Some(message), + status: Some(upgrade_outcome::Status::Timeout(Empty {})), + } + } + + /// Creates a new invalid state upgrade outcome + pub fn invalid_state(message: String, version: Option) -> Self { + Self { + human_readable: Some(message), + status: Some(upgrade_outcome::Status::InvalidState( + upgrade_outcome::InvalidState { version }, + )), + } + } + + /// Creates a new external failure upgrade outcome + pub fn external_failure(message: Option) -> Self { + Self { + human_readable: message, + status: Some(upgrade_outcome::Status::ExternalFailure(Empty {})), + } + } +} + +impl Governance { + pub fn push_to_upgrade_journal(&mut self, event: Event) + where + upgrade_journal_entry::Event: From, + { + let event = upgrade_journal_entry::Event::from(event); + let upgrade_journal_entry = UpgradeJournalEntry { + event: Some(event), + timestamp_seconds: Some(self.env.now()), + }; + match self.proto.upgrade_journal { + None => { + self.proto.upgrade_journal = Some(UpgradeJournal { + entries: vec![upgrade_journal_entry], + }); + } + Some(ref mut journal) => { + journal.entries.push(upgrade_journal_entry); + } + } + } +} + +impl From for upgrade_journal_entry::Event { + fn from(event: upgrade_journal_entry::UpgradeStepsRefreshed) -> Self { + upgrade_journal_entry::Event::UpgradeStepsRefreshed(event) + } +} +impl From for upgrade_journal_entry::Event { + fn from(event: upgrade_journal_entry::UpgradeStarted) -> Self { + upgrade_journal_entry::Event::UpgradeStarted(event) + } +} +impl From for upgrade_journal_entry::Event { + fn from(event: upgrade_journal_entry::UpgradeOutcome) -> Self { + upgrade_journal_entry::Event::UpgradeOutcome(event) + } +} +impl From for upgrade_journal_entry::Event { + fn from(event: upgrade_journal_entry::TargetVersionSet) -> Self { + upgrade_journal_entry::Event::TargetVersionSet(event) + } +} +impl From for upgrade_journal_entry::Event { + fn from(event: upgrade_journal_entry::TargetVersionReset) -> Self { + upgrade_journal_entry::Event::TargetVersionReset(event) + } +}