From be6fdc67295c88ea0e3af466a436cf5b50488b82 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Wed, 6 Sep 2023 15:46:32 +0100 Subject: [PATCH 01/13] Add passing test to check reading and writing a value with the same size --- integration-tests/contract-storage/.gitignore | 9 +++++ integration-tests/contract-storage/Cargo.toml | 23 ++++++++++++ .../contract-storage/e2e_tests.rs | 35 +++++++++++++++++++ integration-tests/contract-storage/lib.rs | 35 +++++++++++++++++++ 4 files changed, 102 insertions(+) create mode 100755 integration-tests/contract-storage/.gitignore create mode 100755 integration-tests/contract-storage/Cargo.toml create mode 100644 integration-tests/contract-storage/e2e_tests.rs create mode 100755 integration-tests/contract-storage/lib.rs diff --git a/integration-tests/contract-storage/.gitignore b/integration-tests/contract-storage/.gitignore new file mode 100755 index 0000000000..8de8f877e4 --- /dev/null +++ b/integration-tests/contract-storage/.gitignore @@ -0,0 +1,9 @@ +# Ignore build artifacts from the local tests sub-crate. +/target/ + +# Ignore backup files creates by cargo fmt. +**/*.rs.bk + +# Remove Cargo.lock when creating an executable, leave it for libraries +# More information here http://doc.crates.io/guide.html#cargotoml-vs-cargolock +Cargo.lock diff --git a/integration-tests/contract-storage/Cargo.toml b/integration-tests/contract-storage/Cargo.toml new file mode 100755 index 0000000000..efbbd47513 --- /dev/null +++ b/integration-tests/contract-storage/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "contract-storage" +version = "4.2.0" +authors = ["Parity Technologies "] +edition = "2021" +publish = false + +[dependencies] +ink = { path = "../../crates/ink", default-features = false } + +[dev-dependencies] +ink_e2e = { path = "../../crates/e2e" } + +[lib] +path = "lib.rs" + +[features] +default = ["std"] +std = [ + "ink/std", +] +ink-as-dependency = [] +e2e-tests = [] diff --git a/integration-tests/contract-storage/e2e_tests.rs b/integration-tests/contract-storage/e2e_tests.rs new file mode 100644 index 0000000000..1f859ad772 --- /dev/null +++ b/integration-tests/contract-storage/e2e_tests.rs @@ -0,0 +1,35 @@ +use super::contract_storage::*; +use ink_e2e::ContractsBackend; + +type E2EResult = std::result::Result>; + +#[ink_e2e::test] +async fn get_contract_storage_consumes_entire_buffer( + mut client: Client, +) -> E2EResult<()> { + // given + let constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), constructor, 0, None) + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + // let get_contract_storage_consumes_entire_buffer = + // call.get_contract_storage_consumes_entire_buffer(); + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_get_storage_same_size(), + 0, + None, + ) + .await + .expect("Calling `insert_balance` failed") + .return_value(); + + assert!(result.is_ok()); + + Ok(()) +} diff --git a/integration-tests/contract-storage/lib.rs b/integration-tests/contract-storage/lib.rs new file mode 100755 index 0000000000..b38a9432c3 --- /dev/null +++ b/integration-tests/contract-storage/lib.rs @@ -0,0 +1,35 @@ +//! A smart contract to test reading and writing contract storage + +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +#[ink::contract] +mod contract_storage { + use ink::prelude::string::String; + + /// A contract for testing reading and writing contract storage. + #[ink(storage)] + #[derive(Default)] + pub struct ContractStorage; + + impl ContractStorage { + #[ink(constructor)] + pub fn new() -> Self { + Self {} + } + + /// Read from the contract storage slot, consuming all the data from the buffer. + #[ink(message)] + pub fn set_and_get_storage_same_size(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + let loaded_value = ink::env::get_contract_storage(&key) + .expect("no value stored under the key"); + assert_eq!(loaded_value, Some(value)); + Ok(()) + } + } +} + +#[cfg(all(test, feature = "e2e-tests"))] +mod e2e_tests; From 358c73668b6baabac44a7be67d27ac9ad49898f0 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Wed, 6 Sep 2023 16:02:03 +0100 Subject: [PATCH 02/13] Add FAILING test for not consuming the entire buffer --- .../contract-storage/e2e_tests.rs | 36 +++++++++++++++++-- integration-tests/contract-storage/lib.rs | 22 ++++++++++-- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/integration-tests/contract-storage/e2e_tests.rs b/integration-tests/contract-storage/e2e_tests.rs index 1f859ad772..29c025d3eb 100644 --- a/integration-tests/contract-storage/e2e_tests.rs +++ b/integration-tests/contract-storage/e2e_tests.rs @@ -16,12 +16,10 @@ async fn get_contract_storage_consumes_entire_buffer( let call = contract.call::(); // when - // let get_contract_storage_consumes_entire_buffer = - // call.get_contract_storage_consumes_entire_buffer(); let result = client .call( &ink_e2e::alice(), - &call.set_and_get_storage_same_size(), + &call.set_and_get_storage_all_data_consumed(), 0, None, ) @@ -33,3 +31,35 @@ async fn get_contract_storage_consumes_entire_buffer( Ok(()) } + +#[ink_e2e::test] +async fn get_contract_storage_fails_when_extra_data( + mut client: Client, +) -> E2EResult<()> { + // given + let constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), constructor, 0, None) + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_get_storage_partial_data_consumed(), + 0, + None, + ) + .await + .expect("Calling `insert_balance` failed") + .return_value(); + + assert!( + result.is_err(), + "Expected an error when only partially consuming the buffer" + ); + + Ok(()) +} diff --git a/integration-tests/contract-storage/lib.rs b/integration-tests/contract-storage/lib.rs index b38a9432c3..7eff5b5a2c 100755 --- a/integration-tests/contract-storage/lib.rs +++ b/integration-tests/contract-storage/lib.rs @@ -4,7 +4,10 @@ #[ink::contract] mod contract_storage { - use ink::prelude::string::String; + use ink::prelude::{ + format, + string::String, + }; /// A contract for testing reading and writing contract storage. #[ink(storage)] @@ -19,15 +22,28 @@ mod contract_storage { /// Read from the contract storage slot, consuming all the data from the buffer. #[ink(message)] - pub fn set_and_get_storage_same_size(&self) -> Result<(), String> { + pub fn set_and_get_storage_all_data_consumed(&self) -> Result<(), String> { let key = 0u32; let value = [0x42; 32]; ink::env::set_contract_storage(&key, &value); let loaded_value = ink::env::get_contract_storage(&key) - .expect("no value stored under the key"); + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; assert_eq!(loaded_value, Some(value)); Ok(()) } + + /// Read from the contract storage slot, only partially consuming data from the + /// buffer. + #[ink(message)] + pub fn set_and_get_storage_partial_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + // Only attempt to read the first byte (the `u8`) of the storage value data + let _loaded_value: Option = ink::env::get_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + Ok(()) + } } } From d4a49f7b4a6fa708297baf052a6be888333077b8 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 7 Sep 2023 12:03:50 +0100 Subject: [PATCH 03/13] Add new DecodeAll trait, and use it. --- crates/env/src/api.rs | 9 +++++--- crates/env/src/backend.rs | 9 +++++--- crates/env/src/engine/off_chain/impls.rs | 13 +++++++---- crates/env/src/engine/on_chain/impls.rs | 13 +++++++---- crates/storage/src/lazy/mod.rs | 9 +++++--- crates/storage/traits/src/lib.rs | 1 + crates/storage/traits/src/storage.rs | 23 +++++++++++++++++++ .../contract-storage/e2e_tests.rs | 6 ++--- 8 files changed, 60 insertions(+), 23 deletions(-) diff --git a/crates/env/src/api.rs b/crates/env/src/api.rs index 3050c21df7..c024bf6726 100644 --- a/crates/env/src/api.rs +++ b/crates/env/src/api.rs @@ -41,7 +41,10 @@ use crate::{ Environment, Result, }; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + Storable, + StorableDecodeAll, +}; /// Returns the address of the caller of the executed contract. /// @@ -210,7 +213,7 @@ where pub fn get_contract_storage(key: &K) -> Result> where K: scale::Encode, - R: Storable, + R: StorableDecodeAll, { ::on_instance(|instance| { EnvBackend::get_contract_storage::(instance, key) @@ -225,7 +228,7 @@ where pub fn take_contract_storage(key: &K) -> Result> where K: scale::Encode, - R: Storable, + R: StorableDecodeAll, { ::on_instance(|instance| { EnvBackend::take_contract_storage::(instance, key) diff --git a/crates/env/src/backend.rs b/crates/env/src/backend.rs index 99455b5786..6c2a960902 100644 --- a/crates/env/src/backend.rs +++ b/crates/env/src/backend.rs @@ -29,7 +29,10 @@ use crate::{ Environment, Result, }; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + Storable, + StorableDecodeAll, +}; /// The flags to indicate further information about the end of a contract execution. #[derive(Default)] @@ -186,7 +189,7 @@ pub trait EnvBackend { fn get_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: Storable; + R: StorableDecodeAll; /// Removes the `value` at `key`, returning the previous `value` at `key` from storage /// if any. @@ -197,7 +200,7 @@ pub trait EnvBackend { fn take_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: Storable; + R: StorableDecodeAll; /// Returns the size of a value stored under the given storage key is returned if any. fn contains_contract_storage(&mut self, key: &K) -> Option diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 04b792df6f..96de604d39 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -46,7 +46,10 @@ use ink_engine::{ ext, ext::Engine, }; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + Storable, + StorableDecodeAll, +}; use schnorrkel::{ PublicKey, Signature, @@ -200,7 +203,7 @@ impl EnvBackend for EnvInstance { fn get_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: Storable, + R: StorableDecodeAll, { let mut output: [u8; 9600] = [0; 9600]; match self.engine.get_storage(&key.encode(), &mut &mut output[..]) { @@ -208,14 +211,14 @@ impl EnvBackend for EnvInstance { Err(ext::Error::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; + let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; Ok(Some(decoded)) } fn take_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: Storable, + R: StorableDecodeAll, { let mut output: [u8; 9600] = [0; 9600]; match self @@ -226,7 +229,7 @@ impl EnvBackend for EnvInstance { Err(ext::Error::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; + let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; Ok(Some(decoded)) } diff --git a/crates/env/src/engine/on_chain/impls.rs b/crates/env/src/engine/on_chain/impls.rs index 06b6fec846..1fcbe5f502 100644 --- a/crates/env/src/engine/on_chain/impls.rs +++ b/crates/env/src/engine/on_chain/impls.rs @@ -48,7 +48,10 @@ use crate::{ ReturnFlags, TypedEnvBackend, }; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + Storable, + StorableDecodeAll, +}; impl CryptoHash for Blake2x128 { fn hash(input: &[u8], output: &mut ::Type) { @@ -226,7 +229,7 @@ impl EnvBackend for EnvInstance { fn get_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: Storable, + R: StorableDecodeAll, { let mut buffer = self.scoped_buffer(); let key = buffer.take_encoded(key); @@ -236,14 +239,14 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; + let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; Ok(Some(decoded)) } fn take_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: Storable, + R: StorableDecodeAll, { let mut buffer = self.scoped_buffer(); let key = buffer.take_encoded(key); @@ -253,7 +256,7 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; + let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; // todo: [AJ] test. Ok(Some(decoded)) } diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index d730f541f3..6142ca2aac 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -30,7 +30,10 @@ use crate::traits::{ }; use core::marker::PhantomData; use ink_primitives::Key; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + Storable, + StorableDecodeAll, +}; use scale::{ Error, Input, @@ -131,7 +134,7 @@ where impl Lazy where - V: Storable, + V: Storable + StorableDecodeAll, KeyType: StorageKey, { /// Reads the `value` from the contract storage, if it exists. @@ -150,7 +153,7 @@ where impl Lazy where - V: Storable + Default, + V: StorableDecodeAll + Default, KeyType: StorageKey, { /// Reads the `value` from the contract storage. diff --git a/crates/storage/traits/src/lib.rs b/crates/storage/traits/src/lib.rs index ad0ed6d697..8a7044b0e7 100644 --- a/crates/storage/traits/src/lib.rs +++ b/crates/storage/traits/src/lib.rs @@ -50,6 +50,7 @@ pub use self::{ AutoStorableHint, Packed, Storable, + StorableDecodeAll, StorableHint, StorageKey, }, diff --git a/crates/storage/traits/src/storage.rs b/crates/storage/traits/src/storage.rs index a0e3c4951d..a4fb3ff7cf 100644 --- a/crates/storage/traits/src/storage.rs +++ b/crates/storage/traits/src/storage.rs @@ -44,6 +44,29 @@ where } } +/// Extension trait to [`Storable`] that ensures that the given input data is consumed completely. +pub trait StorableDecodeAll: Sized { + /// Decode `Self` and consume all of the given input data. + /// + /// If not all data is consumed, an error is returned. + fn decode_all(input: &mut &[u8]) -> Result; +} + +impl

StorableDecodeAll for P +where + P: Storable, +{ + fn decode_all(input: &mut &[u8]) -> Result { + let res = Storable::decode(input)?; + + if input.is_empty() { + Ok(res) + } else { + Err("Input buffer has still data left after decoding!".into()) + } + } +} + pub(crate) mod private { /// Seals the implementation of `Packed`. pub trait Sealed {} diff --git a/integration-tests/contract-storage/e2e_tests.rs b/integration-tests/contract-storage/e2e_tests.rs index 29c025d3eb..5f592bd5ea 100644 --- a/integration-tests/contract-storage/e2e_tests.rs +++ b/integration-tests/contract-storage/e2e_tests.rs @@ -52,13 +52,11 @@ async fn get_contract_storage_fails_when_extra_data( 0, None, ) - .await - .expect("Calling `insert_balance` failed") - .return_value(); + .await; assert!( result.is_err(), - "Expected an error when only partially consuming the buffer" + "Expected the contract to revert when only partially consuming the buffer" ); Ok(()) From 3008310cbcb83e0d3986c3bfb1be53cc10b2a971 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 7 Sep 2023 12:11:08 +0100 Subject: [PATCH 04/13] Fmt --- crates/storage/traits/src/storage.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/storage/traits/src/storage.rs b/crates/storage/traits/src/storage.rs index a4fb3ff7cf..816b865793 100644 --- a/crates/storage/traits/src/storage.rs +++ b/crates/storage/traits/src/storage.rs @@ -44,7 +44,8 @@ where } } -/// Extension trait to [`Storable`] that ensures that the given input data is consumed completely. +/// Extension trait to [`Storable`] that ensures that the given input data is consumed +/// completely. pub trait StorableDecodeAll: Sized { /// Decode `Self` and consume all of the given input data. /// From 076677620a15221eb21437b9709286b064f240e0 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Wed, 13 Sep 2023 16:26:44 +0100 Subject: [PATCH 05/13] Use helper method instead of extension trait --- crates/env/src/api.rs | 9 +++----- crates/env/src/backend.rs | 9 +++----- crates/env/src/engine/off_chain/impls.rs | 11 +++++---- crates/env/src/engine/on_chain/impls.rs | 10 ++++---- crates/storage/src/lazy/mod.rs | 9 +++----- crates/storage/traits/src/lib.rs | 2 +- crates/storage/traits/src/storage.rs | 29 ++++++++---------------- 7 files changed, 30 insertions(+), 49 deletions(-) diff --git a/crates/env/src/api.rs b/crates/env/src/api.rs index c024bf6726..3050c21df7 100644 --- a/crates/env/src/api.rs +++ b/crates/env/src/api.rs @@ -41,10 +41,7 @@ use crate::{ Environment, Result, }; -use ink_storage_traits::{ - Storable, - StorableDecodeAll, -}; +use ink_storage_traits::Storable; /// Returns the address of the caller of the executed contract. /// @@ -213,7 +210,7 @@ where pub fn get_contract_storage(key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll, + R: Storable, { ::on_instance(|instance| { EnvBackend::get_contract_storage::(instance, key) @@ -228,7 +225,7 @@ where pub fn take_contract_storage(key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll, + R: Storable, { ::on_instance(|instance| { EnvBackend::take_contract_storage::(instance, key) diff --git a/crates/env/src/backend.rs b/crates/env/src/backend.rs index 6c2a960902..99455b5786 100644 --- a/crates/env/src/backend.rs +++ b/crates/env/src/backend.rs @@ -29,10 +29,7 @@ use crate::{ Environment, Result, }; -use ink_storage_traits::{ - Storable, - StorableDecodeAll, -}; +use ink_storage_traits::Storable; /// The flags to indicate further information about the end of a contract execution. #[derive(Default)] @@ -189,7 +186,7 @@ pub trait EnvBackend { fn get_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll; + R: Storable; /// Removes the `value` at `key`, returning the previous `value` at `key` from storage /// if any. @@ -200,7 +197,7 @@ pub trait EnvBackend { fn take_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll; + R: Storable; /// Returns the size of a value stored under the given storage key is returned if any. fn contains_contract_storage(&mut self, key: &K) -> Option diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 96de604d39..7547155e69 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -46,9 +46,10 @@ use ink_engine::{ ext, ext::Engine, }; + use ink_storage_traits::{ + decode_all, Storable, - StorableDecodeAll, }; use schnorrkel::{ PublicKey, @@ -203,7 +204,7 @@ impl EnvBackend for EnvInstance { fn get_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll, + R: Storable, { let mut output: [u8; 9600] = [0; 9600]; match self.engine.get_storage(&key.encode(), &mut &mut output[..]) { @@ -211,14 +212,14 @@ impl EnvBackend for EnvInstance { Err(ext::Error::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; + let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } fn take_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll, + R: Storable, { let mut output: [u8; 9600] = [0; 9600]; match self @@ -229,7 +230,7 @@ impl EnvBackend for EnvInstance { Err(ext::Error::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; + let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } diff --git a/crates/env/src/engine/on_chain/impls.rs b/crates/env/src/engine/on_chain/impls.rs index 1fcbe5f502..eec8c36869 100644 --- a/crates/env/src/engine/on_chain/impls.rs +++ b/crates/env/src/engine/on_chain/impls.rs @@ -49,8 +49,8 @@ use crate::{ TypedEnvBackend, }; use ink_storage_traits::{ + decode_all, Storable, - StorableDecodeAll, }; impl CryptoHash for Blake2x128 { @@ -229,7 +229,7 @@ impl EnvBackend for EnvInstance { fn get_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll, + R: Storable, { let mut buffer = self.scoped_buffer(); let key = buffer.take_encoded(key); @@ -239,14 +239,14 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; + let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } fn take_contract_storage(&mut self, key: &K) -> Result> where K: scale::Encode, - R: StorableDecodeAll, + R: Storable, { let mut buffer = self.scoped_buffer(); let key = buffer.take_encoded(key); @@ -256,7 +256,7 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = StorableDecodeAll::decode_all(&mut &output[..])?; // todo: [AJ] test. + let decoded = decode_all(&mut &output[..])?; // todo: [AJ] test. Ok(Some(decoded)) } diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index 6142ca2aac..6b523e43e2 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -30,10 +30,7 @@ use crate::traits::{ }; use core::marker::PhantomData; use ink_primitives::Key; -use ink_storage_traits::{ - Storable, - StorableDecodeAll, -}; +use ink_storage_traits::Storable; use scale::{ Error, Input, @@ -134,7 +131,7 @@ where impl Lazy where - V: Storable + StorableDecodeAll, + V: Storable, KeyType: StorageKey, { /// Reads the `value` from the contract storage, if it exists. @@ -153,7 +150,7 @@ where impl Lazy where - V: StorableDecodeAll + Default, + V: Default, KeyType: StorageKey, { /// Reads the `value` from the contract storage. diff --git a/crates/storage/traits/src/lib.rs b/crates/storage/traits/src/lib.rs index 8a7044b0e7..465676ea25 100644 --- a/crates/storage/traits/src/lib.rs +++ b/crates/storage/traits/src/lib.rs @@ -47,10 +47,10 @@ pub use self::{ ResolverKey, }, storage::{ + decode_all, AutoStorableHint, Packed, Storable, - StorableDecodeAll, StorableHint, StorageKey, }, diff --git a/crates/storage/traits/src/storage.rs b/crates/storage/traits/src/storage.rs index 816b865793..05d92373ed 100644 --- a/crates/storage/traits/src/storage.rs +++ b/crates/storage/traits/src/storage.rs @@ -44,27 +44,16 @@ where } } -/// Extension trait to [`Storable`] that ensures that the given input data is consumed -/// completely. -pub trait StorableDecodeAll: Sized { - /// Decode `Self` and consume all of the given input data. - /// - /// If not all data is consumed, an error is returned. - fn decode_all(input: &mut &[u8]) -> Result; -} - -impl

StorableDecodeAll for P -where - P: Storable, -{ - fn decode_all(input: &mut &[u8]) -> Result { - let res = Storable::decode(input)?; +/// Decode and consume all of the given input data. +/// +/// If not all data is consumed, an error is returned. +pub fn decode_all(input: &mut &[u8]) -> Result { + let res = ::decode(input)?; - if input.is_empty() { - Ok(res) - } else { - Err("Input buffer has still data left after decoding!".into()) - } + if input.is_empty() { + Ok(res) + } else { + Err("Input buffer has still data left after decoding!".into()) } } From cfccd7b43831c96a58edf12cc60749543e4eafb3 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Wed, 13 Sep 2023 16:27:30 +0100 Subject: [PATCH 06/13] Space --- crates/env/src/engine/off_chain/impls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 7547155e69..eb5aafef05 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -46,7 +46,6 @@ use ink_engine::{ ext, ext::Engine, }; - use ink_storage_traits::{ decode_all, Storable, From 00804c137a09921f068048febc162ba7d33cdc04 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Wed, 13 Sep 2023 16:28:15 +0100 Subject: [PATCH 07/13] Storable --- crates/storage/src/lazy/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index 6b523e43e2..d730f541f3 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -150,7 +150,7 @@ where impl Lazy where - V: Default, + V: Storable + Default, KeyType: StorageKey, { /// Reads the `value` from the contract storage. From 61bf94acc856b893f84eb6c79f81e4393389f1c2 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 23 Oct 2023 12:01:45 +0100 Subject: [PATCH 08/13] offchain engine return populated buffer --- crates/engine/src/ext.rs | 14 ++++--------- crates/env/src/engine/off_chain/impls.rs | 25 +++++++++++------------- 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/ext.rs b/crates/engine/src/ext.rs index 44fd531fa8..8c93856c04 100644 --- a/crates/engine/src/ext.rs +++ b/crates/engine/src/ext.rs @@ -241,32 +241,26 @@ impl Engine { } /// Returns the decoded contract storage at the key if any. - pub fn get_storage(&mut self, key: &[u8], output: &mut &mut [u8]) -> Result { + pub fn get_storage(&mut self, key: &[u8]) -> core::result::Result<&[u8], Error> { let callee = self.get_callee(); let account_id = AccountId::from_bytes(&callee[..]); self.debug_info.inc_reads(account_id); match self.database.get_from_contract_storage(&callee, key) { - Some(val) => { - set_output(output, val); - Ok(()) - } + Some(val) => Ok(val), None => Err(Error::KeyNotFound), } } /// Removes the storage entries at the given key, /// returning previously stored value at the key if any. - pub fn take_storage(&mut self, key: &[u8], output: &mut &mut [u8]) -> Result { + pub fn take_storage(&mut self, key: &[u8]) -> core::result::Result, Error> { let callee = self.get_callee(); let account_id = AccountId::from_bytes(&callee[..]); self.debug_info.inc_writes(account_id); match self.database.remove_contract_storage(&callee, key) { - Some(val) => { - set_output(output, &val); - Ok(()) - } + Some(val) => Ok(val), None => Err(Error::KeyNotFound), } } diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index cc9d949d65..9b6c33fd9c 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -205,14 +205,14 @@ impl EnvBackend for EnvInstance { K: scale::Encode, R: Storable, { - let mut output: [u8; 9600] = [0; 9600]; - match self.engine.get_storage(&key.encode(), &mut &mut output[..]) { - Ok(_) => (), - Err(ext::Error::KeyNotFound) => return Ok(None), + match self.engine.get_storage(&key.encode()) { + Ok(res) => { + let decoded = decode_all(&mut &res[..])?; + Ok(Some(decoded)) + } + Err(ext::Error::KeyNotFound) => Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = decode_all(&mut &output[..])?; - Ok(Some(decoded)) } fn take_contract_storage(&mut self, key: &K) -> Result> @@ -220,17 +220,14 @@ impl EnvBackend for EnvInstance { K: scale::Encode, R: Storable, { - let mut output: [u8; 9600] = [0; 9600]; - match self - .engine - .take_storage(&key.encode(), &mut &mut output[..]) - { - Ok(_) => (), + match self.engine.take_storage(&key.encode()) { + Ok(output) => { + let decoded = decode_all(&mut &output[..])?; + Ok(Some(decoded)) + } Err(ext::Error::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = decode_all(&mut &output[..])?; - Ok(Some(decoded)) } fn contains_contract_storage(&mut self, key: &K) -> Option From 723a335c8d6dab6099504bc826c03b1b7c157021 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 23 Oct 2023 12:23:15 +0100 Subject: [PATCH 09/13] Fix up integration test --- integration-tests/contract-storage/e2e_tests.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/integration-tests/contract-storage/e2e_tests.rs b/integration-tests/contract-storage/e2e_tests.rs index 5f592bd5ea..b6a1c6cd2a 100644 --- a/integration-tests/contract-storage/e2e_tests.rs +++ b/integration-tests/contract-storage/e2e_tests.rs @@ -8,9 +8,10 @@ async fn get_contract_storage_consumes_entire_buffer( mut client: Client, ) -> E2EResult<()> { // given - let constructor = ContractStorageRef::new(); + let mut constructor = ContractStorageRef::new(); let contract = client - .instantiate("contract-storage", &ink_e2e::alice(), constructor, 0, None) + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() .await .expect("instantiate failed"); let call = contract.call::(); @@ -20,9 +21,8 @@ async fn get_contract_storage_consumes_entire_buffer( .call( &ink_e2e::alice(), &call.set_and_get_storage_all_data_consumed(), - 0, - None, ) + .submit() .await .expect("Calling `insert_balance` failed") .return_value(); @@ -37,9 +37,10 @@ async fn get_contract_storage_fails_when_extra_data( mut client: Client, ) -> E2EResult<()> { // given - let constructor = ContractStorageRef::new(); + let mut constructor = ContractStorageRef::new(); let contract = client - .instantiate("contract-storage", &ink_e2e::alice(), constructor, 0, None) + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() .await .expect("instantiate failed"); let call = contract.call::(); @@ -49,9 +50,8 @@ async fn get_contract_storage_fails_when_extra_data( .call( &ink_e2e::alice(), &call.set_and_get_storage_partial_data_consumed(), - 0, - None, ) + .submit() .await; assert!( From cf3aa7cdea3f83ceedea5adad0751b4badd28969 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 23 Oct 2023 12:28:48 +0100 Subject: [PATCH 10/13] Test for take_storage --- crates/env/src/engine/on_chain/impls.rs | 2 +- .../contract-storage/e2e_tests.rs | 59 +++++++++++++++++++ integration-tests/contract-storage/lib.rs | 25 ++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/crates/env/src/engine/on_chain/impls.rs b/crates/env/src/engine/on_chain/impls.rs index eec8c36869..3c5df7613e 100644 --- a/crates/env/src/engine/on_chain/impls.rs +++ b/crates/env/src/engine/on_chain/impls.rs @@ -256,7 +256,7 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = decode_all(&mut &output[..])?; // todo: [AJ] test. + let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } diff --git a/integration-tests/contract-storage/e2e_tests.rs b/integration-tests/contract-storage/e2e_tests.rs index b6a1c6cd2a..d5655139bd 100644 --- a/integration-tests/contract-storage/e2e_tests.rs +++ b/integration-tests/contract-storage/e2e_tests.rs @@ -61,3 +61,62 @@ async fn get_contract_storage_fails_when_extra_data( Ok(()) } + +#[ink_e2e::test] +async fn take_contract_storage_consumes_entire_buffer( + mut client: Client, +) -> E2EResult<()> { + // given + let mut constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_take_storage_all_data_consumed(), + ) + .submit() + .await + .expect("Calling `insert_balance` failed") + .return_value(); + + assert!(result.is_ok()); + + Ok(()) +} + +#[ink_e2e::test] +async fn take_contract_storage_fails_when_extra_data( + mut client: Client, +) -> E2EResult<()> { + // given + let mut constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_take_storage_partial_data_consumed(), + ) + .submit() + .await; + + assert!( + result.is_err(), + "Expected the contract to revert when only partially consuming the buffer" + ); + + Ok(()) +} diff --git a/integration-tests/contract-storage/lib.rs b/integration-tests/contract-storage/lib.rs index 7eff5b5a2c..1404711f13 100755 --- a/integration-tests/contract-storage/lib.rs +++ b/integration-tests/contract-storage/lib.rs @@ -44,6 +44,31 @@ mod contract_storage { .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; Ok(()) } + + /// Read from the contract storage slot, consuming all the data from the buffer. + #[ink(message)] + pub fn set_and_take_storage_all_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + let loaded_value = ink::env::take_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + assert_eq!(loaded_value, Some(value)); + Ok(()) + } + + /// Read from the contract storage slot, only partially consuming data from the + /// buffer. + #[ink(message)] + pub fn set_and_take_storage_partial_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + // Only attempt to read the first byte (the `u8`) of the storage value data + let _loaded_value: Option = ink::env::take_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + Ok(()) + } } } From ece4c65a3c795ef6d76c7ea9b3f524b7705c3aea Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 23 Oct 2023 13:20:45 +0100 Subject: [PATCH 11/13] Fix tests, clippy --- crates/engine/src/test_api.rs | 5 ++-- crates/engine/src/tests.rs | 31 ++++-------------------- crates/env/src/engine/off_chain/impls.rs | 2 +- 3 files changed, 8 insertions(+), 30 deletions(-) diff --git a/crates/engine/src/test_api.rs b/crates/engine/src/test_api.rs index 6615e5aa19..2633abb8c0 100644 --- a/crates/engine/src/test_api.rs +++ b/crates/engine/src/test_api.rs @@ -334,17 +334,16 @@ mod tests { // given let mut engine = Engine::new(); let key: &[u8; 32] = &[0x42; 32]; - let mut buf = [0_u8; 32]; // when engine.set_callee(vec![1; 32]); engine.set_storage(key, &[0x05_u8; 5]); engine.set_storage(key, &[0x05_u8; 6]); - engine.get_storage(key, &mut &mut buf[..]).unwrap(); + engine.get_storage(key).unwrap(); engine.set_callee(vec![2; 32]); engine.set_storage(key, &[0x07_u8; 7]); - engine.get_storage(key, &mut &mut buf[..]).unwrap(); + engine.get_storage(key).unwrap(); // then assert_eq!(engine.count_writes(), 3); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ba0a99f668..b9359ab187 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -40,17 +40,16 @@ fn store_load_clear() { let mut engine = Engine::new(); engine.set_callee(vec![1; 32]); let key: &[u8; 32] = &[0x42; 32]; - let output = &mut &mut get_buffer()[..]; - let res = engine.get_storage(key, output); + let res = engine.get_storage(key); assert_eq!(res, Err(Error::KeyNotFound)); engine.set_storage(key, &[0x05_u8; 5]); - let res = engine.get_storage(key, output); - assert_eq!(res, Ok(()),); - assert_eq!(output[..5], [0x05; 5]); + let res = engine.get_storage(key); + assert!(res.is_ok()); + assert_eq!(res.unwrap()[..5], [0x05; 5]); engine.clear_storage(key); - let res = engine.get_storage(key, output); + let res = engine.get_storage(key); assert_eq!(res, Err(Error::KeyNotFound)); } @@ -180,26 +179,6 @@ fn value_transferred() { assert_eq!(output, value); } -#[test] -#[should_panic( - expected = "the output buffer is too small! the decoded storage is of size 16 bytes, but the output buffer has only room for 8." -)] -fn must_panic_when_buffer_too_small() { - // given - let mut engine = Engine::new(); - engine.set_callee(vec![1; 32]); - let key: &[u8; 32] = &[0x42; 32]; - engine.set_storage(key, &[0x05_u8; 16]); - - // when - let mut small_buffer = [0; 8]; - let output = &mut &mut small_buffer[..]; - let _ = engine.get_storage(key, output); - - // then - unreachable!("`get_storage` must already have panicked"); -} - #[test] fn ecdsa_recovery_test_from_contracts_pallet() { // given diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 9b6c33fd9c..f170a8ab53 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -225,7 +225,7 @@ impl EnvBackend for EnvInstance { let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } - Err(ext::Error::KeyNotFound) => return Ok(None), + Err(ext::Error::KeyNotFound) => Ok(None), Err(_) => panic!("encountered unexpected error"), } } From 82fe42019931c84b900f18c1555961416d627220 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 23 Oct 2023 14:47:47 +0100 Subject: [PATCH 12/13] Remove Result type alias --- crates/engine/src/ext.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/ext.rs b/crates/engine/src/ext.rs index 8c93856c04..387eb57127 100644 --- a/crates/engine/src/ext.rs +++ b/crates/engine/src/ext.rs @@ -34,8 +34,6 @@ use crate::{ use scale::Encode; use std::panic::panic_any; -type Result = core::result::Result<(), Error>; - macro_rules! define_error_codes { ( $( @@ -56,7 +54,7 @@ macro_rules! define_error_codes { Unknown, } - impl From for Result { + impl From for Result<(), Error> { #[inline] fn from(return_code: ReturnCode) -> Self { match return_code.0 { @@ -177,7 +175,7 @@ impl Default for Engine { impl Engine { /// Transfers value from the contract to the destination account. - pub fn transfer(&mut self, account_id: &[u8], mut value: &[u8]) -> Result { + pub fn transfer(&mut self, account_id: &[u8], mut value: &[u8]) -> Result<(), Error> { // Note that a transfer of `0` is allowed here let increment = ::decode(&mut value) .map_err(|_| Error::TransferFailed)?; @@ -240,8 +238,8 @@ impl Engine { .map(|v| ::try_from(v.len()).expect("usize to u32 conversion failed")) } - /// Returns the decoded contract storage at the key if any. - pub fn get_storage(&mut self, key: &[u8]) -> core::result::Result<&[u8], Error> { + /// Returns the contract storage bytes at the key if any. + pub fn get_storage(&mut self, key: &[u8]) -> Result<&[u8], Error> { let callee = self.get_callee(); let account_id = AccountId::from_bytes(&callee[..]); @@ -254,7 +252,7 @@ impl Engine { /// Removes the storage entries at the given key, /// returning previously stored value at the key if any. - pub fn take_storage(&mut self, key: &[u8]) -> core::result::Result, Error> { + pub fn take_storage(&mut self, key: &[u8]) -> Result, Error> { let callee = self.get_callee(); let account_id = AccountId::from_bytes(&callee[..]); @@ -419,7 +417,7 @@ impl Engine { _out_address: &mut &mut [u8], _out_return_value: &mut &mut [u8], _salt: &[u8], - ) -> Result { + ) -> Result<(), Error> { unimplemented!("off-chain environment does not yet support `instantiate`"); } @@ -430,7 +428,7 @@ impl Engine { _value: &[u8], _input: &[u8], _output: &mut &mut [u8], - ) -> Result { + ) -> Result<(), Error> { unimplemented!("off-chain environment does not yet support `call`"); } @@ -469,7 +467,7 @@ impl Engine { signature: &[u8; 65], message_hash: &[u8; 32], output: &mut [u8; 33], - ) -> Result { + ) -> Result<(), Error> { use secp256k1::{ ecdsa::{ RecoverableSignature, From 5d2080ff076a7904803620d4cdb79ffba2b3ed00 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 24 Oct 2023 18:00:46 +0100 Subject: [PATCH 13/13] CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55d330edf3..38527123b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Make `set_code_hash` generic - [#1906](https://github.com/paritytech/ink/pull/1906) - Clean E2E configuration parsing - [#1922](https://github.com/paritytech/ink/pull/1922) +### Changed +- Fail when decoding from storage and not all bytes consumed - [#1897](https://github.com/paritytech/ink/pull/1897) + ### Added - Linter: `storage_never_freed` lint - [#1932](https://github.com/paritytech/ink/pull/1932)