Skip to content

Commit

Permalink
Cannot fulfill an expired order (#256)
Browse files Browse the repository at this point in the history
* Cannot fulfill expired order

* add test & fmt

* fix docs
  • Loading branch information
Szegoo authored Aug 23, 2024
1 parent fb8f99f commit 04e0fc1
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 19 deletions.
19 changes: 14 additions & 5 deletions pallets/orders/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
};

Expand All @@ -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);
Expand Down
27 changes: 15 additions & 12 deletions pallets/orders/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down Expand Up @@ -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(())
}
Expand All @@ -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)
}
Expand Down
4 changes: 4 additions & 0 deletions pallets/processor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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)?;

Expand Down
34 changes: 32 additions & 2 deletions pallets/processor/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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(|| {
Expand Down
3 changes: 3 additions & 0 deletions primitives/order/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit 04e0fc1

Please sign in to comment.