Skip to content

feat(release-runscript): set engine-controller authorized_caller on upgrade #10728

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.

Sign up for GitHub

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

Open
pietrodimarco-dfinity wants to merge 3 commits into
base: master
Choose a base branch
from
+27 −4
Open

feat(release-runscript): set engine-controller authorized_caller on upgrade #10728

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14cac99
feat(release-runscript): set engine-controller authorized_caller on u…
pietrodimarco-dfinity Jul 10, 2026
ee12dd2
Potential fix for pull request finding
pietrodimarco-dfinity Jul 15, 2026
4238dde
Automatically fixing code for linting and formatting issues
Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions rs/nervous_system/tools/release-runscript/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,16 +318,39 @@ fn run_create_proposal_texts(cmd: CreateProposalTexts) -> Result<()> {
// Some canisters require an explicit upgrade arg. cycles-minting
// needs a hand-provided one (usually '()'). engine-controller's
// post_upgrade traps when decoding an empty arg, so it must be
// given at least '()'; otherwise the upgrade silently rolls back
// while the proposal still reports as executed.
// given a non-empty record; otherwise the upgrade silently rolls
// back while the proposal still reports as executed. We prompt for
Comment on lines +321 to +322

@daniel-wong-dfinity-org-twin daniel-wong-dfinity-org-twin Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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".)

// the authorized caller principal and build the record around it.
let output = match canister.as_str() {
"cycles-minting" => {
let upgrade_arg = input_with_default("Upgrade arg for CMC?", "()")?;
run_script(script, &[canister, &commit, &upgrade_arg], &ic)
.expect("Failed to run NNS proposal text script")
}
"engine-controller" => run_script(script, &[canister, &commit, "()"], &ic)
.expect("Failed to run NNS proposal text script"),
"engine-controller" => {
// 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).
Comment on lines +331 to +336

@daniel-wong-dfinity-org-twin daniel-wong-dfinity-org-twin Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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 }})"
);
Comment on lines +331 to +350

@daniel-wong-dfinity-org-twin daniel-wong-dfinity-org-twin Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Spinning this out. In general, match arms should be short.

run_script(script, &[canister, &commit, &upgrade_arg], &ic)
.expect("Failed to run NNS proposal text script")
}
_ => run_script(script, &[canister, &commit], &ic)
.expect("Failed to run NNS proposal text script"),
};
Expand Down
Loading