Skip to content

Commit

Permalink
feat(nns/sns): Add allowed_viewers variant case into canister_status …
Browse files Browse the repository at this point in the history
…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`
  • Loading branch information
jasonz-dfinity authored Jan 31, 2025
1 parent 340f17d commit c5e098e
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 5 deletions.
10 changes: 6 additions & 4 deletions rs/nervous_system/clients/src/canister_status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PrincipalId>),
}

/// Partial copy-paste of ic-types::ic_00::DefiniteCanisterSettings.
Expand Down Expand Up @@ -86,7 +88,7 @@ pub struct CanisterStatusResult {
pub reserved_cycles: Option<candid::Nat>,
}

/// 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,
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion rs/nns/handlers/root/impl/canister/root.did
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions rs/nns/handlers/root/unreleased_changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions rs/sns/root/canister/root.did
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ type ListSnsCanistersResponse = record {
type LogVisibility = variant {
controllers;
public;
allowed_viewers: vec principal;
};

type ManageDappCanisterSettingsRequest = record {
Expand Down
4 changes: 4 additions & 0 deletions rs/sns/root/unreleased_changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit c5e098e

Please sign in to comment.