From 047f6fe9bc5d2474eb0373cae72f881381f752a3 Mon Sep 17 00:00:00 2001
From: Nazar Mokrynskyi <nazar@mokrynskyi.com>
Date: Wed, 10 Jan 2024 13:56:44 +0200
Subject: [PATCH] Improve storage monitor API (#2899)

This removes the need to unnecessarily provide a very specific data
structure `DatabaseSource` and removes huge `sc-client-db` dependency
from storage monitor. It is now possible to use storage monitor with any
path.

P.S. I still strongly dislike that it pulls `clap` dependency for such a
small feature, but many other crates do as well, so nothing special
here.
---
 substrate/bin/node/cli/src/service.rs       | 18 +++---
 substrate/client/storage-monitor/Cargo.toml |  3 +-
 substrate/client/storage-monitor/src/lib.rs | 61 +++++++++------------
 3 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs
index 5e6e794f73283..67d21ee69ed12 100644
--- a/substrate/bin/node/cli/src/service.rs
+++ b/substrate/bin/node/cli/src/service.rs
@@ -38,7 +38,7 @@ use sc_transaction_pool_api::OffchainTransactionPoolFactory;
 use sp_api::ProvideRuntimeApi;
 use sp_core::crypto::Pair;
 use sp_runtime::{generic, traits::Block as BlockT, SaturatedConversion};
-use std::sync::Arc;
+use std::{path::Path, sync::Arc};
 
 /// Host functions required for kitchensink runtime and Substrate node.
 #[cfg(not(feature = "runtime-benchmarks"))]
@@ -769,16 +769,18 @@ pub fn new_full_base(
 /// Builds a new service for a full client.
 pub fn new_full(config: Configuration, cli: Cli) -> Result<TaskManager, ServiceError> {
 	let mixnet_config = cli.mixnet_params.config(config.role.is_authority());
-	let database_source = config.database.clone();
+	let database_path = config.database.path().map(Path::to_path_buf);
 	let task_manager = new_full_base(config, mixnet_config, cli.no_hardware_benchmarks, |_, _| ())
 		.map(|NewFullBase { task_manager, .. }| task_manager)?;
 
-	sc_storage_monitor::StorageMonitorService::try_spawn(
-		cli.storage_monitor,
-		database_source,
-		&task_manager.spawn_essential_handle(),
-	)
-	.map_err(|e| ServiceError::Application(e.into()))?;
+	if let Some(database_path) = database_path {
+		sc_storage_monitor::StorageMonitorService::try_spawn(
+			cli.storage_monitor,
+			database_path,
+			&task_manager.spawn_essential_handle(),
+		)
+		.map_err(|e| ServiceError::Application(e.into()))?;
+	}
 
 	Ok(task_manager)
 }
diff --git a/substrate/client/storage-monitor/Cargo.toml b/substrate/client/storage-monitor/Cargo.toml
index 2b46ccbcdc154..7185c62e276e9 100644
--- a/substrate/client/storage-monitor/Cargo.toml
+++ b/substrate/client/storage-monitor/Cargo.toml
@@ -15,7 +15,6 @@ workspace = true
 clap = { version = "4.4.14", features = ["derive", "string"] }
 log = "0.4.17"
 fs4 = "0.7.0"
-sc-client-db = { path = "../db", default-features = false }
 sp-core = { path = "../../primitives/core" }
-tokio = "1.22.0"
+tokio = { version = "1.22.0", features = ["time"] }
 thiserror = "1.0.48"
diff --git a/substrate/client/storage-monitor/src/lib.rs b/substrate/client/storage-monitor/src/lib.rs
index b88b66d2d60d4..28d9063e5e4b7 100644
--- a/substrate/client/storage-monitor/src/lib.rs
+++ b/substrate/client/storage-monitor/src/lib.rs
@@ -17,7 +17,6 @@
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
 use clap::Args;
-use sc_client_db::DatabaseSource;
 use sp_core::traits::SpawnEssentialNamed;
 use std::{
 	io,
@@ -70,43 +69,37 @@ impl StorageMonitorService {
 	/// Creates new StorageMonitorService for given client config
 	pub fn try_spawn(
 		parameters: StorageMonitorParams,
-		database: DatabaseSource,
+		path: PathBuf,
 		spawner: &impl SpawnEssentialNamed,
 	) -> Result<()> {
-		Ok(match (parameters.threshold, database.path()) {
-			(0, _) => {
-				log::info!(
-					target: LOG_TARGET,
-					"StorageMonitorService: threshold `0` given, storage monitoring disabled",
-				);
-			},
-			(_, None) => {
-				log::warn!(
-					target: LOG_TARGET,
-					"StorageMonitorService: no database path to observe",
-				);
-			},
-			(threshold, Some(path)) => {
-				log::debug!(
-					target: LOG_TARGET,
-					"Initializing StorageMonitorService for db path: {path:?}",
-				);
-
-				Self::check_free_space(&path, threshold)?;
+		if parameters.threshold == 0 {
+			log::info!(
+				target: LOG_TARGET,
+				"StorageMonitorService: threshold `0` given, storage monitoring disabled",
+			);
+		} else {
+			log::debug!(
+				target: LOG_TARGET,
+				"Initializing StorageMonitorService for db path: {}",
+				path.display()
+			);
+
+			Self::check_free_space(&path, parameters.threshold)?;
+
+			let storage_monitor_service = StorageMonitorService {
+				path,
+				threshold: parameters.threshold,
+				polling_period: Duration::from_secs(parameters.polling_period.into()),
+			};
 
-				let storage_monitor_service = StorageMonitorService {
-					path: path.to_path_buf(),
-					threshold,
-					polling_period: Duration::from_secs(parameters.polling_period.into()),
-				};
+			spawner.spawn_essential(
+				"storage-monitor",
+				None,
+				Box::pin(storage_monitor_service.run()),
+			);
+		}
 
-				spawner.spawn_essential(
-					"storage-monitor",
-					None,
-					Box::pin(storage_monitor_service.run()),
-				);
-			},
-		})
+		Ok(())
 	}
 
 	/// Main monitoring loop, intended to be spawned as essential task. Quits if free space drop