Address feedbacks

Author: jasonz-dfinity

rs/nns/governance/BUILD.bazel

@@ -78,6 +78,7 @@ DEPENDENCIES = [
     "//rs/nns/governance/api",
     "//rs/nns/governance/init",
     "//rs/nns/gtc_accounts",
+    "//rs/nns/handlers/lifeline/interface",
     "//rs/nns/handlers/root/interface",
     "//rs/nns/sns-wasm",
     "//rs/node_rewards/canister/api",
@@ -386,6 +387,10 @@ rust_test(
     name = "governance_test",
     srcs = glob(["src/**/*.rs"]),
     aliases = ALIASES,
+    data = [
+        "//rs/nns/handlers/lifeline/impl:lifeline.did",
+        "//rs/nns/handlers/root/impl:canister/root.did",
+    ],
     proc_macro_deps = MACRO_DEPENDENCIES + MACRO_DEV_DEPENDENCIES,
     deps = DEPENDENCIES + DEV_DEPENDENCIES + [
         ":build_script",
@@ -397,6 +402,10 @@ rust_test(
     srcs = glob(["src/**/*.rs"]),
     aliases = ALIASES,
     crate_features = test_with_tla(),
+    data = [
+        "//rs/nns/handlers/lifeline/impl:lifeline.did",
+        "//rs/nns/handlers/root/impl:canister/root.did",
+    ],
     proc_macro_deps = MACRO_DEPENDENCIES + MACRO_DEV_DEPENDENCIES,
     deps = DEPENDENCIES + DEV_DEPENDENCIES + [
         ":build_script",

rs/nns/governance/src/proposals/execute_nns_function.rs

@@ -305,7 +305,21 @@ fn maybe_hash_large_blobs(
         value: Some(Value::Map(SelfDescribingValueMap { values: values_map })),
     }
 }
-
+/// If the provided `key` is one of the `field_names`, and the associated `SelfDescribingValue` is a
+/// blob, replaces the value with its SHA-256 hash and modifies the key to be `<key>_hash`.
+/// Otherwise, leaves the entry unchanged.
+///
+/// # Example
+/// ```
+/// let field_names = &["wasm_module", "init_arg"];
+/// let (k, v) = maybe_replace_large_blob_entry_with_hash(
+///     "wasm_module".to_string(),
+///     SelfDescribingValue { value: Some(Value::Blob(vec![1,2,3])) },
+///     field_names,
+/// );
+/// assert_eq!(k, "wasm_module_hash");
+/// assert!(matches!(v.value, Some(Value::Blob(_))));
+/// ```
 fn maybe_replace_large_blob_entry_with_hash(
     key: String,
     value: SelfDescribingValue,
@@ -314,17 +328,16 @@ fn maybe_replace_large_blob_entry_with_hash(
     if !field_names.contains(&key.as_str()) {
         return (key, value);
     }
-    // Use multiple-level match so that the original value can be returned without cloning if it
-    // doesn't match.
-    let SelfDescribingValue {
-        value: Some(Value::Blob(blob)),
-    } = value
-    else {
+
+    let Some(inner_value) = &value.value else {
+        return (key, value);
+    };
+    let Value::Blob(blob) = inner_value else {
         return (key, value);
     };
 
     let key = format!("{key}_hash");
-    let hash = Sha256::hash(&blob).to_vec();
+    let hash = Sha256::hash(blob).to_vec();
     let hashed_value = SelfDescribingValue {
         value: Some(Value::Blob(hash)),
     };

rs/nns/governance/src/proposals/execute_nns_function_tests.rs

@@ -10,12 +10,14 @@ use candid::{Decode, Encode, Nat};
 use ic_base_types::{CanisterId, PrincipalId};
 use ic_crypto_sha2::Sha256;
 use ic_management_canister_types_private::{CanisterMetadataRequest, CanisterMetadataResponse};
+use ic_nervous_system_root::change_canister::AddCanisterRequest;
 use ic_nns_constants::{BITCOIN_MAINNET_CANISTER_ID, CYCLES_MINTING_CANISTER_ID};
 use ic_nns_governance_api::{
     SelfDescribingValue,
     bitcoin::{BitcoinNetwork, BitcoinSetConfigProposal},
     subnet_rental::{RentalConditionId, SubnetRentalRequest},
 };
+use ic_nns_handler_lifeline_interface::HardResetNnsRootToVersionPayload;
 use ic_sns_wasm::pb::v1::{AddWasmRequest, SnsCanisterType, SnsWasm};
 use maplit::hashmap;
 use std::sync::Arc;
@@ -122,6 +124,8 @@ fn test_execute_nns_function_try_from_errors() {
     }
 }
 
+// This tests a "normal" NNS function where the payload is translated through a candid file fetched
+// by the `canister_metadata` method on the management canister.
 #[tokio::test]
 async fn test_to_self_describing_update_subnet_type() {
     // Minimal CMC candid file with only update_subnet_type method
@@ -419,41 +423,19 @@ fn test_re_encode_payload_to_target_canister_overrides_proposal_id_for_add_wasm(
 
 #[tokio::test]
 async fn test_to_self_describing_nns_canister_install() {
-    let root_candid = r#"
-type AddCanisterRequest = record {
-  arg : blob;
-  initial_cycles : nat64;
-  wasm_module : blob;
-  name : text;
-  memory_allocation : opt nat;
-  compute_allocation : opt nat;
-};
-
-service : {
-  add_nns_canister : (AddCanisterRequest) -> ();
-}
-"#;
-
-    #[derive(candid::CandidType)]
-    struct AddCanisterRequest {
-        arg: Vec<u8>,
-        initial_cycles: u64,
-        wasm_module: Vec<u8>,
-        name: String,
-        memory_allocation: Option<u64>,
-        compute_allocation: Option<u64>,
-    }
+    let root_candid =
+        std::fs::read_to_string("rs/nns/handlers/root/impl/canister/root.did").unwrap();
 
     let wasm_module = vec![0_u8, 0x61, 0x73, 0x6D, 1_u8, 0_u8, 0_u8, 0_u8];
     let arg = vec![1_u8, 2_u8, 3_u8];
 
     let request = AddCanisterRequest {
-        arg: arg.clone(),
-        initial_cycles: 1_000_000_000_000_u64,
-        wasm_module: wasm_module.clone(),
         name: "test-canister".to_string(),
-        memory_allocation: None,
+        wasm_module: wasm_module.clone(),
+        arg: arg.clone(),
         compute_allocation: None,
+        memory_allocation: None,
+        initial_cycles: 1_000_000_000_000_u64,
     };
     let payload = Encode!(&request).unwrap();
 
@@ -503,27 +485,13 @@ service : {
 
 #[tokio::test]
 async fn test_to_self_describing_hard_reset_nns_root_to_version() {
-    let lifeline_candid = r#"
-type HardResetRootToVersionPayload = record {
-  wasm_module: blob;
-  init_arg: blob;
-};
-
-service : {
-  hard_reset_root_to_version: (HardResetRootToVersionPayload) -> ();
-}
-"#;
-
-    #[derive(candid::CandidType)]
-    struct HardResetRootToVersionPayload {
-        wasm_module: Vec<u8>,
-        init_arg: Vec<u8>,
-    }
+    let lifeline_candid =
+        std::fs::read_to_string("rs/nns/handlers/lifeline/impl/lifeline.did").unwrap();
 
     let wasm_module = vec![0_u8, 0x61, 0x73, 0x6D, 1_u8, 0_u8, 0_u8, 0_u8];
     let init_arg = vec![4_u8, 5_u8, 6_u8];
 
-    let request = HardResetRootToVersionPayload {
+    let request = HardResetNnsRootToVersionPayload {
         wasm_module: wasm_module.clone(),
         init_arg: init_arg.clone(),
     };