Skip to content

feat(wallet): show wasm_memory_persistence upgrade options on change canister requests - #660

Merged
MRmarioruci merged 3 commits into
mainfrom
claude/wasm-memory-persistence-upgrades-epmpwc
Sep 10, 2026
Merged

feat(wallet): show wasm_memory_persistence upgrade options on change canister requests#660
MRmarioruci merged 3 commits into
mainfrom
claude/wasm-memory-persistence-upgrades-epmpwc

Conversation

@aterga

@aterga aterga commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Completes the wasm_memory_persistence support for external canister upgrades across the CLI and the frontend.

The station (#634), the dfx-orbit CLI and the wallet Install form (#641) already accept wasm_memory_persistence and skip_pre_upgrade on external canister upgrades. The remaining gap was on the review side of the wallet: ChangeExternalCanister requests rendered through the generic UnsupportedOperation JSON dump in both the request list and the request detail dialog, so approvers had no readable view of whether an upgrade keeps or replaces the canister's main memory.

What changed

Wallet (apps/wallet)

  • New ChangeExternalCanisterOperation request view, wired into RequestDetailView and RequestListItem, showing:
    • the target canister (name resolved via get_external_canister in detail mode, falling back to the canister id),
    • the install mode (Install / Reinstall / Upgrade),
    • for upgrades, the Wasm Memory Persistence (Keep / Replace / Default (replace)) and Skip pre-upgrade hook options,
    • the module and argument checksums (truncated in the list, full in the detail view).
  • The compact list view only surfaces the upgrade options when they were explicitly set on the request (an explicit skip_pre_upgrade = false is shown as "No"); the detail view always shows the effective values for an upgrade.
  • The target canister name is fetched with a verified call, like the request itself in the approval dialog, so a non-consensus response cannot attach a misleading name to the verified canister id.
  • Reuses the existing external_canisters.* and terms.* locale keys, so en/fr/pt stay in parity without new strings.

Docs

  • docs/src/content/docs/users/external-canisters.md: documents the Wasm Memory Persistence and skip-pre-upgrade options in the canister upgrade steps, including the Motoko Enhanced Orthogonal Persistence requirement for Keep.

dfx-orbit CLI

  • No code changes needed: request canister install / verify canister install already expose --wasm-memory-persistence keep|replace and --skip-pre-upgrade, and review prints them. Re-verified with cargo test -p dfx-orbit --lib (5 tests pass).

Tests

  • New ChangeExternalCanisterOperation.spec.ts (12 tests): mode labels, upgrade options in list vs. detail mode, explicit skip_pre_upgrade = false in list mode, default persistence shown for plain upgrades, options hidden for install/reinstall, canister name resolution through a verified call and fallback, no lookup in list mode.
  • RequestDetailView.spec.ts: new test asserting change-canister requests render through the dedicated view with their persistence option.
  • vitest over components/requests, components/external-canisters, components/inputs, mappers: 182 tests / 46 files pass.
  • vue-tsc --noEmit, eslint and prettier --check on the changed files pass.
  • CI is green on the current head, including e2e-tests:required. (The first CI run hit the known-flaky disaster-recovery.spec.ts end-to-end test, which fails intermittently on unrelated PRs too; see the comment below.)

Not changed on purpose

  • SystemUpgrade (station / upgrader) does not carry wasm_memory_persistence: both are Rust canisters, for which the IC rejects keep, so the option is not meaningful there.

🤖 Generated with Claude Code

https://claude.ai/code/session_019wAX3aCg6ov8rMvYeBxbgK

…canister requests

The station (#634), the dfx-orbit CLI and the wallet Install form (#641)
already accept `wasm_memory_persistence` and `skip_pre_upgrade` on
external canister upgrades, but reviewers had no readable view of them:
`ChangeExternalCanister` requests rendered through the generic
`UnsupportedOperation` JSON dump in both the request list and the
request detail dialog.

Add a dedicated `ChangeExternalCanisterOperation` request view that shows
the target canister (resolving its name in detail mode when possible),
the install mode, the Wasm memory persistence and skip-pre-upgrade
options of an upgrade, and the module/argument checksums. The compact
list view only surfaces the upgrade options when they were explicitly
set, while the detail view always shows the effective values so
approvers can see whether main memory will be kept or replaced.

Also document the upgrade options in the external canisters user guide.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019wAX3aCg6ov8rMvYeBxbgK

aterga commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

CI status on 516b5ec: every check is green except e2e-tests:required.

The failing test is tests/e2e/disaster-recovery.spec.ts › can recover uninstalled station: on chromium it hit the 600s test timeout three times while waiting for the Disaster Recovery page to render (getByText(/Disaster Recovery State/i)), and on firefox it failed once and passed on retry (reported as flaky). This PR only changes how ChangeExternalCanister requests are rendered in the request list/detail views (plus a docs page), and DisasterRecoveryPage.vue does not use those components, so the failure is not caused by this diff. The same job failed on main in the same way at #636 and #641, with all other jobs green, and there is no fix for the flakiness on main to port yet.

Re-running the failed job once; I'll keep watching the PR.


Generated by Claude Code

Copilot AI left a comment

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.

🟡 Changes recommended

The compact view mishandles explicit false values, and target names are fetched without verification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds readable external-canister upgrade details to wallet request reviews and documents upgrade options.

Changes:

  • Adds a dedicated change-canister request renderer.
  • Displays persistence, pre-upgrade, target, mode, and checksums.
  • Adds documentation and component tests.
File summaries
File Description
docs/src/content/docs/users/external-canisters.md Documents upgrade options.
apps/wallet/src/components/requests/RequestListItem.vue Uses the dedicated list renderer.
apps/wallet/src/components/requests/RequestDetailView.vue Uses the dedicated detail renderer.
apps/wallet/src/components/requests/RequestDetailView.spec.ts Tests detail-view integration.
apps/wallet/src/components/requests/operations/ChangeExternalCanisterOperation.vue Implements request rendering and name resolution.
apps/wallet/src/components/requests/operations/ChangeExternalCanisterOperation.spec.ts Tests rendering behavior.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…names with a verified call

Address review findings on the change canister request view:

- The compact list view decided whether to show the skip-pre-upgrade row
  from the flag's truthiness, so a request that explicitly set
  `skip_pre_upgrade = false` hid the row instead of showing "No". Check
  whether the optional field is present instead, and cover it in the spec.
- The target canister name shown to approvers was fetched with a plain
  query while the request itself is loaded through a verified call. Fetch
  the name with a verified call as well so a non-consensus response cannot
  attach a misleading name to the verified canister id.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019wAX3aCg6ov8rMvYeBxbgK

Copilot AI left a comment

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.

🟢 Approval recommended

The implementation matches the stated behavior and includes strong coverage for relevant modes and options.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@aterga
aterga marked this pull request as ready for review September 3, 2026 21:58
@aterga
aterga requested a review from a team as a code owner September 3, 2026 21:58
@zeropath-ai

zeropath-ai Bot commented Sep 3, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to 0adb7a2.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► apps/wallet/src/components/requests/RequestDetailView.spec.ts
    Add tests for ChangeExternalCanister behavior
► apps/wallet/src/components/requests/RequestDetailView.vue
    Register ChangeExternalCanisterOperation component and map it in componentsMap
► apps/wallet/src/components/requests/RequestListItem.vue
    Register ChangeExternalCanisterOperation in componentsMap
Enhancement ► apps/wallet/src/components/requests/operations/ChangeExternalCanisterOperation.vue
    Add new ChangeExternalCanister operation UI and logic
► apps/wallet/src/components/requests/operations/ChangeExternalCanisterOperation.spec.ts
    Add unit tests for ChangeExternalCanisterOperation

aterga commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Status after merging main into the branch (0adb7a2): every check is green except e2e-tests:required, which failed again on tests/e2e/disaster-recovery.spec.ts › can recover uninstalled station (chromium timed out on all three attempts, firefox failed once and passed on retry).

This is not caused by this PR:

There is no fix for the flaky disaster-recovery e2e test on main to port yet, and I've already used my one re-run on this PR, so I'm not re-running it again myself. A re-run of the failed job from the Actions UI is likely to pass, as it did for d52e5ce.


Generated by Claude Code

@MRmarioruci
MRmarioruci merged commit 288e0b5 into main Sep 10, 2026
42 of 44 checks passed
@MRmarioruci
MRmarioruci deleted the claude/wasm-memory-persistence-upgrades-epmpwc branch September 10, 2026 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants