Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nns/sns): Add allowed_viewers variant case into canister_status responses #3660

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasonz-dfinity
Copy link
Contributor

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

@jasonz-dfinity jasonz-dfinity requested a review from a team as a code owner January 28, 2025 19:16
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).

To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:

  1. Done.

  2. No canister behavior changes.

@github-actions github-actions bot added the feat label Jan 28, 2025
@@ -101,6 +101,12 @@ type LogVisibility = variant {
public;
};

type LogVisibilityInCanisterStatus = variant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NCR. Maybe "CanisterStatusLogVisibility"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants