feat(nns): Support SelfDescribingValue::Null (#8205)

# Why

To make "empty" values more explicit, we opt to support a `Null` variant
to represent absent values.

The downside of diverging from ICRC3 was considered. However, if we
choose to adopt ICRC3 for proposals in the future (if at all), it will
likely affect the entire method (`list_proposals`) rather than the
"action" part of the proposal.

# What

* Introduce a `Null` variant for storage and api
* Define conversions between them
* Define `impl From<Option<T>> for SelfDescribingValue` using `Null` for
`None`
* Remove add_field_with_empty_as_fallback as `null` can be used to
represent the "emptiness"
* Use `null` for some ManageNeuron commands where no content is needed

Author: jasonz-dfinity

rs/nns/governance/api/src/types.rs

@@ -4567,6 +4567,7 @@ pub enum SelfDescribingValue {
     Text(String),
     Nat(Nat),
     Int(Int),
+    Null,
     Array(Vec<SelfDescribingValue>),
     Map(HashMap<String, SelfDescribingValue>),
 }

rs/nns/governance/canister/governance.did

@@ -1480,6 +1480,7 @@ type SelfDescribingValue = variant {
   Int : int;
   Array : vec SelfDescribingValue;
   Map : vec record { text; SelfDescribingValue };
+  Null;
 };
 
 type GetPendingProposalsRequest = record {

rs/nns/governance/derive_self_describing/src/lib.rs

@@ -188,11 +188,11 @@ fn derive_mixed_enum(name: &Ident, data_enum: &DataEnum) -> Result<TokenStream2,
                     })
                 }
                 Fields::Unit => {
-                    // Unit variant in mixed enum: map with variant name as key, empty array as value
+                    // Unit variant in mixed enum: map with variant name as key, null value as value
                     Ok(quote! {
                         #name::#variant_name => {
                             crate::proposals::self_describing::ValueBuilder::new()
-                                .add_empty_field(#variant_name_str)
+                                .add_field(#variant_name_str, crate::proposals::self_describing::SelfDescribingValue::NULL)
                                 .build()
                         }
                     })

rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto

@@ -2651,6 +2651,7 @@ message SelfDescribingValue {
     bytes int = 4;
     SelfDescribingValueArray array = 5;
     SelfDescribingValueMap map = 6;
+    Empty null = 7;
   }
 }
 

rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs

@@ -4138,7 +4138,7 @@ pub struct FinalizeDisburseMaturity {
     ::prost::Message,
 )]
 pub struct SelfDescribingValue {
-    #[prost(oneof = "self_describing_value::Value", tags = "1, 2, 3, 4, 5, 6")]
+    #[prost(oneof = "self_describing_value::Value", tags = "1, 2, 3, 4, 5, 6, 7")]
     pub value: ::core::option::Option<self_describing_value::Value>,
 }
 /// Nested message and enum types in `SelfDescribingValue`.
@@ -4166,6 +4166,8 @@ pub mod self_describing_value {
         Array(super::SelfDescribingValueArray),
         #[prost(message, tag = "6")]
         Map(super::SelfDescribingValueMap),
+        #[prost(message, tag = "7")]
+        Null(super::Empty),
     }
 }
 #[derive(

rs/nns/governance/src/pb/conversions/mod.rs

@@ -4202,6 +4202,7 @@ impl From<pb::SelfDescribingValue> for pb_api::SelfDescribingValue {
                     .map(|(k, v)| (k, Self::from(v)))
                     .collect(),
             ),
+            pb::self_describing_value::Value::Null(_) => Self::Null,
         }
     }
 }
@@ -4231,6 +4232,9 @@ impl From<pb_api::SelfDescribingValue> for pb::SelfDescribingValue {
                     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) }
     }

rs/nns/governance/src/proposals/add_or_remove_node_provider_tests.rs

@@ -281,9 +281,7 @@ fn test_to_self_describing_value() {
         SelfDescribingValue::Map(hashmap! {
             "ToAdd".to_string() => SelfDescribingValue::Map(hashmap! {
                 "id".to_string() => SelfDescribingValue::Text("6fyp7-3ibaa-aaaaa-aaaap-4ai".to_string()),
-                "reward_account".to_string() => SelfDescribingValue::Array(vec![
-                    SelfDescribingValue::Text(account.to_hex())
-                ])
+                "reward_account".to_string() => SelfDescribingValue::Text(account.to_hex())
             })
         })
     );

rs/nns/governance/src/proposals/deregister_known_neuron.rs

@@ -60,7 +60,7 @@ impl LocallyDescribableProposalAction for DeregisterKnownNeuron {
 
     fn to_self_describing_value(&self) -> SelfDescribingValue {
         ValueBuilder::new()
-            .add_field_with_empty_as_fallback("neuron_id", self.id.map(|id| id.id))
+            .add_field("neuron_id", self.id.map(|id| id.id))
             .build()
     }
 }

rs/nns/governance/src/proposals/install_code.rs

@@ -187,10 +187,10 @@ impl LocallyDescribableProposalAction for InstallCode {
         let install_mode = install_mode.map(SelfDescribingProstEnum::<CanisterInstallMode>::new);
 
         ValueBuilder::new()
-            .add_field_with_empty_as_fallback("canister_id", *canister_id)
-            .add_field_with_empty_as_fallback("install_mode", install_mode)
-            .add_field_with_empty_as_fallback("wasm_module_hash", wasm_module_hash.clone())
-            .add_field_with_empty_as_fallback("arg_hash", arg_hash.clone())
+            .add_field("canister_id", *canister_id)
+            .add_field("install_mode", install_mode)
+            .add_field("wasm_module_hash", wasm_module_hash.clone())
+            .add_field("arg_hash", arg_hash.clone())
             .add_field(
                 "skip_stopping_before_installing",
                 skip_stopping_before_installing.unwrap_or_default(),

rs/nns/governance/src/proposals/manage_neuron.rs

@@ -51,13 +51,11 @@ impl LocallyDescribableProposalAction for ManageNeuron {
                     "A ManageNeuron proposal is created with an empty or conflicting \
                     values of id and neuron_id_or_subaccount. This should never happen."
                 );
-                builder.add_empty_field("neuron_id_or_subaccount")
+                builder.add_field("neuron_id_or_subaccount", SelfDescribingValue::NULL)
             }
         };
 
-        builder
-            .add_field_with_empty_as_fallback("command", self.command.clone())
-            .build()
+        builder.add_field("command", self.command.clone()).build()
     }
 }
 
@@ -99,13 +97,13 @@ impl From<Command> for SelfDescribingValue {
                 println!(
                     "A ManageNeuron proposal is created with a MergeMaturity command. This should never happen."
                 );
-                Self::singleton_map("MergeMaturity", Self::EMPTY)
+                Self::singleton_map("MergeMaturity", Self::NULL)
             }
             C::MakeProposal(_) => {
                 println!(
                     "A ManageNeuron proposal is created with a MakeProposal command. This should never happen."
                 );
-                Self::singleton_map("MakeProposal", Self::EMPTY)
+                Self::singleton_map("MakeProposal", Self::NULL)
             }
         }
     }
@@ -117,7 +115,7 @@ impl From<Configure> for SelfDescribingValue {
             println!(
                 "A ManageNeuron proposal is created with an empty operation. This should never happen."
             );
-            return Self::EMPTY;
+            return Self::NULL;
         };
 
         use Operation as O;
@@ -161,14 +159,14 @@ impl From<IncreaseDissolveDelay> for SelfDescribingValue {
 impl From<StartDissolving> for SelfDescribingValue {
     fn from(value: StartDissolving) -> Self {
         let StartDissolving {} = value;
-        SelfDescribingValue::EMPTY
+        SelfDescribingValue::NULL
     }
 }
 
 impl From<StopDissolving> for SelfDescribingValue {
     fn from(value: StopDissolving) -> Self {
         let StopDissolving {} = value;
-        SelfDescribingValue::EMPTY
+        SelfDescribingValue::NULL
     }
 }
 
@@ -198,14 +196,14 @@ impl From<SetDissolveTimestamp> for SelfDescribingValue {
 impl From<JoinCommunityFund> for SelfDescribingValue {
     fn from(value: JoinCommunityFund) -> Self {
         let JoinCommunityFund {} = value;
-        SelfDescribingValue::EMPTY
+        SelfDescribingValue::NULL
     }
 }
 
 impl From<LeaveCommunityFund> for SelfDescribingValue {
     fn from(value: LeaveCommunityFund) -> Self {
         let LeaveCommunityFund {} = value;
-        SelfDescribingValue::EMPTY
+        SelfDescribingValue::NULL
     }
 }
 
@@ -320,7 +318,7 @@ impl From<ClaimOrRefresh> for SelfDescribingValue {
             println!(
                 "A ManageNeuron proposal is created with an empty by. This should never happen."
             );
-            return Self::EMPTY;
+            return Self::NULL;
         };
 
         match by {
@@ -359,7 +357,7 @@ impl From<Merge> for SelfDescribingValue {
 impl From<RefreshVotingPower> for SelfDescribingValue {
     fn from(value: RefreshVotingPower) -> Self {
         let RefreshVotingPower {} = value;
-        SelfDescribingValue::EMPTY
+        SelfDescribingValue::NULL
     }
 }