Cannot fulfill an expired order

pallets/orders/src/benchmarking.rs

@@ -128,7 +128,7 @@ mod benchmarks {
 		let para_id: ParaId = 2000.into();
 		let requirements = Requirements {
 			begin: 0,
-			end: 8,
+			end: 0,
 			core_occupancy: 28800, // Half of a core.
 		};
 
@@ -143,12 +143,21 @@ mod benchmarks {
 			para_id,
 			requirements,
 		)?;
-		crate::Pallet::<T>::contribute(
-			RawOrigin::Signed(creator.clone()).into(),
-			0,
+
+		// manually contribute since the order 'expired':
+		<<T as Config>::Currency as Currency<T::AccountId>>::transfer(
+			&creator.clone(),
+			&T::OrderToAccountId::convert(0),
 			<T as crate::Config>::MinimumContribution::get(),
+			ExistenceRequirement::KeepAlive,
 		)?;
-		crate::Pallet::<T>::do_cancel_order(0, 10)?;
+		Contributions::<T>::insert(
+			0,
+			creator.clone(),
+			<T as crate::Config>::MinimumContribution::get(),
+		);
+
+		crate::Pallet::<T>::do_cancel_order(0)?;
 
 		#[extrinsic_call]
 		_(RawOrigin::Signed(creator.clone()), 0);

pallets/orders/src/lib.rs

@@ -185,7 +185,7 @@ pub mod pallet {
 		pub fn cancel_order(origin: OriginFor<T>, order_id: OrderId) -> DispatchResult {
 			let who = ensure_signed(origin)?;
 
-			Self::do_cancel_order(order_id, Self::current_timeslice())?;
+			Self::do_cancel_order(order_id)?;
 			Self::deposit_event(Event::OrderRemoved { order_id, by: who });
 
 			Ok(())
@@ -275,19 +275,11 @@ pub mod pallet {
 			Ok(())
 		}
 
-		pub(crate) fn do_cancel_order(
-			order_id: OrderId,
-			current_timeslice: Timeslice,
-		) -> DispatchResult {
+		pub(crate) fn do_cancel_order(order_id: OrderId) -> DispatchResult {
 			let order = Orders::<T>::get(order_id).ok_or(Error::<T>::InvalidOrderId)?;
+			// Only expired orders can be cancelled.
+			ensure!(Self::order_expired(&order), Error::<T>::NotAllowed);
 
-			// Allowing order cancellation 1 timeslice before it truly expires makes writing
-			// benchmarks much easier. With this we can set the start and end to 0 and be able to
-			// cancel the order without having to modify the current timeslice.
-			#[cfg(feature = "runtime-benchmarks")]
-			ensure!(order.requirements.end <= current_timeslice, Error::<T>::NotAllowed);
-			#[cfg(not(feature = "runtime-benchmarks"))]
-			ensure!(order.requirements.end < current_timeslice, Error::<T>::NotAllowed);
 			Orders::<T>::remove(order_id);
 			Ok(())
 		}
@@ -304,6 +296,17 @@ pub mod pallet {
 			Orders::<T>::get(order_id)
 		}
 
+		fn order_expired(order: &Order<T::AccountId>) -> bool {
+			// Defining the order expiry 1 timeslice before it truly expires makes writing
+			// benchmarks much easier. With this approach, we can set the start and end to 0,
+			// thereby defining the order as expired, to allow actions like order cancellation.
+			#[cfg(feature = "runtime-benchmarks")]
+			return order.requirements.end <= Self::current_timeslice();
+
+			#[cfg(not(feature = "runtime-benchmarks"))]
+			return order.requirements.end < Self::current_timeslice();
+		}
+
 		fn remove_order(order_id: &OrderId) {
 			Orders::<T>::remove(order_id)
 		}

pallets/processor/src/lib.rs

@@ -154,6 +154,8 @@ pub mod pallet {
 		NotOwner,
 		/// We didn't find the task to which the region is supposed to be assigned.
 		RegionAssignmentNotFound,
+		/// The order expired and can't be fulfilled.
+		OrderExpired,
 	}
 
 	#[pallet::call]
@@ -187,7 +189,9 @@ pub mod pallet {
 			ensure!(region.owner == who, Error::<T>::NotOwner);
 
 			let record = region.record.get().ok_or(Error::<T>::RecordUnavailable)?;
+
 			let order = T::Orders::order(&order_id).ok_or(Error::<T>::UnknownOrder)?;
+			ensure!(!T::Orders::order_expired(&order), Error::<T>::OrderExpired);
 
 			Self::ensure_matching_requirements(region_id, record, order.requirements)?;
 

pallets/processor/src/tests.rs

@@ -15,8 +15,8 @@
 
 use crate::{
 	mock::{
-		assignments, new_test_ext, Balances, Orders, Processor, Regions, RuntimeOrigin, System,
-		Test,
+		assignments, new_test_ext, Balances, Orders, Processor, Regions, RelayBlockNumber,
+		RuntimeOrigin, System, Test,
 	},
 	Error, Event,
 };
@@ -108,6 +108,36 @@ fn fulfill_order_works() {
 	});
 }
 
+#[test]
+fn cannot_fulfill_expired_order() {
+	new_test_ext(vec![]).execute_with(|| {
+		let region_owner = 1;
+		let order_creator = 2000;
+		let requirements = Requirements {
+			begin: 1,
+			end: 8,
+			core_occupancy: 28800, // Half of a core.
+		};
+
+		<Test as crate::Config>::Currency::make_free_balance_be(&order_creator, 1000u32.into());
+		assert_ok!(Orders::create_order(
+			RuntimeOrigin::signed(order_creator.clone()),
+			2000.into(),
+			requirements.clone()
+		));
+
+		let region_id = RegionId { begin: 0, core: 0, mask: CoreMask::complete() };
+		assert_ok!(Regions::mint_into(&region_id.into(), &region_owner));
+		assert_ok!(Regions::set_record(region_id, RegionRecord { end: 0, owner: 1, paid: None }));
+
+		RelayBlockNumber::set(10 * 80);
+		assert_noop!(
+			Processor::fulfill_order(RuntimeOrigin::signed(region_owner), 0, region_id),
+			Error::<Test>::OrderExpired
+		);
+	});
+}
+
 #[test]
 fn assign_works() {
 	new_test_ext(vec![]).execute_with(|| {

primitives/order/src/lib.rs

@@ -52,6 +52,9 @@ pub trait OrderInspect<AccountId: Clone> {
 	/// If `None` the order was not found.
 	fn order(order_id: &OrderId) -> Option<Order<AccountId>>;
 
+	/// Returns whether an order expired or not.
+	fn order_expired(order_id: &Order<AccountId>) -> bool;
+
 	/// Remove an order with the associated id.
 	fn remove_order(order_id: &OrderId);
 }