From 08e698506a284fe79c56beaea7dfd6b6c2a0030b Mon Sep 17 00:00:00 2001 From: max-dfinity <100170574+max-dfinity@users.noreply.github.com> Date: Thu, 10 Oct 2024 09:35:37 -0700 Subject: [PATCH] refactor(sns): migrate sns governance to use ic_cdk (#1941) This migrates sns governance to use ic_cdk instead of dfn_core --------- Co-authored-by: Arshavir Ter-Gabrielyan --- Cargo.lock | 4 +- rs/sns/governance/BUILD.bazel | 6 +- rs/sns/governance/Cargo.toml | 5 +- rs/sns/governance/canister/canister.rs | 343 ++++++------------- rs/sns/governance/src/canister_control.rs | 3 +- rs/sns/governance/src/governance.rs | 41 ++- rs/sns/governance/src/proposal.rs | 6 +- rs/sns/governance/src/types.rs | 13 +- rs/sns/governance/token_valuation/src/lib.rs | 2 +- rs/sns/integration_tests/src/proposals.rs | 8 +- 10 files changed, 167 insertions(+), 264 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4e4b4f8db67..90d31be3398 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11491,8 +11491,6 @@ dependencies = [ "candid_parser", "clap 3.2.25", "comparable", - "dfn_candid", - "dfn_core", "futures", "hex", "ic-base-types", @@ -11500,6 +11498,8 @@ dependencies = [ "ic-canister-log 0.2.0", "ic-canister-profiler", "ic-canisters-http-types", + "ic-cdk 0.16.0", + "ic-cdk-macros 0.9.0", "ic-crypto-sha2", "ic-icrc1-ledger", "ic-ledger-core", diff --git a/rs/sns/governance/BUILD.bazel b/rs/sns/governance/BUILD.bazel index 2303157bdb4..16fa794b719 100644 --- a/rs/sns/governance/BUILD.bazel +++ b/rs/sns/governance/BUILD.bazel @@ -32,8 +32,6 @@ DEPENDENCIES = [ "//rs/protobuf", "//rs/rust_canisters/canister_log", "//rs/rust_canisters/canister_profiler", - "//rs/rust_canisters/dfn_candid", - "//rs/rust_canisters/dfn_core", "//rs/rust_canisters/http_types", "//rs/sns/governance/proposal_criticality", "//rs/sns/governance/proposals_amount_total_limit", @@ -46,7 +44,9 @@ DEPENDENCIES = [ "@crate_index//:candid", "@crate_index//:clap_3_2_25", "@crate_index//:comparable", + "@crate_index//:futures", "@crate_index//:hex", + "@crate_index//:ic-cdk", "@crate_index//:ic-metrics-encoder", "@crate_index//:lazy_static", "@crate_index//:maplit", @@ -64,6 +64,7 @@ MACRO_DEPENDENCIES = [ # Keep sorted. "//rs/nervous_system/common/build_metadata", "@crate_index//:async-trait", + "@crate_index//:ic-cdk-macros", "@crate_index//:rust_decimal_macros", "@crate_index//:strum_macros", ] @@ -86,7 +87,6 @@ DEV_DEPENDENCIES = [ "//rs/test_utilities/types", "@crate_index//:assert_matches", "@crate_index//:candid_parser", - "@crate_index//:futures", "@crate_index//:pretty_assertions", "@crate_index//:proptest", "@crate_index//:tempfile", diff --git a/rs/sns/governance/Cargo.toml b/rs/sns/governance/Cargo.toml index 8fb7d27d0a3..b21b03dfcf4 100644 --- a/rs/sns/governance/Cargo.toml +++ b/rs/sns/governance/Cargo.toml @@ -33,10 +33,11 @@ base64 = { workspace = true } candid = { workspace = true } clap = { version = "3.2.25", features = ["derive"] } comparable = { version = "0.5", features = ["derive"] } -dfn_candid = { path = "../../rust_canisters/dfn_candid" } -dfn_core = { path = "../../rust_canisters/dfn_core" } hex = { workspace = true } +futures = { workspace = true } ic-base-types = { path = "../../types/base_types" } +ic-cdk = { workspace = true } +ic-cdk-macros = { workspace = true } ic-canisters-http-types = { path = "../../rust_canisters/http_types" } ic-canister-log = { path = "../../rust_canisters/canister_log" } ic-canister-profiler = { path = "../../rust_canisters/canister_profiler" } diff --git a/rs/sns/governance/canister/canister.rs b/rs/sns/governance/canister/canister.rs index f98d498c1c7..b29a8099bb8 100644 --- a/rs/sns/governance/canister/canister.rs +++ b/rs/sns/governance/canister/canister.rs @@ -10,16 +10,11 @@ // the did definition of the method. use async_trait::async_trait; -use candid::candid_method; -use dfn_candid::{candid, candid_one, candid_one_with_config, CandidOne}; -use dfn_core::{ - api::{call_bytes_with_cleanup, caller, id, now, Funds}, - over, over_async, over_init, -}; -use ic_base_types::CanisterId; +use ic_base_types::{CanisterId, PrincipalId}; use ic_canister_log::log; use ic_canister_profiler::{measure_span, measure_span_async}; use ic_canisters_http_types::{HttpRequest, HttpResponse, HttpResponseBuilder}; +use ic_cdk::{caller as cdk_caller, heartbeat, init, post_upgrade, pre_upgrade, query, update}; use ic_nervous_system_canisters::{cmc::CMCCanister, ledger::IcpLedgerCanister}; use ic_nervous_system_clients::{ canister_status::CanisterStatusResultV2, ledger_client::LedgerCanister, @@ -28,7 +23,7 @@ use ic_nervous_system_common::{ dfn_core_stable_mem_utils::{BufferedStableMemReader, BufferedStableMemWriter}, serve_logs, serve_logs_v2, serve_metrics, }; -use ic_nervous_system_runtime::DfnRuntime; +use ic_nervous_system_runtime::CdkRuntime; use ic_nns_constants::LEDGER_CANISTER_ID as NNS_LEDGER_CANISTER_ID; #[cfg(feature = "test")] use ic_sns_governance::pb::v1::{ @@ -57,7 +52,11 @@ use ic_sns_governance::{ use prost::Message; use rand::{RngCore, SeedableRng}; use rand_chacha::ChaCha20Rng; -use std::{boxed::Box, convert::TryFrom, time::SystemTime}; +use std::{ + boxed::Box, + convert::TryFrom, + time::{Duration, SystemTime}, +}; /// Size of the buffer for stable memory reads and writes. /// @@ -104,10 +103,7 @@ impl CanisterEnv { // unpredictability since the pseudo-random numbers could still be predicted after // inception. rng: { - let now_nanos = now() - .duration_since(SystemTime::UNIX_EPOCH) - .unwrap() - .as_nanos(); + let now_nanos = now_nanoseconds() as u128; let mut seed = [0u8; 32]; seed[..16].copy_from_slice(&now_nanos.to_be_bytes()); seed[16..32].copy_from_slice(&now_nanos.to_be_bytes()); @@ -121,12 +117,7 @@ impl CanisterEnv { #[async_trait] impl Environment for CanisterEnv { fn now(&self) -> u64 { - self.time_warp.apply( - now() - .duration_since(SystemTime::UNIX_EPOCH) - .expect("Could not get the duration.") - .as_secs(), - ) + self.time_warp.apply(now_seconds()) } fn set_time_warp(&mut self, new_time_warp: TimeWarp) { @@ -154,7 +145,11 @@ impl Environment for CanisterEnv { /* message: */ String, ), > { - call_bytes_with_cleanup(canister_id, method_name, &arg, Funds::zero()).await + // Due to object safety constraints in Rust, call_canister sends and returns bytes, so we are using + // call_raw here instead of call, which requires known candid types. + ic_cdk::api::call::call_raw(canister_id.get().0, method_name, &arg, 0) + .await + .map_err(|(rejection_code, message)| (Some(rejection_code as i32), message)) } #[cfg(target_arch = "wasm32")] @@ -176,23 +171,39 @@ impl Environment for CanisterEnv { /// Return the canister's ID. fn canister_id(&self) -> CanisterId { - id() + CanisterId::unchecked_from_principal(PrincipalId::from(ic_cdk::id())) } /// Return the canister version. fn canister_version(&self) -> Option { - Some(dfn_core::api::canister_version()) + Some(ic_cdk::api::canister_version()) + } +} + +fn now_nanoseconds() -> u64 { + if cfg!(target_arch = "wasm32") { + ic_cdk::api::time() + } else { + SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .expect("Failed to get time since epoch") + .as_nanos() + .try_into() + .expect("Failed to convert time to u64") } } -#[export_name = "canister_init"] -fn canister_init() { - over_init(|CandidOne(arg)| canister_init_(arg)) +fn now_seconds() -> u64 { + Duration::from_nanos(now_nanoseconds()).as_secs() +} + +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. -#[candid_method(init)] +#[init] fn canister_init_(init_payload: GovernanceProto) { let init_payload = ValidGovernanceProto::try_from(init_payload).expect( "Cannot start canister, because the deserialized \ @@ -217,8 +228,8 @@ fn canister_init_(init_payload: GovernanceProto) { init_payload, Box::new(CanisterEnv::new()), Box::new(LedgerCanister::new(ledger_canister_id)), - Box::new(IcpLedgerCanister::::new(NNS_LEDGER_CANISTER_ID)), - Box::new(CMCCanister::::new()), + Box::new(IcpLedgerCanister::::new(NNS_LEDGER_CANISTER_ID)), + Box::new(CMCCanister::::new()), ); let governance = if cfg!(feature = "test") { governance.enable_test_features() @@ -233,7 +244,7 @@ fn canister_init_(init_payload: GovernanceProto) { /// governance's state to stable memory so that it is preserved during the upgrade and can /// be deserialized again in canister_post_upgrade. That is, the stable memory allows /// saving the state and restoring it after the upgrade. -#[export_name = "canister_pre_upgrade"] +#[pre_upgrade] fn canister_pre_upgrade() { log!(INFO, "Executing pre upgrade"); @@ -250,9 +261,8 @@ fn canister_pre_upgrade() { /// Executes some logic after executing an upgrade, including deserializing what has been written /// to stable memory in canister_pre_upgrade and initializing the governance's state with it. -#[export_name = "canister_post_upgrade"] +#[post_upgrade] fn canister_post_upgrade() { - dfn_core::printer::hook(); log!(INFO, "Executing post upgrade"); let reader = BufferedStableMemReader::new(STABLE_MEM_BUFFER_SIZE); @@ -292,29 +302,17 @@ fn populate_finalize_disbursement_timestamp_seconds(governance_proto: &mut Gover } } -#[cfg(feature = "test")] -#[export_name = "canister_update set_time_warp"] -/// Test only feature. When used, a delta is applied to the canister's system timestamp. -fn set_time_warp() { - over(candid_one, set_time_warp_); -} - /// Test only feature. Internal method for calling set_time_warp. #[cfg(feature = "test")] -fn set_time_warp_(new_time_warp: TimeWarp) { +#[update(hidden = true)] +fn set_time_warp(new_time_warp: TimeWarp) { governance_mut().env.set_time_warp(new_time_warp); } /// Returns the governance's NervousSystemParameters -#[export_name = "canister_query get_nervous_system_parameters"] -fn get_nervous_system_parameters() { +#[query] +fn get_nervous_system_parameters(_: ()) -> NervousSystemParameters { log!(INFO, "get_nervous_system_parameters"); - over(candid_one, get_nervous_system_parameters_) -} - -/// Internal method for calling get_nervous_system_parameters. -#[candid_method(query, rename = "get_nervous_system_parameters")] -fn get_nervous_system_parameters_(_: ()) -> NervousSystemParameters { governance() .proto .parameters @@ -323,30 +321,18 @@ fn get_nervous_system_parameters_(_: ()) -> NervousSystemParameters { } /// Returns metadata describing the SNS. -#[export_name = "canister_query get_metadata"] -fn get_metadata() { +#[query] +fn get_metadata(request: GetMetadataRequest) -> GetMetadataResponse { log!(INFO, "get_metadata"); - over(candid_one, get_metadata_) -} - -/// Internal method for calling get_metadata. -#[candid_method(query, rename = "get_metadata")] -fn get_metadata_(request: GetMetadataRequest) -> GetMetadataResponse { governance().get_metadata(&request) } /// Returns the initialization parameters used to spawn an SNS -#[export_name = "canister_query get_sns_initialization_parameters"] -fn get_sns_initialization_parameters() { - log!(INFO, "get_sns_initialization_parameters"); - over(candid_one, get_sns_initialization_parameters_) -} - -/// Internal method for calling get_sns_initialization_parameters. -#[candid_method(query, rename = "get_sns_initialization_parameters")] -fn get_sns_initialization_parameters_( +#[query] +fn get_sns_initialization_parameters( request: GetSnsInitializationParametersRequest, ) -> GetSnsInitializationParametersResponse { + log!(INFO, "get_sns_initialization_parameters"); governance().get_sns_initialization_parameters(&request) } @@ -361,36 +347,23 @@ fn get_sns_initialization_parameters_( /// - split the neuron /// - claim or refresh the neuron /// - merge the neuron's maturity into the neuron's stake -#[export_name = "canister_update manage_neuron"] -fn manage_neuron() { +#[update] +async fn manage_neuron(request: ManageNeuron) -> ManageNeuronResponse { log!(INFO, "manage_neuron"); - over_async(candid_one, manage_neuron_) -} - -/// Internal method for calling manage_neuron. -#[candid_method(update, rename = "manage_neuron")] -async fn manage_neuron_(manage_neuron: ManageNeuron) -> ManageNeuronResponse { let governance = governance_mut(); measure_span_async( governance.profiling_information, "manage_neuron", - governance.manage_neuron(&manage_neuron, &caller()), + governance.manage_neuron(&request, &caller()), ) .await } #[cfg(feature = "test")] -#[export_name = "canister_update update_neuron"] +#[update] /// Test only feature. Update neuron parameters. -fn update_neuron() { +fn update_neuron(neuron: Neuron) -> Option { log!(INFO, "update_neuron"); - over(candid_one, update_neuron_) -} - -#[cfg(feature = "test")] -#[candid_method(update, rename = "update_neuron")] -/// Internal method for calling update_neuron. -fn update_neuron_(neuron: Neuron) -> Option { let governance = governance_mut(); measure_span(governance.profiling_information, "update_neuron", || { governance.update_neuron(neuron).err() @@ -398,16 +371,10 @@ fn update_neuron_(neuron: Neuron) -> Option { } /// Returns the full neuron corresponding to the neuron with ID `neuron_id`. -#[export_name = "canister_query get_neuron"] -fn get_neuron() { +#[query] +fn get_neuron(request: GetNeuron) -> GetNeuronResponse { log!(INFO, "get_neuron"); - over(candid_one, get_neuron_) -} - -/// Internal method for calling get_neuron. -#[candid_method(query, rename = "get_neuron")] -fn get_neuron_(get_neuron: GetNeuron) -> GetNeuronResponse { - governance().get_neuron(get_neuron) + governance().get_neuron(request) } /// Returns a list of neurons of size `limit` using `start_page_at` to @@ -423,28 +390,16 @@ fn get_neuron_(get_neuron: GetNeuron) -> GetNeuronResponse { /// page at a time. /// /// If this method is called as a query call, the returned list is not certified. -#[export_name = "canister_query list_neurons"] -fn list_neurons() { +#[query] +fn list_neurons(request: ListNeurons) -> ListNeuronsResponse { log!(INFO, "list_neurons"); - over(candid_one, list_neurons_) -} - -/// Internal method for calling list_neurons. -#[candid_method(query, rename = "list_neurons")] -fn list_neurons_(list_neurons: ListNeurons) -> ListNeuronsResponse { - governance().list_neurons(&list_neurons) + governance().list_neurons(&request) } /// Returns the full proposal corresponding to the `proposal_id`. -#[export_name = "canister_query get_proposal"] -fn get_proposal() { - over(candid_one, get_proposal_) -} - -/// Internal method for calling get_proposal. -#[candid_method(query, rename = "get_proposal")] -fn get_proposal_(get_proposal: GetProposal) -> GetProposalResponse { - governance().get_proposal(&get_proposal) +#[query] +fn get_proposal(request: GetProposal) -> GetProposalResponse { + governance().get_proposal(&request) } /// Returns a list of proposals of size `limit` using `before_proposal` to @@ -459,69 +414,40 @@ fn get_proposal_(get_proposal: GetProposal) -> GetProposalResponse { /// list_proposals will return a page of size `limit` starting at the most recent proposal. /// /// If this method is called as a query call, the returned list is not certified. -#[export_name = "canister_query list_proposals"] -fn list_proposals() { +#[query] +fn list_proposals(request: ListProposals) -> ListProposalsResponse { log!(INFO, "list_proposals"); - over(candid_one, list_proposals_) -} - -/// Internal method for calling list_proposals. -#[candid_method(query, rename = "list_proposals")] -fn list_proposals_(list_proposals: ListProposals) -> ListProposalsResponse { - governance().list_proposals(&list_proposals, &caller()) + governance().list_proposals(&request, &caller()) } /// Returns the current list of available NervousSystemFunctions. -#[export_name = "canister_query list_nervous_system_functions"] -fn list_nervous_system_functions() { +#[query] +fn list_nervous_system_functions() -> ListNervousSystemFunctionsResponse { log!(INFO, "list_nervous_system_functions"); - over(candid, |()| list_nervous_system_functions_()) -} - -/// Internal method for calling list_nervous_system_functions. -#[candid_method(query, rename = "list_nervous_system_functions")] -fn list_nervous_system_functions_() -> ListNervousSystemFunctionsResponse { governance().list_nervous_system_functions() } /// Returns the latest reward event. -#[export_name = "canister_query get_latest_reward_event"] -fn get_latest_reward_event() { +#[query] +fn get_latest_reward_event() -> RewardEvent { log!(INFO, "get_latest_reward_event"); - over(candid, |()| get_latest_reward_event_()); -} - -#[candid_method(query, rename = "get_latest_reward_event")] -fn get_latest_reward_event_() -> RewardEvent { governance().latest_reward_event() } /// Deprecated method. Previously returned the root canister's status. /// No longer necessary now that canisters can get their own status. -#[export_name = "canister_update get_root_canister_status"] -fn get_root_canister_status() { - over_async(candid_one, get_root_canister_status_) -} - -/// Internal method for calling get_root_canister_status. -#[candid_method(update, rename = "get_root_canister_status")] +#[update] #[allow(clippy::let_unit_value)] // clippy false positive -async fn get_root_canister_status_(_: ()) -> CanisterStatusResultV2 { +async fn get_root_canister_status(_: ()) -> CanisterStatusResultV2 { panic!("This method is deprecated and should not be used. Please use the root canister's `get_sns_canisters_summary` method.") } /// Gets the current SNS version, as understood by Governance. This is useful /// for diagnosing upgrade problems, such as if multiple ledger archives are not /// running the same version. -#[export_name = "canister_query get_running_sns_version"] -fn get_running_sns_version() { +#[query] +fn get_running_sns_version(_: GetRunningSnsVersionRequest) -> GetRunningSnsVersionResponse { log!(INFO, "get_running_sns_version"); - over(candid_one, get_running_sns_version_) -} - -/// Internal method for calling get_sns_version. -#[candid_method(query, rename = "get_running_sns_version")] -fn get_running_sns_version_(_: GetRunningSnsVersionRequest) -> GetRunningSnsVersionResponse { GetRunningSnsVersionResponse { deployed_version: governance().proto.deployed_version.clone(), pending_version: governance().proto.pending_version.clone(), @@ -529,17 +455,11 @@ fn get_running_sns_version_(_: GetRunningSnsVersionRequest) -> GetRunningSnsVers } /// Marks an in progress upgrade that has passed its deadline as failed. -#[export_name = "canister_update fail_stuck_upgrade_in_progress"] -fn fail_stuck_upgrade_in_progress() { - log!(INFO, "fail_stuck_upgrade_in_progress"); - over(candid_one, fail_stuck_upgrade_in_progress_) -} - -/// Internal method for calling fail_stuck_upgrade_in_progress. -#[candid_method(update, rename = "fail_stuck_upgrade_in_progress")] -fn fail_stuck_upgrade_in_progress_( +#[update] +fn fail_stuck_upgrade_in_progress( request: FailStuckUpgradeInProgressRequest, ) -> FailStuckUpgradeInProgressResponse { + log!(INFO, "fail_stuck_upgrade_in_progress"); governance_mut().fail_stuck_upgrade_in_progress(request) } @@ -548,27 +468,16 @@ fn fail_stuck_upgrade_in_progress_( /// In practice, the only mode that the swap canister would ever choose is /// Normal. Also, in practice, the current value of mode should be /// PreInitializationSwap. whenever the swap canister calls this. -#[export_name = "canister_update set_mode"] -fn set_mode() { +#[update] +fn set_mode(request: SetMode) -> SetModeResponse { log!(INFO, "set_mode"); - over(candid_one, set_mode_); -} - -/// Internal method for calling set_mode. -#[candid_method(update, rename = "set_mode")] -fn set_mode_(request: SetMode) -> SetModeResponse { governance_mut().set_mode(request.mode, caller()); SetModeResponse {} } -#[export_name = "canister_query get_mode"] -fn get_mode() { +#[query] +fn get_mode(request: GetMode) -> GetModeResponse { log!(INFO, "get_mode"); - over(candid_one, get_mode_); -} - -#[candid_method(query, rename = "get_mode")] -fn get_mode_(request: GetMode) -> GetModeResponse { governance().get_mode(request) } @@ -586,17 +495,11 @@ fn get_mode_(request: GetMode) -> GetModeResponse { /// This method is idempotent. If called with a `NeuronRecipes` of an already /// created Neuron, the `ClaimSwapNeuronsResponse.skipped_claims` field will be /// incremented and execution will continue. -#[export_name = "canister_update claim_swap_neurons"] -fn claim_swap_neurons() { - log!(INFO, "claim_swap_neurons"); - over(candid_one, claim_swap_neurons_) -} - -/// Internal method for calling claim_swap_neurons. -#[candid_method(update, rename = "claim_swap_neurons")] -fn claim_swap_neurons_( +#[update] +fn claim_swap_neurons( claim_swap_neurons_request: ClaimSwapNeuronsRequest, ) -> ClaimSwapNeuronsResponse { + log!(INFO, "claim_swap_neurons"); let governance = governance_mut(); measure_span( governance.profiling_information, @@ -606,17 +509,9 @@ fn claim_swap_neurons_( } /// This is not really useful to the public. It is, however, useful to integration tests. -#[export_name = "canister_query get_maturity_modulation"] -fn get_maturity_modulation() { +#[update] +fn get_maturity_modulation(request: GetMaturityModulationRequest) -> GetMaturityModulationResponse { log!(INFO, "get_maturity_modulation"); - over(candid_one, get_maturity_modulation_) -} - -/// Internal method for calling get_maturity_modulation. -#[candid_method(update, rename = "get_maturity_modulation")] -fn get_maturity_modulation_( - request: GetMaturityModulationRequest, -) -> GetMaturityModulationResponse { let governance = governance_mut(); measure_span( governance.profiling_information, @@ -626,24 +521,16 @@ fn get_maturity_modulation_( } /// The canister's heartbeat. -#[export_name = "canister_heartbeat"] -fn canister_heartbeat() { - let future = governance_mut().heartbeat(); - - // The canister_heartbeat must be synchronous, so we cannot .await the future. - dfn_core::api::futures::spawn(future); +#[heartbeat] +async fn canister_heartbeat() { + governance_mut().heartbeat().await } -ic_nervous_system_common_build_metadata::define_get_build_metadata_candid_method! {} - -/// Resources to serve for a given http_request -#[export_name = "canister_query http_request"] -fn http_request() { - over(candid_one_with_config, serve_http) -} +ic_nervous_system_common_build_metadata::define_get_build_metadata_candid_method_cdk! {} /// Serve an HttpRequest made to this canister -pub fn serve_http(request: HttpRequest) -> HttpResponse { +#[query(hidden = true, decoding_quota = 10000)] +pub fn http_request(request: HttpRequest) -> HttpResponse { match request.path() { "/metrics" => serve_metrics(encode_metrics), "/logs" => serve_logs_v2(request, &INFO, &ERROR), @@ -702,52 +589,34 @@ fn encode_metrics(w: &mut ic_metrics_encoder::MetricsEncoder>) -> std::i /// the .wasm; using `candid::export_service` here would involve unnecessary /// runtime computation. #[cfg(not(feature = "test"))] -#[export_name = "canister_query __get_candid_interface_tmp_hack"] -fn expose_candid() { - over(candid, |_: ()| include_str!("governance.did").to_string()) -} -#[cfg(feature = "test")] -#[export_name = "canister_query __get_candid_interface_tmp_hack"] -fn expose_candid() { - over(candid, |_: ()| { - include_str!("governance_test.did").to_string() - }) +#[query(hidden = true)] +fn __get_candid_interface_tmp_hack() -> String { + include_str!("governance.did").to_string() } -/// Adds maturity to a neuron for testing #[cfg(feature = "test")] -#[export_name = "canister_update add_maturity"] -fn add_maturity() { - over(candid_one, add_maturity_) +#[query(hidden = true)] +fn __get_candid_interface_tmp_hack() -> String { + include_str!("governance_test.did").to_string() } +/// Adds maturity to a neuron for testing #[cfg(feature = "test")] -#[candid_method(update, rename = "add_maturity")] -fn add_maturity_(request: AddMaturityRequest) -> AddMaturityResponse { +#[update] +fn add_maturity(request: AddMaturityRequest) -> AddMaturityResponse { governance_mut().add_maturity(request) } -#[export_name = "canister_query get_upgrade_journal"] -fn get_upgrade_journal() { - over(candid_one, get_upgrade_journal_) -} - -#[candid_method(query, rename = "get_upgrade_journal")] -fn get_upgrade_journal_(arg: GetUpgradeJournalRequest) -> GetUpgradeJournalResponse { +#[query] +fn get_upgrade_journal(arg: GetUpgradeJournalRequest) -> GetUpgradeJournalResponse { let GetUpgradeJournalRequest {} = arg; governance().get_upgrade_journal() } /// Mints tokens for testing #[cfg(feature = "test")] -#[export_name = "canister_update mint_tokens"] -fn mint_tokens() { - over_async(candid_one, mint_tokens_) -} - -#[cfg(feature = "test")] -#[candid_method(update, rename = "mint_tokens")] -async fn mint_tokens_(request: MintTokensRequest) -> MintTokensResponse { +#[update] +async fn mint_tokens(request: MintTokensRequest) -> MintTokensResponse { governance_mut().mint_tokens(request).await } diff --git a/rs/sns/governance/src/canister_control.rs b/rs/sns/governance/src/canister_control.rs index f11e0798505..d9a1a8ff1be 100644 --- a/rs/sns/governance/src/canister_control.rs +++ b/rs/sns/governance/src/canister_control.rs @@ -9,8 +9,7 @@ use crate::{ types::Environment, }; use candid::{Decode, Encode}; -use dfn_core::CanisterId; -use ic_base_types::PrincipalId; +use ic_base_types::{CanisterId, PrincipalId}; use ic_canister_log::log; use ic_nervous_system_clients::{ canister_id_record::CanisterIdRecord, diff --git a/rs/sns/governance/src/governance.rs b/rs/sns/governance/src/governance.rs index c42b51339ed..9cdaa3a6389 100644 --- a/rs/sns/governance/src/governance.rs +++ b/rs/sns/governance/src/governance.rs @@ -73,10 +73,13 @@ use crate::{ }, }; use candid::{Decode, Encode}; -use dfn_core::api::{spawn, CanisterId}; -use ic_base_types::PrincipalId; +#[cfg(not(target_arch = "wasm32"))] +use futures::FutureExt; +use ic_base_types::{CanisterId, PrincipalId}; use ic_canister_log::log; use ic_canister_profiler::SpanStats; +#[cfg(target_arch = "wasm32")] +use ic_cdk::spawn; use ic_ledger_core::Tokens; use ic_management_canister_types::{ CanisterChangeDetails, CanisterInfoRequest, CanisterInfoResponse, CanisterInstallMode, @@ -113,6 +116,7 @@ use std::{ HashMap, HashSet, }, convert::{TryFrom, TryInto}, + future::Future, ops::Bound::{Excluded, Unbounded}, str::FromStr, string::ToString, @@ -699,6 +703,24 @@ pub struct Governance { pub test_features_enabled: bool, } +/// This function is used to spawn a future in a way that is compatible with both the WASM and +/// non-WASM environments that are used for testing. This only actually spawns in the case where +/// the WASM is running in the IC, or has some other source of asynchrony. Otherwise, it +/// immediately executes. +fn spawn_in_canister_env(future: impl Future + Sized + 'static) { + #[cfg(target_arch = "wasm32")] + { + spawn(future); + } + // This is needed for tests + #[cfg(not(target_arch = "wasm32"))] + { + future + .now_or_never() + .expect("Future could not execute in non-WASM environment"); + } +} + impl Governance { pub fn new( proto: ValidGovernanceProto, @@ -2013,7 +2035,7 @@ impl Governance { // - in prod, "self" is a reference to the GOVERNANCE static variable, which is // initialized only once (in canister_init or canister_post_upgrade) let governance: &'static mut Governance = unsafe { std::mem::transmute(self) }; - spawn(governance.perform_action(proposal_id, action)); + spawn_in_canister_env(governance.perform_action(proposal_id, action)); } /// For a given proposal (given by its ID), selects and performs the right 'action', @@ -4265,7 +4287,7 @@ impl Governance { .remove_neuron_permissions(&neuron_id, caller, r) .map(|_| ManageNeuronResponse::remove_neuron_permissions_response()), C::ClaimOrRefresh(claim_or_refresh) => self - .claim_or_refresh_neuron(&neuron_id, claim_or_refresh) + .claim_or_refresh_neuron(&neuron_id, caller, claim_or_refresh) .await .map(|_| ManageNeuronResponse::claim_or_refresh_neuron_response(neuron_id)), } @@ -4323,10 +4345,10 @@ impl Governance { } } - /// Calls dfn_core::api::caller. async fn claim_or_refresh_neuron( &mut self, neuron_id: &NeuronId, + caller: &PrincipalId, claim_or_refresh: &ClaimOrRefresh, ) -> Result<(), GovernanceError> { let locator = &claim_or_refresh.by.as_ref().ok_or_else(|| { @@ -4338,11 +4360,8 @@ impl Governance { match locator { By::MemoAndController(memo_and_controller) => { - self.claim_or_refresh_neuron_by_memo_and_controller( - &dfn_core::api::caller(), - memo_and_controller, - ) - .await + self.claim_or_refresh_neuron_by_memo_and_controller(caller, memo_and_controller) + .await } By::NeuronId(_) => self.refresh_neuron(neuron_id).await, @@ -4602,7 +4621,7 @@ impl Governance { /// Runs periodic tasks that are not directly triggered by user input. pub async fn heartbeat(&mut self) { - use dfn_core::println; + use ic_cdk::println; self.process_proposals(); diff --git a/rs/sns/governance/src/proposal.rs b/rs/sns/governance/src/proposal.rs index 59b38cd78e5..a447715d1af 100644 --- a/rs/sns/governance/src/proposal.rs +++ b/rs/sns/governance/src/proposal.rs @@ -1,4 +1,3 @@ -use crate::sns_upgrade::get_proposal_id_that_added_wasm; use crate::{ canister_control::perform_execute_generic_nervous_system_function_validate_and_render_call, governance::{ @@ -24,13 +23,12 @@ use crate::{ RegisterDappCanisters, Tally, TransferSnsTreasuryFunds, UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Valuation as ValuationPb, Vote, }, - sns_upgrade::{get_upgrade_params, UpgradeSnsParams}, + sns_upgrade::{get_proposal_id_that_added_wasm, get_upgrade_params, UpgradeSnsParams}, types::Environment, validate_chars_count, validate_len, validate_required_field, }; use candid::Principal; -use dfn_core::api::CanisterId; -use ic_base_types::PrincipalId; +use ic_base_types::{CanisterId, PrincipalId}; use ic_canister_log::log; use ic_crypto_sha2::Sha256; use ic_nervous_system_common::{ diff --git a/rs/sns/governance/src/types.rs b/rs/sns/governance/src/types.rs index 415243a0e03..96b15df641b 100644 --- a/rs/sns/governance/src/types.rs +++ b/rs/sns/governance/src/types.rs @@ -2014,6 +2014,13 @@ pub struct LedgerUpdateLock { impl Drop for LedgerUpdateLock { /// Drops the lock on the neuron. fn drop(&mut self) { + // In the case of a panic, the state of the ledger account representing the neuron's stake + // may be inconsistent with the internal state of governance. In that case, + // we want to prevent further operations with that neuron until the issue can be + // investigated and resolved, which will require code changes. + if ic_cdk::api::call::is_recovering_from_trap() { + return; + } // It's always ok to dereference the governance when a LedgerUpdateLock // goes out of scope. Indeed, in the scope of any Governance method, // &self always remains alive. The 'mut' is not an issue, because @@ -2551,7 +2558,11 @@ pub mod test_helpers { /// Map of expected calls to a result, where key is hash of arguments (See `compute_call_canister_key`). #[allow(clippy::type_complexity)] pub canister_calls_map: HashMap< - (dfn_core::CanisterId, std::string::String, std::vec::Vec), + ( + ic_base_types::CanisterId, + std::string::String, + std::vec::Vec, + ), CanisterCallResult, >, diff --git a/rs/sns/governance/token_valuation/src/lib.rs b/rs/sns/governance/token_valuation/src/lib.rs index 3f59a86c5cd..73d2e4eb2fa 100644 --- a/rs/sns/governance/token_valuation/src/lib.rs +++ b/rs/sns/governance/token_valuation/src/lib.rs @@ -136,7 +136,7 @@ impl ValuationFactors { /// IcpsPerIcpClient in this crate. /// * `xdrs_per_icp_client` - Supplies the ICP -> XDR conversion rate. This is probably the most /// interesting of the clients used. A object suitable for production can be constructed by -/// calling new_standard_xdrs_per_icp_client:: with zero arguments. +/// calling new_standard_xdrs_per_icp_client:: with zero arguments. async fn try_get_balance_valuation_factors( account: Account, icrc1_client: &mut dyn Icrc1Client, diff --git a/rs/sns/integration_tests/src/proposals.rs b/rs/sns/integration_tests/src/proposals.rs index 7d4c45a2da2..baee1429c94 100644 --- a/rs/sns/integration_tests/src/proposals.rs +++ b/rs/sns/integration_tests/src/proposals.rs @@ -435,8 +435,14 @@ fn test_bad_proposal_id_candid_encoding() { .query_("get_proposal", bytes, b"This is not valid candid!".to_vec()) .await; + let expected_error = "failed to decode"; match res { - Err(e) => assert!(e.contains("Deserialization Failed")), + Err(e) => assert!( + e.contains(expected_error), + "Expected error string \"{}\" not present in actual error. Error was: {:?}", + expected_error, + e + ), Ok(_) => panic!("get_proposal should fail to deserialize"), }; Ok(())