diff --git a/rs/nns/governance/api/src/types.rs b/rs/nns/governance/api/src/types.rs index 8f5a7d008167..8cfd684bec98 100644 --- a/rs/nns/governance/api/src/types.rs +++ b/rs/nns/governance/api/src/types.rs @@ -2065,6 +2065,7 @@ pub struct ProposalInfo { #[derive(candid::CandidType, candid::Deserialize, serde::Serialize, Clone, PartialEq, Debug)] pub enum SuccessfulProposalExecutionValue { CreateCanisterAndInstallCode(CreateCanisterAndInstallCodeOk), + TakeCanisterSnapshot(TakeCanisterSnapshotOk), } /// The result of a successful CreateCanisterAndInstallCode proposal execution. @@ -2073,6 +2074,24 @@ pub struct CreateCanisterAndInstallCodeOk { pub canister_id: Option, } +/// The result of a successful TakeCanisterSnapshot proposal execution. +#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, Clone, PartialEq, Debug)] +pub struct TakeCanisterSnapshotOk { + pub snapshot_id: Vec, +} + +impl From for SuccessfulProposalExecutionValue { + fn from(ok: CreateCanisterAndInstallCodeOk) -> Self { + Self::CreateCanisterAndInstallCode(ok) + } +} + +impl From for SuccessfulProposalExecutionValue { + fn from(ok: TakeCanisterSnapshotOk) -> Self { + Self::TakeCanisterSnapshot(ok) + } +} + /// Network economics contains the parameters for several operations related /// to the economy of the network. When submitting a NetworkEconomics proposal /// default values (0) are considered unchanged, so a valid proposal only needs diff --git a/rs/nns/governance/canister/governance.did b/rs/nns/governance/canister/governance.did index 2260c7dd1354..d869241e657b 100644 --- a/rs/nns/governance/canister/governance.did +++ b/rs/nns/governance/canister/governance.did @@ -251,8 +251,13 @@ type CreateCanisterAndInstallCodeOk = record { canister_id : opt principal; }; +type TakeCanisterSnapshotOk = record { + snapshot_id : blob; +}; + type SuccessfulProposalExecutionValue = variant { CreateCanisterAndInstallCode : CreateCanisterAndInstallCodeOk; + TakeCanisterSnapshot : TakeCanisterSnapshotOk; }; type CreateServiceNervousSystem = record { diff --git a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto index 3361a55b2b26..7443ba903405 100644 --- a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto +++ b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto @@ -1261,6 +1261,7 @@ message ProposalData { message SuccessfulProposalExecutionValue { oneof proposal_type { CreateCanisterAndInstallCodeOk create_canister_and_install_code = 1; + TakeCanisterSnapshotOk take_canister_snapshot = 2; } } @@ -1268,6 +1269,10 @@ message CreateCanisterAndInstallCodeOk { ic_base_types.pb.v1.PrincipalId canister_id = 1; } +message TakeCanisterSnapshotOk { + bytes snapshot_id = 1; +} + // This structure contains data for settling the Neurons' Fund participation in an SNS token swap. message NeuronsFundData { // Initial Neurons' Fund reserves computed at the time of execution of the proposal through which diff --git a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs index 4451398efc45..c641f3afaba1 100644 --- a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs +++ b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs @@ -1542,7 +1542,7 @@ pub struct ProposalData { pub struct SuccessfulProposalExecutionValue { #[prost( oneof = "successful_proposal_execution_value::ProposalType", - tags = "1" + tags = "1, 2" )] pub proposal_type: ::core::option::Option, } @@ -1560,6 +1560,8 @@ pub mod successful_proposal_execution_value { pub enum ProposalType { #[prost(message, tag = "1")] CreateCanisterAndInstallCode(super::CreateCanisterAndInstallCodeOk), + #[prost(message, tag = "2")] + TakeCanisterSnapshot(super::TakeCanisterSnapshotOk), } } #[derive( @@ -1575,6 +1577,19 @@ pub struct CreateCanisterAndInstallCodeOk { #[prost(message, optional, tag = "1")] pub canister_id: ::core::option::Option<::ic_base_types::PrincipalId>, } +#[derive( + candid::CandidType, + candid::Deserialize, + serde::Serialize, + comparable::Comparable, + Clone, + PartialEq, + ::prost::Message, +)] +pub struct TakeCanisterSnapshotOk { + #[prost(bytes = "vec", tag = "1")] + pub snapshot_id: ::prost::alloc::vec::Vec, +} /// This structure contains data for settling the Neurons' Fund participation in an SNS token swap. #[derive( candid::CandidType, diff --git a/rs/nns/governance/src/pb/conversions/mod.rs b/rs/nns/governance/src/pb/conversions/mod.rs index bd9716e3eb85..ece01cd112e7 100644 --- a/rs/nns/governance/src/pb/conversions/mod.rs +++ b/rs/nns/governance/src/pb/conversions/mod.rs @@ -1771,20 +1771,18 @@ impl From for pb::DerivedProposalInformation { } } -impl From for api::SuccessfulProposalExecutionValue { +impl From for Option { fn from(item: pb::SuccessfulProposalExecutionValue) -> Self { - match item.proposal_type { - Some(ProposalType::CreateCanisterAndInstallCode(ok)) => { + let result = match item.proposal_type? { + ProposalType::CreateCanisterAndInstallCode(ok) => { api::SuccessfulProposalExecutionValue::CreateCanisterAndInstallCode(ok.into()) } - None => { - // This shouldn't happen, but if it does, we need a fallback. - // Use a CreateCanisterAndInstallCode with None canister_id. - api::SuccessfulProposalExecutionValue::CreateCanisterAndInstallCode( - api::CreateCanisterAndInstallCodeOk { canister_id: None }, - ) + ProposalType::TakeCanisterSnapshot(ok) => { + api::SuccessfulProposalExecutionValue::TakeCanisterSnapshot(ok.into()) } - } + }; + + Some(result) } } @@ -1830,6 +1828,35 @@ impl From for pb::SuccessfulProposalExecutio } } +impl From for pb::TakeCanisterSnapshotOk { + fn from(ok: root::TakeCanisterSnapshotOk) -> Self { + Self { snapshot_id: ok.id } + } +} + +impl From for pb::SuccessfulProposalExecutionValue { + fn from(ok: root::TakeCanisterSnapshotOk) -> Self { + Self::from(pb::TakeCanisterSnapshotOk::from(ok)) + } +} + +// Upgrade Ok to result. +impl From for pb::SuccessfulProposalExecutionValue { + fn from(ok: pb::TakeCanisterSnapshotOk) -> Self { + Self { + proposal_type: Some(ProposalType::TakeCanisterSnapshot(ok)), + } + } +} + +impl From for api::TakeCanisterSnapshotOk { + fn from(item: pb::TakeCanisterSnapshotOk) -> Self { + Self { + snapshot_id: item.snapshot_id, + } + } +} + impl From for api::SwapBackgroundInformation { fn from(item: pb::SwapBackgroundInformation) -> Self { Self { diff --git a/rs/nns/governance/src/pb/proposal_conversions.rs b/rs/nns/governance/src/pb/proposal_conversions.rs index f763a21fa0a8..ff34ce9da023 100644 --- a/rs/nns/governance/src/pb/proposal_conversions.rs +++ b/rs/nns/governance/src/pb/proposal_conversions.rs @@ -566,7 +566,7 @@ pub(crate) fn proposal_data_to_info( let success_value = data .success_value .clone() - .map(|success_value| success_value.into()); + .and_then(Option::::from); let proposal = data .proposal diff --git a/rs/nns/governance/src/proposals/load_canister_snapshot.rs b/rs/nns/governance/src/proposals/load_canister_snapshot.rs index 88a895973806..14d3bdbd2add 100644 --- a/rs/nns/governance/src/proposals/load_canister_snapshot.rs +++ b/rs/nns/governance/src/proposals/load_canister_snapshot.rs @@ -45,7 +45,6 @@ impl LoadCanisterSnapshot { } impl CallCanister for LoadCanisterSnapshot { - // TODO: Use a typed reply instead of () so that the result is recorded. type Reply = (); fn canister_and_function(&self) -> Result<(CanisterId, &str), GovernanceError> { diff --git a/rs/nns/governance/src/proposals/take_canister_snapshot.rs b/rs/nns/governance/src/proposals/take_canister_snapshot.rs index ff5702020360..da5519953817 100644 --- a/rs/nns/governance/src/proposals/take_canister_snapshot.rs +++ b/rs/nns/governance/src/proposals/take_canister_snapshot.rs @@ -1,14 +1,18 @@ use crate::{ - pb::v1::{GovernanceError, TakeCanisterSnapshot, Topic}, + pb::v1::{GovernanceError, TakeCanisterSnapshot, Topic, governance_error::ErrorType}, proposals::{ - call_canister::CallCanister, invalid_proposal_error, self_describing::DocumentedAction, + call_canister::{CallCanister, CallCanisterReply}, + invalid_proposal_error, + self_describing::DocumentedAction, topic_to_manage_canister, }, }; -use candid::Encode; +use candid::{Decode, Encode}; use ic_base_types::{CanisterId, PrincipalId}; use ic_nns_constants::ROOT_CANISTER_ID; -use ic_nns_handler_root_interface::TakeCanisterSnapshotRequest; +use ic_nns_handler_root_interface::{ + TakeCanisterSnapshotOk, TakeCanisterSnapshotRequest, TakeCanisterSnapshotResponse, +}; impl TakeCanisterSnapshot { pub fn validate(&self) -> Result<(), GovernanceError> { @@ -33,8 +37,7 @@ impl TakeCanisterSnapshot { } impl CallCanister for TakeCanisterSnapshot { - // TODO: Use a typed reply instead of () so that the result is recorded. - type Reply = (); + type Reply = TakeCanisterSnapshotOk; fn canister_and_function(&self) -> Result<(CanisterId, &str), GovernanceError> { Ok((ROOT_CANISTER_ID, "take_canister_snapshot")) @@ -47,6 +50,25 @@ impl CallCanister for TakeCanisterSnapshot { } } +impl CallCanisterReply for TakeCanisterSnapshotOk { + fn try_decode(encoded_reply: &[u8]) -> Result, GovernanceError> { + let response = Decode!(encoded_reply, TakeCanisterSnapshotResponse).map_err(|err| { + GovernanceError::new_with_message( + ErrorType::External, + format!("Failed to decode TakeCanisterSnapshotResponse: {err}"), + ) + })?; + + match response { + TakeCanisterSnapshotResponse::Ok(ok) => Ok(Some(ok)), + TakeCanisterSnapshotResponse::Err(err) => Err(GovernanceError::new_with_message( + ErrorType::External, + format!("Root returned error for TakeCanisterSnapshot: {:?}", err), + )), + } + } +} + pub fn convert_take_canister_snapshot_from_proposal_to_root_request( original: &TakeCanisterSnapshot, ) -> Result { diff --git a/rs/nns/governance/unreleased_changelog.md b/rs/nns/governance/unreleased_changelog.md index 94126a0ff421..ccd9b45e43f1 100644 --- a/rs/nns/governance/unreleased_changelog.md +++ b/rs/nns/governance/unreleased_changelog.md @@ -9,6 +9,9 @@ on the process that this file is part of, see ## Added +* `TakeCanisterSnapshot` proposals now store the new snapshot ID in the + `success_value` field. + ## Changed ## Deprecated diff --git a/rs/nns/integration_tests/src/canister_snapshot.rs b/rs/nns/integration_tests/src/canister_snapshot.rs index ec2fe85ed688..25b4ad6914cd 100644 --- a/rs/nns/integration_tests/src/canister_snapshot.rs +++ b/rs/nns/integration_tests/src/canister_snapshot.rs @@ -1,5 +1,5 @@ use candid::{Nat, Principal}; -use ic_base_types::PrincipalId; +use ic_base_types::{CanisterId, PrincipalId, SnapshotId}; use ic_ledger_core::tokens::{CheckedAdd, CheckedSub}; use ic_management_canister_types_private::CanisterSnapshotResponse; use ic_nervous_system_common::E8; @@ -10,7 +10,10 @@ use ic_nervous_system_integration_tests::pocket_ic_helpers::{ }; use ic_nns_constants::{GOVERNANCE_CANISTER_ID, LEDGER_CANISTER_ID, ROOT_CANISTER_ID}; use ic_nns_governance::pb::v1::ProposalStatus; -use ic_nns_governance_api::{MakeProposalRequest, Motion, ProposalActionRequest}; +use ic_nns_governance_api::{ + MakeProposalRequest, Motion, ProposalActionRequest, SuccessfulProposalExecutionValue, + TakeCanisterSnapshotOk, +}; use icp_ledger::{AccountIdentifier, DEFAULT_TRANSFER_FEE, Tokens}; use icrc_ledger_types::icrc1::{account::Account, transfer::TransferArg}; use pocket_ic::{PocketIcBuilder, nonblocking::PocketIc}; @@ -181,6 +184,16 @@ async fn test_canister_snapshot() { } assert_snapshot_seems_reasonable(first_snapshot); + // Step 3A.2: Verify that the snapshot ID is recorded in success_value. + let snapshot_id_from_proposal = match &first_proposal_info.success_value { + Some(SuccessfulProposalExecutionValue::TakeCanisterSnapshot(ok)) => ok.snapshot_id.clone(), + other => panic!("Expected TakeCanisterSnapshot success value, got: {other:#?}"), + }; + assert_eq!( + snapshot_id_from_proposal, + first_snapshot.snapshot_id().as_slice(), + ); + // Scenario B: Replace an existing snapshot. // Step 2(B): Run code under test. @@ -237,6 +250,16 @@ async fn test_canister_snapshot() { let second_snapshot = &list_canister_snapshots_response[0]; assert_ne!(second_snapshot.snapshot_id(), first_snapshot.snapshot_id()); + // Also verify that the replace proposal's success_value records the new snapshot ID. + let snapshot_id_from_replace_proposal = match &replace_proposal_info.success_value { + Some(SuccessfulProposalExecutionValue::TakeCanisterSnapshot(ok)) => ok.snapshot_id.clone(), + other => panic!("Expected TakeCanisterSnapshot success value, got: {other:#?}"), + }; + assert_eq!( + snapshot_id_from_replace_proposal, + second_snapshot.snapshot_id().as_slice(), + ); + // Generic checks of the second snapshot. assert_snapshot_seems_reasonable(second_snapshot); @@ -450,7 +473,7 @@ async fn test_governance_canister_snapshot() { "Take a snapshot of the Governance canister.".to_string(), ], ); - let _take_snapshot_proposal_id = extract_proposal_id(&output); + let take_canister_snapshot_proposal_id = extract_proposal_id(&output); // Wait for the effect of the proposal happen. This cannot be done in // the normal way (i.e. wait for the proposal status to become Executed, @@ -474,6 +497,22 @@ async fn test_governance_canister_snapshot() { let snapshot = &snapshots[0]; assert_eq!(snapshot.id.get_canister_id(), GOVERNANCE_CANISTER_ID); + // Verify that success_value is populated. When the target is Governance, + // Root returns a placeholder/optimistic response to avoid a deadlock + // (it cannot stop Governance while Governance is waiting for it). The + // snapshot ID in that placeholder is zeroed. + let take_canister_snapshot_proposal_info = + nns::governance::get_proposal_info(&pocket_ic, take_canister_snapshot_proposal_id) + .await + .unwrap(); + let optimistic_snapshot_id = SnapshotId::from((CanisterId::from_u64(0), 0_u64)).to_vec(); + assert_eq!( + take_canister_snapshot_proposal_info.success_value.unwrap(), + SuccessfulProposalExecutionValue::from(TakeCanisterSnapshotOk { + snapshot_id: optimistic_snapshot_id, + }), + ); + // Change Governance's state by creating ANOTHER (Motion) proposal. // This will get blown away at the end when we load the snapshot, // because the snapshot was taken before this proposal.