diff --git a/pallets/orders/src/benchmarking.rs b/pallets/orders/src/benchmarking.rs index 639f9f1b..33d1fef9 100644 --- a/pallets/orders/src/benchmarking.rs +++ b/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::::contribute( - RawOrigin::Signed(creator.clone()).into(), - 0, + + // manually contribute since the order 'expired': + <::Currency as Currency>::transfer( + &creator.clone(), + &T::OrderToAccountId::convert(0), ::MinimumContribution::get(), + ExistenceRequirement::KeepAlive, )?; - crate::Pallet::::do_cancel_order(0, 10)?; + Contributions::::insert( + 0, + creator.clone(), + ::MinimumContribution::get(), + ); + + crate::Pallet::::do_cancel_order(0)?; #[extrinsic_call] _(RawOrigin::Signed(creator.clone()), 0); diff --git a/pallets/orders/src/lib.rs b/pallets/orders/src/lib.rs index f7d5902a..20cc2d08 100644 --- a/pallets/orders/src/lib.rs +++ b/pallets/orders/src/lib.rs @@ -185,7 +185,7 @@ pub mod pallet { pub fn cancel_order(origin: OriginFor, 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::::get(order_id).ok_or(Error::::InvalidOrderId)?; + // Only expired orders can be cancelled. + ensure!(Self::order_expired(&order), Error::::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::::NotAllowed); - #[cfg(not(feature = "runtime-benchmarks"))] - ensure!(order.requirements.end < current_timeslice, Error::::NotAllowed); Orders::::remove(order_id); Ok(()) } @@ -304,6 +296,17 @@ pub mod pallet { Orders::::get(order_id) } + fn order_expired(order: &Order) -> 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::::remove(order_id) } diff --git a/pallets/processor/src/lib.rs b/pallets/processor/src/lib.rs index f743b7df..ffc6f778 100644 --- a/pallets/processor/src/lib.rs +++ b/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::::NotOwner); let record = region.record.get().ok_or(Error::::RecordUnavailable)?; + let order = T::Orders::order(&order_id).ok_or(Error::::UnknownOrder)?; + ensure!(!T::Orders::order_expired(&order), Error::::OrderExpired); Self::ensure_matching_requirements(region_id, record, order.requirements)?; diff --git a/pallets/processor/src/tests.rs b/pallets/processor/src/tests.rs index ff4e20c8..8fb472cf 100644 --- a/pallets/processor/src/tests.rs +++ b/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. + }; + + ::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(®ion_id.into(), ®ion_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::::OrderExpired + ); + }); +} + #[test] fn assign_works() { new_test_ext(vec![]).execute_with(|| { diff --git a/primitives/order/src/lib.rs b/primitives/order/src/lib.rs index 3fba74f5..14a2f855 100644 --- a/primitives/order/src/lib.rs +++ b/primitives/order/src/lib.rs @@ -52,6 +52,9 @@ pub trait OrderInspect { /// If `None` the order was not found. fn order(order_id: &OrderId) -> Option>; + /// Returns whether an order expired or not. + fn order_expired(order_id: &Order) -> bool; + /// Remove an order with the associated id. fn remove_order(order_id: &OrderId); }