From df7d443e6219c462b305152b63ca265171feb6ee Mon Sep 17 00:00:00 2001 From: Andre Popovitch Date: Fri, 10 Jan 2025 15:26:03 -0600 Subject: [PATCH] fix: SNS Gov canister should deserialize to proto Governance, not API Governance (#3391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I think this was a bug in the code previously. [here](https://github.com/dfinity/ic/blob/4b9b18fcada06a30f9d2286eddcacff6521ace47/rs/sns/governance/canister/canister.rs#L258-L270) is where we serialize Governance: ```rust #[pre_upgrade] fn canister_pre_upgrade() { log!(INFO, "Executing pre upgrade"); let mut writer = BufferedStableMemWriter::new(STABLE_MEM_BUFFER_SIZE); governance() .proto // This gets the `Governance` that's from `ic-sns-governance` .encode(&mut writer) .expect("Error. Couldn't serialize canister pre-upgrade."); writer.flush(); // or `drop(writer)` log!(INFO, "Completed pre upgrade"); } ``` And this is where we were deserializing it: ```rust #[post_upgrade] fn canister_post_upgrade() { log!(INFO, "Executing post upgrade"); let reader = BufferedStableMemReader::new(STABLE_MEM_BUFFER_SIZE); match GovernanceProto::decode(reader) { // this is deserializing to the Governance that's in `ic-sns-governance-api` Err(err) => { // [...] } Ok(mut governance_proto) => { // Post-process GovernanceProto // TODO: Delete this once it's been released. populate_finalize_disbursement_timestamp_seconds(&mut governance_proto); canister_init_(governance_proto); // then in here we pass to canister_init_ Ok(()) } } .expect("Couldn't upgrade canister."); // [...] } ``` `canister_init_`: ```rust #[init] fn canister_init_(init_payload: GovernanceProto) { let init_payload = sns_gov_pb::Governance::from(init_payload); // we convert the `ic-sns-governance-api` Governance to the `ic-sns-governance` Governance // [...] } ``` This seems not quite right to me because we serialize one Governance (`ic-sns-governance`), but deserialize to the other Governance (`ic-sns-governance-api`) and then convert. Why not just deserialize to the `ic-sns-governance` Governance directly? This only works because the `ic-sns-governance-api` types still set `prost` derives, but I don't think we want to be protobuffing the `ic-sns-governance-api` types anyway. (In fact, since we don't want to be protobuffing them, I actually remove their prost implementations in the next PR) --- However, we should still initialize the canister using the API types. Doing this ensures that no proto types are part of our public candid interface. So the `canister_init` method has been restored and annotated with `#[init]`, and takes the API type like `canister_init_` did previously. Now `canister_init_` takes the proto type, so it can be used by `canister_post_upgrade` and by `canister_init` after `canister_init` does the conversion. --- [← Previous PR](https://github.com/dfinity/ic/pull/3393) | [Next PR →](https://github.com/dfinity/ic/pull/3392) --- rs/sns/governance/canister/canister.rs | 14 ++++++++------ rs/sns/governance/canister/tests.rs | 16 +++++++--------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/rs/sns/governance/canister/canister.rs b/rs/sns/governance/canister/canister.rs index 2c5e83ebaf4..f29c9e353b0 100644 --- a/rs/sns/governance/canister/canister.rs +++ b/rs/sns/governance/canister/canister.rs @@ -35,7 +35,7 @@ use ic_sns_governance_api::pb::v1::{ GetModeResponse, GetNeuron, GetNeuronResponse, GetProposal, GetProposalResponse, GetRunningSnsVersionRequest, GetRunningSnsVersionResponse, GetSnsInitializationParametersRequest, GetSnsInitializationParametersResponse, - GetUpgradeJournalRequest, GetUpgradeJournalResponse, Governance as GovernanceProto, + GetUpgradeJournalRequest, GetUpgradeJournalResponse, Governance as GovernanceApi, ListNervousSystemFunctionsResponse, ListNeurons, ListNeuronsResponse, ListProposals, ListProposalsResponse, ManageNeuron, ManageNeuronResponse, NervousSystemParameters, RewardEvent, SetMode, SetModeResponse, @@ -208,11 +208,13 @@ fn caller() -> PrincipalId { PrincipalId::from(cdk_caller()) } -/// In contrast to canister_init(), this method does not do deserialization. -/// In addition to canister_init, this method is called by canister_post_upgrade. #[init] -fn canister_init_(init_payload: GovernanceProto) { +fn canister_init(init_payload: GovernanceApi) { let init_payload = sns_gov_pb::Governance::from(init_payload); + canister_init_(init_payload); +} + +fn canister_init_(init_payload: sns_gov_pb::Governance) { let init_payload = ValidGovernanceProto::try_from(init_payload).expect( "Cannot start canister, because the deserialized \ GovernanceProto is invalid in some way", @@ -277,7 +279,7 @@ fn canister_post_upgrade() { let reader = BufferedStableMemReader::new(STABLE_MEM_BUFFER_SIZE); - match GovernanceProto::decode(reader) { + match sns_gov_pb::Governance::decode(reader) { Err(err) => { log!( ERROR, @@ -304,7 +306,7 @@ fn canister_post_upgrade() { log!(INFO, "Completed post upgrade"); } -fn populate_finalize_disbursement_timestamp_seconds(governance_proto: &mut GovernanceProto) { +fn populate_finalize_disbursement_timestamp_seconds(governance_proto: &mut sns_gov_pb::Governance) { for neuron in governance_proto.neurons.values_mut() { for disbursement in neuron.disburse_maturity_in_progress.iter_mut() { disbursement.finalize_disbursement_timestamp_seconds = Some( diff --git a/rs/sns/governance/canister/tests.rs b/rs/sns/governance/canister/tests.rs index f5c7e7a90c2..496348898b1 100644 --- a/rs/sns/governance/canister/tests.rs +++ b/rs/sns/governance/canister/tests.rs @@ -1,7 +1,11 @@ use super::*; use assert_matches::assert_matches; use candid_parser::utils::{service_equal, CandidSource}; -use ic_sns_governance_api::pb::v1::{DisburseMaturityInProgress, Neuron}; +use ic_sns_governance::pb::v1::{ + governance::{Version, Versions}, + upgrade_journal_entry::{Event, UpgradeStepsRefreshed}, + DisburseMaturityInProgress, Neuron, UpgradeJournal, UpgradeJournalEntry, +}; use maplit::btreemap; use pretty_assertions::assert_eq; use std::collections::HashSet; @@ -61,7 +65,7 @@ fn test_set_time_warp() { fn test_populate_finalize_disbursement_timestamp_seconds() { // Step 1: prepare a neuron with 2 in progress disbursement, one with // finalize_disbursement_timestamp_seconds as None, and the other has incorrect timestamp. - let mut governance_proto = GovernanceProto { + let mut governance_proto = sns_gov_pb::Governance { neurons: btreemap! { "1".to_string() => Neuron { disburse_maturity_in_progress: vec![ @@ -86,7 +90,7 @@ fn test_populate_finalize_disbursement_timestamp_seconds() { populate_finalize_disbursement_timestamp_seconds(&mut governance_proto); // Step 3: verifies that both disbursements have the correct finalization timestamps. - let expected_governance_proto = GovernanceProto { + let expected_governance_proto = sns_gov_pb::Governance { neurons: btreemap! { "1".to_string() => Neuron { disburse_maturity_in_progress: vec![ @@ -111,12 +115,6 @@ fn test_populate_finalize_disbursement_timestamp_seconds() { #[test] fn test_upgrade_journal() { - use ic_sns_governance::pb::v1::{ - governance::{Version, Versions}, - upgrade_journal_entry::{Event, UpgradeStepsRefreshed}, - UpgradeJournal, UpgradeJournalEntry, - }; - let journal = UpgradeJournal { entries: vec![UpgradeJournalEntry { timestamp_seconds: Some(1000),