Add storage migration to MemoryManager (v6 to v7) #1566
Conversation
przydatek
changed the title
src/internet_identity/src/storage.rs
Outdated
| pub fn from_memory_v6_to_v7(memory: M) -> Option<Self> { | ||
| let maybe_storage_v6 = Self::from_memory(memory.clone()); | ||
| maybe_storage_v6.as_ref()?; | ||
| let storage_v6 = maybe_storage_v6.unwrap(); |
There was a problem hiding this comment.
It would be nicer to return None on empty rather than panicking, no?
There was a problem hiding this comment.
It will not panic, due to the check on the previous line.
But I noticed now that both lines can be combined, so I did that, and hopefully it is more clear now. Also, I added a test for the case when maybe_storage_v6 is None.
| @@ -311,7 +338,9 @@ impl<M: Memory + Clone> Storage<M> { | |||
| header_memory, | |||
| anchor_memory, | |||
| maybe_memory_manager, | |||
| } | |||
| }; | |||
| storage.flush(); | |||
There was a problem hiding this comment.
Why is this flush now necessary?
There was a problem hiding this comment.
So that a new, empty storage (without any anchors) is properly initialised.
There was a problem hiding this comment.
So up to now, the storage was initialized only half-way and then flushed later. Definitely a good change. 👍
| .allocate_anchor() | ||
| .expect("Failure allocating an anchor."); | ||
| anchor | ||
| .add_device(sample_device()) |
There was a problem hiding this comment.
It would be nice to make the devices different (i.e. depend on the index) because this would detect mix-ups in the data.
| @@ -311,7 +338,9 @@ impl<M: Memory + Clone> Storage<M> { | |||
| header_memory, | |||
| anchor_memory, | |||
| maybe_memory_manager, | |||
| } | |||
| }; | |||
| storage.flush(); | |||
There was a problem hiding this comment.
So that a new, empty storage (without any anchors) is properly initialised.
src/internet_identity/src/storage.rs
Outdated
| pub fn from_memory_v6_to_v7(memory: M) -> Option<Self> { | ||
| let maybe_storage_v6 = Self::from_memory(memory.clone()); | ||
| maybe_storage_v6.as_ref()?; | ||
| let storage_v6 = maybe_storage_v6.unwrap(); |
There was a problem hiding this comment.
It will not panic, due to the check on the previous line.
But I noticed now that both lines can be combined, so I did that, and hopefully it is more clear now. Also, I added a test for the case when maybe_storage_v6 is None.
| .allocate_anchor() | ||
| .expect("Failure allocating an anchor."); | ||
| anchor | ||
| .add_device(sample_device()) |
| @@ -311,7 +338,9 @@ impl<M: Memory + Clone> Storage<M> { | |||
| header_memory, | |||
| anchor_memory, | |||
| maybe_memory_manager, | |||
| } | |||
| }; | |||
| storage.flush(); | |||
There was a problem hiding this comment.
So up to now, the storage was initialized only half-way and then flushed later. Definitely a good change. 👍
Add functionality to migrate storage from v6 to v7, where v7 uses
ic_stable_structures::MemoryManager. The migration does not copy the entire data, but rather uses 2nd page of the memory (which was unused/reserved so far) forMemoryManager's metadata. The anchor-memory starts in both versions from the 3rd page.The migration is not enabled yet, and will be controlled by a flag added in a subsequent MR.
(GIX-1423)