Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pallet-broker] add extrinsic to remove a lease #7026

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Large diffs are not rendered by default.

Large diffs are not rendered by default.

17 changes: 17 additions & 0 deletions prdoc/pr_7026.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "[pallet-broker] add extrinsic to remove a lease"

doc:
- audience: Runtime User
description: |
A new `remove_lease` extrinsic is introduced to the broker pallet to allow a lease to be removed by the root origin.

crates:
- name: pallet-broker
bump: major
- name: coretime-rococo-runtime
bump: patch
- name: coretime-westend-runtime
bump: patch
19 changes: 19 additions & 0 deletions substrate/frame/broker/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,25 @@ mod benches {
Ok(())
}

#[benchmark]
fn remove_lease() -> Result<(), BenchmarkError> {
let task = 1u32;
let until = 10u32.into();

// Assume Leases to be almost filled for worst case
setup_leases::<T>(T::MaxLeasedCores::get(), task, until);

let origin =
T::AdminOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;

#[extrinsic_call]
_(origin as T::RuntimeOrigin, task);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the worst case would be removing the last task in the vec. Here it removes the first one.

Though in practice the cost should be negligible as MaxLeasedCores isn't so big.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, would be nice to target the worst case, especially here where it's as straightforward as just changing the task id.


assert_eq!(Leases::<T>::get().len(), T::MaxLeasedCores::get().saturating_sub(1) as usize);

Ok(())
}

#[benchmark]
fn start_sales(n: Linear<0, { MAX_CORE_COUNT.into() }>) -> Result<(), BenchmarkError> {
let config = new_config_record::<T>();
Expand Down
9 changes: 9 additions & 0 deletions substrate/frame/broker/src/dispatchable_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@ impl<T: Config> Pallet<T> {
Ok(())
}

pub(crate) fn do_remove_lease(task: TaskId) -> DispatchResult {
let mut r = Leases::<T>::get();
let i = r.iter().position(|lease| lease.task == task).ok_or(Error::<T>::LeaseNotFound)?;
ethernomad marked this conversation as resolved.
Show resolved Hide resolved
r.remove(i);
Leases::<T>::put(r);
Self::deposit_event(Event::<T>::LeaseRemoved { task });
Ok(())
}

pub(crate) fn do_start_sales(
end_price: BalanceOf<T>,
extra_cores: CoreIndex,
Expand Down
17 changes: 17 additions & 0 deletions substrate/frame/broker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,11 @@ pub mod pallet {
/// longer apply).
until: Timeslice,
},
/// A lease has been removed.
LeaseRemoved {
/// The task to which a core was assigned.
task: TaskId,
},
/// A lease is about to end.
LeaseEnding {
/// The task to which a core was assigned.
Expand Down Expand Up @@ -512,6 +517,8 @@ pub mod pallet {
TooManyReservations,
/// The maximum amount of leases has already been reached.
TooManyLeases,
/// The lease does not exist.
LeaseNotFound,
/// The revenue for the Instantaneous Core Sales of this period is not (yet) known and thus
/// this operation cannot proceed.
UnknownRevenue,
Expand Down Expand Up @@ -969,6 +976,16 @@ pub mod pallet {
Ok(Pays::No.into())
}

/// Remove a lease.
///
/// - `origin`: Must be Root or pass `AdminOrigin`.
/// - `task`: The workload of the lease which should be removed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - `task`: The workload of the lease which should be removed.
/// - `task`: The task id of the lease which should be removed.

Also from memory there can be multiple leases for a given task_id, although not how this is intended to be used in the system. I don't think it's worth complicating the extrinsic to also check timeslice etc. You should however include in the docs that it removes the first in the vector, which is not necessarily the first to end in time. I'm AFK so can't check right now, but would be good to also cover this behaviour in the test.

#[pallet::call_index(24)]
pub fn remove_lease(origin: OriginFor<T>, task: TaskId) -> DispatchResult {
T::AdminOrigin::ensure_origin_or_root(origin)?;
Self::do_remove_lease(task)
}

#[pallet::call_index(99)]
#[pallet::weight(T::WeightInfo::swap_leases())]
pub fn swap_leases(origin: OriginFor<T>, id: TaskId, other: TaskId) -> DispatchResult {
Expand Down
12 changes: 12 additions & 0 deletions substrate/frame/broker/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1205,6 +1205,18 @@ fn leases_are_limited() {
});
}

#[test]
fn remove_lease_works() {
TestExt::new().execute_with(|| {
Leases::<Test>::put(
BoundedVec::try_from(vec![LeaseRecordItem { task: 1u32, until: 10u32 }]).unwrap(),
);
assert_noop!(Broker::do_remove_lease(2), Error::<Test>::LeaseNotFound);
assert_ok!(Broker::do_remove_lease(1));
assert_noop!(Broker::do_remove_lease(1), Error::<Test>::LeaseNotFound);
});
}

#[test]
fn purchase_requires_valid_status_and_sale_info() {
TestExt::new().execute_with(|| {
Expand Down
Loading
Loading