-
Notifications
You must be signed in to change notification settings - Fork 16
feat(dfx-orbit,wallet): support --wasm-memory-persistence for external canister upgrades #641
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
Open
aterga
wants to merge
7
commits into
main
Choose a base branch
from
claude/wasm-memory-persistence-orbit-7akm1u
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+870
−14
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9443400
feat(dfx-orbit,wallet): support --wasm-memory-persistence for externa…
claude 91ee8c2
style(wallet): fix prettier formatting in CanisterInstallForm spec
claude b315b72
test(dfx-orbit): satisfy clippy in install_mode unit tests
claude a833e80
test(wallet): cover skip_pre_upgrade folding and persistence select m…
claude b88cd2e
refactor(dfx-orbit,wallet): address review feedback on upgrade options
claude 595ddfd
test(dfx-orbit): prove wasm_memory_persistence=keep against an EOP mo…
claude 3cbcdd5
fix(integration-tests): decompress the module before appending the EO…
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
apps/wallet/src/components/inputs/CanisterWasmMemoryPersistenceSelect.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { VSelect } from 'vuetify/components'; | ||
| import { mount } from '~/test.utils'; | ||
| import CanisterWasmMemoryPersistenceSelect from './CanisterWasmMemoryPersistenceSelect.vue'; | ||
|
|
||
| describe('CanisterWasmMemoryPersistenceSelect', () => { | ||
| it('renders a select', () => { | ||
| const wrapper = mount(CanisterWasmMemoryPersistenceSelect, { | ||
| props: { modelValue: undefined }, | ||
| }); | ||
|
|
||
| expect(wrapper.findComponent(VSelect).exists()).toBe(true); | ||
| }); | ||
|
|
||
| it('maps the candid value to the select option', () => { | ||
| expect( | ||
| mount(CanisterWasmMemoryPersistenceSelect, { props: { modelValue: undefined } }) | ||
| .findComponent(VSelect) | ||
| .props('modelValue'), | ||
| ).toBe('default'); | ||
|
|
||
| expect( | ||
| mount(CanisterWasmMemoryPersistenceSelect, { props: { modelValue: { keep: null } } }) | ||
| .findComponent(VSelect) | ||
| .props('modelValue'), | ||
| ).toBe('keep'); | ||
|
|
||
| expect( | ||
| mount(CanisterWasmMemoryPersistenceSelect, { props: { modelValue: { replace: null } } }) | ||
| .findComponent(VSelect) | ||
| .props('modelValue'), | ||
| ).toBe('replace'); | ||
| }); | ||
|
|
||
| it('emits the candid union value when an option is selected', async () => { | ||
| const wrapper = mount(CanisterWasmMemoryPersistenceSelect, { | ||
| props: { modelValue: undefined }, | ||
| }); | ||
| const select = wrapper.findComponent(VSelect); | ||
|
|
||
| select.vm.$emit('update:modelValue', 'keep'); | ||
| await wrapper.vm.$nextTick(); | ||
| expect(wrapper.emitted('update:modelValue')?.at(-1)?.[0]).toEqual({ keep: null }); | ||
|
|
||
| select.vm.$emit('update:modelValue', 'replace'); | ||
| await wrapper.vm.$nextTick(); | ||
| expect(wrapper.emitted('update:modelValue')?.at(-1)?.[0]).toEqual({ replace: null }); | ||
| }); | ||
|
|
||
| it('emits undefined when the default option is selected', async () => { | ||
| const wrapper = mount(CanisterWasmMemoryPersistenceSelect, { | ||
| props: { modelValue: { keep: null } }, | ||
| }); | ||
|
|
||
| wrapper.findComponent(VSelect).vm.$emit('update:modelValue', 'default'); | ||
| await wrapper.vm.$nextTick(); | ||
|
|
||
| expect(wrapper.emitted('update:modelValue')?.at(-1)?.[0]).toBeUndefined(); | ||
| }); | ||
| }); |
81 changes: 81 additions & 0 deletions
81
apps/wallet/src/components/inputs/CanisterWasmMemoryPersistenceSelect.vue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| <template> | ||
| <VSelect | ||
| v-model="selected" | ||
| :label="label" | ||
| :variant="props.variant" | ||
| :density="props.density" | ||
| :readonly="props.readonly" | ||
| :items="items" | ||
| :hint="props.hint" | ||
| :persistent-hint="props.hint !== undefined" | ||
| :prepend-icon="mdiMemory" | ||
| /> | ||
| </template> | ||
| <script setup lang="ts"> | ||
| import { mdiMemory } from '@mdi/js'; | ||
| import { computed } from 'vue'; | ||
| import { useI18n } from 'vue-i18n'; | ||
| import { VSelect } from 'vuetify/components'; | ||
| import { WasmMemoryPersistence } from '~/components/external-canisters/external-canisters.types'; | ||
|
|
||
| type WasmMemoryPersistenceOption = 'default' | 'keep' | 'replace'; | ||
|
|
||
| const props = withDefaults( | ||
| defineProps<{ | ||
| modelValue?: WasmMemoryPersistence; | ||
| readonly?: boolean; | ||
| label?: string; | ||
| hint?: string; | ||
| density?: 'comfortable' | 'compact' | 'default'; | ||
| variant?: 'filled' | 'outlined' | 'plain' | 'solo' | 'underlined'; | ||
| }>(), | ||
| { | ||
| modelValue: undefined, | ||
| readonly: false, | ||
| label: undefined, | ||
| hint: undefined, | ||
| density: 'comfortable', | ||
| variant: 'filled', | ||
| }, | ||
| ); | ||
|
|
||
| const emit = defineEmits<{ | ||
| (event: 'update:modelValue', payload?: WasmMemoryPersistence): void; | ||
| }>(); | ||
|
|
||
| const i18n = useI18n(); | ||
| const label = computed( | ||
| () => props.label ?? i18n.t('external_canisters.wasm_memory_persistence.label'), | ||
| ); | ||
|
|
||
| const selected = computed<WasmMemoryPersistenceOption>({ | ||
| get: () => { | ||
| if (!props.modelValue) { | ||
| return 'default'; | ||
| } | ||
|
|
||
| return 'keep' in props.modelValue ? 'keep' : 'replace'; | ||
| }, | ||
| set: value => { | ||
| switch (value) { | ||
| case 'keep': | ||
| emit('update:modelValue', { keep: null }); | ||
| break; | ||
| case 'replace': | ||
| emit('update:modelValue', { replace: null }); | ||
| break; | ||
| case 'default': | ||
| // `default` means "let the IC decide" and maps to an absent | ||
| // wasm_memory_persistence option in the request. | ||
| emit('update:modelValue', undefined); | ||
| break; | ||
| } | ||
| }, | ||
|
aterga marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| const items = computed<{ title: string; value: WasmMemoryPersistenceOption }[]>(() => [ | ||
| { title: i18n.t('external_canisters.wasm_memory_persistence.default'), value: 'default' }, | ||
| { title: i18n.t('external_canisters.wasm_memory_persistence.keep'), value: 'keep' }, | ||
| { title: i18n.t('external_canisters.wasm_memory_persistence.replace'), value: 'replace' }, | ||
| ]); | ||
| </script> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like sweeping a problem under the rug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 absentwasm_memory_persistenceoption 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the IC decide automatically?? If so, then, it seems that this feature is not super necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No — the IC never infers
keep. What it does is fail closed: upgrading a canister whose module declares EOP withoutkeepis 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 withoutkeep, succeeds with it).Generated by Claude Code