From c5e098e8cc8e62249e6d7f4ed09e6c2ed87fc800 Mon Sep 17 00:00:00 2001 From: jasonz-dfinity <133917836+jasonz-dfinity@users.noreply.github.com> Date: Fri, 31 Jan 2025 13:06:14 -0800 Subject: [PATCH] feat(nns/sns): Add allowed_viewers variant case into canister_status responses (#3660) # Why Currently, the management canister can return the LogVisibility::AllowedViewers variant for the canister status of a canister under NNS/SNS root control. Although it's currently impossible for NNS/SNS root to change the log visibility of any canister under their control into this variant, it's still possible for a canister with this variant to be brought under its control. When the `canister_status` is called in this case, the `canister_status` call made by root to the management canister will fail at decoding the response. This PR aims to improve that. # What - Add the `allowed_viewers` variant into `LogVisibility` where it's part of the `canister_status` response (not when it's part of `update_settings`). Since the response from SNS/NNS root also uses this enum, this change also affects NNS/SNS root response types. - Update NNS/SNS Root `canister_status` response types # Why It's Safe We need to reason about the data flow in such sequence (true for both NNS/SNS Root, and will refer to them as Root below), when `Root::canister_status` is called: 1. Root gets `canister_status` response from the management canister as bytes 2. Root decodes the response into `CanisterStatusResultFromManagementCanister` 3. Root converts `CanisterStatusResultFromManagementCanister` to `CanisterStatusResult` or `CanisterStatusResultV2` 4. Root encodes `CanisterStatusResult` or `CanisterStatusResultV2` into bytes 5. Clients of root (using root.did) decodes the response according to .did We can then reason about the above data flow in several cases: - When the log_visibility of the canister is `controllers/public`, there is no change. - When the log_visibility of the canister is `allowed_viewers` and the client uses the old `root.did`: - Root will successfully decode the response from the management canister (step 2), while it fails before this PR - The step 4 will result in `CanisterStatusResult` or `CanisterStatusResultV2` having the `allowed_viewers` variant - The client using the old `root.did` (no `allowed_viewers`) will not understand the variant, but since `log_visibility : opt LogVisibility`, it will decode it to `log_visibility : null` - When the log_visibility of the canister is `allowed_viewers` and the client uses the new `root.did`, the client will be able to see `allowed_viewers` --- rs/nervous_system/clients/src/canister_status.rs | 10 ++++++---- rs/nns/handlers/root/impl/canister/root.did | 8 +++++++- rs/nns/handlers/root/unreleased_changelog.md | 4 ++++ rs/sns/root/canister/root.did | 1 + rs/sns/root/unreleased_changelog.md | 4 ++++ 5 files changed, 22 insertions(+), 5 deletions(-) diff --git a/rs/nervous_system/clients/src/canister_status.rs b/rs/nervous_system/clients/src/canister_status.rs index 1b95ee0692e..5338deaa37b 100644 --- a/rs/nervous_system/clients/src/canister_status.rs +++ b/rs/nervous_system/clients/src/canister_status.rs @@ -47,9 +47,11 @@ impl std::fmt::Display for CanisterStatusType { pub enum LogVisibility { #[default] #[serde(rename = "controllers")] - Controllers = 1, + Controllers, #[serde(rename = "public")] - Public = 2, + Public, + #[serde(rename = "allowed_viewers")] + AllowedViewers(Vec), } /// Partial copy-paste of ic-types::ic_00::DefiniteCanisterSettings. @@ -86,7 +88,7 @@ pub struct CanisterStatusResult { pub reserved_cycles: Option, } -/// Copy-paste of ic-types::ic_00::CanisterStatusResult. +/// Copy-paste of `ic_management_canister_types::CanisterStatusResultV2`. #[derive(Clone, Eq, PartialEq, Debug, Default, CandidType, Deserialize)] pub struct CanisterStatusResultFromManagementCanister { pub status: CanisterStatusType, @@ -98,7 +100,7 @@ pub struct CanisterStatusResultFromManagementCanister { pub reserved_cycles: candid::Nat, } -/// Partial copy-paste of ic-types::ic_00::DefiniteCanisterSettings. +/// Partial copy-paste of `ic_management_canister_types::DefiniteCanisterSettingsArgs`. /// /// Only the fields that we need are copied. /// Candid deserialization is supposed to be tolerant to having data for unknown diff --git a/rs/nns/handlers/root/impl/canister/root.did b/rs/nns/handlers/root/impl/canister/root.did index 3e7099b6891..4fb1189f628 100644 --- a/rs/nns/handlers/root/impl/canister/root.did +++ b/rs/nns/handlers/root/impl/canister/root.did @@ -89,7 +89,7 @@ type DefiniteCanisterSettings = record { freezing_threshold : opt nat; controllers : vec principal; reserved_cycles_limit : opt nat; - log_visibility : opt LogVisibility; + log_visibility : opt CanisterStatusLogVisibility; wasm_memory_limit : opt nat; memory_allocation : opt nat; compute_allocation : opt nat; @@ -101,6 +101,12 @@ type LogVisibility = variant { public; }; +type CanisterStatusLogVisibility = variant { + controllers; + public; + allowed_viewers : vec principal; +}; + type StopOrStartCanisterRequest = record { action : CanisterAction; canister_id : principal; diff --git a/rs/nns/handlers/root/unreleased_changelog.md b/rs/nns/handlers/root/unreleased_changelog.md index 94126a0ff42..37be2d66b62 100644 --- a/rs/nns/handlers/root/unreleased_changelog.md +++ b/rs/nns/handlers/root/unreleased_changelog.md @@ -11,6 +11,10 @@ on the process that this file is part of, see ## Changed +- The `LogVisibility` returned from `canister_status` has one more variant `allowed_viewers`, + consistent with the corresponding management canister API. Calling `canister_status` for a + canister with such a log visibility setting will no longer panic. + ## Deprecated ## Removed diff --git a/rs/sns/root/canister/root.did b/rs/sns/root/canister/root.did index 352ee49fd64..b0c57420db2 100644 --- a/rs/sns/root/canister/root.did +++ b/rs/sns/root/canister/root.did @@ -112,6 +112,7 @@ type ListSnsCanistersResponse = record { type LogVisibility = variant { controllers; public; + allowed_viewers: vec principal; }; type ManageDappCanisterSettingsRequest = record { diff --git a/rs/sns/root/unreleased_changelog.md b/rs/sns/root/unreleased_changelog.md index 94126a0ff42..37be2d66b62 100644 --- a/rs/sns/root/unreleased_changelog.md +++ b/rs/sns/root/unreleased_changelog.md @@ -11,6 +11,10 @@ on the process that this file is part of, see ## Changed +- The `LogVisibility` returned from `canister_status` has one more variant `allowed_viewers`, + consistent with the corresponding management canister API. Calling `canister_status` for a + canister with such a log visibility setting will no longer panic. + ## Deprecated ## Removed