From 7e40231c5fed860fc809c73a2a1afe07dee779c7 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Mon, 30 Nov 2020 05:14:41 +0100 Subject: [PATCH 1/4] [storage] Implement `Drop` for `Pack` --- crates/primitives/src/key_ptr.rs | 5 + .../src/collections/binary_heap/mod.rs | 10 ++ .../src/collections/binary_heap/storage.rs | 2 + .../src/collections/binary_heap/tests.rs | 15 +- .../storage/src/collections/hashmap/tests.rs | 35 +++++ crates/storage/src/collections/stash/tests.rs | 34 ++++ crates/storage/src/pack.rs | 147 ++++++++++++++---- 7 files changed, 215 insertions(+), 33 deletions(-) diff --git a/crates/primitives/src/key_ptr.rs b/crates/primitives/src/key_ptr.rs index a607e9ba31a..8cd97d0442d 100644 --- a/crates/primitives/src/key_ptr.rs +++ b/crates/primitives/src/key_ptr.rs @@ -41,4 +41,9 @@ impl KeyPtr { self.key += old_shift; &self.key } + + /// Returns the underlying offset key. + pub fn key(&self) -> Key { + self.key + } } diff --git a/crates/storage/src/collections/binary_heap/mod.rs b/crates/storage/src/collections/binary_heap/mod.rs index 8f789bc32e7..03c32209838 100644 --- a/crates/storage/src/collections/binary_heap/mod.rs +++ b/crates/storage/src/collections/binary_heap/mod.rs @@ -33,6 +33,7 @@ use crate::{ }; pub use children_vec::Iter; +use ink_primitives::Key; pub use reverse::Reverse; /// A priority queue implemented with a binary heap. @@ -50,6 +51,14 @@ where { /// The individual elements of the heap. elements: ChildrenVec, + /// The offset key for the N cells. + /// + /// If the lazy chunk has been initialized during contract initialization + /// the key will be `None` since there won't be a storage region associated + /// to the lazy chunk which prevents it from lazily loading elements. This, + /// however, is only checked at contract runtime. We might incorporate + /// compile-time checks for this particular use case later on. + key: Option, } impl BinaryHeap @@ -60,6 +69,7 @@ where pub fn new() -> Self { Self { elements: ChildrenVec::new(), + key: None, } } diff --git a/crates/storage/src/collections/binary_heap/storage.rs b/crates/storage/src/collections/binary_heap/storage.rs index 8232f645099..0e331d0c82d 100644 --- a/crates/storage/src/collections/binary_heap/storage.rs +++ b/crates/storage/src/collections/binary_heap/storage.rs @@ -85,8 +85,10 @@ where const FOOTPRINT: u64 = as SpreadLayout>::FOOTPRINT; fn pull_spread(ptr: &mut KeyPtr) -> Self { + let key = ptr.key(); Self { elements: SpreadLayout::pull_spread(ptr), + key: Some(key), } } diff --git a/crates/storage/src/collections/binary_heap/tests.rs b/crates/storage/src/collections/binary_heap/tests.rs index 27ecc34224e..3f0977f3a1c 100644 --- a/crates/storage/src/collections/binary_heap/tests.rs +++ b/crates/storage/src/collections/binary_heap/tests.rs @@ -210,7 +210,10 @@ fn spread_layout_push_pull_works() -> ink_env::Result<()> { // both instances are equal: let heap2 = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); - assert_eq!(heap1, heap2); + // we compare only the `elements` of the heap, since for `heap1` the `heap1.key` + // is `None`, since no pull was executed on this object yet. `heap2.key` will be + // `Some(root_key)` though, since we just pulled it from storage. + assert_eq!(heap1.elements, heap2.elements); Ok(()) }) } @@ -255,6 +258,16 @@ fn drop_works() { assert!(setup_result.is_ok(), "setup should not panic"); + let contract_id = ink_env::test::get_current_contract_account_id::< + ink_env::DefaultEnvironment, + >() + .expect("Cannot get contract id"); + let used_cells = ink_env::test::count_used_storage_cells::< + ink_env::DefaultEnvironment, + >(&contract_id) + .expect("Used cells must be returned"); + assert_eq!(used_cells, 0); + let _ = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); Ok(()) diff --git a/crates/storage/src/collections/hashmap/tests.rs b/crates/storage/src/collections/hashmap/tests.rs index 9a471dfcd82..4afd2037019 100644 --- a/crates/storage/src/collections/hashmap/tests.rs +++ b/crates/storage/src/collections/hashmap/tests.rs @@ -352,3 +352,38 @@ fn storage_is_cleared_completely_after_pull_lazy() { }) .unwrap() } + +#[test] +#[should_panic(expected = "storage entry was empty")] +fn drop_works() { + ink_env::test::run_test::(|_| { + let root_key = Key::from([0x42; 32]); + + // if the setup panics it should not cause the test to pass + let setup_result = std::panic::catch_unwind(|| { + let hmap = filled_hmap(); + SpreadLayout::push_spread(&hmap, &mut KeyPtr::from(root_key)); + let _ = as SpreadLayout>::pull_spread( + &mut KeyPtr::from(root_key), + ); + // hmap is dropped which should clear the cells + }); + assert!(setup_result.is_ok(), "setup should not panic"); + + let contract_id = ink_env::test::get_current_contract_account_id::< + ink_env::DefaultEnvironment, + >() + .expect("Cannot get contract id"); + let used_cells = ink_env::test::count_used_storage_cells::< + ink_env::DefaultEnvironment, + >(&contract_id) + .expect("used cells must be returned"); + assert_eq!(used_cells, 0); + + let _ = as SpreadLayout>::pull_spread( + &mut KeyPtr::from(root_key), + ); + Ok(()) + }) + .unwrap() +} diff --git a/crates/storage/src/collections/stash/tests.rs b/crates/storage/src/collections/stash/tests.rs index 07e37f26f81..00d79e3f7b7 100644 --- a/crates/storage/src/collections/stash/tests.rs +++ b/crates/storage/src/collections/stash/tests.rs @@ -781,3 +781,37 @@ fn storage_is_cleared_completely_after_pull_lazy() { }) .unwrap() } + +#[test] +#[should_panic(expected = "storage entry was empty")] +fn drop_works() { + ink_env::test::run_test::(|_| { + let root_key = Key::from([0x42; 32]); + + // if the setup panics it should not cause the test to pass + let setup_result = std::panic::catch_unwind(|| { + let stash = create_holey_stash(); + SpreadLayout::push_spread(&stash, &mut KeyPtr::from(root_key)); + let _ = as SpreadLayout>::pull_spread(&mut KeyPtr::from( + root_key, + )); + // stash is dropped which should clear the cells + }); + assert!(setup_result.is_ok(), "setup should not panic"); + + let contract_id = ink_env::test::get_current_contract_account_id::< + ink_env::DefaultEnvironment, + >() + .expect("Cannot get contract id"); + let used_cells = ink_env::test::count_used_storage_cells::< + ink_env::DefaultEnvironment, + >(&contract_id) + .expect("used cells must be returned"); + assert_eq!(used_cells, 0); + + let _ = + as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); + Ok(()) + }) + .unwrap() +} diff --git a/crates/storage/src/pack.rs b/crates/storage/src/pack.rs index e49d2fc3565..7984362330c 100644 --- a/crates/storage/src/pack.rs +++ b/crates/storage/src/pack.rs @@ -13,6 +13,7 @@ // limitations under the License. use crate::traits::{ + clear_spread_root_opt, forward_clear_packed, forward_pull_packed, forward_push_packed, @@ -20,6 +21,7 @@ use crate::traits::{ PackedLayout, SpreadLayout, }; +use ink_prelude::vec::Vec; use ink_primitives::Key; /// Packs the inner `T` so that it only occupies a single contract storage cell. @@ -53,21 +55,66 @@ use ink_primitives::Key; /// /// As a general advice pack values together that are frequently used together. /// Also pack many very small elements (e.g. `u8`, `bool`, `u16`) together. -#[derive(Debug, Copy, Clone, scale::Encode, scale::Decode)] -pub struct Pack { +#[derive(Debug, Clone)] +pub struct Pack +where + T: PackedLayout, +{ /// The packed `T` value. inner: T, + /// The key to load the packed value from. + /// + /// # Note + /// + /// This can be `None` on contract initialization, but will be + /// initialized with a concrete value on `pull_spread`. + key: Option, } -impl Pack { - /// Creates a new packed value. - pub fn new(value: T) -> Self { - Self { inner: value } +impl scale::Encode for Pack +where + T: scale::Encode + PackedLayout, +{ + #[inline] + fn size_hint(&self) -> usize { + ::size_hint(&self.inner) + } + + #[inline] + fn encode_to(&self, dest: &mut O) { + ::encode_to(&self.inner, dest) + } + + #[inline] + fn encode(&self) -> Vec { + ::encode(&self.inner) + } + + #[inline] + fn using_encoded R>(&self, f: F) -> R { + ::using_encoded(&self.inner, f) + } +} + +impl scale::Decode for Pack +where + T: scale::Decode + PackedLayout, +{ + fn decode(input: &mut I) -> Result { + Ok(Self::new(::decode(input)?)) } +} - /// Returns the packed value. - pub fn into_inner(pack: Self) -> T { - pack.inner +impl Pack +where + T: PackedLayout, +{ + /// Creates a new packed value. + pub fn new(value: T) -> Self { + Self { + inner: value, + key: None, + } } /// Returns a shared reference to the packed value. @@ -79,6 +126,23 @@ impl Pack { pub fn as_inner_mut(pack: &mut Pack) -> &mut T { &mut pack.inner } + + /// Sets the key. + fn set_key(mut self, value: Option) -> Self { + self.key = value; + self + } +} + +impl Drop for Pack +where + T: PackedLayout, +{ + fn drop(&mut self) { + if let Some(key) = self.key { + clear_spread_root_opt::(&key, || Some(&self.inner)) + } + } } #[cfg(feature = "std")] @@ -93,7 +157,7 @@ const _: () = { impl StorageLayout for Pack where - T: TypeInfo + 'static, + T: PackedLayout + TypeInfo + 'static, { fn layout(key_ptr: &mut KeyPtr) -> Layout { Layout::Cell(CellLayout::new::(LayoutKey::from(key_ptr.advance_by(1)))) @@ -108,7 +172,7 @@ where const FOOTPRINT: u64 = 1; fn pull_spread(ptr: &mut KeyPtr) -> Self { - Pack::from(forward_pull_packed::(ptr)) + Pack::from(forward_pull_packed::(ptr)).set_key(Some(ptr.key())) } fn push_spread(&self, ptr: &mut KeyPtr) { @@ -135,7 +199,10 @@ where } } -impl From for Pack { +impl From for Pack +where + T: PackedLayout, +{ fn from(value: T) -> Self { Self::new(value) } @@ -143,14 +210,17 @@ impl From for Pack { impl Default for Pack where - T: Default, + T: Default + PackedLayout, { fn default() -> Self { Self::new(Default::default()) } } -impl core::ops::Deref for Pack { +impl core::ops::Deref for Pack +where + T: PackedLayout, +{ type Target = T; fn deref(&self) -> &Self::Target { @@ -158,7 +228,10 @@ impl core::ops::Deref for Pack { } } -impl core::ops::DerefMut for Pack { +impl core::ops::DerefMut for Pack +where + T: PackedLayout, +{ fn deref_mut(&mut self) -> &mut Self::Target { Self::as_inner_mut(self) } @@ -166,18 +239,18 @@ impl core::ops::DerefMut for Pack { impl core::cmp::PartialEq for Pack where - T: PartialEq, + T: PartialEq + PackedLayout, { fn eq(&self, other: &Self) -> bool { PartialEq::eq(Self::as_inner(self), Self::as_inner(other)) } } -impl core::cmp::Eq for Pack where T: Eq {} +impl core::cmp::Eq for Pack where T: Eq + PackedLayout {} impl core::cmp::PartialOrd for Pack where - T: PartialOrd, + T: PartialOrd + PackedLayout, { fn partial_cmp(&self, other: &Self) -> Option { PartialOrd::partial_cmp(Self::as_inner(self), Self::as_inner(other)) @@ -198,7 +271,7 @@ where impl core::cmp::Ord for Pack where - T: core::cmp::Ord, + T: core::cmp::Ord + PackedLayout, { fn cmp(&self, other: &Self) -> core::cmp::Ordering { Ord::cmp(Self::as_inner(self), Self::as_inner(other)) @@ -207,7 +280,7 @@ where impl core::fmt::Display for Pack where - T: core::fmt::Display, + T: core::fmt::Display + PackedLayout, { fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { core::fmt::Display::fmt(Self::as_inner(self), f) @@ -216,32 +289,44 @@ where impl core::hash::Hash for Pack where - T: core::hash::Hash, + T: core::hash::Hash + PackedLayout, { fn hash(&self, state: &mut H) { Self::as_inner(self).hash(state); } } -impl core::convert::AsRef for Pack { +impl core::convert::AsRef for Pack +where + T: PackedLayout, +{ fn as_ref(&self) -> &T { Self::as_inner(self) } } -impl core::convert::AsMut for Pack { +impl core::convert::AsMut for Pack +where + T: PackedLayout, +{ fn as_mut(&mut self) -> &mut T { Self::as_inner_mut(self) } } -impl ink_prelude::borrow::Borrow for Pack { +impl ink_prelude::borrow::Borrow for Pack +where + T: PackedLayout, +{ fn borrow(&self) -> &T { Self::as_inner(self) } } -impl ink_prelude::borrow::BorrowMut for Pack { +impl ink_prelude::borrow::BorrowMut for Pack +where + T: PackedLayout, +{ fn borrow_mut(&mut self) -> &mut T { Self::as_inner_mut(self) } @@ -254,6 +339,7 @@ mod tests { pull_packed_root, push_packed_root, KeyPtr, + PackedLayout, SpreadLayout, }; use core::{ @@ -295,7 +381,7 @@ mod tests { ); assert_eq!(Pack::as_inner(&pack), &expected); assert_eq!(Pack::as_inner_mut(&mut pack), &mut expected); - assert_eq!(Pack::into_inner(pack), expected); + assert_eq!(pack.inner, expected); } #[test] @@ -305,7 +391,7 @@ mod tests { assert_eq!(from, Pack::new(expected)); assert_eq!(Pack::as_inner(&from), &expected); assert_eq!(Pack::as_inner_mut(&mut from), &mut expected); - assert_eq!(Pack::into_inner(from), expected); + assert_eq!(from.inner, expected); } #[test] @@ -313,13 +399,10 @@ mod tests { use core::fmt::Debug; fn assert_default() where - T: Debug + Default + PartialEq, + T: Debug + Default + PartialEq + PackedLayout, { let pack_default = as Default>::default(); - assert_eq!( - >::into_inner(pack_default), - ::default() - ); + assert_eq!(pack_default.inner, ::default()); } assert_default::(); assert_default::(); From 50192e1468061100cb232fcad36b4ce81eec4698 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Wed, 2 Dec 2020 15:09:48 +0100 Subject: [PATCH 2/4] Implement comments --- crates/primitives/src/key_ptr.rs | 4 ++-- .../src/collections/binary_heap/mod.rs | 10 --------- .../src/collections/binary_heap/storage.rs | 2 -- .../src/collections/binary_heap/tests.rs | 5 +---- crates/storage/src/pack.rs | 21 +++++++++++-------- 5 files changed, 15 insertions(+), 27 deletions(-) diff --git a/crates/primitives/src/key_ptr.rs b/crates/primitives/src/key_ptr.rs index 8cd97d0442d..18053ff7466 100644 --- a/crates/primitives/src/key_ptr.rs +++ b/crates/primitives/src/key_ptr.rs @@ -43,7 +43,7 @@ impl KeyPtr { } /// Returns the underlying offset key. - pub fn key(&self) -> Key { - self.key + pub fn key(&self) -> &Key { + &self.key } } diff --git a/crates/storage/src/collections/binary_heap/mod.rs b/crates/storage/src/collections/binary_heap/mod.rs index 03c32209838..8f789bc32e7 100644 --- a/crates/storage/src/collections/binary_heap/mod.rs +++ b/crates/storage/src/collections/binary_heap/mod.rs @@ -33,7 +33,6 @@ use crate::{ }; pub use children_vec::Iter; -use ink_primitives::Key; pub use reverse::Reverse; /// A priority queue implemented with a binary heap. @@ -51,14 +50,6 @@ where { /// The individual elements of the heap. elements: ChildrenVec, - /// The offset key for the N cells. - /// - /// If the lazy chunk has been initialized during contract initialization - /// the key will be `None` since there won't be a storage region associated - /// to the lazy chunk which prevents it from lazily loading elements. This, - /// however, is only checked at contract runtime. We might incorporate - /// compile-time checks for this particular use case later on. - key: Option, } impl BinaryHeap @@ -69,7 +60,6 @@ where pub fn new() -> Self { Self { elements: ChildrenVec::new(), - key: None, } } diff --git a/crates/storage/src/collections/binary_heap/storage.rs b/crates/storage/src/collections/binary_heap/storage.rs index 0e331d0c82d..8232f645099 100644 --- a/crates/storage/src/collections/binary_heap/storage.rs +++ b/crates/storage/src/collections/binary_heap/storage.rs @@ -85,10 +85,8 @@ where const FOOTPRINT: u64 = as SpreadLayout>::FOOTPRINT; fn pull_spread(ptr: &mut KeyPtr) -> Self { - let key = ptr.key(); Self { elements: SpreadLayout::pull_spread(ptr), - key: Some(key), } } diff --git a/crates/storage/src/collections/binary_heap/tests.rs b/crates/storage/src/collections/binary_heap/tests.rs index 3f0977f3a1c..05d9468c3de 100644 --- a/crates/storage/src/collections/binary_heap/tests.rs +++ b/crates/storage/src/collections/binary_heap/tests.rs @@ -210,10 +210,7 @@ fn spread_layout_push_pull_works() -> ink_env::Result<()> { // both instances are equal: let heap2 = as SpreadLayout>::pull_spread(&mut KeyPtr::from(root_key)); - // we compare only the `elements` of the heap, since for `heap1` the `heap1.key` - // is `None`, since no pull was executed on this object yet. `heap2.key` will be - // `Some(root_key)` though, since we just pulled it from storage. - assert_eq!(heap1.elements, heap2.elements); + assert_eq!(heap1, heap2); Ok(()) }) } diff --git a/crates/storage/src/pack.rs b/crates/storage/src/pack.rs index 7984362330c..1b26404c3ae 100644 --- a/crates/storage/src/pack.rs +++ b/crates/storage/src/pack.rs @@ -13,7 +13,7 @@ // limitations under the License. use crate::traits::{ - clear_spread_root_opt, + clear_spread_root, forward_clear_packed, forward_pull_packed, forward_push_packed, @@ -117,6 +117,14 @@ where } } + /// Creates a new packed value. + pub fn new_with_key(value: T, key: Key) -> Self { + Self { + inner: value, + key: Some(key), + } + } + /// Returns a shared reference to the packed value. pub fn as_inner(pack: &Pack) -> &T { &pack.inner @@ -126,12 +134,6 @@ where pub fn as_inner_mut(pack: &mut Pack) -> &mut T { &mut pack.inner } - - /// Sets the key. - fn set_key(mut self, value: Option) -> Self { - self.key = value; - self - } } impl Drop for Pack @@ -140,7 +142,7 @@ where { fn drop(&mut self) { if let Some(key) = self.key { - clear_spread_root_opt::(&key, || Some(&self.inner)) + clear_spread_root::(&self.inner, &key) } } } @@ -172,7 +174,8 @@ where const FOOTPRINT: u64 = 1; fn pull_spread(ptr: &mut KeyPtr) -> Self { - Pack::from(forward_pull_packed::(ptr)).set_key(Some(ptr.key())) + let inner = forward_pull_packed::(ptr); + Pack::new_with_key(inner, *ptr.key()) } fn push_spread(&self, ptr: &mut KeyPtr) { From ad010afc117633b93f7313306ff58c9472150218 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Wed, 2 Dec 2020 18:27:43 +0100 Subject: [PATCH 3/4] Make `new_with_key` non-public --- crates/storage/src/pack.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/storage/src/pack.rs b/crates/storage/src/pack.rs index 1b26404c3ae..f9621387465 100644 --- a/crates/storage/src/pack.rs +++ b/crates/storage/src/pack.rs @@ -117,14 +117,6 @@ where } } - /// Creates a new packed value. - pub fn new_with_key(value: T, key: Key) -> Self { - Self { - inner: value, - key: Some(key), - } - } - /// Returns a shared reference to the packed value. pub fn as_inner(pack: &Pack) -> &T { &pack.inner @@ -134,6 +126,14 @@ where pub fn as_inner_mut(pack: &mut Pack) -> &mut T { &mut pack.inner } + + /// Creates a new packed value with a `key`. + fn new_with_key(value: T, key: Key) -> Self { + Self { + inner: value, + key: Some(key), + } + } } impl Drop for Pack From 7a6c18efac8edab7888834facb91d1b352bbbb2b Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Wed, 2 Dec 2020 20:11:02 +0100 Subject: [PATCH 4/4] Remove `new_with_key` --- crates/storage/src/pack.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/crates/storage/src/pack.rs b/crates/storage/src/pack.rs index f9621387465..c0c85cb3005 100644 --- a/crates/storage/src/pack.rs +++ b/crates/storage/src/pack.rs @@ -126,14 +126,6 @@ where pub fn as_inner_mut(pack: &mut Pack) -> &mut T { &mut pack.inner } - - /// Creates a new packed value with a `key`. - fn new_with_key(value: T, key: Key) -> Self { - Self { - inner: value, - key: Some(key), - } - } } impl Drop for Pack @@ -175,7 +167,10 @@ where fn pull_spread(ptr: &mut KeyPtr) -> Self { let inner = forward_pull_packed::(ptr); - Pack::new_with_key(inner, *ptr.key()) + Self { + inner, + key: Some(*ptr.key()), + } } fn push_spread(&self, ptr: &mut KeyPtr) {