Feature/bucket remove (#268)

<!-- Describe what change this PR is implementing -->

<!--- What types of changes does your code introduce? -->
- [ ] Tech Debt (Code improvements)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Dependency upgrade (A change in substrate or any 3rd party crate
version)

<!--- Check the following box with an x if the following applies: -->
- [x] This change requires a runtime migration.
- [ ] Modifies `on_initialize`
- [ ] Modifies `on_finalize`

<!--- All boxes need to be checked. Follow this checklist before
requiring PR review -->
- [x] Change has been tested locally.
- [x] Change adds / updates tests.
- [x] Changelog doc updated.

---------

Co-authored-by: rakanalh <rakan.alhneiti@gmail.com>

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [vNext]
 - DAC nodes can be registered
 - Node mode is bitwise-or instead of enum
+- [C,D] `pallet-ddc-customers`: implemented bucket removal
 - Added ChargeError event to payout pallet
 
 ## [4.8.9]

node/cli/Cargo.toml

@@ -26,17 +26,19 @@ clap = { workspace = true, optional = true }
 url = { workspace = true }
 
 # Substrate dependencies
-frame-benchmarking-cli = { workspace = true, optional = true }
-sc-cli = { workspace = true, optional = true }
-sc-service = { workspace = true, optional = true }
-try-runtime-cli = { workspace = true, optional = true }
+frame-benchmarking-cli = { workspace = true, default-features = true, optional = true }
+sc-cli = { workspace = true, default-features = true, optional = true }
+sc-executor = { workspace = true, default-features = true }
+sc-service = { workspace = true, default-features = true, optional = true }
+sp-io = { workspace = true, default-features = true }
+try-runtime-cli = { workspace = true, default-features = true, optional = true }
 
 # Local
 cere-client = { workspace = true, optional = true }
 cere-service = { workspace = true, optional = true }
 
 [build-dependencies]
-substrate-build-script-utils = { workspace = true }
+substrate-build-script-utils = { workspace = true, default-features = true }
 
 [features]
 default = ["cli", "cere-dev-native"]
@@ -49,7 +51,6 @@ cli = [
   "cere-client",
 ]
 runtime-benchmarks = ["cere-service/runtime-benchmarks"]
-try-runtime = ["cere-service/try-runtime"]
-
+try-runtime = ["cere-service/try-runtime", "try-runtime-cli/try-runtime"]
 cere-native = ["cere-service/cere-native"]
 cere-dev-native = ["cere-service/cere-dev-native"]

node/cli/src/command.rs

@@ -208,32 +208,11 @@ pub fn run() -> sc_cli::Result<()> {
 			}
 		},
 		#[cfg(feature = "try-runtime")]
-		Some(Subcommand::TryRuntime(cmd)) => {
-			let runner = cli.create_runner(cmd)?;
-			let chain_spec = &runner.config().chain_spec;
-
-			let registry = &runner.config().prometheus_config.as_ref().map(|cfg| &cfg.registry);
-			let task_manager =
-				sc_service::TaskManager::new(runner.config().tokio_handle.clone(), *registry)
-					.map_err(|e| Error::Service(ServiceError::Prometheus(e)))?;
-
-			#[cfg(feature = "cere-dev-native")]
-			if chain_spec.is_cere_dev() {
-				return runner.async_run(|config| {
-            Ok((cmd.run::<cere_service::cere_dev_runtime::Block, cere_service::CereDevExecutorDispatch>(config), task_manager))
-        })
-			}
-
-			#[cfg(feature = "cere-native")]
-			{
-				return runner.async_run(|config| {
-          Ok((cmd.run::<cere_service::cere_runtime::Block, cere_service::CereExecutorDispatch>(config), task_manager))
-        })
-			}
-
-			#[cfg(not(feature = "cere-native"))]
-			panic!("No runtime feature (cere, cere-dev) is enabled")
-		},
+		Some(Subcommand::TryRuntime(_)) => Err("The `try-runtime` subcommand has been migrated to a \
+			standalone CLI (https://github.com/paritytech/try-runtime-cli). It is no longer \
+			being maintained here and will be removed entirely some time after January 2024. \
+			Please remove this subcommand from your runtime and use the standalone CLI."
+			.into()),
 		#[cfg(not(feature = "try-runtime"))]
 		Some(Subcommand::TryRuntime) => Err("TryRuntime wasn't enabled when building the node. \
 				You can enable it with `--features try-runtime`."

pallets/chainbridge/Cargo.toml

@@ -45,3 +45,7 @@ runtime-benchmarks = [
   "sp-runtime/runtime-benchmarks",
   "pallet-balances/runtime-benchmarks",
 ]
+try-runtime = [
+  "frame-system/try-runtime",
+  "pallet-balances/try-runtime",
+]

pallets/ddc-clusters/Cargo.toml

@@ -60,3 +60,8 @@ runtime-benchmarks = [
   "frame-system/runtime-benchmarks",
   "sp-runtime/runtime-benchmarks",
 ]
+try-runtime = [
+  "frame-system/try-runtime",
+  "pallet-contracts/try-runtime",
+  "pallet-ddc-nodes/try-runtime",
+]

pallets/ddc-customers/Cargo.toml

@@ -14,6 +14,7 @@ codec = { workspace = true }
 log = { workspace = true }
 rand_chacha = { workspace = true, optional = true }
 scale-info = { workspace = true }
+serde = { workspace = true, features = ["derive"] }
 
 # Substrate dependencies
 frame-benchmarking = { workspace = true, optional = true }
@@ -46,6 +47,7 @@ std = [
   "scale-info/std",
   "sp-runtime/std",
   "sp-std/std",
+  "pallet-balances/std",
 ]
 runtime-benchmarks = [
   "ddc-primitives/runtime-benchmarks",
@@ -54,3 +56,6 @@ runtime-benchmarks = [
   "frame-system/runtime-benchmarks",
   "sp-runtime/runtime-benchmarks",
 ]
+try-runtime = [
+  "frame-system/try-runtime",
+]

pallets/ddc-customers/src/benchmarking.rs

@@ -156,6 +156,7 @@ benchmarks! {
 			owner_id: user.clone(),
 			cluster_id,
 			is_public: false,
+			is_removed: false,
 		};
 
 		<BucketsCount<T>>::set(bucket_id);
@@ -173,6 +174,29 @@ benchmarks! {
 		assert!(bucket.is_public);
 	}
 
+	remove_bucket {
+		let cluster_id = ClusterId::from([1; 20]);
+		let user = account::<T::AccountId>("user", USER_SEED, 0u32);
+
+		let bucket_id = 1;
+		let bucket = Bucket {
+			bucket_id,
+			owner_id: user.clone(),
+			cluster_id,
+			is_public: false,
+			is_removed: false,
+		};
+
+		<BucketsCount<T>>::set(bucket_id);
+		<Buckets<T>>::insert(bucket_id, bucket);
+
+		whitelist_account!(user);
+	}: _(RawOrigin::Signed(user), bucket_id)
+	verify {
+		let bucket = <Buckets<T>>::get(bucket_id).unwrap();
+		assert!(bucket.is_removed);
+	}
+
 	impl_benchmark_test_suite!(
 		DdcCustomers,
 		crate::mock::ExtBuilder.build(),

pallets/ddc-customers/src/lib.rs

@@ -27,13 +27,16 @@ use frame_support::{
 };
 pub use pallet::*;
 use scale_info::TypeInfo;
+use serde::{Deserialize, Serialize};
 use sp_io::hashing::blake2_128;
 use sp_runtime::{
 	traits::{AccountIdConversion, CheckedAdd, CheckedSub, Saturating, Zero},
 	RuntimeDebug, SaturatedConversion,
 };
 use sp_std::prelude::*;
 
+pub mod migration;
+
 /// The balance type of this pallet.
 pub type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
@@ -55,12 +58,15 @@ pub struct UnlockChunk<T: Config> {
 	block: T::BlockNumber,
 }
 
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
 #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo)]
-pub struct Bucket<AccountId> {
+#[scale_info(skip_type_params(T))]
+pub struct Bucket<T: Config> {
 	bucket_id: BucketId,
-	owner_id: AccountId,
+	owner_id: T::AccountId,
 	cluster_id: ClusterId,
 	is_public: bool,
+	is_removed: bool,
 }
 
 #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo)]
@@ -130,7 +136,7 @@ pub mod pallet {
 
 	/// The current storage version.
 	const STORAGE_VERSION: frame_support::traits::StorageVersion =
-		frame_support::traits::StorageVersion::new(0);
+		frame_support::traits::StorageVersion::new(1);
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
@@ -167,11 +173,10 @@ pub mod pallet {
 	pub type BucketsCount<T: Config> =
 		StorageValue<Value = BucketId, QueryKind = ValueQuery, OnEmpty = DefaultBucketCount<T>>;
 
-	/// Map from bucket ID to to the bucket structure
+	/// Map from bucket ID to the bucket structure
 	#[pallet::storage]
 	#[pallet::getter(fn buckets)]
-	pub type Buckets<T: Config> =
-		StorageMap<_, Twox64Concat, BucketId, Bucket<T::AccountId>, OptionQuery>;
+	pub type Buckets<T: Config> = StorageMap<_, Twox64Concat, BucketId, Bucket<T>, OptionQuery>;
 
 	#[pallet::event]
 	#[pallet::generate_deposit(pub(crate) fn deposit_event)]
@@ -193,6 +198,8 @@ pub mod pallet {
 		BucketCreated { bucket_id: BucketId },
 		/// Bucket with specific id updated
 		BucketUpdated { bucket_id: BucketId },
+		/// Bucket with specific id marked as removed
+		BucketRemoved { bucket_id: BucketId },
 	}
 
 	#[pallet::error]
@@ -223,12 +230,14 @@ pub mod pallet {
 		ArithmeticUnderflow,
 		// Transferring balance to pallet's vault has failed
 		TransferFailed,
+		/// Bucket is already removed
+		AlreadyRemoved,
 	}
 
 	#[pallet::genesis_config]
 	pub struct GenesisConfig<T: Config> {
 		pub feeder_account: Option<T::AccountId>,
-		pub buckets: Vec<(ClusterId, T::AccountId, BalanceOf<T>, bool)>,
+		pub buckets: Vec<(Bucket<T>, BalanceOf<T>)>,
 	}
 
 	#[cfg(feature = "std")]
@@ -258,23 +267,17 @@ pub mod pallet {
 				}
 			}
 
-			for (cluster_id, owner_id, deposit, is_public) in &self.buckets {
+			for (bucket, deposit) in &self.buckets {
 				let cur_bucket_id = <BucketsCount<T>>::get()
 					.checked_add(1)
 					.ok_or(Error::<T>::ArithmeticOverflow)
 					.unwrap();
 				<BucketsCount<T>>::set(cur_bucket_id);
 
-				let bucket = Bucket {
-					bucket_id: cur_bucket_id,
-					owner_id: owner_id.clone(),
-					cluster_id: *cluster_id,
-					is_public: *is_public,
-				};
 				<Buckets<T>>::insert(cur_bucket_id, bucket);
 
 				let ledger = AccountsLedger::<T> {
-					owner: owner_id.clone(),
+					owner: bucket.owner_id.clone(),
 					total: *deposit,
 					active: *deposit,
 					unlocking: Default::default(),
@@ -311,6 +314,7 @@ pub mod pallet {
 				owner_id: bucket_owner,
 				cluster_id,
 				is_public: bucket_params.is_public,
+				is_removed: false,
 			};
 
 			<BucketsCount<T>>::set(cur_bucket_id);
@@ -511,6 +515,30 @@ pub mod pallet {
 
 			Ok(())
 		}
+
+		/// Mark existing bucket with specified bucket id as removed
+		///
+		/// Only an owner can remove a bucket
+		#[pallet::call_index(6)]
+		#[pallet::weight(T::WeightInfo::remove_bucket())]
+		pub fn remove_bucket(origin: OriginFor<T>, bucket_id: BucketId) -> DispatchResult {
+			let owner = ensure_signed(origin)?;
+
+			<Buckets<T>>::try_mutate(bucket_id, |maybe_bucket| -> DispatchResult {
+				let mut bucket = maybe_bucket.as_mut().ok_or(Error::<T>::NoBucketWithId)?;
+				ensure!(bucket.owner_id == owner, Error::<T>::NotBucketOwner);
+				ensure!(!bucket.is_removed, Error::<T>::AlreadyRemoved);
+
+				// Mark the bucket as removed
+				bucket.is_removed = true;
+
+				Ok(())
+			})?;
+
+			Self::deposit_event(Event::<T>::BucketRemoved { bucket_id });
+
+			Ok(())
+		}
 	}
 
 	impl<T: Config> Pallet<T> {