From 8fcd235e38e6108abd562ee791309da87732e255 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Fri, 17 Mar 2023 23:17:21 +0100 Subject: [PATCH] contracts: Refactor `trait Ext::*_storage_transparent` functions (#13600) * Refactor _transparent methods rewrote commits, stashed the typo changes to remove some diff noise fixed my unverified email commit * remove type alias * Get rid of From impl blocks * Get rid of KeyType impl block * remove unnecessary Key export * Update frame/contracts/src/exec.rs Co-authored-by: Sasha Gryaznov * PR review comment --------- Co-authored-by: Sasha Gryaznov --- substrate/bin/node/executor/tests/basic.rs | 6 +- .../frame/contracts/src/benchmarking/mod.rs | 28 +- substrate/frame/contracts/src/exec.rs | 267 ++++++++---------- substrate/frame/contracts/src/lib.rs | 8 +- substrate/frame/contracts/src/storage.rs | 10 +- substrate/frame/contracts/src/tests.rs | 6 +- substrate/frame/contracts/src/wasm/mod.rs | 68 ++--- substrate/frame/contracts/src/wasm/runtime.rs | 103 +++---- 8 files changed, 200 insertions(+), 296 deletions(-) diff --git a/substrate/bin/node/executor/tests/basic.rs b/substrate/bin/node/executor/tests/basic.rs index ecfe02aa4159..9200fe735be4 100644 --- a/substrate/bin/node/executor/tests/basic.rs +++ b/substrate/bin/node/executor/tests/basic.rs @@ -710,11 +710,7 @@ fn deploying_wasm_contract_should_work() { t.execute_with(|| { // Verify that the contract does exist by querying some of its storage items // It does not matter that the storage item itself does not exist. - assert!(&pallet_contracts::Pallet::::get_storage( - addr, - pallet_contracts::StorageKey::::default().to_vec() - ) - .is_ok()); + assert!(&pallet_contracts::Pallet::::get_storage(addr, vec![]).is_ok()); }); } diff --git a/substrate/frame/contracts/src/benchmarking/mod.rs b/substrate/frame/contracts/src/benchmarking/mod.rs index 96368ebb3f60..2fb033cf85d8 100644 --- a/substrate/frame/contracts/src/benchmarking/mod.rs +++ b/substrate/frame/contracts/src/benchmarking/mod.rs @@ -30,7 +30,7 @@ use self::{ sandbox::Sandbox, }; use crate::{ - exec::{AccountIdOf, FixSizedKey, VarSizedKey}, + exec::{AccountIdOf, Key}, wasm::CallFlags, Pallet as Contracts, *, }; @@ -135,10 +135,10 @@ where } /// Store the supplied storage items into this contracts storage. - fn store(&self, items: &Vec<(FixSizedKey, Vec)>) -> Result<(), &'static str> { + fn store(&self, items: &Vec<([u8; 32], Vec)>) -> Result<(), &'static str> { let info = self.info()?; for item in items { - info.write(&item.0 as &FixSizedKey, Some(item.1.clone()), None, false) + info.write(&Key::Fix(item.0), Some(item.1.clone()), None, false) .map_err(|_| "Failed to write storage to restoration dest")?; } >::insert(&self.account_id, info); @@ -1044,7 +1044,7 @@ benchmarks! { let info = instance.info()?; for key in keys { info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![]), None, false, @@ -1088,7 +1088,7 @@ benchmarks! { let instance = Contract::::new(code, vec![])?; let info = instance.info()?; info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![]), None, false, @@ -1131,7 +1131,7 @@ benchmarks! { let instance = Contract::::new(code, vec![])?; let info = instance.info()?; info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![42u8; n as usize]), None, false, @@ -1180,7 +1180,7 @@ benchmarks! { let info = instance.info()?; for key in keys { info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![]), None, false, @@ -1223,7 +1223,7 @@ benchmarks! { let instance = Contract::::new(code, vec![])?; let info = instance.info()?; info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![42u8; n as usize]), None, false, @@ -1276,7 +1276,7 @@ benchmarks! { let info = instance.info()?; for key in keys { info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![]), None, false, @@ -1325,7 +1325,7 @@ benchmarks! { let instance = Contract::::new(code, vec![])?; let info = instance.info()?; info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![42u8; n as usize]), None, false, @@ -1373,7 +1373,7 @@ benchmarks! { let info = instance.info()?; for key in keys { info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![]), None, false, @@ -1416,7 +1416,7 @@ benchmarks! { let instance = Contract::::new(code, vec![])?; let info = instance.info()?; info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![42u8; n as usize]), None, false, @@ -1469,7 +1469,7 @@ benchmarks! { let info = instance.info()?; for key in keys { info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![]), None, false, @@ -1518,7 +1518,7 @@ benchmarks! { let instance = Contract::::new(code, vec![])?; let info = instance.info()?; info.write( - &VarSizedKey::::try_from(key).map_err(|e| "Key has wrong length")?, + &Key::::try_from_var(key).map_err(|e| "Key has wrong length")?, Some(vec![42u8; n as usize]), None, false, diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index 35ef43da5ed4..bd63c524c615 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/substrate/frame/contracts/src/exec.rs @@ -35,7 +35,7 @@ use smallvec::{Array, SmallVec}; use sp_core::ecdsa::Public as ECDSAPublic; use sp_io::{crypto::secp256k1_ecdsa_recover_compressed, hashing::blake2_256}; use sp_runtime::traits::{Convert, Hash}; -use sp_std::{marker::PhantomData, mem, prelude::*}; +use sp_std::{marker::PhantomData, mem, prelude::*, vec::Vec}; pub type AccountIdOf = ::AccountId; pub type MomentOf = <::Time as Time>::Moment; @@ -46,32 +46,39 @@ pub type ExecResult = Result; /// A type that represents a topic of an event. At the moment a hash is used. pub type TopicOf = ::Hash; -/// Type for fix sized storage key. -pub type FixSizedKey = [u8; 32]; - /// Type for variable sized storage key. Used for transparent hashing. -pub type VarSizedKey = BoundedVec::MaxStorageKeyLen>; - -/// Trait for hashing storage keys. -pub trait StorageKey -where - T: Config, -{ - fn hash(&self) -> Vec; +type VarSizedKey = BoundedVec::MaxStorageKeyLen>; + +/// Combined key type for both fixed and variable sized storage keys. +pub enum Key { + /// Variant for fixed sized keys. + Fix([u8; 32]), + /// Variant for variable sized keys. + Var(VarSizedKey), } -impl StorageKey for FixSizedKey { - fn hash(&self) -> Vec { - blake2_256(self.as_slice()).to_vec() +impl Key { + /// Copies self into a new vec. + pub fn to_vec(&self) -> Vec { + match self { + Key::Fix(v) => v.to_vec(), + Key::Var(v) => v.to_vec(), + } } -} -impl StorageKey for VarSizedKey -where - T: Config, -{ - fn hash(&self) -> Vec { - Blake2_128Concat::hash(self.as_slice()) + pub fn hash(&self) -> Vec { + match self { + Key::Fix(v) => blake2_256(v.as_slice()).to_vec(), + Key::Var(v) => Blake2_128Concat::hash(v.as_slice()), + } + } + + pub fn try_from_fix(v: Vec) -> Result> { + <[u8; 32]>::try_from(v).map(Self::Fix) + } + + pub fn try_from_var(v: Vec) -> Result> { + VarSizedKey::::try_from(v).map(Self::Var) } } @@ -169,44 +176,19 @@ pub trait Ext: sealing::Sealed { /// /// Returns `None` if the `key` wasn't previously set by `set_storage` or /// was deleted. - fn get_storage(&mut self, key: &FixSizedKey) -> Option>; - - /// This is a variation of `get_storage()` to be used with transparent hashing. - /// These two will be merged into a single function after some refactoring is done. - /// Returns the storage entry of the executing account by the given `key`. - /// - /// Returns `None` if the `key` wasn't previously set by `set_storage` or - /// was deleted. - fn get_storage_transparent(&mut self, key: &VarSizedKey) -> Option>; + fn get_storage(&mut self, key: &Key) -> Option>; /// Returns `Some(len)` (in bytes) if a storage item exists at `key`. /// /// Returns `None` if the `key` wasn't previously set by `set_storage` or /// was deleted. - fn get_storage_size(&mut self, key: &FixSizedKey) -> Option; - - /// This is the variation of `get_storage_size()` to be used with transparent hashing. - /// These two will be merged into a single function after some refactoring is done. - /// Returns `Some(len)` (in bytes) if a storage item exists at `key`. - /// - /// Returns `None` if the `key` wasn't previously set by `set_storage` or - /// was deleted. - fn get_storage_size_transparent(&mut self, key: &VarSizedKey) -> Option; + fn get_storage_size(&mut self, key: &Key) -> Option; /// Sets the storage entry by the given key to the specified value. If `value` is `None` then /// the storage entry is deleted. fn set_storage( &mut self, - key: &FixSizedKey, - value: Option>, - take_old: bool, - ) -> Result; - - /// This is the variation of `set_storage()` to be used with transparent hashing. - /// These two will be merged into a single function after some refactoring is done. - fn set_storage_transparent( - &mut self, - key: &VarSizedKey, + key: &Key, value: Option>, take_old: bool, ) -> Result; @@ -1236,46 +1218,23 @@ where Self::transfer(ExistenceRequirement::KeepAlive, &self.top_frame().account_id, to, value) } - fn get_storage(&mut self, key: &FixSizedKey) -> Option> { - self.top_frame_mut().contract_info().read(key) - } - - fn get_storage_transparent(&mut self, key: &VarSizedKey) -> Option> { + fn get_storage(&mut self, key: &Key) -> Option> { self.top_frame_mut().contract_info().read(key) } - fn get_storage_size(&mut self, key: &FixSizedKey) -> Option { - self.top_frame_mut().contract_info().size(key) - } - - fn get_storage_size_transparent(&mut self, key: &VarSizedKey) -> Option { - self.top_frame_mut().contract_info().size(key) + fn get_storage_size(&mut self, key: &Key) -> Option { + self.top_frame_mut().contract_info().size(key.into()) } fn set_storage( &mut self, - key: &FixSizedKey, + key: &Key, value: Option>, take_old: bool, ) -> Result { let frame = self.top_frame_mut(); frame.contract_info.get(&frame.account_id).write( - key, - value, - Some(&mut frame.nested_storage), - take_old, - ) - } - - fn set_storage_transparent( - &mut self, - key: &VarSizedKey, - value: Option>, - take_old: bool, - ) -> Result { - let frame = self.top_frame_mut(); - frame.contract_info.get(&frame.account_id).write( - key, + key.into(), value, Some(&mut frame.nested_storage), take_old, @@ -1688,7 +1647,7 @@ mod tests { place_contract(&dest, success_ch); set_balance(&origin, 100); let balance = get_balance(&dest); - let mut storage_meter = storage::meter::Meter::new(&origin, Some(0), 55).unwrap(); + let mut storage_meter = storage::meter::Meter::new(&origin, Some(0), value).unwrap(); let _ = MockStack::run_call( origin.clone(), @@ -2986,35 +2945,41 @@ mod tests { let code_hash = MockLoader::insert(Call, |ctx, _| { // Write assert_eq!( - ctx.ext.set_storage(&[1; 32], Some(vec![1, 2, 3]), false), + ctx.ext.set_storage(&Key::Fix([1; 32]), Some(vec![1, 2, 3]), false), + Ok(WriteOutcome::New) + ); + assert_eq!( + ctx.ext.set_storage(&Key::Fix([2; 32]), Some(vec![4, 5, 6]), true), + Ok(WriteOutcome::New) + ); + assert_eq!(ctx.ext.set_storage(&Key::Fix([3; 32]), None, false), Ok(WriteOutcome::New)); + assert_eq!(ctx.ext.set_storage(&Key::Fix([4; 32]), None, true), Ok(WriteOutcome::New)); + assert_eq!( + ctx.ext.set_storage(&Key::Fix([5; 32]), Some(vec![]), false), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage(&[2; 32], Some(vec![4, 5, 6]), true), + ctx.ext.set_storage(&Key::Fix([6; 32]), Some(vec![]), true), Ok(WriteOutcome::New) ); - assert_eq!(ctx.ext.set_storage(&[3; 32], None, false), Ok(WriteOutcome::New)); - assert_eq!(ctx.ext.set_storage(&[4; 32], None, true), Ok(WriteOutcome::New)); - assert_eq!(ctx.ext.set_storage(&[5; 32], Some(vec![]), false), Ok(WriteOutcome::New)); - assert_eq!(ctx.ext.set_storage(&[6; 32], Some(vec![]), true), Ok(WriteOutcome::New)); // Overwrite assert_eq!( - ctx.ext.set_storage(&[1; 32], Some(vec![42]), false), + ctx.ext.set_storage(&Key::Fix([1; 32]), Some(vec![42]), false), Ok(WriteOutcome::Overwritten(3)) ); assert_eq!( - ctx.ext.set_storage(&[2; 32], Some(vec![48]), true), + ctx.ext.set_storage(&Key::Fix([2; 32]), Some(vec![48]), true), Ok(WriteOutcome::Taken(vec![4, 5, 6])) ); - assert_eq!(ctx.ext.set_storage(&[3; 32], None, false), Ok(WriteOutcome::New)); - assert_eq!(ctx.ext.set_storage(&[4; 32], None, true), Ok(WriteOutcome::New)); + assert_eq!(ctx.ext.set_storage(&Key::Fix([3; 32]), None, false), Ok(WriteOutcome::New)); + assert_eq!(ctx.ext.set_storage(&Key::Fix([4; 32]), None, true), Ok(WriteOutcome::New)); assert_eq!( - ctx.ext.set_storage(&[5; 32], Some(vec![]), false), + ctx.ext.set_storage(&Key::Fix([5; 32]), Some(vec![]), false), Ok(WriteOutcome::Overwritten(0)) ); assert_eq!( - ctx.ext.set_storage(&[6; 32], Some(vec![]), true), + ctx.ext.set_storage(&Key::Fix([6; 32]), Some(vec![]), true), Ok(WriteOutcome::Taken(vec![])) ); @@ -3043,52 +3008,52 @@ mod tests { } #[test] - fn set_storage_transparent_works() { + fn set_storage_varsized_key_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { // Write assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([1; 64].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([1; 64].to_vec()).unwrap(), Some(vec![1, 2, 3]), false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([2; 19].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([2; 19].to_vec()).unwrap(), Some(vec![4, 5, 6]), true ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([3; 19].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([3; 19].to_vec()).unwrap(), None, false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([4; 64].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([4; 64].to_vec()).unwrap(), None, true ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([5; 30].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([5; 30].to_vec()).unwrap(), Some(vec![]), false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([6; 128].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([6; 128].to_vec()).unwrap(), Some(vec![]), true ), @@ -3097,48 +3062,48 @@ mod tests { // Overwrite assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([1; 64].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([1; 64].to_vec()).unwrap(), Some(vec![42, 43, 44]), false ), Ok(WriteOutcome::Overwritten(3)) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([2; 19].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([2; 19].to_vec()).unwrap(), Some(vec![48]), true ), Ok(WriteOutcome::Taken(vec![4, 5, 6])) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([3; 19].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([3; 19].to_vec()).unwrap(), None, false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([4; 64].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([4; 64].to_vec()).unwrap(), None, true ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([5; 30].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([5; 30].to_vec()).unwrap(), Some(vec![]), false ), Ok(WriteOutcome::Overwritten(0)) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([6; 128].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([6; 128].to_vec()).unwrap(), Some(vec![]), true ), @@ -3173,13 +3138,16 @@ mod tests { fn get_storage_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { assert_eq!( - ctx.ext.set_storage(&[1; 32], Some(vec![1, 2, 3]), false), + ctx.ext.set_storage(&Key::Fix([1; 32]), Some(vec![1, 2, 3]), false), Ok(WriteOutcome::New) ); - assert_eq!(ctx.ext.set_storage(&[2; 32], Some(vec![]), false), Ok(WriteOutcome::New)); - assert_eq!(ctx.ext.get_storage(&[1; 32]), Some(vec![1, 2, 3])); - assert_eq!(ctx.ext.get_storage(&[2; 32]), Some(vec![])); - assert_eq!(ctx.ext.get_storage(&[3; 32]), None); + assert_eq!( + ctx.ext.set_storage(&Key::Fix([2; 32]), Some(vec![]), false), + Ok(WriteOutcome::New) + ); + assert_eq!(ctx.ext.get_storage(&Key::Fix([1; 32])), Some(vec![1, 2, 3])); + assert_eq!(ctx.ext.get_storage(&Key::Fix([2; 32])), Some(vec![])); + assert_eq!(ctx.ext.get_storage(&Key::Fix([3; 32])), None); exec_success() }); @@ -3209,13 +3177,16 @@ mod tests { fn get_storage_size_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { assert_eq!( - ctx.ext.set_storage(&[1; 32], Some(vec![1, 2, 3]), false), + ctx.ext.set_storage(&Key::Fix([1; 32]), Some(vec![1, 2, 3]), false), Ok(WriteOutcome::New) ); - assert_eq!(ctx.ext.set_storage(&[2; 32], Some(vec![]), false), Ok(WriteOutcome::New)); - assert_eq!(ctx.ext.get_storage_size(&[1; 32]), Some(3)); - assert_eq!(ctx.ext.get_storage_size(&[2; 32]), Some(0)); - assert_eq!(ctx.ext.get_storage_size(&[3; 32]), None); + assert_eq!( + ctx.ext.set_storage(&Key::Fix([2; 32]), Some(vec![]), false), + Ok(WriteOutcome::New) + ); + assert_eq!(ctx.ext.get_storage_size(&Key::Fix([1; 32])), Some(3)); + assert_eq!(ctx.ext.get_storage_size(&Key::Fix([2; 32])), Some(0)); + assert_eq!(ctx.ext.get_storage_size(&Key::Fix([3; 32])), None); exec_success() }); @@ -3242,40 +3213,34 @@ mod tests { } #[test] - fn get_storage_transparent_works() { + fn get_storage_varsized_key_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([1; 19].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([1; 19].to_vec()).unwrap(), Some(vec![1, 2, 3]), false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([2; 16].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([2; 16].to_vec()).unwrap(), Some(vec![]), false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.get_storage_transparent( - &VarSizedKey::::try_from([1; 19].to_vec()).unwrap() - ), + ctx.ext.get_storage(&Key::::try_from_var([1; 19].to_vec()).unwrap()), Some(vec![1, 2, 3]) ); assert_eq!( - ctx.ext.get_storage_transparent( - &VarSizedKey::::try_from([2; 16].to_vec()).unwrap() - ), + ctx.ext.get_storage(&Key::::try_from_var([2; 16].to_vec()).unwrap()), Some(vec![]) ); assert_eq!( - ctx.ext.get_storage_transparent( - &VarSizedKey::::try_from([3; 8].to_vec()).unwrap() - ), + ctx.ext.get_storage(&Key::::try_from_var([3; 8].to_vec()).unwrap()), None ); @@ -3304,40 +3269,34 @@ mod tests { } #[test] - fn get_storage_size_transparent_works() { + fn get_storage_size_varsized_key_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([1; 19].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([1; 19].to_vec()).unwrap(), Some(vec![1, 2, 3]), false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from([2; 16].to_vec()).unwrap(), + ctx.ext.set_storage( + &Key::::try_from_var([2; 16].to_vec()).unwrap(), Some(vec![]), false ), Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.get_storage_size_transparent( - &VarSizedKey::::try_from([1; 19].to_vec()).unwrap() - ), + ctx.ext.get_storage_size(&Key::::try_from_var([1; 19].to_vec()).unwrap()), Some(3) ); assert_eq!( - ctx.ext.get_storage_size_transparent( - &VarSizedKey::::try_from([2; 16].to_vec()).unwrap() - ), + ctx.ext.get_storage_size(&Key::::try_from_var([2; 16].to_vec()).unwrap()), Some(0) ); assert_eq!( - ctx.ext.get_storage_size_transparent( - &VarSizedKey::::try_from([3; 8].to_vec()).unwrap() - ), + ctx.ext.get_storage_size(&Key::::try_from_var([3; 8].to_vec()).unwrap()), None ); diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 0aff18fd6208..7aac94d11148 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -100,7 +100,7 @@ pub mod weights; mod tests; use crate::{ - exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Stack as ExecStack}, + exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Key, Stack as ExecStack}, gas::GasMeter, storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract}, wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate}, @@ -131,7 +131,7 @@ use sp_std::{fmt::Debug, marker::PhantomData, prelude::*}; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, - exec::{Frame, VarSizedKey as StorageKey}, + exec::Frame, migration::Migration, pallet::*, schedule::{HostFnWeights, InstructionWeights, Limits, Schedule}, @@ -1265,7 +1265,9 @@ impl Pallet { ContractInfoOf::::get(&address).ok_or(ContractAccessError::DoesntExist)?; let maybe_value = contract_info.read( - &StorageKey::::try_from(key).map_err(|_| ContractAccessError::KeyDecodingFailed)?, + &Key::::try_from_var(key) + .map_err(|_| ContractAccessError::KeyDecodingFailed)? + .into(), ); Ok(maybe_value) } diff --git a/substrate/frame/contracts/src/storage.rs b/substrate/frame/contracts/src/storage.rs index 26162b55ca39..19c5f391d670 100644 --- a/substrate/frame/contracts/src/storage.rs +++ b/substrate/frame/contracts/src/storage.rs @@ -20,7 +20,7 @@ pub mod meter; use crate::{ - exec::{AccountIdOf, StorageKey}, + exec::{AccountIdOf, Key}, weights::WeightInfo, AddressGenerator, BalanceOf, CodeHash, Config, ContractInfoOf, DeletionQueue, Error, Pallet, TrieId, SENTINEL, @@ -132,7 +132,7 @@ impl ContractInfo { /// /// The read is performed from the `trie_id` only. The `address` is not necessary. If the /// contract doesn't store under the given `key` `None` is returned. - pub fn read>(&self, key: &K) -> Option> { + pub fn read(&self, key: &Key) -> Option> { child::get_raw(&self.child_trie_info(), key.hash().as_slice()) } @@ -140,7 +140,7 @@ impl ContractInfo { /// /// Returns `None` if the `key` wasn't previously set by `set_storage` or /// was deleted. - pub fn size>(&self, key: &K) -> Option { + pub fn size(&self, key: &Key) -> Option { child::len(&self.child_trie_info(), key.hash().as_slice()) } @@ -151,9 +151,9 @@ impl ContractInfo { /// /// This function also records how much storage was created or removed if a `storage_meter` /// is supplied. It should only be absent for testing or benchmarking code. - pub fn write>( + pub fn write( &self, - key: &K, + key: &Key, new_value: Option>, storage_meter: Option<&mut meter::NestedMeter>, take: bool, diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index ba9873f07efe..30a39eebfcfd 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -21,7 +21,7 @@ use crate::{ ChainExtension, Environment, Ext, InitState, RegisteredChainExtension, Result as ExtensionResult, RetVal, ReturnFlags, SysConfig, }, - exec::{FixSizedKey, Frame}, + exec::{Frame, Key}, tests::test_utils::{get_contract, get_contract_checked}, wasm::{Determinism, PrefabWasmModule, ReturnCode as RuntimeReturnCode}, weights::WeightInfo, @@ -2145,7 +2145,7 @@ fn lazy_removal_partial_remove_works() { // Put value into the contracts child trie for val in &vals { - info.write(&val.0 as &FixSizedKey, Some(val.2.clone()), None, false).unwrap(); + info.write(&Key::Fix(val.0), Some(val.2.clone()), None, false).unwrap(); } >::insert(&addr, info.clone()); @@ -2330,7 +2330,7 @@ fn lazy_removal_does_not_use_all_weight() { // Put value into the contracts child trie for val in &vals { - info.write(&val.0 as &FixSizedKey, Some(val.2.clone()), None, false).unwrap(); + info.write(&Key::Fix(val.0), Some(val.2.clone()), None, false).unwrap(); } >::insert(&addr, info.clone()); diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index f74df2c36144..cfda9c37611b 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -366,10 +366,7 @@ impl Executable for PrefabWasmModule { mod tests { use super::*; use crate::{ - exec::{ - AccountIdOf, BlockNumberOf, ErrorOrigin, ExecError, Executable, Ext, FixSizedKey, - SeedOf, VarSizedKey, - }, + exec::{AccountIdOf, BlockNumberOf, ErrorOrigin, ExecError, Executable, Ext, Key, SeedOf}, gas::GasMeter, storage::WriteOutcome, tests::{RuntimeCall, Test, ALICE, BOB}, @@ -521,40 +518,15 @@ mod tests { self.terminations.push(TerminationEntry { beneficiary: beneficiary.clone() }); Ok(()) } - fn get_storage(&mut self, key: &FixSizedKey) -> Option> { + fn get_storage(&mut self, key: &Key) -> Option> { self.storage.get(&key.to_vec()).cloned() } - fn get_storage_transparent(&mut self, key: &VarSizedKey) -> Option> { - self.storage.get(&key.to_vec()).cloned() - } - fn get_storage_size(&mut self, key: &FixSizedKey) -> Option { - self.storage.get(&key.to_vec()).map(|val| val.len() as u32) - } - fn get_storage_size_transparent(&mut self, key: &VarSizedKey) -> Option { + fn get_storage_size(&mut self, key: &Key) -> Option { self.storage.get(&key.to_vec()).map(|val| val.len() as u32) } fn set_storage( &mut self, - key: &FixSizedKey, - value: Option>, - take_old: bool, - ) -> Result { - let key = key.to_vec(); - let entry = self.storage.entry(key.clone()); - let result = match (entry, take_old) { - (Entry::Vacant(_), _) => WriteOutcome::New, - (Entry::Occupied(entry), false) => - WriteOutcome::Overwritten(entry.remove().len() as u32), - (Entry::Occupied(entry), true) => WriteOutcome::Taken(entry.remove()), - }; - if let Some(value) = value { - self.storage.insert(key, value); - } - Ok(result) - } - fn set_storage_transparent( - &mut self, - key: &VarSizedKey, + key: &Key, value: Option>, take_old: bool, ) -> Result { @@ -1071,14 +1043,14 @@ mod tests { "#; let mut ext = MockExt::default(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([1u8; 64].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([1u8; 64].to_vec()).unwrap(), Some(vec![42u8]), false, ) .unwrap(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([2u8; 19].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([2u8; 19].to_vec()).unwrap(), Some(vec![]), false, ) @@ -2547,15 +2519,15 @@ mod tests { let mut ext = MockExt::default(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([1u8; 64].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([1u8; 64].to_vec()).unwrap(), Some(vec![42u8]), false, ) .unwrap(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([2u8; 19].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([2u8; 19].to_vec()).unwrap(), Some(vec![]), false, ) @@ -2631,14 +2603,14 @@ mod tests { let mut ext = MockExt::default(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([1u8; 64].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([1u8; 64].to_vec()).unwrap(), Some(vec![42u8]), false, ) .unwrap(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([2u8; 19].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([2u8; 19].to_vec()).unwrap(), Some(vec![]), false, ) @@ -2728,15 +2700,15 @@ mod tests { let mut ext = MockExt::default(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([1u8; 64].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([1u8; 64].to_vec()).unwrap(), Some(vec![42u8]), false, ) .unwrap(); - ext.set_storage_transparent( - &VarSizedKey::::try_from([2u8; 19].to_vec()).unwrap(), + ext.set_storage( + &Key::::try_from_var([2u8; 19].to_vec()).unwrap(), Some(vec![]), false, ) diff --git a/substrate/frame/contracts/src/wasm/runtime.rs b/substrate/frame/contracts/src/wasm/runtime.rs index b30bad38bfce..a8a19e6f9515 100644 --- a/substrate/frame/contracts/src/wasm/runtime.rs +++ b/substrate/frame/contracts/src/wasm/runtime.rs @@ -18,7 +18,7 @@ //! Environment definition of the wasm smart-contract runtime. use crate::{ - exec::{ExecError, ExecResult, Ext, FixSizedKey, TopicOf, VarSizedKey}, + exec::{ExecError, ExecResult, Ext, Key, TopicOf}, gas::{ChargedAmount, Token}, schedule::HostFnWeights, BalanceOf, CodeHash, Config, DebugBufferVec, Error, SENTINEL, @@ -73,19 +73,7 @@ enum KeyType { Fix, /// Variable sized key used in transparent hashing, /// cannot be larger than MaxStorageKeyLen. - Variable(u32), -} - -impl KeyType { - fn len(&self) -> Result { - match self { - KeyType::Fix => Ok(32u32), - KeyType::Variable(len) => { - ensure!(len <= &::MaxStorageKeyLen::get(), Error::::DecodingFailed); - Ok(*len) - }, - } - } + Var(u32), } /// Every error that can be returned to a contract when it calls any of the host functions. @@ -752,6 +740,29 @@ impl<'a, E: Ext + 'a> Runtime<'a, E> { (err, _) => Self::err_into_return_code(err), } } + fn decode_key( + &self, + memory: &[u8], + key_type: KeyType, + key_ptr: u32, + ) -> Result, TrapReason> { + let res = match key_type { + KeyType::Fix => { + let key = self.read_sandbox_memory(memory, key_ptr, 32u32)?; + Key::try_from_fix(key) + }, + KeyType::Var(len) => { + ensure!( + len <= <::T as Config>::MaxStorageKeyLen::get(), + Error::::DecodingFailed + ); + let key = self.read_sandbox_memory(memory, key_ptr, len)?; + Key::try_from_var(key) + }, + }; + + res.map_err(|_| Error::::DecodingFailed.into()) + } fn set_storage( &mut self, @@ -767,20 +778,9 @@ impl<'a, E: Ext + 'a> Runtime<'a, E> { if value_len > max_size { return Err(Error::::ValueTooLarge.into()) } - let key = self.read_sandbox_memory(memory, key_ptr, key_type.len::()?)?; + let key = self.decode_key(memory, key_type, key_ptr)?; let value = Some(self.read_sandbox_memory(memory, value_ptr, value_len)?); - let write_outcome = match key_type { - KeyType::Fix => self.ext.set_storage( - &FixSizedKey::try_from(key).map_err(|_| Error::::DecodingFailed)?, - value, - false, - )?, - KeyType::Variable(_) => self.ext.set_storage_transparent( - &VarSizedKey::::try_from(key).map_err(|_| Error::::DecodingFailed)?, - value, - false, - )?, - }; + let write_outcome = self.ext.set_storage(&key, value, false)?; self.adjust_gas( charged, @@ -796,19 +796,8 @@ impl<'a, E: Ext + 'a> Runtime<'a, E> { key_ptr: u32, ) -> Result { let charged = self.charge_gas(RuntimeCosts::ClearStorage(self.ext.max_value_size()))?; - let key = self.read_sandbox_memory(memory, key_ptr, key_type.len::()?)?; - let outcome = match key_type { - KeyType::Fix => self.ext.set_storage( - &FixSizedKey::try_from(key).map_err(|_| Error::::DecodingFailed)?, - None, - false, - )?, - KeyType::Variable(_) => self.ext.set_storage_transparent( - &VarSizedKey::::try_from(key).map_err(|_| Error::::DecodingFailed)?, - None, - false, - )?, - }; + let key = self.decode_key(memory, key_type, key_ptr)?; + let outcome = self.ext.set_storage(&key, None, false)?; self.adjust_gas(charged, RuntimeCosts::ClearStorage(outcome.old_len())); Ok(outcome.old_len_with_sentinel()) @@ -823,15 +812,8 @@ impl<'a, E: Ext + 'a> Runtime<'a, E> { out_len_ptr: u32, ) -> Result { let charged = self.charge_gas(RuntimeCosts::GetStorage(self.ext.max_value_size()))?; - let key = self.read_sandbox_memory(memory, key_ptr, key_type.len::()?)?; - let outcome = match key_type { - KeyType::Fix => self.ext.get_storage( - &FixSizedKey::try_from(key).map_err(|_| Error::::DecodingFailed)?, - ), - KeyType::Variable(_) => self.ext.get_storage_transparent( - &VarSizedKey::::try_from(key).map_err(|_| Error::::DecodingFailed)?, - ), - }; + let key = self.decode_key(memory, key_type, key_ptr)?; + let outcome = self.ext.get_storage(&key); if let Some(value) = outcome { self.adjust_gas(charged, RuntimeCosts::GetStorage(value.len() as u32)); @@ -857,15 +839,8 @@ impl<'a, E: Ext + 'a> Runtime<'a, E> { key_ptr: u32, ) -> Result { let charged = self.charge_gas(RuntimeCosts::ContainsStorage(self.ext.max_value_size()))?; - let key = self.read_sandbox_memory(memory, key_ptr, key_type.len::()?)?; - let outcome = match key_type { - KeyType::Fix => self.ext.get_storage_size( - &FixSizedKey::try_from(key).map_err(|_| Error::::DecodingFailed)?, - ), - KeyType::Variable(_) => self.ext.get_storage_size_transparent( - &VarSizedKey::::try_from(key).map_err(|_| Error::::DecodingFailed)?, - ), - }; + let key = self.decode_key(memory, key_type, key_ptr)?; + let outcome = self.ext.get_storage_size(&key); self.adjust_gas(charged, RuntimeCosts::ClearStorage(outcome.unwrap_or(0))); Ok(outcome.unwrap_or(SENTINEL)) @@ -1092,7 +1067,7 @@ pub mod env { value_ptr: u32, value_len: u32, ) -> Result { - ctx.set_storage(memory, KeyType::Variable(key_len), key_ptr, value_ptr, value_len) + ctx.set_storage(memory, KeyType::Var(key_len), key_ptr, value_ptr, value_len) } /// Clear the value at the given key in the contract storage. @@ -1118,7 +1093,7 @@ pub mod env { #[version(1)] #[prefixed_alias] fn clear_storage(ctx: _, memory: _, key_ptr: u32, key_len: u32) -> Result { - ctx.clear_storage(memory, KeyType::Variable(key_len), key_ptr) + ctx.clear_storage(memory, KeyType::Var(key_len), key_ptr) } /// Retrieve the value under the given key from storage. @@ -1175,7 +1150,7 @@ pub mod env { out_ptr: u32, out_len_ptr: u32, ) -> Result { - ctx.get_storage(memory, KeyType::Variable(key_len), key_ptr, out_ptr, out_len_ptr) + ctx.get_storage(memory, KeyType::Var(key_len), key_ptr, out_ptr, out_len_ptr) } /// Checks whether there is a value stored under the given key. @@ -1212,7 +1187,7 @@ pub mod env { #[version(1)] #[prefixed_alias] fn contains_storage(ctx: _, memory: _, key_ptr: u32, key_len: u32) -> Result { - ctx.contains_storage(memory, KeyType::Variable(key_len), key_ptr) + ctx.contains_storage(memory, KeyType::Var(key_len), key_ptr) } /// Retrieve and remove the value under the given key from storage. @@ -1239,8 +1214,8 @@ pub mod env { ) -> Result { let charged = ctx.charge_gas(RuntimeCosts::TakeStorage(ctx.ext.max_value_size()))?; let key = ctx.read_sandbox_memory(memory, key_ptr, key_len)?; - if let crate::storage::WriteOutcome::Taken(value) = ctx.ext.set_storage_transparent( - &VarSizedKey::::try_from(key).map_err(|_| Error::::DecodingFailed)?, + if let crate::storage::WriteOutcome::Taken(value) = ctx.ext.set_storage( + &Key::::try_from_var(key).map_err(|_| Error::::DecodingFailed)?, None, true, )? {