From 45bc809bbdce53b2c43a874d0bb78d363d7f8179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 12:43:19 +0100 Subject: [PATCH 01/10] Add custom error messages for StorageNoopGuard Turn StorageNoopGuard into struct with storage_root and error_message. Add from_error_message constructor. Add set_error_message setter. --- .../support/src/storage/storage_noop_guard.rs | 48 +++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index d00e6e18ecc48..315c650935830 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -37,11 +37,29 @@ /// }); /// ``` #[must_use] -pub struct StorageNoopGuard(sp_std::vec::Vec); +pub struct StorageNoopGuard { + storage_root: sp_std::vec::Vec, + error_message: &'static str, +} impl Default for StorageNoopGuard { fn default() -> Self { - Self(sp_io::storage::root(sp_runtime::StateVersion::V1)) + Self { + storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), + error_message: "StorageNoopGuard detected wrongful storage changes.", + } + } +} + +impl StorageNoopGuard { + /// Creates a new `StorageNoopGuard` with a custom error message + pub fn from_error_message(error_message: &'static str) -> Self { + Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } + } + + /// Sets a custom error message for a `StorageNoopGuard` + pub fn set_error_message(&mut self, error_message: &'static str) { + self.error_message = error_message; } } @@ -53,8 +71,9 @@ impl Drop for StorageNoopGuard { } assert_eq!( sp_io::storage::root(sp_runtime::StateVersion::V1), - self.0, - "StorageNoopGuard detected wrongful storage changes.", + self.storage_root, + "{}", + self.error_message, ); } } @@ -111,4 +130,25 @@ mod tests { panic!("Something else"); }); } + + #[test] + #[should_panic(expected = "StorageNoopGuard found unexpected storage changes.")] + fn storage_noop_guard_panics_created_from_error_message() { + TestExternalities::default().execute_with(|| { + let _guard = StorageNoopGuard::from_error_message( + "StorageNoopGuard found unexpected storage changes.", + ); + frame_support::storage::unhashed::put(b"key", b"value"); + }); + } + + #[test] + #[should_panic(expected = "StorageNoopGuard found unexpected storage changes.")] + fn storage_noop_guard_panics_with_set_error_message() { + TestExternalities::default().execute_with(|| { + let mut guard = StorageNoopGuard::default(); + guard.set_error_message("StorageNoopGuard found unexpected storage changes."); + frame_support::storage::unhashed::put(b"key", b"value"); + }); + } } From 40319f0cb3a0d6b08c6066531e5f4962646c50dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 12:48:38 +0100 Subject: [PATCH 02/10] Add new() alias to default() for StorageNoopGuard --- .../support/src/storage/storage_noop_guard.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 315c650935830..32e002ace9ea0 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -52,6 +52,11 @@ impl Default for StorageNoopGuard { } impl StorageNoopGuard { + /// Alias to default() + pub fn new() -> Self { + Self::default() + } + /// Creates a new `StorageNoopGuard` with a custom error message pub fn from_error_message(error_message: &'static str) -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } @@ -151,4 +156,13 @@ mod tests { frame_support::storage::unhashed::put(b"key", b"value"); }); } + + #[test] + #[should_panic(expected = "StorageNoopGuard detected wrongful storage changes.")] + fn storage_noop_guard_panics_new_alias() { + TestExternalities::default().execute_with(|| { + let _guard = StorageNoopGuard::new(); + frame_support::storage::unhashed::put(b"key", b"value"); + }); + } } From bad96b0a285e8a51748f9d92c263054c2a2f18a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 14:07:43 +0100 Subject: [PATCH 03/10] Fix doc comments Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- substrate/frame/support/src/storage/storage_noop_guard.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 32e002ace9ea0..2c37b1bc90758 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -52,17 +52,17 @@ impl Default for StorageNoopGuard { } impl StorageNoopGuard { - /// Alias to default() + /// Alias to `default()`. pub fn new() -> Self { Self::default() } - /// Creates a new `StorageNoopGuard` with a custom error message + /// Creates a new `StorageNoopGuard` with a custom error message. pub fn from_error_message(error_message: &'static str) -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } } - /// Sets a custom error message for a `StorageNoopGuard` + /// Sets a custom error message for a `StorageNoopGuard`. pub fn set_error_message(&mut self, error_message: &'static str) { self.error_message = error_message; } From 332d26f9481af473e96cf41a125fde8df1f56f06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 14:22:36 +0100 Subject: [PATCH 04/10] Update default error message Co-authored-by: Liam Aharon --- substrate/frame/support/src/storage/storage_noop_guard.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 2c37b1bc90758..2dd88f2264ddd 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -46,7 +46,7 @@ impl Default for StorageNoopGuard { fn default() -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), - error_message: "StorageNoopGuard detected wrongful storage changes.", + error_message: "`StorageNoopGuard` detected an attempted storage change.", } } } From 9c90e7b679e6422d0ed11ef2a61dbd05b5cf9b3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 14:27:38 +0100 Subject: [PATCH 05/10] Properly link StorageNoopGuard in doc comments --- substrate/frame/support/src/storage/storage_noop_guard.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 2dd88f2264ddd..0738d4a09bc20 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -57,12 +57,12 @@ impl StorageNoopGuard { Self::default() } - /// Creates a new `StorageNoopGuard` with a custom error message. + /// Creates a new [`StorageNoopGuard`] with a custom error message. pub fn from_error_message(error_message: &'static str) -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } } - /// Sets a custom error message for a `StorageNoopGuard`. + /// Sets a custom error message for a [`StorageNoopGuard`]. pub fn set_error_message(&mut self, error_message: &'static str) { self.error_message = error_message; } From e612f8106841f727fd7eec1dea0f5dd9aeeac24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 14:34:54 +0100 Subject: [PATCH 06/10] Implement From str for StorageNoopGuard --- .../support/src/storage/storage_noop_guard.rs | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 0738d4a09bc20..1c4119ea2f531 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -51,6 +51,12 @@ impl Default for StorageNoopGuard { } } +impl From<&'static str> for StorageNoopGuard { + fn from(error_message: &'static str) -> Self { + StorageNoopGuard { storage_root: sp_std::vec::Vec::new(), error_message } + } +} + impl StorageNoopGuard { /// Alias to `default()`. pub fn new() -> Self { @@ -89,7 +95,7 @@ mod tests { use sp_io::TestExternalities; #[test] - #[should_panic(expected = "StorageNoopGuard detected wrongful storage changes.")] + #[should_panic(expected = "`StorageNoopGuard` detected an attempted storage change.")] fn storage_noop_guard_panics_on_changed() { TestExternalities::default().execute_with(|| { let _guard = StorageNoopGuard::default(); @@ -107,7 +113,7 @@ mod tests { } #[test] - #[should_panic(expected = "StorageNoopGuard detected wrongful storage changes.")] + #[should_panic(expected = "`StorageNoopGuard` detected an attempted storage change.")] fn storage_noop_guard_panics_on_early_drop() { TestExternalities::default().execute_with(|| { let guard = StorageNoopGuard::default(); @@ -137,28 +143,38 @@ mod tests { } #[test] - #[should_panic(expected = "StorageNoopGuard found unexpected storage changes.")] + #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] fn storage_noop_guard_panics_created_from_error_message() { TestExternalities::default().execute_with(|| { let _guard = StorageNoopGuard::from_error_message( - "StorageNoopGuard found unexpected storage changes.", + "`StorageNoopGuard` found unexpected storage changes.", ); frame_support::storage::unhashed::put(b"key", b"value"); }); } #[test] - #[should_panic(expected = "StorageNoopGuard found unexpected storage changes.")] + #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] + fn storage_noop_guard_panics_created_from() { + TestExternalities::default().execute_with(|| { + let _guard = + StorageNoopGuard::from("`StorageNoopGuard` found unexpected storage changes."); + frame_support::storage::unhashed::put(b"key", b"value"); + }); + } + + #[test] + #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] fn storage_noop_guard_panics_with_set_error_message() { TestExternalities::default().execute_with(|| { let mut guard = StorageNoopGuard::default(); - guard.set_error_message("StorageNoopGuard found unexpected storage changes."); + guard.set_error_message("`StorageNoopGuard` found unexpected storage changes."); frame_support::storage::unhashed::put(b"key", b"value"); }); } #[test] - #[should_panic(expected = "StorageNoopGuard detected wrongful storage changes.")] + #[should_panic(expected = "`StorageNoopGuard` detected an attempted storage change.")] fn storage_noop_guard_panics_new_alias() { TestExternalities::default().execute_with(|| { let _guard = StorageNoopGuard::new(); From 7bc5b6157c19d2cfa7838cfefbecdb165ad2ae88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 14:48:33 +0100 Subject: [PATCH 07/10] Remove From implementation --- .../support/src/storage/storage_noop_guard.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 1c4119ea2f531..485c41d010782 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -51,12 +51,6 @@ impl Default for StorageNoopGuard { } } -impl From<&'static str> for StorageNoopGuard { - fn from(error_message: &'static str) -> Self { - StorageNoopGuard { storage_root: sp_std::vec::Vec::new(), error_message } - } -} - impl StorageNoopGuard { /// Alias to `default()`. pub fn new() -> Self { @@ -153,16 +147,6 @@ mod tests { }); } - #[test] - #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] - fn storage_noop_guard_panics_created_from() { - TestExternalities::default().execute_with(|| { - let _guard = - StorageNoopGuard::from("`StorageNoopGuard` found unexpected storage changes."); - frame_support::storage::unhashed::put(b"key", b"value"); - }); - } - #[test] #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] fn storage_noop_guard_panics_with_set_error_message() { From 182f6ee541efd564c8c1ec43e50ab988451a29cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 14:56:26 +0100 Subject: [PATCH 08/10] Change type of the error message to String Since this is for testing it doesn't need to be optimised unnecessarily --- .../support/src/storage/storage_noop_guard.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 485c41d010782..1278f1f64cdd1 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -39,14 +39,14 @@ #[must_use] pub struct StorageNoopGuard { storage_root: sp_std::vec::Vec, - error_message: &'static str, + error_message: String, } impl Default for StorageNoopGuard { fn default() -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), - error_message: "`StorageNoopGuard` detected an attempted storage change.", + error_message: String::from("`StorageNoopGuard` detected an attempted storage change."), } } } @@ -58,12 +58,12 @@ impl StorageNoopGuard { } /// Creates a new [`StorageNoopGuard`] with a custom error message. - pub fn from_error_message(error_message: &'static str) -> Self { + pub fn from_error_message(error_message: String) -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } } /// Sets a custom error message for a [`StorageNoopGuard`]. - pub fn set_error_message(&mut self, error_message: &'static str) { + pub fn set_error_message(&mut self, error_message: String) { self.error_message = error_message; } } @@ -140,9 +140,9 @@ mod tests { #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] fn storage_noop_guard_panics_created_from_error_message() { TestExternalities::default().execute_with(|| { - let _guard = StorageNoopGuard::from_error_message( + let _guard = StorageNoopGuard::from_error_message(String::from( "`StorageNoopGuard` found unexpected storage changes.", - ); + )); frame_support::storage::unhashed::put(b"key", b"value"); }); } @@ -152,7 +152,9 @@ mod tests { fn storage_noop_guard_panics_with_set_error_message() { TestExternalities::default().execute_with(|| { let mut guard = StorageNoopGuard::default(); - guard.set_error_message("`StorageNoopGuard` found unexpected storage changes."); + guard.set_error_message(String::from( + "`StorageNoopGuard` found unexpected storage changes.", + )); frame_support::storage::unhashed::put(b"key", b"value"); }); } From b5bbaa1bfaac6a0004a6e90036d7339aa4c9d110 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 15:47:49 +0100 Subject: [PATCH 09/10] Revert "Change type of the error message to String" This reverts commit 182f6ee541efd564c8c1ec43e50ab988451a29cf. --- .../support/src/storage/storage_noop_guard.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 1278f1f64cdd1..485c41d010782 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -39,14 +39,14 @@ #[must_use] pub struct StorageNoopGuard { storage_root: sp_std::vec::Vec, - error_message: String, + error_message: &'static str, } impl Default for StorageNoopGuard { fn default() -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), - error_message: String::from("`StorageNoopGuard` detected an attempted storage change."), + error_message: "`StorageNoopGuard` detected an attempted storage change.", } } } @@ -58,12 +58,12 @@ impl StorageNoopGuard { } /// Creates a new [`StorageNoopGuard`] with a custom error message. - pub fn from_error_message(error_message: String) -> Self { + pub fn from_error_message(error_message: &'static str) -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } } /// Sets a custom error message for a [`StorageNoopGuard`]. - pub fn set_error_message(&mut self, error_message: String) { + pub fn set_error_message(&mut self, error_message: &'static str) { self.error_message = error_message; } } @@ -140,9 +140,9 @@ mod tests { #[should_panic(expected = "`StorageNoopGuard` found unexpected storage changes.")] fn storage_noop_guard_panics_created_from_error_message() { TestExternalities::default().execute_with(|| { - let _guard = StorageNoopGuard::from_error_message(String::from( + let _guard = StorageNoopGuard::from_error_message( "`StorageNoopGuard` found unexpected storage changes.", - )); + ); frame_support::storage::unhashed::put(b"key", b"value"); }); } @@ -152,9 +152,7 @@ mod tests { fn storage_noop_guard_panics_with_set_error_message() { TestExternalities::default().execute_with(|| { let mut guard = StorageNoopGuard::default(); - guard.set_error_message(String::from( - "`StorageNoopGuard` found unexpected storage changes.", - )); + guard.set_error_message("`StorageNoopGuard` found unexpected storage changes."); frame_support::storage::unhashed::put(b"key", b"value"); }); } From 42d568d8562acf681b060cddc21ad59212321ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=B3nal=20Murray?= Date: Wed, 27 Sep 2023 15:57:02 +0100 Subject: [PATCH 10/10] Switch str lifetime from static to same as guard --- .../support/src/storage/storage_noop_guard.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/substrate/frame/support/src/storage/storage_noop_guard.rs b/substrate/frame/support/src/storage/storage_noop_guard.rs index 485c41d010782..c4d40fa99a35c 100644 --- a/substrate/frame/support/src/storage/storage_noop_guard.rs +++ b/substrate/frame/support/src/storage/storage_noop_guard.rs @@ -37,12 +37,12 @@ /// }); /// ``` #[must_use] -pub struct StorageNoopGuard { +pub struct StorageNoopGuard<'a> { storage_root: sp_std::vec::Vec, - error_message: &'static str, + error_message: &'a str, } -impl Default for StorageNoopGuard { +impl<'a> Default for StorageNoopGuard<'a> { fn default() -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), @@ -51,24 +51,24 @@ impl Default for StorageNoopGuard { } } -impl StorageNoopGuard { +impl<'a> StorageNoopGuard<'a> { /// Alias to `default()`. pub fn new() -> Self { Self::default() } /// Creates a new [`StorageNoopGuard`] with a custom error message. - pub fn from_error_message(error_message: &'static str) -> Self { + pub fn from_error_message(error_message: &'a str) -> Self { Self { storage_root: sp_io::storage::root(sp_runtime::StateVersion::V1), error_message } } /// Sets a custom error message for a [`StorageNoopGuard`]. - pub fn set_error_message(&mut self, error_message: &'static str) { + pub fn set_error_message(&mut self, error_message: &'a str) { self.error_message = error_message; } } -impl Drop for StorageNoopGuard { +impl<'a> Drop for StorageNoopGuard<'a> { fn drop(&mut self) { // No need to double panic, eg. inside a test assertion failure. if sp_std::thread::panicking() {