Skip to content

Commit

Permalink
refactor(sns): Add helpers for the upgrade journal (#2527)
Browse files Browse the repository at this point in the history 
The upgrade journal event types were very verbose to construct
previously. This PR hopes to make them a litlte nicer

Addresses this:
#2034 (comment)
  • Loading branch information
@anchpop
anchpop authored Nov 13, 2024
1 parent 32c6bb2 commit 140a46b
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 140 deletions.
47 changes: 17 additions & 30 deletions rs/nervous_system/integration_tests/tests/advance_target_version.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
116 changes: 33 additions & 83 deletions rs/sns/governance/src/governance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<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),
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,
));
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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),
Expand Down Expand Up @@ -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::{
Expand Down
1 change: 1 addition & 0 deletions rs/sns/governance/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ mod request_impls;
pub mod reward;
pub mod sns_upgrade;
pub mod types;
pub mod upgrade_journal;

mod treasury;

Expand Down
35 changes: 8 additions & 27 deletions rs/sns/governance/src/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -2749,25 +2749,6 @@ pub mod test_helpers {
}
}
}

impl From<upgrade_journal_entry::UpgradeStepsRefreshed> for upgrade_journal_entry::Event {
fn from(event: upgrade_journal_entry::UpgradeStepsRefreshed) -> Self {
upgrade_journal_entry::Event::UpgradeStepsRefreshed(event)
}
}
impl From<upgrade_journal_entry::UpgradeStarted> for upgrade_journal_entry::Event {
fn from(event: upgrade_journal_entry::UpgradeStarted) -> Self {
upgrade_journal_entry::Event::UpgradeStarted(event)
}
}
impl From<upgrade_journal_entry::UpgradeOutcome> 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<upgrade_journal_entry::TargetVersionSet> 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::*;
Expand Down
Loading

0 comments on commit 140a46b

Please sign in to comment.