Skip to content

Commit

Permalink
Refactor err-if-proposal-in-progress
Browse files Browse the repository at this point in the history
  • Loading branch information
@anchpop
anchpop committed Nov 6, 2024
1 parent 7d5b998 commit c559561
Show file tree
Hide file tree
Showing 2 changed files with 216 additions and 66 deletions.
258 changes: 192 additions & 66 deletions rs/sns/governance/src/governance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ use crate::{
NeuronPermissionList, NeuronPermissionType, Proposal, ProposalData,
ProposalDecisionStatus, ProposalId, ProposalRewardStatus, RegisterDappCanisters,
RewardEvent, Tally, TransferSnsTreasuryFunds, UpgradeJournal, UpgradeJournalEntry,
UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Vote, WaitForQuietState,
UpgradeSnsControlledCanister, Vote, WaitForQuietState,
},
},
proposal::{
Expand Down Expand Up @@ -2434,6 +2434,22 @@ impl Governance {
}
}

pub fn upgrade_proposals_in_progress(&self) -> BTreeSet</* Proposal Id*/ u64> {
self.proto
.proposals
.iter()
.filter_map(|(id, proposal_data)| {
if proposal_data.status() == ProposalDecisionStatus::Adopted
&& proposal_data.is_upgrade_proposal()
{
Some(*id)
} else {
None
}
})
.collect::<BTreeSet<_>>()
}

/// Executes a UpgradeSnsControlledCanister proposal by calling the root canister
/// to upgrade an SNS controlled canister. This does not upgrade "core" SNS canisters
/// (i.e. Root, Governance, Ledger, Ledger Archives, or Sale)
Expand All @@ -2442,7 +2458,7 @@ impl Governance {
proposal_id: u64,
upgrade: UpgradeSnsControlledCanister,
) -> Result<(), GovernanceError> {
err_if_another_upgrade_is_in_progress(&self.proto.proposals, proposal_id)?;
self.check_no_other_upgrades_in_progress(proposal_id)?;

let sns_canisters =
get_all_sns_canisters(&*self.env, self.proto.root_canister_id_or_panic())
Expand Down Expand Up @@ -2529,13 +2545,45 @@ impl Governance {
})
}

pub fn check_no_other_upgrades_in_progress(
&self,
proposal_id: u64,
) -> Result<(), GovernanceError> {
let upgrade_proposals_in_progress = self.upgrade_proposals_in_progress();
if !upgrade_proposals_in_progress.is_empty()
&& upgrade_proposals_in_progress != BTreeSet::from([proposal_id])
{
return Err(GovernanceError::new_with_message(
ErrorType::ResourceExhausted,
format!(
"Another upgrade is currently in progress (proposal IDs {}). \
Please, try again later.",
upgrade_proposals_in_progress
.into_iter()
.map(|id| id.to_string())
.collect::<Vec<String>>()
.join(", ")
),
));
}

if self.proto.pending_version.is_some() {
return Err(GovernanceError::new_with_message(
ErrorType::ResourceExhausted,
"Upgrade lock currently acquired, not upgrading".to_string(),
));
}

Ok(())
}

/// Return `Ok(true)` if the upgrade was completed successfully, return `Ok(false)` if an
/// upgrade was successfully kicked-off, but its completion is pending.
async fn perform_upgrade_to_next_sns_version(
&mut self,
proposal_id: u64,
) -> Result<bool, GovernanceError> {
err_if_another_upgrade_is_in_progress(&self.proto.proposals, proposal_id)?;
self.check_no_other_upgrades_in_progress(proposal_id)?;

let current_version = self.proto.deployed_version_or_panic();
let root_canister_id = self.proto.root_canister_id_or_panic();
Expand Down Expand Up @@ -2735,7 +2783,7 @@ impl Governance {
proposal_id: u64,
manage_ledger_parameters: ManageLedgerParameters,
) -> Result<(), GovernanceError> {
err_if_another_upgrade_is_in_progress(&self.proto.proposals, proposal_id)?;
self.check_no_other_upgrades_in_progress(proposal_id)?;

let current_version = self.proto.deployed_version_or_panic();
let ledger_canister_id = self.proto.ledger_canister_id_or_panic();
Expand Down Expand Up @@ -5745,42 +5793,6 @@ thread_local! {
static ATTEMPTED_FIXING_MEMORY_ALLOCATIONS: RefCell<bool> = const { RefCell::new(false) };
}

fn err_if_another_upgrade_is_in_progress(
id_to_proposal_data: &BTreeMap</* proposal ID */ u64, ProposalData>,
executing_proposal_id: u64,
) -> Result<(), GovernanceError> {
let upgrade_action_ids: [u64; 3] = [
(&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into(),
(&Action::UpgradeSnsToNextVersion(UpgradeSnsToNextVersion::default())).into(),
(&Action::ManageLedgerParameters(ManageLedgerParameters::default())).into(),
];

for (other_proposal_id, proposal_data) in id_to_proposal_data {
if *other_proposal_id == executing_proposal_id {
continue;
}

if !upgrade_action_ids.contains(&proposal_data.action) {
continue;
}

if proposal_data.status() != ProposalDecisionStatus::Adopted {
continue;
}

return Err(GovernanceError::new_with_message(
ErrorType::ResourceExhausted,
format!(
"Another upgrade is currently in progress (proposal ID {}). \
Please, try again later.",
other_proposal_id,
),
));
}

Ok(())
}

/// Affects the perception of time by users of CanisterEnv (i.e. Governance).
///
/// Specifically, the time that Governance sees is the real time + delta.
Expand Down Expand Up @@ -7070,6 +7082,9 @@ mod tests {
Box::new(FakeCmc::new()),
);

let upgrade_proposals_in_progress = governance.upgrade_proposals_in_progress();
assert_eq!(upgrade_proposals_in_progress, BTreeSet::from([1]));

// Step 2: Execute code under test.
governance.process_proposal(2);

Expand Down Expand Up @@ -7112,15 +7127,20 @@ mod tests {
"The second upgrade proposal did not fail. final_proposal_data: {:#?}",
final_proposal_data,
);
assert_eq!(
let final_failure_reason = ErrorType::try_from(
final_proposal_data
.failure_reason
.as_ref()
.unwrap()
.error_type,
ErrorType::ResourceExhausted as i32,
"The second upgrade proposal failed, but failure_reason was not as expected. \
)
.unwrap();
assert_eq!(
final_failure_reason,
ErrorType::ResourceExhausted,
"The second upgrade proposal failed, but failure_reason ({:?}) was not as expected. \
final_proposal_data: {:#?}",
final_failure_reason,
final_proposal_data,
);
}
Expand Down Expand Up @@ -8841,14 +8861,24 @@ mod tests {
};
assert_eq!(proposal.status(), Status::Adopted);

let governance = Governance::new(
GovernanceProto {
proposals: btreemap! {
proposal_id => proposal,
},
..basic_governance_proto()
}
.try_into()
.unwrap(),
Box::<NativeEnvironment>::default(),
Box::new(DoNothingLedger {}),
Box::new(DoNothingLedger {}),
Box::new(FakeCmc::new()),
);

// Step 2: Run code under test.
let some_other_proposal_id = 99_u64;
let result = err_if_another_upgrade_is_in_progress(
&btreemap! {
proposal_id => proposal,
},
some_other_proposal_id,
);
let result = governance.check_no_other_upgrades_in_progress(some_other_proposal_id);

// Step 3: Inspect result.
assert!(result.is_ok(), "{:#?}", result);
Expand Down Expand Up @@ -8876,14 +8906,24 @@ mod tests {
};
assert_eq!(proposal.status(), Status::Open);

let governance = Governance::new(
GovernanceProto {
proposals: btreemap! {
proposal_id => proposal,
},
..basic_governance_proto()
}
.try_into()
.unwrap(),
Box::<NativeEnvironment>::default(),
Box::new(DoNothingLedger {}),
Box::new(DoNothingLedger {}),
Box::new(FakeCmc::new()),
);

// Step 2: Run code under test.
let some_other_proposal_id = 99_u64;
let result = err_if_another_upgrade_is_in_progress(
&btreemap! {
proposal_id => proposal,
},
some_other_proposal_id,
);
let result = governance.check_no_other_upgrades_in_progress(some_other_proposal_id);

// Step 3: Inspect result.
assert!(result.is_ok(), "{:#?}", result);
Expand Down Expand Up @@ -8913,14 +8953,24 @@ mod tests {
};
assert_eq!(proposal.status(), Status::Executed);

let governance = Governance::new(
GovernanceProto {
proposals: btreemap! {
proposal_id => proposal,
},
..basic_governance_proto()
}
.try_into()
.unwrap(),
Box::<NativeEnvironment>::default(),
Box::new(DoNothingLedger {}),
Box::new(DoNothingLedger {}),
Box::new(FakeCmc::new()),
);

// Step 2: Run code under test.
let some_other_proposal_id = 99_u64;
let result = err_if_another_upgrade_is_in_progress(
&btreemap! {
proposal_id => proposal,
},
some_other_proposal_id,
);
let result = governance.check_no_other_upgrades_in_progress(some_other_proposal_id);

// Step 3: Inspect result.
assert!(result.is_ok(), "{:#?}", result);
Expand Down Expand Up @@ -8949,17 +8999,93 @@ mod tests {
};
assert_eq!(proposal.status(), Status::Adopted);

let proposals = btreemap! {
proposal_id => proposal,
};
let governance = Governance::new(
GovernanceProto {
proposals: btreemap! {
proposal_id => proposal,
},
..basic_governance_proto()
}
.try_into()
.unwrap(),
Box::<NativeEnvironment>::default(),
Box::new(DoNothingLedger {}),
Box::new(DoNothingLedger {}),
Box::new(FakeCmc::new()),
);

// Step 2 & 3: Run code under test, and inspect results.
let result = err_if_another_upgrade_is_in_progress(&proposals, proposal_id);
let result = governance.check_no_other_upgrades_in_progress(proposal_id);
assert!(result.is_ok(), "{:#?}", result);

// Other upgrades should be blocked by proposal 1 though.
let some_other_proposal_id = 99_u64;
match err_if_another_upgrade_is_in_progress(&proposals, some_other_proposal_id) {
match governance.check_no_other_upgrades_in_progress(some_other_proposal_id) {
Ok(_) => panic!("Some other upgrade proposal was not blocked."),
Err(err) => assert_eq!(
err.error_type,
ErrorType::ResourceExhausted as i32,
"{:#?}",
err,
),
}
}

#[test]
fn test_upgrade_proposals_blocked_by_pending_upgrade() {
// Step 1: Prepare the world.
use ProposalDecisionStatus as Status;

let upgrade_action_id: u64 =
(&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into();

let proposal_id = 1_u64;
let proposal = ProposalData {
action: upgrade_action_id,
id: Some(proposal_id.into()),
decided_timestamp_seconds: 1,
latest_tally: Some(Tally {
yes: 1,
no: 0,
total: 1,
timestamp_seconds: 1,
}),
..Default::default()
};
assert_eq!(proposal.status(), Status::Adopted);

let governance = Governance::new(
GovernanceProto {
proposals: btreemap! {
proposal_id => proposal,
},
// There's already an upgrade pending
pending_version: Some(UpgradeInProgress {
..Default::default()
}),
..basic_governance_proto()
}
.try_into()
.unwrap(),
Box::<NativeEnvironment>::default(),
Box::new(DoNothingLedger {}),
Box::new(DoNothingLedger {}),
Box::new(FakeCmc::new()),
);

// Step 2 & 3: Run code under test, and inspect results.
match governance.check_no_other_upgrades_in_progress(proposal_id) {
Ok(_) => panic!("Some other upgrade proposal was not blocked."),
Err(err) => assert_eq!(
err.error_type,
ErrorType::ResourceExhausted as i32,
"{:#?}",
err,
),
}

let some_other_proposal_id = 99_u64;
match governance.check_no_other_upgrades_in_progress(some_other_proposal_id) {
Ok(_) => panic!("Some other upgrade proposal was not blocked."),
Err(err) => assert_eq!(
err.error_type,
Expand Down
24 changes: 24 additions & 0 deletions rs/sns/governance/src/proposal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2197,6 +2197,30 @@ impl ProposalData {
ballots: limited_ballots,
}
}

/// "Upgrade proposals" are those that upgrade the SNS or a canister it controls.
pub(crate) fn is_upgrade_proposal(&self) -> bool {
let action_is_upgrade = matches!(
self.proposal,
Some(Proposal {
action: Some(
Action::UpgradeSnsControlledCanister(_)
| Action::UpgradeSnsToNextVersion(_)
| Action::ManageLedgerParameters(_)
),
..
})
);
// In production, the above condition is exactly what we want. However, in some tests, we only set the action_id
// and not the action.
let upgrade_action_ids: [u64; 3] = [
(&Action::UpgradeSnsControlledCanister(UpgradeSnsControlledCanister::default())).into(),
(&Action::UpgradeSnsToNextVersion(UpgradeSnsToNextVersion::default())).into(),
(&Action::ManageLedgerParameters(ManageLedgerParameters::default())).into(),
];
let action_id_is_upgrade = upgrade_action_ids.contains(&self.action);
action_is_upgrade || action_id_is_upgrade
}
}

impl ProposalDecisionStatus {
Expand Down

0 comments on commit c559561

Please sign in to comment.