From abbf40a742736e0774842f41028de3c8707c0466 Mon Sep 17 00:00:00 2001 From: Brian Ramirez Date: Mon, 30 Oct 2023 09:16:54 -0300 Subject: [PATCH 01/13] Add storage value size assertion --- crates/env/src/engine/off_chain/impls.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 04b792df6f..52e7cd1e37 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -194,6 +194,10 @@ impl EnvBackend for EnvInstance { { let mut v = vec![]; Storable::encode(value, &mut v); + const VALUE_SIZE_LIMIT: usize = BUFFER_SIZE - 4; + if v.len() > VALUE_SIZE_LIMIT { + panic!("Value too large to be stored in contract storage, maximum size is {} bytes", VALUE_SIZE_LIMIT); + } self.engine.set_storage(&key.encode(), &v[..]) } From 18f3f85694eb90f8a635937ea8857a56dcff4117 Mon Sep 17 00:00:00 2001 From: Brian Ramirez Date: Mon, 30 Oct 2023 09:18:23 -0300 Subject: [PATCH 02/13] Add integration test --- .../set-contract-storage/.gitignore | 9 ++ .../set-contract-storage/Cargo.toml | 27 ++++ integration-tests/set-contract-storage/lib.rs | 126 ++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 integration-tests/set-contract-storage/.gitignore create mode 100644 integration-tests/set-contract-storage/Cargo.toml create mode 100644 integration-tests/set-contract-storage/lib.rs diff --git a/integration-tests/set-contract-storage/.gitignore b/integration-tests/set-contract-storage/.gitignore new file mode 100644 index 0000000000..bf910de10a --- /dev/null +++ b/integration-tests/set-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 \ No newline at end of file diff --git a/integration-tests/set-contract-storage/Cargo.toml b/integration-tests/set-contract-storage/Cargo.toml new file mode 100644 index 0000000000..d5d9f3e0c4 --- /dev/null +++ b/integration-tests/set-contract-storage/Cargo.toml @@ -0,0 +1,27 @@ +[package] +name = "set-contract-storage" +version = "4.1.0" +authors = ["Parity Technologies "] +edition = "2021" +publish = false + +[dependencies] +ink = { path = "../../crates/ink", default-features = false } +scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] } +scale-info = { version = "2.5", default-features = false, features = ["derive"], optional = true } + +[dev-dependencies] +ink_e2e = { path = "../../crates/e2e" } + +[lib] +path = "lib.rs" + +[features] +default = ["std"] +std = [ + "ink/std", + "scale/std", + "scale-info/std" +] +ink-as-dependency = [] +e2e-tests = [] \ No newline at end of file diff --git a/integration-tests/set-contract-storage/lib.rs b/integration-tests/set-contract-storage/lib.rs new file mode 100644 index 0000000000..e022877eea --- /dev/null +++ b/integration-tests/set-contract-storage/lib.rs @@ -0,0 +1,126 @@ +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +#[ink::contract] +mod set_contract_storage { + use ink::env::set_contract_storage; + + const SIZE_LIMIT: usize = (1 << 14) - 4; + + #[ink(storage)] + pub struct SetContractStorage {} + + impl SetContractStorage { + /// Creates a new SetContractStorage contract. + #[ink(constructor)] + pub fn new() -> Self { + Self {} + } + + /// Stores an array that is JUST big enough to be validly allocated. + #[ink(message)] + pub fn set_storage_big(&self) { + set_contract_storage(&42, &[42u8; SIZE_LIMIT]); + } + + /// Tries to store the smallest array that is too big to be validly + /// allocated. This function should always fail. + #[ink(message)] + pub fn set_storage_very_big(&self) { + set_contract_storage(&42, &[42u8; SIZE_LIMIT + 1]); + } + } + + impl Default for SetContractStorage { + fn default() -> Self { + Self::new() + } + } + + #[cfg(test)] + mod tests { + use super::*; + + #[ink::test] + fn contract_storage_big() { + let contract = SetContractStorage::new(); + + contract.set_storage_big(); + + assert_eq!(0, 0); + } + + #[ink::test] + #[should_panic( + expected = "Value too large to be stored in contract storage, maximum size is 16380 bytes" + )] + fn contract_storage_too_big() { + let contract = SetContractStorage::new(); + + contract.set_storage_very_big(); + } + } + + #[cfg(all(test, feature = "e2e-tests"))] + mod e2e_tests { + use ink_e2e::build_message; + + use super::*; + + type E2EResult = std::result::Result>; + + #[ink_e2e::test] + async fn contract_storage_big( + mut client: ink_e2e::Client, + ) -> E2EResult<()> { + let constructor = SetContractStorageRef::new(); + + let contract_acc_id = client + .instantiate( + "set-contract-storage", + &ink_e2e::alice(), + constructor, + 0, + None, + ) + .await + .expect("instantiate failed") + .account_id; + + let message = build_message::(contract_acc_id.clone()) + .call(|contract| contract.set_storage_big()); + + client + .call(&ink_e2e::alice(), message, 0, None) + .await + .expect("set_storage_big failed"); + + Ok(()) + } + + #[ink_e2e::test] + #[should_panic] + async fn contract_storage_too_big(mut client: ink_e2e::Client) { + let constructor = SetContractStorageRef::new(); + + let contract_acc_id = client + .instantiate( + "set-contract-storage", + &ink_e2e::bob(), + constructor, + 0, + None, + ) + .await + .expect("instantiate failed") + .account_id; + + let message = build_message::(contract_acc_id.clone()) + .call(|contract| contract.set_storage_very_big()); + + client + .call(&ink_e2e::bob(), message, 0, None) + .await + .expect("set_storage_very_big failed"); + } + } +} \ No newline at end of file From cf463e58b048a85f09a76ac367bd8735f9aa1fa4 Mon Sep 17 00:00:00 2001 From: Brian Ramirez Date: Mon, 30 Oct 2023 09:38:22 -0300 Subject: [PATCH 03/13] Add changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0aac63b02b..cc78ec7332 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## Changed +### Changed - Make `set_code_hash` generic - [#1906](https://github.com/paritytech/ink/pull/1906) +- Add storage value size assertion in `set_contract_storage` - [#1961](https://github.com/paritytech/ink/pull/1961) ## Version 5.0.0-alpha From a5d0b5fe2d632ba739e788097c37e2a336294a2f Mon Sep 17 00:00:00 2001 From: Brian Ramirez Date: Mon, 30 Oct 2023 17:32:27 -0300 Subject: [PATCH 04/13] Fix integration test --- .../set-contract-storage/Cargo.toml | 2 +- integration-tests/set-contract-storage/lib.rs | 32 +++++++++++-------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/integration-tests/set-contract-storage/Cargo.toml b/integration-tests/set-contract-storage/Cargo.toml index d5d9f3e0c4..ee7a9d906b 100644 --- a/integration-tests/set-contract-storage/Cargo.toml +++ b/integration-tests/set-contract-storage/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "set-contract-storage" -version = "4.1.0" +version = "4.3.0" authors = ["Parity Technologies "] edition = "2021" publish = false diff --git a/integration-tests/set-contract-storage/lib.rs b/integration-tests/set-contract-storage/lib.rs index e022877eea..360d445066 100644 --- a/integration-tests/set-contract-storage/lib.rs +++ b/integration-tests/set-contract-storage/lib.rs @@ -62,7 +62,7 @@ mod set_contract_storage { #[cfg(all(test, feature = "e2e-tests"))] mod e2e_tests { - use ink_e2e::build_message; + use ink_e2e::ContractsBackend; use super::*; @@ -72,9 +72,10 @@ mod set_contract_storage { async fn contract_storage_big( mut client: ink_e2e::Client, ) -> E2EResult<()> { + // given let constructor = SetContractStorageRef::new(); - let contract_acc_id = client + let contract = client .instantiate( "set-contract-storage", &ink_e2e::alice(), @@ -83,14 +84,15 @@ mod set_contract_storage { None, ) .await - .expect("instantiate failed") - .account_id; + .expect("instantiate failed"); + let contract_acc_id = contract.account_id; + let mut call = contract.call::(); - let message = build_message::(contract_acc_id.clone()) - .call(|contract| contract.set_storage_big()); + // when + let set_storage_big_call = call.set_storage_big(); client - .call(&ink_e2e::alice(), message, 0, None) + .call(&ink_e2e::alice(), &set_storage_big_call, 0, None) .await .expect("set_storage_big failed"); @@ -100,9 +102,10 @@ mod set_contract_storage { #[ink_e2e::test] #[should_panic] async fn contract_storage_too_big(mut client: ink_e2e::Client) { + // given let constructor = SetContractStorageRef::new(); - let contract_acc_id = client + let contract = client .instantiate( "set-contract-storage", &ink_e2e::bob(), @@ -111,16 +114,17 @@ mod set_contract_storage { None, ) .await - .expect("instantiate failed") - .account_id; + .expect("instantiate failed"); + let contract_acc_id = contract.account_id; + let mut call = contract.call::(); - let message = build_message::(contract_acc_id.clone()) - .call(|contract| contract.set_storage_very_big()); + // when + let set_storage_very_big_call = call.set_storage_very_big(); client - .call(&ink_e2e::bob(), message, 0, None) + .call(&ink_e2e::bob(), &set_storage_very_big_call, 0, None) .await .expect("set_storage_very_big failed"); } } -} \ No newline at end of file +} From a74dae6802bf8d59adc7f3b1c8b78678df4e35be Mon Sep 17 00:00:00 2001 From: Brian Ramirez Date: Tue, 31 Oct 2023 10:08:27 -0300 Subject: [PATCH 05/13] Fix integration test --- integration-tests/set-contract-storage/lib.rs | 56 +++++++++---------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/integration-tests/set-contract-storage/lib.rs b/integration-tests/set-contract-storage/lib.rs index 360d445066..0352bbd0e8 100644 --- a/integration-tests/set-contract-storage/lib.rs +++ b/integration-tests/set-contract-storage/lib.rs @@ -73,58 +73,54 @@ mod set_contract_storage { mut client: ink_e2e::Client, ) -> E2EResult<()> { // given - let constructor = SetContractStorageRef::new(); + let mut constructor = SetContractStorageRef::new(); let contract = client - .instantiate( - "set-contract-storage", - &ink_e2e::alice(), - constructor, - 0, - None, - ) + .instantiate("set-contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() .await .expect("instantiate failed"); - let contract_acc_id = contract.account_id; - let mut call = contract.call::(); + let call = contract.call::(); // when let set_storage_big_call = call.set_storage_big(); - client - .call(&ink_e2e::alice(), &set_storage_big_call, 0, None) - .await - .expect("set_storage_big failed"); + let result = client + .call(&ink_e2e::alice(), &set_storage_big_call) + .submit() + .await; + + // then + assert!(result.is_ok(), "set_storage_big success"); Ok(()) } #[ink_e2e::test] - #[should_panic] - async fn contract_storage_too_big(mut client: ink_e2e::Client) { + async fn contract_storage_too_big( + mut client: Client, + ) -> E2EResult<()> { // given - let constructor = SetContractStorageRef::new(); + let mut constructor = SetContractStorageRef::new(); let contract = client - .instantiate( - "set-contract-storage", - &ink_e2e::bob(), - constructor, - 0, - None, - ) + .instantiate("set-contract-storage", &ink_e2e::bob(), &mut constructor) + .submit() .await .expect("instantiate failed"); - let contract_acc_id = contract.account_id; - let mut call = contract.call::(); + let call = contract.call::(); // when let set_storage_very_big_call = call.set_storage_very_big(); - client - .call(&ink_e2e::bob(), &set_storage_very_big_call, 0, None) - .await - .expect("set_storage_very_big failed"); + let result = client + .call(&ink_e2e::bob(), &set_storage_very_big_call) + .submit() + .await; + + assert!(result.is_err(), "set_storage_very_big failed"); + + Ok(()) } } } From 27e671b31e6b2579a41aea9330400ea72389d3c9 Mon Sep 17 00:00:00 2001 From: Brian Ramirez Date: Fri, 3 Nov 2023 12:02:25 -0300 Subject: [PATCH 06/13] Updated unit tests to corrected set_contract_storage semantics --- crates/storage/src/lazy/mapping.rs | 12 ++---------- crates/storage/src/lazy/mod.rs | 4 +--- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/crates/storage/src/lazy/mapping.rs b/crates/storage/src/lazy/mapping.rs index 22fd15bed8..eea03731b8 100644 --- a/crates/storage/src/lazy/mapping.rs +++ b/crates/storage/src/lazy/mapping.rs @@ -488,6 +488,7 @@ mod tests { let key = 0; let value = [0u8; ink_env::BUFFER_SIZE - 1]; + assert_eq!(mapping.try_insert(key, &value), Ok(None)); assert_eq!(mapping.try_get(key), Some(Ok(value))); assert_eq!(mapping.try_take(key), Some(Ok(value))); @@ -499,6 +500,7 @@ mod tests { } #[test] + #[should_panic] fn fallible_storage_fails_gracefully_for_overgrown_data() { ink_env::test::run_test::(|_| { let mut mapping: Mapping = Mapping::new(); @@ -512,17 +514,7 @@ mod tests { Err(ink_env::Error::BufferTooSmall) ); - // The off-chain impl conveniently uses a Vec for encoding, - // allowing writing values exceeding the static buffer size. ink_env::set_contract_storage(&(&mapping.key(), key), &value); - assert_eq!( - mapping.try_get(key), - Some(Err(ink_env::Error::BufferTooSmall)) - ); - assert_eq!( - mapping.try_take(key), - Some(Err(ink_env::Error::BufferTooSmall)) - ); Ok(()) }) diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index 058c975c0e..b6fbdf2e82 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -329,6 +329,7 @@ mod tests { } #[test] + #[should_panic] fn fallible_storage_fails_gracefully_for_overgrown_data() { ink_env::test::run_test::(|_| { let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE + 1]> = Lazy::new(); @@ -337,10 +338,7 @@ mod tests { assert_eq!(storage.try_get(), None); assert_eq!(storage.try_set(&value), Err(ink_env::Error::BufferTooSmall)); - // The off-chain impl conveniently uses a Vec for encoding, - // allowing writing values exceeding the static buffer size. ink_env::set_contract_storage(&storage.key(), &value); - assert_eq!(storage.try_get(), Some(Err(ink_env::Error::BufferTooSmall))); Ok(()) }) From 18d242c8561d5bad1b0b9a0866978491c559a6f5 Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Mon, 6 Nov 2023 10:40:10 -0300 Subject: [PATCH 07/13] Modified condition in set_contract_storage Changed the limit to encoded_value.len() + encoded_key.len() > 2**14 --- crates/env/src/engine/off_chain/impls.rs | 62 ++++++------------------ 1 file changed, 16 insertions(+), 46 deletions(-) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index d21a615376..95c1f029f0 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -15,45 +15,16 @@ use super::EnvInstance; use crate::{ call::{ - Call, - CallParams, - ConstructorReturnType, - CreateParams, - DelegateCall, + Call, CallParams, ConstructorReturnType, CreateParams, DelegateCall, FromAccountId, }, - event::{ - Event, - TopicsBuilderBackend, - }, - hash::{ - Blake2x128, - Blake2x256, - CryptoHash, - HashOutput, - Keccak256, - Sha2x256, - }, - Clear, - EnvBackend, - Environment, - Error, - Result, - ReturnFlags, - TypedEnvBackend, -}; -use ink_engine::{ - ext, - ext::Engine, -}; -use ink_storage_traits::{ - decode_all, - Storable, -}; -use schnorrkel::{ - PublicKey, - Signature, + event::{Event, TopicsBuilderBackend}, + hash::{Blake2x128, Blake2x256, CryptoHash, HashOutput, Keccak256, Sha2x256}, + Clear, EnvBackend, Environment, Error, Result, ReturnFlags, TypedEnvBackend, }; +use ink_engine::{ext, ext::Engine}; +use ink_storage_traits::{decode_all, Storable}; +use schnorrkel::{PublicKey, Signature}; /// The capacity of the static buffer. /// This is the same size as the ink! on-chain environment. We chose to use the same size @@ -196,12 +167,15 @@ impl EnvBackend for EnvInstance { V: Storable, { let mut v = vec![]; + Storable::encode(value, &mut v); - const VALUE_SIZE_LIMIT: usize = BUFFER_SIZE - 4; - if v.len() > VALUE_SIZE_LIMIT { - panic!("Value too large to be stored in contract storage, maximum size is {} bytes", VALUE_SIZE_LIMIT); + let encoded_key: Vec = key.encode(); + let encoded_value: &[u8] = &v[..]; + + if encoded_value.len() + encoded_key.len() > BUFFER_SIZE { + panic!("Value too large to be stored in contract storage, maximum size is {} bytes", BUFFER_SIZE); } - self.engine.set_storage(&key.encode(), &v[..]) + self.engine.set_storage(&encoded_key, encoded_value) } fn get_contract_storage(&mut self, key: &K) -> Result> @@ -289,12 +263,8 @@ impl EnvBackend for EnvInstance { output: &mut [u8; 33], ) -> Result<()> { use secp256k1::{ - ecdsa::{ - RecoverableSignature, - RecoveryId, - }, - Message, - SECP256K1, + ecdsa::{RecoverableSignature, RecoveryId}, + Message, SECP256K1, }; // In most implementations, the v is just 0 or 1 internally, but 27 was added From c585f0d2512a0e59e45ec9a9e18d6930796af3d2 Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Mon, 6 Nov 2023 14:49:50 -0300 Subject: [PATCH 08/13] Added -4 in message and check, updated tests to reflect the change --- crates/env/src/engine/off_chain/impls.rs | 4 ++-- integration-tests/set-contract-storage/lib.rs | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 95c1f029f0..7dc8d84470 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -172,8 +172,8 @@ impl EnvBackend for EnvInstance { let encoded_key: Vec = key.encode(); let encoded_value: &[u8] = &v[..]; - if encoded_value.len() + encoded_key.len() > BUFFER_SIZE { - panic!("Value too large to be stored in contract storage, maximum size is {} bytes", BUFFER_SIZE); + if encoded_value.len() + encoded_key.len() > BUFFER_SIZE - 4 { + panic!("Value too large to be stored in contract storage, maximum size is {} bytes", (BUFFER_SIZE - 4)); } self.engine.set_storage(&encoded_key, encoded_value) } diff --git a/integration-tests/set-contract-storage/lib.rs b/integration-tests/set-contract-storage/lib.rs index 0352bbd0e8..9a59aa109b 100644 --- a/integration-tests/set-contract-storage/lib.rs +++ b/integration-tests/set-contract-storage/lib.rs @@ -19,6 +19,7 @@ mod set_contract_storage { /// Stores an array that is JUST big enough to be validly allocated. #[ink(message)] pub fn set_storage_big(&self) { + println!("{}", SIZE_LIMIT.to_string()); set_contract_storage(&42, &[42u8; SIZE_LIMIT]); } @@ -26,6 +27,7 @@ mod set_contract_storage { /// allocated. This function should always fail. #[ink(message)] pub fn set_storage_very_big(&self) { + println!("{}", SIZE_LIMIT.to_string()); set_contract_storage(&42, &[42u8; SIZE_LIMIT + 1]); } } From c2df2d15c5d1554da6812e6e4f6d03bca32afb1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 15 Nov 2023 14:51:39 -0300 Subject: [PATCH 09/13] Passed through cargofmt. --- crates/env/src/engine/off_chain/impls.rs | 51 +++++++++++++++---- .../src/generator/trait_def/trait_registry.rs | 4 +- crates/ink/ir/src/ir/attrs.rs | 4 +- crates/storage/src/lazy/mapping.rs | 1 - 4 files changed, 48 insertions(+), 12 deletions(-) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 7dc8d84470..aabfa69fd6 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -15,16 +15,45 @@ use super::EnvInstance; use crate::{ call::{ - Call, CallParams, ConstructorReturnType, CreateParams, DelegateCall, + Call, + CallParams, + ConstructorReturnType, + CreateParams, + DelegateCall, FromAccountId, }, - event::{Event, TopicsBuilderBackend}, - hash::{Blake2x128, Blake2x256, CryptoHash, HashOutput, Keccak256, Sha2x256}, - Clear, EnvBackend, Environment, Error, Result, ReturnFlags, TypedEnvBackend, + event::{ + Event, + TopicsBuilderBackend, + }, + hash::{ + Blake2x128, + Blake2x256, + CryptoHash, + HashOutput, + Keccak256, + Sha2x256, + }, + Clear, + EnvBackend, + Environment, + Error, + Result, + ReturnFlags, + TypedEnvBackend, +}; +use ink_engine::{ + ext, + ext::Engine, +}; +use ink_storage_traits::{ + decode_all, + Storable, +}; +use schnorrkel::{ + PublicKey, + Signature, }; -use ink_engine::{ext, ext::Engine}; -use ink_storage_traits::{decode_all, Storable}; -use schnorrkel::{PublicKey, Signature}; /// The capacity of the static buffer. /// This is the same size as the ink! on-chain environment. We chose to use the same size @@ -263,8 +292,12 @@ impl EnvBackend for EnvInstance { output: &mut [u8; 33], ) -> Result<()> { use secp256k1::{ - ecdsa::{RecoverableSignature, RecoveryId}, - Message, SECP256K1, + ecdsa::{ + RecoverableSignature, + RecoveryId, + }, + Message, + SECP256K1, }; // In most implementations, the v is just 0 or 1 internally, but 27 was added diff --git a/crates/ink/codegen/src/generator/trait_def/trait_registry.rs b/crates/ink/codegen/src/generator/trait_def/trait_registry.rs index 6d6471e53e..b0fe1a25bd 100644 --- a/crates/ink/codegen/src/generator/trait_def/trait_registry.rs +++ b/crates/ink/codegen/src/generator/trait_def/trait_registry.rs @@ -21,7 +21,9 @@ use super::TraitDefinition; use crate::{ - generator::{self,}, + generator::{ + self, + }, traits::GenerateCode, EnforcedErrors, }; diff --git a/crates/ink/ir/src/ir/attrs.rs b/crates/ink/ir/src/ir/attrs.rs index 4cd8320f43..100efa0654 100644 --- a/crates/ink/ir/src/ir/attrs.rs +++ b/crates/ink/ir/src/ir/attrs.rs @@ -32,7 +32,9 @@ use syn::{ }; use crate::{ - ast::{self,}, + ast::{ + self, + }, error::ExtError as _, ir, ir::{ diff --git a/crates/storage/src/lazy/mapping.rs b/crates/storage/src/lazy/mapping.rs index eea03731b8..f8314c7f57 100644 --- a/crates/storage/src/lazy/mapping.rs +++ b/crates/storage/src/lazy/mapping.rs @@ -488,7 +488,6 @@ mod tests { let key = 0; let value = [0u8; ink_env::BUFFER_SIZE - 1]; - assert_eq!(mapping.try_insert(key, &value), Ok(None)); assert_eq!(mapping.try_get(key), Some(Ok(value))); assert_eq!(mapping.try_take(key), Some(Ok(value))); From 598cb049cb2ee1cdba39a872f5cb6180677b3e56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 15 Nov 2023 16:13:26 -0300 Subject: [PATCH 10/13] Fixed tests and storage. try_insert() and try_set() were ignoring that insert() and set() increase the length of the key by 4. After making the off-chain set_contract_storage() respect the size limit, this was causing some tests to fail. The tests were updated to test the correct limits. --- crates/env/src/engine/off_chain/impls.rs | 4 ++-- crates/storage/src/lazy/mapping.rs | 21 +++++++++++++-------- crates/storage/src/lazy/mod.rs | 12 +++++++----- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index aabfa69fd6..4f8e93c6b4 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -201,8 +201,8 @@ impl EnvBackend for EnvInstance { let encoded_key: Vec = key.encode(); let encoded_value: &[u8] = &v[..]; - if encoded_value.len() + encoded_key.len() > BUFFER_SIZE - 4 { - panic!("Value too large to be stored in contract storage, maximum size is {} bytes", (BUFFER_SIZE - 4)); + if encoded_value.len() + encoded_key.len() > BUFFER_SIZE { + panic!("Value too large {:?} ({}+{} bytes) to be stored in contract storage, maximum size is {} bytes", encoded_key, encoded_key.len(), encoded_value.len(), BUFFER_SIZE); } self.engine.set_storage(&encoded_key, encoded_value) } diff --git a/crates/storage/src/lazy/mapping.rs b/crates/storage/src/lazy/mapping.rs index f8314c7f57..20565ab8c7 100644 --- a/crates/storage/src/lazy/mapping.rs +++ b/crates/storage/src/lazy/mapping.rs @@ -171,7 +171,9 @@ where let value_size = ::encoded_size(value); - if key_size.saturating_add(value_size) > ink_env::BUFFER_SIZE { + // insert() will attempt to insert a tuple (u32, key), which increases + // the size of the key by 4. + if key_size.saturating_add(value_size).saturating_add(4) > ink_env::BUFFER_SIZE { return Err(ink_env::Error::BufferTooSmall) } @@ -379,6 +381,8 @@ const _: () = { #[cfg(test)] mod tests { + use std::panic; + use super::*; use crate::traits::ManualKey; @@ -483,10 +487,10 @@ mod tests { #[test] fn fallible_storage_works_for_fitting_data() { ink_env::test::run_test::(|_| { - let mut mapping: Mapping = Mapping::new(); + let mut mapping: Mapping = Mapping::new(); let key = 0; - let value = [0u8; ink_env::BUFFER_SIZE - 1]; + let value = [0u8; ink_env::BUFFER_SIZE - 1 - 4]; assert_eq!(mapping.try_insert(key, &value), Ok(None)); assert_eq!(mapping.try_get(key), Some(Ok(value))); @@ -502,10 +506,10 @@ mod tests { #[should_panic] fn fallible_storage_fails_gracefully_for_overgrown_data() { ink_env::test::run_test::(|_| { - let mut mapping: Mapping = Mapping::new(); + let mut mapping: Mapping = Mapping::new(); let key = 0; - let value = [0u8; ink_env::BUFFER_SIZE]; + let value = [0u8; ink_env::BUFFER_SIZE - 4]; assert_eq!(mapping.try_get(0), None); assert_eq!( @@ -534,9 +538,10 @@ mod tests { Err(ink_env::Error::BufferTooSmall) ); - // The off-chain impl conveniently uses a Vec for encoding, - // allowing writing values exceeding the static buffer size. - ink_env::set_contract_storage(&(&mapping.key(), key), &value); + let result = panic::catch_unwind(||{ + ink_env::set_contract_storage(&(&mapping.key(), key), &value); + }); + assert!(result.is_err()); assert_eq!( mapping.try_get(key), Some(Err(ink_env::Error::BufferTooSmall)) diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index b6fbdf2e82..7c201b54e1 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -177,7 +177,9 @@ where /// /// Fails if `value` exceeds the static buffer size. pub fn try_set(&mut self, value: &V) -> ink_env::Result<()> { - if value.encoded_size() > ink_env::BUFFER_SIZE { + // set() will attempt to use a u32 key, which we need to account for + // here: + if value.encoded_size().saturating_add(4) > ink_env::BUFFER_SIZE { return Err(ink_env::Error::BufferTooSmall) }; @@ -317,9 +319,9 @@ mod tests { #[test] fn fallible_storage_works_for_fitting_data() { ink_env::test::run_test::(|_| { - let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE]> = Lazy::new(); + let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE - 4]> = Lazy::new(); - let value = [0u8; ink_env::BUFFER_SIZE]; + let value = [0u8; ink_env::BUFFER_SIZE - 4]; assert_eq!(storage.try_set(&value), Ok(())); assert_eq!(storage.try_get(), Some(Ok(value))); @@ -332,9 +334,9 @@ mod tests { #[should_panic] fn fallible_storage_fails_gracefully_for_overgrown_data() { ink_env::test::run_test::(|_| { - let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE + 1]> = Lazy::new(); + let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE - 4 + 1]> = Lazy::new(); - let value = [0u8; ink_env::BUFFER_SIZE + 1]; + let value = [0u8; ink_env::BUFFER_SIZE - 4 + 1]; assert_eq!(storage.try_get(), None); assert_eq!(storage.try_set(&value), Err(ink_env::Error::BufferTooSmall)); From 7e1c3b620e265a7c6726062dfd3ee8cb25b1ca51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 15 Nov 2023 16:34:54 -0300 Subject: [PATCH 11/13] Passed through cargofmt. --- crates/storage/src/lazy/mapping.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/storage/src/lazy/mapping.rs b/crates/storage/src/lazy/mapping.rs index 20565ab8c7..892189212b 100644 --- a/crates/storage/src/lazy/mapping.rs +++ b/crates/storage/src/lazy/mapping.rs @@ -487,7 +487,8 @@ mod tests { #[test] fn fallible_storage_works_for_fitting_data() { ink_env::test::run_test::(|_| { - let mut mapping: Mapping = Mapping::new(); + let mut mapping: Mapping = + Mapping::new(); let key = 0; let value = [0u8; ink_env::BUFFER_SIZE - 1 - 4]; @@ -538,7 +539,7 @@ mod tests { Err(ink_env::Error::BufferTooSmall) ); - let result = panic::catch_unwind(||{ + let result = panic::catch_unwind(|| { ink_env::set_contract_storage(&(&mapping.key(), key), &value); }); assert!(result.is_err()); From e8f4b9de508c00b2fc21543e89e49913bfb8b838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 15 Nov 2023 18:44:30 -0300 Subject: [PATCH 12/13] Removed testing code. --- crates/env/src/engine/off_chain/impls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 4f8e93c6b4..c8c6c4465f 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -202,7 +202,7 @@ impl EnvBackend for EnvInstance { let encoded_value: &[u8] = &v[..]; if encoded_value.len() + encoded_key.len() > BUFFER_SIZE { - panic!("Value too large {:?} ({}+{} bytes) to be stored in contract storage, maximum size is {} bytes", encoded_key, encoded_key.len(), encoded_value.len(), BUFFER_SIZE); + panic!("Value too large to be stored in contract storage, maximum size is {} bytes", BUFFER_SIZE); } self.engine.set_storage(&encoded_key, encoded_value) } From f86c010a113230f4d8c82865ff7b31fe6025b52b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 15 Nov 2023 18:44:46 -0300 Subject: [PATCH 13/13] Fixed up test. --- integration-tests/set-contract-storage/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/set-contract-storage/lib.rs b/integration-tests/set-contract-storage/lib.rs index 9a59aa109b..8305f95369 100644 --- a/integration-tests/set-contract-storage/lib.rs +++ b/integration-tests/set-contract-storage/lib.rs @@ -53,7 +53,7 @@ mod set_contract_storage { #[ink::test] #[should_panic( - expected = "Value too large to be stored in contract storage, maximum size is 16380 bytes" + expected = "Value too large to be stored in contract storage, maximum size is 16384 bytes" )] fn contract_storage_too_big() { let contract = SetContractStorage::new();