From db821c2b8e6bcad0a6969f3747a1a98d75f99f38 Mon Sep 17 00:00:00 2001 From: Jason Zhu Date: Mon, 5 Jan 2026 23:53:58 +0000 Subject: [PATCH 1/3] Omit logos for CreateServiceNervousSystem proposals --- rs/nns/governance/src/pb/conversions/mod.rs | 75 ---- rs/nns/governance/src/pb/conversions/tests.rs | 144 +------ .../governance/src/pb/proposal_conversions.rs | 382 +++++++++++++++++- 3 files changed, 380 insertions(+), 221 deletions(-) diff --git a/rs/nns/governance/src/pb/conversions/mod.rs b/rs/nns/governance/src/pb/conversions/mod.rs index 61c21285f491..96b454408f0e 100644 --- a/rs/nns/governance/src/pb/conversions/mod.rs +++ b/rs/nns/governance/src/pb/conversions/mod.rs @@ -1,7 +1,6 @@ use crate::pb::proposal_conversions::{ProposalDisplayOptions, convert_proposal}; use crate::pb::v1 as pb; -use candid::{Int, Nat}; use ic_crypto_sha2::Sha256; use ic_nns_governance_api as pb_api; use ic_protobuf::registry::replica_version::v1::{ @@ -9,7 +8,6 @@ use ic_protobuf::registry::replica_version::v1::{ GuestLaunchMeasurementMetadata as PbGuestLaunchMeasurementMetadata, GuestLaunchMeasurements as PbGuestLaunchMeasurements, }; -use std::collections::HashMap; #[cfg(test)] mod tests; @@ -4176,76 +4174,3 @@ impl From for pb_api::MaturityDisbursement { } } } - -impl From for pb_api::SelfDescribingValue { - fn from(item: pb::SelfDescribingValue) -> Self { - let Some(value) = item.value else { - return Self::Map(HashMap::new()); - }; - match value { - pb::self_describing_value::Value::Blob(v) => Self::Blob(v), - pb::self_describing_value::Value::Text(v) => Self::Text(v), - pb::self_describing_value::Value::Nat(v) => { - let nat = Nat::decode(&mut v.as_slice()).unwrap(); - Self::Nat(nat) - } - pb::self_describing_value::Value::Int(v) => { - let int = Int::decode(&mut v.as_slice()).unwrap(); - Self::Int(int) - } - pb::self_describing_value::Value::Array(v) => { - Self::Array(v.values.into_iter().map(Self::from).collect()) - } - pb::self_describing_value::Value::Map(v) => Self::Map( - v.values - .into_iter() - .map(|(k, v)| (k, Self::from(v))) - .collect(), - ), - pb::self_describing_value::Value::Null(_) => Self::Null, - } - } -} - -impl From for pb::SelfDescribingValue { - fn from(item: pb_api::SelfDescribingValue) -> Self { - let value = match item { - pb_api::SelfDescribingValue::Blob(v) => pb::self_describing_value::Value::Blob(v), - pb_api::SelfDescribingValue::Text(v) => pb::self_describing_value::Value::Text(v), - pb_api::SelfDescribingValue::Nat(v) => { - let mut bytes = Vec::new(); - v.encode(&mut bytes).unwrap(); - pb::self_describing_value::Value::Nat(bytes) - } - pb_api::SelfDescribingValue::Int(v) => { - let mut bytes = Vec::new(); - v.encode(&mut bytes).unwrap(); - pb::self_describing_value::Value::Int(bytes) - } - pb_api::SelfDescribingValue::Array(v) => { - pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { - values: v.into_iter().map(Self::from).collect(), - }) - } - pb_api::SelfDescribingValue::Map(v) => { - pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { - values: v.into_iter().map(|(k, v)| (k, Self::from(v))).collect(), - }) - } - pb_api::SelfDescribingValue::Null => { - pb::self_describing_value::Value::Null(pb::Empty {}) - } - }; - Self { value: Some(value) } - } -} - -impl From for pb_api::SelfDescribingProposalAction { - fn from(item: pb::SelfDescribingProposalAction) -> Self { - Self { - type_name: Some(item.type_name), - type_description: Some(item.type_description), - value: item.value.map(pb_api::SelfDescribingValue::from), - } - } -} diff --git a/rs/nns/governance/src/pb/conversions/tests.rs b/rs/nns/governance/src/pb/conversions/tests.rs index 163ec090b584..b764d3397db3 100644 --- a/rs/nns/governance/src/pb/conversions/tests.rs +++ b/rs/nns/governance/src/pb/conversions/tests.rs @@ -1,8 +1,7 @@ use super::*; -use candid::{Int, Nat}; + use ic_base_types::PrincipalId; use icp_ledger::protobuf::AccountIdentifier; -use maplit::hashmap; #[test] fn test_node_provider_conversions_always_create_32_byte_account_identifier() { @@ -130,144 +129,3 @@ fn test_reward_to_account_invalid_account_identifier_just_return_what_is_stored( icp_ledger::AccountIdentifier::try_from(&converted_reward_to_account.to_account.unwrap()) .expect_err("Should fail!"); } - -#[test] -fn test_value_conversions() { - // Prepare test data for all value types - let nat_value = Nat::from(12345u64); - let int_value = Int::from(-9876i64); - - // Encode Nat and Int to bytes - let mut nat_bytes = Vec::new(); - nat_value.encode(&mut nat_bytes).unwrap(); - - let mut int_bytes = Vec::new(); - int_value.encode(&mut int_bytes).unwrap(); - - // Create a comprehensive map with all possible SelfDescribingValue types - let value_pb = pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Map( - pb::SelfDescribingValueMap { - values: hashmap! { - // Test Text type - "text_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Text("some text".to_string())), - }, - // Test Blob type - "blob_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Blob(vec![1, 2, 3, 4, 5])), - }, - // Test Nat type - "nat_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Nat(nat_bytes.clone())), - }, - // Test Int type - "int_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Int(int_bytes.clone())), - }, - // Test Array type with various elements - "array_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { - values: vec![ - pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Text("first".to_string())), - }, - pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Text("second".to_string())), - }, - pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Blob(vec![10, 20, 30])), - }, - ], - })), - }, - // Test nested Map type - "nested_map_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { - values: hashmap! { - "nested_text".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Text("nested value".to_string())), - }, - "nested_blob".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Blob(vec![255, 254, 253])), - }, - "nested_nat".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Nat(nat_bytes.clone())), - }, - }, - })), - }, - // Test empty Array - "empty_array_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { - values: vec![], - })), - }, - // Test empty Map - "empty_map_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { - values: hashmap! {}, - })), - }, - // Test Array containing Maps - "array_of_maps_field".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { - values: vec![ - pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { - values: hashmap! { - "key1".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Text("value1".to_string())), - }, - }, - })), - }, - pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { - values: hashmap! { - "key2".to_string() => pb::SelfDescribingValue { - value: Some(pb::self_describing_value::Value::Text("value2".to_string())), - }, - }, - })), - }, - ], - })), - }, - }, - }, - )), - }; - - let value_pb_api = pb_api::SelfDescribingValue::from(value_pb); - - assert_eq!( - value_pb_api, - pb_api::SelfDescribingValue::Map(hashmap! { - "text_field".to_string() => pb_api::SelfDescribingValue::Text("some text".to_string()), - "blob_field".to_string() => pb_api::SelfDescribingValue::Blob(vec![1, 2, 3, 4, 5]), - "nat_field".to_string() => pb_api::SelfDescribingValue::Nat(nat_value.clone()), - "int_field".to_string() => pb_api::SelfDescribingValue::Int(int_value.clone()), - "array_field".to_string() => pb_api::SelfDescribingValue::Array(vec![ - pb_api::SelfDescribingValue::Text("first".to_string()), - pb_api::SelfDescribingValue::Text("second".to_string()), - pb_api::SelfDescribingValue::Blob(vec![10, 20, 30]), - ]), - "nested_map_field".to_string() => pb_api::SelfDescribingValue::Map(hashmap! { - "nested_text".to_string() => pb_api::SelfDescribingValue::Text("nested value".to_string()), - "nested_blob".to_string() => pb_api::SelfDescribingValue::Blob(vec![255, 254, 253]), - "nested_nat".to_string() => pb_api::SelfDescribingValue::Nat(nat_value.clone()), - }), - "empty_array_field".to_string() => pb_api::SelfDescribingValue::Array(vec![]), - "empty_map_field".to_string() => pb_api::SelfDescribingValue::Map(hashmap! {}), - "array_of_maps_field".to_string() => pb_api::SelfDescribingValue::Array(vec![ - pb_api::SelfDescribingValue::Map(hashmap! { - "key1".to_string() => pb_api::SelfDescribingValue::Text("value1".to_string()), - }), - pb_api::SelfDescribingValue::Map(hashmap! { - "key2".to_string() => pb_api::SelfDescribingValue::Text("value2".to_string()), - }), - ]), - }) - ); -} diff --git a/rs/nns/governance/src/pb/proposal_conversions.rs b/rs/nns/governance/src/pb/proposal_conversions.rs index 18572857ad68..a56d0c5deb23 100644 --- a/rs/nns/governance/src/pb/proposal_conversions.rs +++ b/rs/nns/governance/src/pb/proposal_conversions.rs @@ -1,7 +1,8 @@ use crate::{governance::EXECUTE_NNS_FUNCTION_PAYLOAD_LISTING_BYTES_MAX, pb::v1 as pb}; +use candid::{Int, Nat}; use ic_nns_common::pb::v1::NeuronId; -use ic_nns_governance_api::{self as pb_api, SelfDescribingProposalAction}; +use ic_nns_governance_api as pb_api; use std::collections::{BTreeSet, HashMap}; #[derive(Debug, Clone, Copy, PartialEq)] @@ -262,6 +263,150 @@ fn convert_action( } } +fn convert_self_describing_action( + item: &pb::SelfDescribingProposalAction, + omit_create_service_nervous_system_logos: bool, +) -> pb_api::SelfDescribingProposalAction { + let pb::SelfDescribingProposalAction { + type_name, + type_description, + value, + } = item; + + let type_name = Some(type_name.clone()); + let type_description = Some(type_description.clone()); + let value = value.as_ref().map(|value| { + convert_self_describing_value( + value, + &PathToOmit::default_paths_to_omit(omit_create_service_nervous_system_logos), + ) + }); + + pb_api::SelfDescribingProposalAction { + type_name, + type_description, + value, + } +} + +struct PathToOmit { + // Must have at least one element. + path: Vec<&'static str>, +} + +enum OmitAction { + DoNothing, + OmitCurrent, + OmitDescendant(PathToOmit), +} + +impl PathToOmit { + fn default_paths_to_omit(omit_create_service_nervous_system_logos: bool) -> Vec { + if omit_create_service_nervous_system_logos { + vec![ + Self { path: vec!["logo"] }, + Self { + path: vec!["ledger_parameters", "token_logo"], + }, + ] + } else { + vec![] + } + } + + fn matches(&self, field_name: &str) -> OmitAction { + let Some((&first, rest)) = self.path.split_first() else { + // This should never happen, but we handle it anyway. + return OmitAction::DoNothing; + }; + if first != field_name { + return OmitAction::DoNothing; + } + if rest.is_empty() { + return OmitAction::OmitCurrent; + } + OmitAction::OmitDescendant(PathToOmit { + path: rest.to_vec(), + }) + } +} + +fn convert_self_describing_field( + field_name: &str, + paths_to_omit: &[PathToOmit], + original_value: &pb::SelfDescribingValue, +) -> pb_api::SelfDescribingValue { + let match_results = paths_to_omit + .iter() + .map(|path| path.matches(field_name)) + .collect::>(); + if match_results + .iter() + .any(|result| matches!(result, OmitAction::OmitCurrent)) + { + return pb_api::SelfDescribingValue::Null; + } + let descendant_paths_to_omit = match_results + .into_iter() + .filter_map(|result| match result { + OmitAction::OmitDescendant(path) => Some(path), + _ => None, + }) + .collect::>(); + convert_self_describing_value(original_value, &descendant_paths_to_omit) +} + +fn convert_self_describing_value( + item: &pb::SelfDescribingValue, + paths_to_omit: &[PathToOmit], +) -> pb_api::SelfDescribingValue { + let pb::SelfDescribingValue { value: Some(value) } = item else { + return pb_api::SelfDescribingValue::Map(HashMap::new()); + }; + match value { + pb::self_describing_value::Value::Blob(v) => pb_api::SelfDescribingValue::Blob(v.clone()), + pb::self_describing_value::Value::Text(v) => pb_api::SelfDescribingValue::Text(v.clone()), + pb::self_describing_value::Value::Nat(v) => { + let nat = Nat::decode(&mut v.as_slice()).unwrap(); + pb_api::SelfDescribingValue::Nat(nat) + } + pb::self_describing_value::Value::Int(v) => { + let int = Int::decode(&mut v.as_slice()).unwrap(); + pb_api::SelfDescribingValue::Int(int) + } + pb::self_describing_value::Value::Null(_) => pb_api::SelfDescribingValue::Null, + pb::self_describing_value::Value::Array(v) => pb_api::SelfDescribingValue::Array( + v.values + .iter() + .map(|value| convert_self_describing_value(value, &[])) + .collect(), + ), + pb::self_describing_value::Value::Map(v) => pb_api::SelfDescribingValue::Map( + v.values + .iter() + .map(|(k, v)| { + ( + k.clone(), + convert_self_describing_field(k, paths_to_omit, v), + ) + }) + .collect(), + ), + } +} + +// This is only used for tests, because the `pb::SelfDescribingValue` can be large, and a direct +// conversion would most likely involve cloning the source value. Previously, we had an issue where +// it caused too many instructions due to the cloning, even when the large fields were omitted after +// cloning. The `cfg(test)` can be removed, however, if we decide to use SelfDescribingValue in +// other scenarios where there is less concern about cloning large fields. Ideally, we would +#[cfg(test)] +impl From for pb_api::SelfDescribingValue { + fn from(value: pb::SelfDescribingValue) -> Self { + convert_self_describing_value(&value, &[]) + } +} + pub(crate) fn convert_proposal( item: &pb::Proposal, display_options: ProposalDisplayOptions, @@ -284,10 +429,22 @@ pub(crate) fn convert_proposal( } else { None }; + let is_create_service_nervous_system_proposal = action.as_ref().is_some_and(|action| { + matches!( + action, + pb_api::proposal::Action::CreateServiceNervousSystem(_) + ) + }); let self_describing_action = if display_options.show_self_describing_action() { self_describing_action - .clone() - .map(SelfDescribingProposalAction::from) + .as_ref() + .map(|self_describing_action| { + convert_self_describing_action( + self_describing_action, + is_create_service_nervous_system_proposal + && display_options.omit_create_service_nervous_system_large_fields(), + ) + }) } else { None }; @@ -376,8 +533,227 @@ pub(crate) fn proposal_data_to_info( mod tests { use super::*; + use crate::{ + pb::v1::{CreateServiceNervousSystem, create_service_nervous_system::LedgerParameters}, + proposals::self_describing::LocallyDescribableProposalAction, + }; + use ic_base_types::PrincipalId; use ic_crypto_sha2::Sha256; + use ic_nervous_system_proto::pb::v1::Image; + use maplit::hashmap; + + #[test] + fn test_self_describing_value_conversions() { + let nat_value = Nat::from(12345_u64); + let int_value = Int::from(-9876_i64); + + let mut nat_bytes = Vec::new(); + nat_value.encode(&mut nat_bytes).unwrap(); + + let mut int_bytes = Vec::new(); + int_value.encode(&mut int_bytes).unwrap(); + + let value_pb = pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Map( + pb::SelfDescribingValueMap { + values: hashmap! { + "text_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Text("some text".to_string())), + }, + "blob_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Blob(vec![1, 2, 3, 4, 5])), + }, + "nat_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Nat(nat_bytes.clone())), + }, + "int_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Int(int_bytes.clone())), + }, + "array_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { + values: vec![ + pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Text("first".to_string())), + }, + pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Text("second".to_string())), + }, + pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Blob(vec![10, 20, 30])), + }, + ], + })), + }, + "nested_map_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { + values: hashmap! { + "nested_text".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Text("nested value".to_string())), + }, + "nested_blob".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Blob(vec![255, 254, 253])), + }, + "nested_nat".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Nat(nat_bytes.clone())), + }, + }, + })), + }, + "empty_array_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { + values: vec![], + })), + }, + "empty_map_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { + values: hashmap! {}, + })), + }, + "array_of_maps_field".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Array(pb::SelfDescribingValueArray { + values: vec![ + pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { + values: hashmap! { + "key1".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Text("value1".to_string())), + }, + }, + })), + }, + pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Map(pb::SelfDescribingValueMap { + values: hashmap! { + "key2".to_string() => pb::SelfDescribingValue { + value: Some(pb::self_describing_value::Value::Text("value2".to_string())), + }, + }, + })), + }, + ], + })), + }, + }, + }, + )), + }; + + let value_pb_api = pb_api::SelfDescribingValue::from(value_pb); + + assert_eq!( + value_pb_api, + pb_api::SelfDescribingValue::Map(hashmap! { + "text_field".to_string() => pb_api::SelfDescribingValue::Text("some text".to_string()), + "blob_field".to_string() => pb_api::SelfDescribingValue::Blob(vec![1, 2, 3, 4, 5]), + "nat_field".to_string() => pb_api::SelfDescribingValue::Nat(nat_value.clone()), + "int_field".to_string() => pb_api::SelfDescribingValue::Int(int_value.clone()), + "array_field".to_string() => pb_api::SelfDescribingValue::Array(vec![ + pb_api::SelfDescribingValue::Text("first".to_string()), + pb_api::SelfDescribingValue::Text("second".to_string()), + pb_api::SelfDescribingValue::Blob(vec![10, 20, 30]), + ]), + "nested_map_field".to_string() => pb_api::SelfDescribingValue::Map(hashmap! { + "nested_text".to_string() => pb_api::SelfDescribingValue::Text("nested value".to_string()), + "nested_blob".to_string() => pb_api::SelfDescribingValue::Blob(vec![255, 254, 253]), + "nested_nat".to_string() => pb_api::SelfDescribingValue::Nat(nat_value.clone()), + }), + "empty_array_field".to_string() => pb_api::SelfDescribingValue::Array(vec![]), + "empty_map_field".to_string() => pb_api::SelfDescribingValue::Map(hashmap! {}), + "array_of_maps_field".to_string() => pb_api::SelfDescribingValue::Array(vec![ + pb_api::SelfDescribingValue::Map(hashmap! { + "key1".to_string() => pb_api::SelfDescribingValue::Text("value1".to_string()), + }), + pb_api::SelfDescribingValue::Map(hashmap! { + "key2".to_string() => pb_api::SelfDescribingValue::Text("value2".to_string()), + }), + ]), + }) + ); + } + + #[test] + fn test_self_describing_value_omit_logos() { + // Not all fields are included in the test, for simplicity. + let create_service_nervous_system = CreateServiceNervousSystem { + name: Some("some name".to_string()), + logo: Some(Image { + base64_encoding: Some("base64 encoding of a logo".to_string()), + }), + ledger_parameters: Some(LedgerParameters { + token_name: Some("some token name".to_string()), + token_logo: Some(Image { + base64_encoding: Some("base64 encoding of a token logo".to_string()), + }), + ..Default::default() + }), + ..Default::default() + }; + let self_describing_action = create_service_nervous_system.to_self_describing_action(); + + // Sanity check that the self-describing value does have logos when we don't omit them. + let self_describing_value_with_logos = + convert_self_describing_action(&self_describing_action, false) + .value + .unwrap(); + let map = match self_describing_value_with_logos { + pb_api::SelfDescribingValue::Map(map) => map, + _ => panic!("Expected a map"), + }; + assert_eq!( + map.get("name").unwrap(), + &pb_api::SelfDescribingValue::Text("some name".to_string()) + ); + assert_eq!( + map.get("logo").unwrap(), + &pb_api::SelfDescribingValue::Map(hashmap! { + "base64_encoding".to_string() => pb_api::SelfDescribingValue::Text("base64 encoding of a logo".to_string()), + }) + ); + let ledger_parameters = map.get("ledger_parameters").unwrap(); + let ledger_parameters_map = match ledger_parameters { + pb_api::SelfDescribingValue::Map(map) => map, + _ => panic!("Expected a map"), + }; + assert_eq!( + ledger_parameters_map.get("token_name").unwrap(), + &pb_api::SelfDescribingValue::Text("some token name".to_string()) + ); + assert_eq!( + ledger_parameters_map.get("token_logo").unwrap(), + &pb_api::SelfDescribingValue::Map(hashmap! { + "base64_encoding".to_string() => pb_api::SelfDescribingValue::Text("base64 encoding of a token logo".to_string()), + }) + ); + + // Now check that the self-describing value does not have logos when we omit them, while the other fields are still present. + let self_describing_value_without_logos = + convert_self_describing_action(&self_describing_action, true) + .value + .unwrap(); + let map = match self_describing_value_without_logos { + pb_api::SelfDescribingValue::Map(map) => map, + _ => panic!("Expected a map"), + }; + assert_eq!( + map.get("name").unwrap(), + &pb_api::SelfDescribingValue::Text("some name".to_string()) + ); + assert_eq!(map.get("logo"), Some(&pb_api::SelfDescribingValue::Null)); + let ledger_parameters = map.get("ledger_parameters").unwrap(); + let ledger_parameters_map = match ledger_parameters { + pb_api::SelfDescribingValue::Map(map) => map, + _ => panic!("Expected a map"), + }; + assert_eq!( + ledger_parameters_map.get("token_name").unwrap(), + &pb_api::SelfDescribingValue::Text("some token name".to_string()) + ); + assert_eq!( + ledger_parameters_map.get("token_logo"), + Some(&pb_api::SelfDescribingValue::Null) + ); + } #[test] fn install_code_request_to_internal() { From ae2cf95bcd3e622df464a35c2833959ada52470c Mon Sep 17 00:00:00 2001 From: Jason Zhu Date: Thu, 8 Jan 2026 00:07:49 +0000 Subject: [PATCH 2/3] Address feedback --- .../governance/src/pb/proposal_conversions.rs | 96 ++++++++++++------- 1 file changed, 62 insertions(+), 34 deletions(-) diff --git a/rs/nns/governance/src/pb/proposal_conversions.rs b/rs/nns/governance/src/pb/proposal_conversions.rs index a56d0c5deb23..3883ea1abe10 100644 --- a/rs/nns/governance/src/pb/proposal_conversions.rs +++ b/rs/nns/governance/src/pb/proposal_conversions.rs @@ -275,10 +275,21 @@ fn convert_self_describing_action( let type_name = Some(type_name.clone()); let type_description = Some(type_description.clone()); + + let paths_to_omit = if omit_create_service_nervous_system_logos { + vec![ + RecordPath { fields_names: vec!["logo"] }, + RecordPath { + fields_names: vec!["ledger_parameters", "token_logo"], + }, + ] + } else { + vec![] + }; let value = value.as_ref().map(|value| { convert_self_describing_value( value, - &PathToOmit::default_paths_to_omit(omit_create_service_nervous_system_logos), + paths_to_omit, ) }); @@ -289,33 +300,44 @@ fn convert_self_describing_action( } } -struct PathToOmit { + +/// For example, suppose we have +/// +/// ``` +/// struct Plane { +/// wing: Wing, +/// landing_gear: LandingGear, +/// } +/// +/// struct LandingGear { +/// wheel: Wheel, +/// } +/// +/// struct Wheel { +/// tire: Tire, +/// } +/// +/// let plane: Plane = ...; +/// ``` +/// +/// To reach the `Tire` from `plane`, we can use the path +/// `vec!["landing_gear", "wheel", "tire"]`, because `plane.landing_gear.wheel.tire` +/// evaluates to the `Tire`. +#[derive(Clone)] +struct RecordPath { // Must have at least one element. - path: Vec<&'static str>, + fields_names: Vec<&'static str>, } enum OmitAction { DoNothing, OmitCurrent, - OmitDescendant(PathToOmit), + OmitDescendant(RecordPath), } -impl PathToOmit { - fn default_paths_to_omit(omit_create_service_nervous_system_logos: bool) -> Vec { - if omit_create_service_nervous_system_logos { - vec![ - Self { path: vec!["logo"] }, - Self { - path: vec!["ledger_parameters", "token_logo"], - }, - ] - } else { - vec![] - } - } - +impl RecordPath { fn matches(&self, field_name: &str) -> OmitAction { - let Some((&first, rest)) = self.path.split_first() else { + let Some((&first, rest)) = self.fields_names.split_first() else { // This should never happen, but we handle it anyway. return OmitAction::DoNothing; }; @@ -325,15 +347,15 @@ impl PathToOmit { if rest.is_empty() { return OmitAction::OmitCurrent; } - OmitAction::OmitDescendant(PathToOmit { - path: rest.to_vec(), + OmitAction::OmitDescendant(RecordPath { + fields_names: rest.to_vec(), }) } } fn convert_self_describing_field( field_name: &str, - paths_to_omit: &[PathToOmit], + paths_to_omit: Vec, original_value: &pb::SelfDescribingValue, ) -> pb_api::SelfDescribingValue { let match_results = paths_to_omit @@ -353,16 +375,22 @@ fn convert_self_describing_field( _ => None, }) .collect::>(); - convert_self_describing_value(original_value, &descendant_paths_to_omit) + convert_self_describing_value(original_value, descendant_paths_to_omit) } fn convert_self_describing_value( item: &pb::SelfDescribingValue, - paths_to_omit: &[PathToOmit], + paths_to_omit: Vec, ) -> pb_api::SelfDescribingValue { - let pb::SelfDescribingValue { value: Some(value) } = item else { + let pb::SelfDescribingValue { value } = item; + + // This should be unreacheable, because we always construct a SelfDescribingValue with a value. + // Ideally the type should be non-optional, but prost always generates an optional field for + // messages. + let Some(value) = value else { return pb_api::SelfDescribingValue::Map(HashMap::new()); }; + match value { pb::self_describing_value::Value::Blob(v) => pb_api::SelfDescribingValue::Blob(v.clone()), pb::self_describing_value::Value::Text(v) => pb_api::SelfDescribingValue::Text(v.clone()), @@ -378,16 +406,21 @@ fn convert_self_describing_value( pb::self_describing_value::Value::Array(v) => pb_api::SelfDescribingValue::Array( v.values .iter() - .map(|value| convert_self_describing_value(value, &[])) + .map(|value| convert_self_describing_value(value, vec![])) .collect(), ), + + // This is where `paths_to_omit` takes effect - the resursion (calling + // `convert_self_describing_value` happens indirectly through + // `convert_self_describing_field`, which calls `convert_self_describing_value` if the field + // should not be omitted. pb::self_describing_value::Value::Map(v) => pb_api::SelfDescribingValue::Map( v.values .iter() .map(|(k, v)| { ( k.clone(), - convert_self_describing_field(k, paths_to_omit, v), + convert_self_describing_field(k, paths_to_omit.clone(), v), ) }) .collect(), @@ -395,15 +428,11 @@ fn convert_self_describing_value( } } -// This is only used for tests, because the `pb::SelfDescribingValue` can be large, and a direct -// conversion would most likely involve cloning the source value. Previously, we had an issue where -// it caused too many instructions due to the cloning, even when the large fields were omitted after -// cloning. The `cfg(test)` can be removed, however, if we decide to use SelfDescribingValue in -// other scenarios where there is less concern about cloning large fields. Ideally, we would +// To avoid cloning large values in production, this is only available in tests. #[cfg(test)] impl From for pb_api::SelfDescribingValue { fn from(value: pb::SelfDescribingValue) -> Self { - convert_self_describing_value(&value, &[]) + convert_self_describing_value(&value, vec![]) } } @@ -674,7 +703,6 @@ mod tests { #[test] fn test_self_describing_value_omit_logos() { - // Not all fields are included in the test, for simplicity. let create_service_nervous_system = CreateServiceNervousSystem { name: Some("some name".to_string()), logo: Some(Image { From 6e13c06948f2324da2768a0a1f94626b19def001 Mon Sep 17 00:00:00 2001 From: Jason Zhu Date: Thu, 8 Jan 2026 19:12:30 +0000 Subject: [PATCH 3/3] Address more feedback and format --- .../governance/src/pb/proposal_conversions.rs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/rs/nns/governance/src/pb/proposal_conversions.rs b/rs/nns/governance/src/pb/proposal_conversions.rs index 3883ea1abe10..e59b58a65f75 100644 --- a/rs/nns/governance/src/pb/proposal_conversions.rs +++ b/rs/nns/governance/src/pb/proposal_conversions.rs @@ -276,9 +276,11 @@ fn convert_self_describing_action( let type_name = Some(type_name.clone()); let type_description = Some(type_description.clone()); - let paths_to_omit = if omit_create_service_nervous_system_logos { + let paths_to_omit = if omit_create_service_nervous_system_logos { vec![ - RecordPath { fields_names: vec!["logo"] }, + RecordPath { + fields_names: vec!["logo"], + }, RecordPath { fields_names: vec!["ledger_parameters", "token_logo"], }, @@ -286,12 +288,9 @@ fn convert_self_describing_action( } else { vec![] }; - let value = value.as_ref().map(|value| { - convert_self_describing_value( - value, - paths_to_omit, - ) - }); + let value = value + .as_ref() + .map(|value| convert_self_describing_value(value, paths_to_omit)); pb_api::SelfDescribingProposalAction { type_name, @@ -300,7 +299,6 @@ fn convert_self_describing_action( } } - /// For example, suppose we have /// /// ``` @@ -324,7 +322,7 @@ fn convert_self_describing_action( /// `vec!["landing_gear", "wheel", "tire"]`, because `plane.landing_gear.wheel.tire` /// evaluates to the `Tire`. #[derive(Clone)] -struct RecordPath { +struct RecordPath { // Must have at least one element. fields_names: Vec<&'static str>, } @@ -384,10 +382,10 @@ fn convert_self_describing_value( ) -> pb_api::SelfDescribingValue { let pb::SelfDescribingValue { value } = item; - // This should be unreacheable, because we always construct a SelfDescribingValue with a value. - // Ideally the type should be non-optional, but prost always generates an optional field for - // messages. let Some(value) = value else { + // This should be unreacheable, because we always construct a SelfDescribingValue with a value. + // Ideally the type should be non-optional, but prost always generates an optional field for + // messages. return pb_api::SelfDescribingValue::Map(HashMap::new()); }; @@ -428,7 +426,9 @@ fn convert_self_describing_value( } } -// To avoid cloning large values in production, this is only available in tests. +// To avoid cloning large values in production, this is only available in tests. Since multiple +// tests need to convert SelfDescribingValue to `api::SelfDescribingValue`, we define it here rather +// than in each test. #[cfg(test)] impl From for pb_api::SelfDescribingValue { fn from(value: pb::SelfDescribingValue) -> Self {