Merge pull request #1328 from galacticcouncil/fix/dca-termiante

fix: terminate dca with linear search

Author: dmoka

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = 'pallet-dca'
-version = "1.11.0"
+version = "1.12.0"
 description = 'A pallet to manage DCA scheduling'
 authors = ['GalacticCouncil']
 edition = '2021'

pallets/dca/Cargo.toml

@@ -649,11 +649,11 @@ pub mod pallet {
 				|maybe_schedule_ids| -> DispatchResult {
 					let schedule_ids = maybe_schedule_ids.as_mut().ok_or(Error::<T>::ScheduleNotFound)?;
 
-					let index = schedule_ids
-						.binary_search(&schedule_id)
-						.map_err(|_| Error::<T>::ScheduleNotFound)?;
-
-					schedule_ids.remove(index);
+					let idx = schedule_ids
+						.iter()
+						.position(|x| *x == schedule_id)
+						.ok_or(Error::<T>::ScheduleNotFound)?;
+					schedule_ids.remove(idx);
 
 					if schedule_ids.is_empty() {
 						*maybe_schedule_ids = None;

pallets/dca/src/lib.rs

@@ -384,6 +384,95 @@ fn terminate_should_work_with_no_blocknumber_when_just_scheduled() {
 		});
 }
 
+#[test]
+fn terminate_should_not_depend_on_schedule_ids_per_block_ordering() {
+	ExtBuilder::default()
+		.with_endowed_accounts(vec![(ALICE, HDX, 10000 * ONE), (BOB, HDX, 10000 * ONE)])
+		.build()
+		.execute_with(|| {
+			let (block_600, schedule_id_0, schedule_id_1) = arrange_unsorted_schedule_ids_per_block_600();
+
+			let reserved_before = Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE);
+			assert!(reserved_before > 0);
+
+			assert_ok!(DCA::terminate(RuntimeOrigin::signed(ALICE), schedule_id_1, None));
+
+			assert_that_schedule_has_been_removed_from_storages!(ALICE, schedule_id_1);
+			assert_eq!(DCA::schedule_ids_per_block(block_600).to_vec(), vec![schedule_id_0]);
+			assert_eq!(
+				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE),
+				0
+			);
+		});
+}
+
+fn arrange_unsorted_schedule_ids_per_block_600() -> (u64, ScheduleId, ScheduleId) {
+	// Builds `ScheduleIdsPerBlock[600] == [1, 0]` via normal flow.
+	let block_502 = 502u64;
+	let block_600 = 600u64;
+	let schedule_id_0: ScheduleId = 0;
+	let schedule_id_1: ScheduleId = 1;
+
+	set_block_number(500);
+
+	let schedule_replans_into_block_600 = ScheduleBuilder::new()
+		.with_owner(BOB)
+		.with_period(98)
+		.with_order(Order::Sell {
+			asset_in: HDX,
+			asset_out: BTC,
+			amount_in: ONE,
+			min_amount_out: 0,
+			route: create_bounded_vec(vec![Trade {
+				pool: PoolType::Omnipool,
+				asset_in: HDX,
+				asset_out: BTC,
+			}]),
+		})
+		.build();
+	assert_ok!(DCA::schedule(
+		RuntimeOrigin::signed(BOB),
+		schedule_replans_into_block_600,
+		None
+	));
+	assert!(DCA::schedules(schedule_id_0).is_some());
+
+	let schedule_planned_for_block_600 = ScheduleBuilder::new()
+		.with_total_amount(0)
+		.with_order(Order::Sell {
+			asset_in: HDX,
+			asset_out: BTC,
+			amount_in: ONE,
+			min_amount_out: 0,
+			route: create_bounded_vec(vec![Trade {
+				pool: PoolType::Omnipool,
+				asset_in: HDX,
+				asset_out: BTC,
+			}]),
+		})
+		.build();
+	assert_ok!(DCA::schedule(
+		RuntimeOrigin::signed(ALICE),
+		schedule_planned_for_block_600,
+		Some(block_600)
+	));
+	assert!(DCA::schedules(schedule_id_1).is_some());
+
+	set_block_number(block_502);
+
+	assert!(DCA::schedules(schedule_id_1).is_some());
+	assert_eq!(DCA::schedule_execution_block(schedule_id_1), Some(block_600));
+	assert_eq!(
+		DCA::schedule_ids_per_block(block_600).to_vec(),
+		vec![schedule_id_1, schedule_id_0]
+	);
+	assert!(DCA::schedule_ids_per_block(block_600)
+		.iter()
+		.any(|id| *id == schedule_id_1));
+
+	(block_600, schedule_id_0, schedule_id_1)
+}
+
 pub fn set_block_number(to: u64) {
 	System::set_block_number(to);
 	DCA::on_initialize(to);

pallets/dca/src/tests/terminate.rs

@@ -1,6 +1,6 @@
 [package]
 name = "hydradx-runtime"
-version = "382.0.0"
+version = "383.0.0"
 authors = ["GalacticCouncil"]
 edition = "2021"
 license = "Apache 2.0"

runtime/hydradx/Cargo.toml

@@ -519,17 +519,25 @@ runtime_benchmarks! {
 
 		let amount_sell = 200 * ONE;
 		let schedule1 = schedule_fake(caller.clone(), HDX, DAI, amount_sell);
-		let schedule_id : ScheduleId = 0;
 
 		set_period(99);
-		let execution_block = 100u32;
-		assert_ok!(DCA::schedule(RawOrigin::Signed(caller).into(), schedule1, Option::Some(execution_block)));
-		ScheduleExtraGas::<Runtime>::insert(schedule_id, 1_000_000);
 
+		let execution_block = 105u32;
+
+		// Fill block with MaxSchedulesPerBlock schedules to test worst case for linear search
+		for _ in 0..MaxSchedulesPerBlock::get() {
+			assert_ok!(DCA::schedule(RawOrigin::Signed(caller.clone()).into(), schedule1.clone(), Option::Some(execution_block)));
+		}
+
+		// Terminate the last schedule which is at the last index in ScheduleIdsPerBlock
+		// This is worst case for linear search - must iterate through all elements
+		let schedule_id: ScheduleId = MaxSchedulesPerBlock::get() - 1;
+		ScheduleExtraGas::<Runtime>::insert(schedule_id, 1_000_000);
 
 	}: _(RawOrigin::Root, schedule_id, None)
 	verify {
 		assert!(<Schedules<Runtime>>::get::<ScheduleId>(schedule_id).is_none());
+		assert_eq!((MaxSchedulesPerBlock::get() - 1) as usize, <ScheduleIdsPerBlock<Runtime>>::get::<BlockNumber>(execution_block).len());
 	}
 
 	unlock_reserves {

runtime/hydradx/src/benchmarking/dca.rs

@@ -122,7 +122,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("hydradx"),
 	impl_name: create_runtime_str!("hydradx"),
 	authoring_version: 1,
-	spec_version: 382,
+	spec_version: 383,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,

runtime/hydradx/src/lib.rs

@@ -19,7 +19,7 @@
 //! Autogenerated weights for `pallet_dca`
 //!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 43.0.0
-//! DATE: 2026-01-10, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! DATE: 2026-01-16, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! WORST CASE MAP SIZE: `1000000`
 //! HOSTNAME: `bench-bot`, CPU: `Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz`
 //! WASM-EXECUTION: `Compiled`, CHAIN: `None`, DB CACHE: `1024`
@@ -82,8 +82,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `19910`
 		//  Estimated: `31230`
-		// Minimum execution time: 241_462_000 picoseconds.
-		Weight::from_parts(244_164_000, 31230)
+		// Minimum execution time: 244_500_000 picoseconds.
+		Weight::from_parts(245_810_000, 31230)
 			.saturating_add(T::DbWeight::get().reads(19_u64))
 			.saturating_add(T::DbWeight::get().writes(9_u64))
 	}
@@ -137,8 +137,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `26447`
 		//  Estimated: `31230`
-		// Minimum execution time: 451_876_000 picoseconds.
-		Weight::from_parts(455_359_000, 31230)
+		// Minimum execution time: 452_314_000 picoseconds.
+		Weight::from_parts(456_558_000, 31230)
 			.saturating_add(T::DbWeight::get().reads(41_u64))
 			.saturating_add(T::DbWeight::get().writes(12_u64))
 	}
@@ -164,8 +164,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `19658`
 		//  Estimated: `31230`
-		// Minimum execution time: 245_109_000 picoseconds.
-		Weight::from_parts(247_849_000, 31230)
+		// Minimum execution time: 246_503_000 picoseconds.
+		Weight::from_parts(248_412_000, 31230)
 			.saturating_add(T::DbWeight::get().reads(19_u64))
 			.saturating_add(T::DbWeight::get().writes(9_u64))
 	}
@@ -219,8 +219,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `26707`
 		//  Estimated: `31230`
-		// Minimum execution time: 453_637_000 picoseconds.
-		Weight::from_parts(456_962_000, 31230)
+		// Minimum execution time: 455_030_000 picoseconds.
+		Weight::from_parts(457_587_000, 31230)
 			.saturating_add(T::DbWeight::get().reads(41_u64))
 			.saturating_add(T::DbWeight::get().writes(12_u64))
 	}
@@ -230,8 +230,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `1113`
 		//  Estimated: `3510`
-		// Minimum execution time: 20_010_000 picoseconds.
-		Weight::from_parts(20_454_000, 3510)
+		// Minimum execution time: 20_129_000 picoseconds.
+		Weight::from_parts(20_578_000, 3510)
 			.saturating_add(T::DbWeight::get().reads(1_u64))
 	}
 	/// Storage: `MultiTransactionPayment::AcceptedCurrencies` (r:2 w:0)
@@ -268,8 +268,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `19305`
 		//  Estimated: `28710`
-		// Minimum execution time: 236_756_000 picoseconds.
-		Weight::from_parts(238_471_000, 28710)
+		// Minimum execution time: 235_459_000 picoseconds.
+		Weight::from_parts(237_584_000, 28710)
 			.saturating_add(T::DbWeight::get().reads(24_u64))
 			.saturating_add(T::DbWeight::get().writes(9_u64))
 	}
@@ -293,10 +293,10 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 	/// Proof: `DCA::ScheduleOwnership` (`max_values`: None, `max_size`: Some(60), added: 2535, mode: `MaxEncodedLen`)
 	fn terminate() -> Weight {
 		// Proof Size summary in bytes:
-		//  Measured:  `2642`
+		//  Measured:  `4072`
 		//  Estimated: `4714`
-		// Minimum execution time: 95_747_000 picoseconds.
-		Weight::from_parts(96_689_000, 4714)
+		// Minimum execution time: 101_721_000 picoseconds.
+		Weight::from_parts(102_910_000, 4714)
 			.saturating_add(T::DbWeight::get().reads(6_u64))
 			.saturating_add(T::DbWeight::get().writes(9_u64))
 	}
@@ -310,8 +310,8 @@ impl<T: frame_system::Config> pallet_dca::WeightInfo for HydraWeight<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `2144`
 		//  Estimated: `4714`
-		// Minimum execution time: 62_338_000 picoseconds.
-		Weight::from_parts(62_941_000, 4714)
+		// Minimum execution time: 61_941_000 picoseconds.
+		Weight::from_parts(62_696_000, 4714)
 			.saturating_add(T::DbWeight::get().reads(3_u64))
 			.saturating_add(T::DbWeight::get().writes(2_u64))
 	}