feat(dfx-orbit,wallet): support --wasm-memory-persistence for external canister upgrades#641
feat(dfx-orbit,wallet): support --wasm-memory-persistence for external canister upgrades#641aterga wants to merge 7 commits into
Conversation
…l canister upgrades Expose the upgrade options that the station already accepts (`CanisterUpgradeOptionsInput`) in the two places users trigger external canister upgrades: the dfx-orbit CLI and the wallet Install screen. dfx-orbit CLI: - Add `--wasm-memory-persistence <keep|replace>` and `--skip-pre-upgrade` flags to `request/verify canister install`. The flags are folded into `CanisterInstallMode::Upgrade(Some(..))` and rejected for install/reinstall. - `verify` now compares the full install mode (including the upgrade options), so a request with a mismatched persistence mode is rejected. - Show the upgrade options in the change-canister request display. Wallet Install screen: - Add a "Wasm Memory Persistence" select and a "Skip pre-upgrade hook" toggle that appear only for the upgrade mode, wired into the install model's `mode`. Also derive `PartialEq`/`Eq` for `station_api::CanisterInstallMode` to support the CLI's full-mode verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
Allow clippy::unwrap_used in the test module (the crate denies it at the root) and avoid a redundant local rebinding flagged by clippy::redundant_locals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
|
✅ No security or compliance issues detected. Reviewed everything up to 3cbcdd5. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
Pull request overview
Wires the station’s existing upgrade-only options (wasm_memory_persistence, skip_pre_upgrade) through the two main user entry points—dfx-orbit CLI and the wallet’s external-canister Install screen—so upgrades can preserve Motoko EOP main memory and optionally bypass trapping pre_upgrade hooks.
Changes:
dfx-orbit: adds--wasm-memory-persistence <keep|replace>and--skip-pre-upgrade, folds them intoCanisterInstallMode::Upgrade(Some(..)), rejects them for install/reinstall, and verifies the full mode (including upgrade options).- Wallet UI: adds an upgrade-only select + toggle, collapses back to plain
{ upgrade: [] }when unset, and adds i18n strings. - API: derives
PartialEq/EqforCanisterInstallModeto support full-mode verification comparisons.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/dfx-orbit/src/canister/install.rs | Adds CLI flags, folds them into upgrade mode, improves verification to compare full mode, and prints upgrade options in request display; includes unit tests. |
| tools/dfx-orbit/src/canister.rs | Re-exports the new CLI enum for wasm memory persistence. |
| tools/dfx-orbit/README.md | Documents new CLI flags and usage for preserving main memory on upgrade. |
| tests/integration/src/dfx_orbit/install.rs | Adds an integration test covering upgrade-options round-trip + verifier mismatch rejection. |
| core/station/api/src/common.rs | Enables PartialEq/Eq on CanisterInstallMode (for CLI full-mode comparisons). |
| apps/wallet/src/locales/pt.locale.ts | Adds Portuguese strings for the new upgrade options UI. |
| apps/wallet/src/locales/fr.locale.ts | Adds French strings for the new upgrade options UI. |
| apps/wallet/src/locales/en.locale.ts | Adds English strings for the new upgrade options UI. |
| apps/wallet/src/components/inputs/CanisterWasmMemoryPersistenceSelect.vue | New select input mapping UI choice to candid-style union values. |
| apps/wallet/src/components/external-canisters/external-canisters.types.ts | Adds derived TS helper types for upgrade options + wasm memory persistence. |
| apps/wallet/src/components/external-canisters/CanisterInstallForm.vue | Adds upgrade-only persistence select + skip-pre-upgrade toggle; merges/collapses upgrade options into mode. |
| apps/wallet/src/components/external-canisters/CanisterInstallForm.spec.ts | Extends form tests for the new UI behavior (with some missing coverage addressed in review comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…apping
Address review feedback by adding a dedicated spec for
CanisterWasmMemoryPersistenceSelect (select option <-> candid union
mapping) and tests asserting the skip_pre_upgrade toggle folds into
mode.upgrade[0].skip_pre_upgrade and collapses back to { upgrade: [] }
when cleared.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
daniel-wong-dfinity-org-twin
left a comment
daniel-wong-dfinity-org-twin
left a comment
There was a problem hiding this comment.
For the record, I really can't properly review the TS stuff. Nevertheless, I muddled through that portion of the review as best as I can, since reviewers are scarce these days...
| let persistence = match persistence { | ||
| WasmMemoryPersistence::Keep => "keep", | ||
| WasmMemoryPersistence::Replace => "replace", | ||
| }; |
There was a problem hiding this comment.
I know that this just uses the same pattern as existing code (so just keep on doing this), but it was not a good pattern to begin with. What we are doing here is converting to a string. Such functionality is so fundamental that has optimized it to be LESS THAN 1 line: #[derive(Debug)]. Let's use that, as god intended:
writeln!(output, "Wasm memory persistence: {persistence:?}");
The only "problem" with this is that instead of "keep", you get "Keep". If you really hate the capital "K", you can get rid of it very concisely as follows:
let persistence = format!("{:?}", persistence).to_lowercase();
writeln!(output, "Wasm memory persistence: {persistence}");
There was a problem hiding this comment.
Done in b88cd2e — the display string is now derived via format!("{persistence:?}").to_lowercase(), as suggested.
Generated by Claude Code
There was a problem hiding this comment.
Ok, but how about mode? That's why I said,
I know that this just uses the same pattern as existing code (so just keep on doing this)
This thread was more of a rant than an actual suggestion.
There was a problem hiding this comment.
Understood. For mode specifically, the Debug trick doesn't quite work: the Upgrade variant carries a payload, so {mode:?} would print Upgrade(Some(CanisterUpgradeOptionsInput { .. })) rather than a clean Upgrade — hence the hand-written match stays there.
Generated by Claude Code
| // The upgrade request is still pending, so the canister remains empty. | ||
| let status = canister_status(&env, Some(canister_ids.station), test_canister); | ||
| assert_eq!(status.module_hash, None); |
There was a problem hiding this comment.
Wouldn't it make more sense to follow through, i.e. install the wasm? I mean, ultimately, what we really want to see is that Keep and skip_pre_upgrade were actually, you know, PERFORMED as requested, right??
There was a problem hiding this comment.
Done in b88cd2e — the test now installs the module first, then the approved upgrade executes with wasm_memory_persistence = replace + skip_pre_upgrade and wait_for_request asserts it completed, so the options are exercised through the management canister rather than left pending. One caveat: it uses replace instead of keep, because the test canister isn't built with Enhanced Orthogonal Persistence and the IC rejects keep for modules without the EOP metadata section. Truly proving keep preserves memory would need a Motoko EOP fixture wasm, which felt out of scope here.
Generated by Claude Code
There was a problem hiding this comment.
out of scope here.
I don't think so. The whole motivation here is so that I can install a canister that was recently written in Motoko (and therefore, uses EOP).
There was a problem hiding this comment.
Fair point — done in 595ddfd. New integration test canister_upgrade_with_wasm_memory_persistence_keep: it marks the test module as EOP by appending the icp:private enhanced-orthogonal-persistence custom section (which is exactly how the IC detects EOP support — no Motoko toolchain needed), installs it, and then shows the same upgrade request fails without keep (the IC's safety check refuses to drop an EOP canister's main memory) and executes successfully with keep. That exercises the real motivating scenario end-to-end through the station.
Generated by Claude Code
| emit('update:modelValue', { replace: null }); | ||
| break; | ||
| default: | ||
| emit('update:modelValue', undefined); |
There was a problem hiding this comment.
This looks like sweeping a problem under the rug.
There was a problem hiding this comment.
Done in b88cd2e — the catch-all default: arm is gone; 'default' is now an explicit case with a comment explaining it means "let the IC decide" and maps to an absent wasm_memory_persistence option in the request. The select's items are constrained to the three options, so every case is now spelled out.
Generated by Claude Code
There was a problem hiding this comment.
Does the IC decide automatically?? If so, then, it seems that this feature is not super necessary.
There was a problem hiding this comment.
No — the IC never infers keep. What it does is fail closed: upgrading a canister whose module declares EOP without keep is rejected outright (so no silent memory loss, but also no upgrade). It's dfx that infers client-side from the Wasm metadata and fills the flag in for you. So the option is necessary — without it, EOP canisters simply cannot be upgraded through Orbit at all. The new integration test in 595ddfd demonstrates both halves (rejected without keep, succeeds with it).
Generated by Claude Code
- Name conversion targets explicitly (WasmMemoryPersistence::from over
Into::into) and replace .then_some with a plain if/else.
- Rename ensure_no_upgrade_options to err_if_upgrade_flags_were_passed.
- Derive the display string for the persistence mode via Debug instead of
a hand-written match.
- Split the combined `pub use self::{..}` re-export into per-module
statements.
- Fix the Enhanced Orthogonal Persistence docs link (docs moved to
docs.internetcomputer.org).
- Integration test now installs the module first and executes the
approved upgrade with wasm_memory_persistence=replace +
skip_pre_upgrade, proving the options are forwarded to the IC (keep
cannot execute against the non-EOP test wasm).
- Noun-y test variable names, assert the exact rejection error message,
and fold the redundant plain-mode mapping tests into one.
- Make the "default" option mapping explicit in the persistence select
instead of a catch-all switch arm.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
…dule Address review feedback: the motivating use case is upgrading Motoko canisters with Enhanced Orthogonal Persistence, so exercise exactly that. The new integration test marks the test module as EOP by appending the `icp:private enhanced-orthogonal-persistence` custom section (which is how the IC detects EOP support), installs it, and then shows the same upgrade request fails without `keep` (the IC's safety check against accidentally dropping main memory) and executes successfully with it. Also link the README to the docs section on IC main memory retention and correct the wording: the IC refuses to upgrade EOP canisters without `keep` rather than silently clearing their memory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
…P section
The bundled test module is gzipped, and the IC reads the uncompressed
size of gzipped modules from the trailing bytes of the gzip stream — so
appending the custom section after the stream made the IC read the tail
of the section name ("ence") as a 1.7 GB module size. Decompress first;
the raw module with the appended section installs fine.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
Summary
The station already accepts
wasm_memory_persistenceandskip_pre_upgradeon external-canister upgrades (viaCanisterUpgradeOptionsInput, added in #634), but neither of the two places users actually trigger an upgrade exposed them. This PR wires those upgrade options through the dfx-orbit CLI and the wallet Install screen.Without
wasm_memory_persistence = keep, Motoko canisters that use Enhanced Orthogonal Persistence cannot be safely upgraded through Orbit — the IC defaults toreplace, wiping their main memory.What changed
dfx-orbit CLI (
tools/dfx-orbit)request canister install/verify canister install:--wasm-memory-persistence keep|replace--skip-pre-upgradeCanisterInstallMode::Upgrade(Some(..))and are rejected forinstall/reinstall(the IC only honours them on upgrades).verifynow compares the full install mode (including the upgrade options), so a request whose stored persistence mode differs from what the verifier expects is rejected instead of silently passing.Wallet Install screen (
apps/wallet)upgrade, wired into the install model'smode.{ upgrade: [] }, keeping it identical to the previous behaviour.CanisterWasmMemoryPersistenceSelectinput component and locale strings (en/fr/pt).API
PartialEq/Eqforstation_api::CanisterInstallModeso the CLI can compare the full mode during verification (additive; does not change the candid interface).Tests
install_mode()folds each flag correctly, producesUpgrade(None)when unset, and rejects the flags for install/reinstall with the exact error message.wasm_memory_persistence = replace+skip_pre_upgraderound-trips through the station;verifyaccepts a matching request and rejects a mismatched persistence mode; the approved upgrade then executes, proving the options are forwarded to the management canister. (keepcannot execute against the non-EOP test wasm — the IC rejects it for modules without the EOP metadata section.)upgrade, both options fold intomode, clearing them collapses back to a plain upgrade, and the persistence select's option↔candid-union mapping is covered by a dedicated spec.Verification performed here
walletvue-tsc --noEmittype-check: clean.walletvitest for the touched components/inputs: passing.prettier --checkandeslinton the changed wallet files: clean.rustfmt --checkon the changed Rust files: clean.cargo build/cargo test/clippycould not run in this environment because thedfinity/response-verificationgit dependency is outside the sandbox's egress allow-list. CI exercises the Rust build, unit tests, and PocketIC integration tests.🤖 Generated with Claude Code