feat(release-runscript): set engine-controller authorized_caller on upgrade#10728
feat(release-runscript): set engine-controller authorized_caller on upgrade#10728pietrodimarco-dfinity wants to merge 3 commits into
Conversation
…pgrade
The release-runscript hardcoded `()` as the engine-controller upgrade
argument, which cannot set the canister's authorized caller. Change the
Create Proposal Texts step to prompt for the authorized caller principal
and build the upgrade arg:
(opt record {
authorized_caller = opt principal "<CALLER>";
initial_dkg_subnet_id = null
})
This candid string is passed as CANDID_ARGS to
prepare-nns-upgrade-proposal-text.sh, which encodes it with
`didc encode | xxd -r -p` to compute the proposal's arg_hash and
verification section.
An empty answer emits `authorized_caller = null`, making the canister
fall back to its built-in default caller (rather than the previous
behavior of producing an invalid `opt principal ""`). The canister
re-applies args on every post_upgrade, so the caller must be supplied
each release to retain it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the release-runscript NNS release tool so engine-controller upgrades can set (or retain) authorized_caller by prompting for a principal and constructing a non-empty Candid upgrade argument, instead of hardcoding ().
Changes:
- Prompt for an engine-controller
authorized_callerprincipal during proposal-text generation. - Build a structured
(opt record { ... })Candid upgrade arg (withinitial_dkg_subnet_id = null) for engine-controller upgrades.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 4238dde. Security Overview
Detected Code Changes
|
|
I removed ephemera content from PR description (note to reviewer, and test status sections). PTAL (in case you want to restore any of that content). Why such content does not belong: The PR (title and) description are used as the main content of the eventual commit message. Therefore, it should not contain information that is only relevant at review time, at least IMHO. OTOH, GitHub really does not provide a good way to convey such information. What I like to do is reply to my own PR, but this can easily be missed by reviewers. |
| // given a non-empty record; otherwise the upgrade silently rolls | ||
| // back while the proposal still reports as executed. We prompt for |
There was a problem hiding this comment.
while the proposal still reports as executed
This is tangential. Therefore, I would either not mention it, or put it in partheneses. I would also maybe mention that this behavior is a bug.
(We have no plan to fix it, because while it is pretty misleading, it's not "fatal".)
| // An empty answer emits `null`, which makes the canister | ||
| // fall back to its built-in default authorized caller; a | ||
| // non-empty value is wrapped in `opt principal "..."`. | ||
| // The canister re-applies this on every post_upgrade, so | ||
| // the caller must be supplied each release to retain it | ||
| // (otherwise it reverts to the default). | ||
| let new_caller = input( | ||
| "Authorized caller principal for engine-controller? (leave empty for canister default)", | ||
| )?; | ||
| let authorized_caller = if new_caller.is_empty() { | ||
| "null".to_string() | ||
| } else { | ||
| let principal = candid::Principal::from_text(&new_caller).map_err(|e| { | ||
| anyhow::anyhow!("Invalid principal '{new_caller}': {e}") | ||
| })?; | ||
| format!("opt principal \"{}\"", principal.to_text()) | ||
| }; | ||
| let upgrade_arg = format!( | ||
| "(opt record {{ authorized_caller = {authorized_caller}; initial_dkg_subnet_id = null }})" | ||
| ); |
There was a problem hiding this comment.
Spinning this out. In general, match arms should be short.
| // An empty answer emits `null`, which makes the canister | ||
| // fall back to its built-in default authorized caller; a | ||
| // non-empty value is wrapped in `opt principal "..."`. | ||
| // The canister re-applies this on every post_upgrade, so | ||
| // the caller must be supplied each release to retain it | ||
| // (otherwise it reverts to the default). |
There was a problem hiding this comment.
An empty answer emits
null
This is just a direct transcription of code to prose. And the code is not particularly opaque. Do not write such comments. Comments are supposed to supply information that is not readily gleaned from the code, such as intent.
a non-empty value is wrapped in
opt principal "..."
Ditto: this is just code to prose transcription.
makes the canister fall back to its built-in default authorized caller
This seems tangential to the code being commented.
The real beneficiary of this information is the user of this program, not maintainer. But the user cannot read this comment. What the user can see is the prompt, which already has some explanation (albeit very brief): "(leave empty for canister default)". Therefore, this doesn't seem to add information beyond what can already be readily gleaned from the code.
The canister re-applies this on every post_upgrade, so the caller must be supplied each release to retain it (otherwise it reverts to the default).
Ditto: this is more of a concern for the user of this program, not maintainer.
TANGENT: Wouldn't it make more sense for the Engine Controller canister to not require passing the same information on every upgrade? Seems pretty surprising.
What
The
release-runscriptrelease tool hardcoded()as the engine-controller upgrade argument in the Create Proposal Texts step.()cannot set the canister'sauthorized_caller, so it was impossible to change the authorized caller through the normal release flow.This changes the engine-controller branch to prompt for the authorized caller principal and build the upgrade arg:
Why
The engine-controller's
post_upgradere-applies its init args on every upgrade (apply_init_argsinrs/engine_controller/canister/canister.rs). Ifauthorized_callerisnull, the canister falls back to its built-inDEFAULT_AUTHORIZED_CALLER. So to set (or retain) a specific authorized caller, the value must be supplied on each release — which the tool previously couldn't do.The candid string is passed as
CANDID_ARGStoprepare-nns-upgrade-proposal-text.sh, which encodes it viadidc encode | xxd -r -pto compute the proposal'sarg_hashand the verification section.🤖 Generated with Claude Code