From 8453d9cbc62e835605eb765fff6ad7ea41b29d03 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Tue, 7 Jan 2020 14:12:57 +0100 Subject: [PATCH 1/4] translace values for prefixed storages --- frame/support/src/storage/mod.rs | 54 +++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 53e7ceb45df4f..1bfecce827f2a 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -403,6 +403,7 @@ pub trait StoragePrefixedMap { /// Storage prefix. Used for generating final key. fn storage_prefix() -> &'static [u8]; + /// Final full prefix that prefixes all keys. fn final_prefix() -> [u8; 32] { let mut final_key = [0u8; 32]; final_key[0..16].copy_from_slice(&Twox128::hash(Self::module_prefix())); @@ -410,10 +411,12 @@ pub trait StoragePrefixedMap { final_key } + /// Remove all value of the storage. fn remove_all() { sp_io::storage::clear_prefix(&Self::final_prefix()) } + /// Iter over all value of the storage. fn iter() -> PrefixIterator { let prefix = Self::final_prefix(); PrefixIterator { @@ -422,6 +425,45 @@ pub trait StoragePrefixedMap { phantom_data: Default::default(), } } + + /// Translate the values from some previous `OldValue` to the current type. + /// + /// `TV` translates values. + /// + /// Returns `Err` if the map could not be interpreted as the old type, and Ok if it could. + /// The `Err` contains the first hashed key which could not be migrated, or `None` if the + /// head of the list could not be read. + /// + /// # Warning + /// + /// This function must be used with care, before being updated the storage still contains the + /// old type, thus other calls (such as `get`) will fail at decoding it. + /// + /// # Usage + /// + /// This would typically be called inside the module implementation of on_initialize, while + /// ensuring **no usage of this storage are made before the call to `on_initialize`**. (More + /// precisely prior initialized modules doesn't make use of this storage). + fn translate_values(translate_val: TV) -> Result<(), Vec> + where OldValue: Decode, TV: Fn(OldValue) -> Value + { + let prefix = Self::final_prefix(); + let mut previous_key = prefix.to_vec(); + while let Some(next_key) = sp_io::storage::next_key(&previous_key) + .filter(|n| n.starts_with(&prefix[..])) + { + let value: OldValue = unhashed::get(&next_key) + // We failed to read the value. + // TODO TODO: should we remove all value after this ? + .ok_or_else(|| next_key.clone())?; + + unhashed::put(&next_key[..], &translate_val(value)); + + previous_key = next_key; + } + + Ok(()) + } } #[cfg(test)] @@ -463,6 +505,7 @@ mod test { let k = [twox_128(b"MyModule"), twox_128(b"MyStorage")].concat(); assert_eq!(MyStorage::final_prefix().to_vec(), k); + // test iteration assert_eq!(MyStorage::iter().collect::>(), vec![]); unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u64); @@ -472,9 +515,18 @@ mod test { assert_eq!(MyStorage::iter().collect::>(), vec![1, 2, 3, 4]); + // test removal MyStorage::remove_all(); - assert_eq!(MyStorage::iter().collect::>(), vec![]); + + // test migration + unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u32); + unhashed::put(&[&k[..], &vec![2][..]].concat(), &2u32); + + MyStorage::translate_values(|v: u32| v as u64).unwrap(); + assert_eq!(MyStorage::iter().collect::>(), vec![1, 2]); + + // test that other values are not modified. assert_eq!(unhashed::get(&key_before[..]), Some(32u64)); assert_eq!(unhashed::get(&key_after[..]), Some(33u64)); }); From 4f4152870043f85047596ed28cb41c310af5e1e6 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Tue, 7 Jan 2020 14:19:32 +0100 Subject: [PATCH 2/4] improve doc --- frame/support/src/storage/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 1bfecce827f2a..9a36773ac16f7 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -431,8 +431,7 @@ pub trait StoragePrefixedMap { /// `TV` translates values. /// /// Returns `Err` if the map could not be interpreted as the old type, and Ok if it could. - /// The `Err` contains the first hashed key which could not be migrated, or `None` if the - /// head of the list could not be read. + /// The `Err` contains the first hashed key which could not be migrated. /// /// # Warning /// @@ -453,8 +452,7 @@ pub trait StoragePrefixedMap { .filter(|n| n.starts_with(&prefix[..])) { let value: OldValue = unhashed::get(&next_key) - // We failed to read the value. - // TODO TODO: should we remove all value after this ? + // We failed to read the value. Stop the translation and return an error. .ok_or_else(|| next_key.clone())?; unhashed::put(&next_key[..], &translate_val(value)); From bca8c110d12d499596c3df333fea2f8a2e1b5e0c Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 9 Jan 2020 10:51:02 +0100 Subject: [PATCH 3/4] new impl --- frame/support/src/storage/mod.rs | 37 +++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 9a36773ac16f7..91783ae3d4b59 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -431,7 +431,8 @@ pub trait StoragePrefixedMap { /// `TV` translates values. /// /// Returns `Err` if the map could not be interpreted as the old type, and Ok if it could. - /// The `Err` contains the first hashed key which could not be migrated. + /// The `Err` contains the number of value that couldn't be interpreted, those value are + /// removed from the map. /// /// # Warning /// @@ -443,24 +444,31 @@ pub trait StoragePrefixedMap { /// This would typically be called inside the module implementation of on_initialize, while /// ensuring **no usage of this storage are made before the call to `on_initialize`**. (More /// precisely prior initialized modules doesn't make use of this storage). - fn translate_values(translate_val: TV) -> Result<(), Vec> + fn translate_values(translate_val: TV) -> Result<(), u32> where OldValue: Decode, TV: Fn(OldValue) -> Value { let prefix = Self::final_prefix(); let mut previous_key = prefix.to_vec(); + let mut errors = 0; while let Some(next_key) = sp_io::storage::next_key(&previous_key) .filter(|n| n.starts_with(&prefix[..])) { - let value: OldValue = unhashed::get(&next_key) - // We failed to read the value. Stop the translation and return an error. - .ok_or_else(|| next_key.clone())?; - - unhashed::put(&next_key[..], &translate_val(value)); + if let Some(value) = unhashed::get(&next_key) { + unhashed::put(&next_key[..], &translate_val(value)); + } else { + // We failed to read the value. Remove the key and increment errors. + unhashed::kill(&next_key[..]); + errors += 1; + } previous_key = next_key; } - Ok(()) + if errors == 0 { + Ok(()) + } else { + Err(errors) + } } } @@ -518,12 +526,21 @@ mod test { assert_eq!(MyStorage::iter().collect::>(), vec![]); // test migration - unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u32); - unhashed::put(&[&k[..], &vec![2][..]].concat(), &2u32); + unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u128); + unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u128); MyStorage::translate_values(|v: u32| v as u64).unwrap(); assert_eq!(MyStorage::iter().collect::>(), vec![1, 2]); + // test migration 2 + unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u128); + unhashed::put(&[&k[..], &vec![1, 1][..]].concat(), &3u64); + unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u128); + + assert_eq!(MyStorage::translate_values(|v: u128| v as u64), Err(1)); + assert_eq!(MyStorage::iter().collect::>(), vec![1, 2]); + + // test that other values are not modified. assert_eq!(unhashed::get(&key_before[..]), Some(32u64)); assert_eq!(unhashed::get(&key_after[..]), Some(33u64)); From 1fb6cbdae5fa2a7688a4d337e3fd699cf396f0f4 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 9 Jan 2020 13:31:22 +0100 Subject: [PATCH 4/4] update test --- frame/support/src/storage/mod.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 91783ae3d4b59..0908adae3b604 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -526,20 +526,25 @@ mod test { assert_eq!(MyStorage::iter().collect::>(), vec![]); // test migration - unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u128); - unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u128); + unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u32); + unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u32); + assert_eq!(MyStorage::iter().collect::>(), vec![]); MyStorage::translate_values(|v: u32| v as u64).unwrap(); assert_eq!(MyStorage::iter().collect::>(), vec![1, 2]); + MyStorage::remove_all(); // test migration 2 unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u128); - unhashed::put(&[&k[..], &vec![1, 1][..]].concat(), &3u64); - unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u128); - - assert_eq!(MyStorage::translate_values(|v: u128| v as u64), Err(1)); - assert_eq!(MyStorage::iter().collect::>(), vec![1, 2]); + unhashed::put(&[&k[..], &vec![1, 1][..]].concat(), &2u64); + unhashed::put(&[&k[..], &vec![8][..]].concat(), &3u128); + unhashed::put(&[&k[..], &vec![10][..]].concat(), &4u32); + // (contains some value that successfully decoded to u64) + assert_eq!(MyStorage::iter().collect::>(), vec![1, 2, 3]); + assert_eq!(MyStorage::translate_values(|v: u128| v as u64), Err(2)); + assert_eq!(MyStorage::iter().collect::>(), vec![1, 3]); + MyStorage::remove_all(); // test that other values are not modified. assert_eq!(unhashed::get(&key_before[..]), Some(32u64));