reschedule (#6860)

* reschedule

* add tests

* Apply suggestions from code review

Co-authored-by: Dan Forbes <dan@danforbes.dev>

* add test for reschedule named perodic

* update test

* handle the case when reschedule does not change time

Co-authored-by: Dan Forbes <dan@danforbes.dev>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

substrate/frame/scheduler/src/lib.rs

@@ -186,10 +186,12 @@ decl_error! {
 	pub enum Error for Module<T: Trait> {
 		/// Failed to schedule a call
 		FailedToSchedule,
-		/// Failed to cancel a scheduled call
-		FailedToCancel,
+		/// Cannot find the scheduled call.
+		NotFound,
 		/// Given target block number is in the past.
 		TargetBlockNumberInPast,
+		/// Reschedule failed because it does not change scheduled time.
+		RescheduleNoChange,
 	}
 }
 
@@ -467,13 +469,7 @@ impl<T: Trait> Module<T> {
 		));
 	}
 
-	fn do_schedule(
-		when: DispatchTime<T::BlockNumber>,
-		maybe_periodic: Option<schedule::Period<T::BlockNumber>>,
-		priority: schedule::Priority,
-		origin: T::PalletsOrigin,
-		call: <T as Trait>::Call
-	) -> Result<TaskAddress<T::BlockNumber>, DispatchError> {
+	fn resolve_time(when: DispatchTime<T::BlockNumber>) -> Result<T::BlockNumber, DispatchError> {
 		let now = frame_system::Module::<T>::block_number();
 
 		let when = match when {
@@ -485,6 +481,18 @@ impl<T: Trait> Module<T> {
 			return Err(Error::<T>::TargetBlockNumberInPast.into())
 		}
 
+		Ok(when)
+	}
+
+	fn do_schedule(
+		when: DispatchTime<T::BlockNumber>,
+		maybe_periodic: Option<schedule::Period<T::BlockNumber>>,
+		priority: schedule::Priority,
+		origin: T::PalletsOrigin,
+		call: <T as Trait>::Call
+	) -> Result<TaskAddress<T::BlockNumber>, DispatchError> {
+		let when = Self::resolve_time(when)?;
+
 		// sanitize maybe_periodic
 		let maybe_periodic = maybe_periodic
 			.filter(|p| p.1 > 1 && !p.0.is_zero())
@@ -531,10 +539,34 @@ impl<T: Trait> Module<T> {
 			Self::deposit_event(RawEvent::Canceled(when, index));
 			Ok(())
 		} else {
-			Err(Error::<T>::FailedToCancel)?
+			Err(Error::<T>::NotFound)?
 		}
 	}
 
+	fn do_reschedule(
+		(when, index): TaskAddress<T::BlockNumber>,
+		new_time: DispatchTime<T::BlockNumber>,
+	) -> Result<TaskAddress<T::BlockNumber>, DispatchError> {
+		let new_time = Self::resolve_time(new_time)?;
+
+		if new_time == when {
+			return Err(Error::<T>::RescheduleNoChange.into());
+		}
+
+		Agenda::<T>::try_mutate(when, |agenda| -> DispatchResult {
+			let task = agenda.get_mut(index as usize).ok_or(Error::<T>::NotFound)?;
+			let task = task.take().ok_or(Error::<T>::NotFound)?;
+			Agenda::<T>::append(new_time, Some(task));
+			Ok(())
+		})?;
+
+		let new_index = Agenda::<T>::decode_len(new_time).unwrap_or(1) as u32 - 1;
+		Self::deposit_event(RawEvent::Canceled(when, index));
+		Self::deposit_event(RawEvent::Scheduled(new_time, new_index));
+
+		Ok((new_time, new_index))
+	}
+
 	fn do_schedule_named(
 		id: Vec<u8>,
 		when: DispatchTime<T::BlockNumber>,
@@ -548,16 +580,7 @@ impl<T: Trait> Module<T> {
 			return Err(Error::<T>::FailedToSchedule)?
 		}
 
-		let now = frame_system::Module::<T>::block_number();
-
-		let when = match when {
-			DispatchTime::At(x) => x,
-			DispatchTime::After(x) => now.saturating_add(x)
-		};
-
-		if when <= now {
-			return Err(Error::<T>::TargetBlockNumberInPast.into())
-		}
+		let when = Self::resolve_time(when)?;
 
 		// sanitize maybe_periodic
 		let maybe_periodic = maybe_periodic
@@ -601,10 +624,41 @@ impl<T: Trait> Module<T> {
 				Self::deposit_event(RawEvent::Canceled(when, index));
 				Ok(())
 			} else {
-				Err(Error::<T>::FailedToCancel)?
+				Err(Error::<T>::NotFound)?
 			}
 		})
 	}
+
+	fn do_reschedule_named(
+		id: Vec<u8>,
+		new_time: DispatchTime<T::BlockNumber>,
+	) -> Result<TaskAddress<T::BlockNumber>, DispatchError> {
+		let new_time = Self::resolve_time(new_time)?;
+
+		Lookup::<T>::try_mutate_exists(id, |lookup| -> Result<TaskAddress<T::BlockNumber>, DispatchError> {
+			let (when, index) = lookup.ok_or(Error::<T>::NotFound)?;
+
+			if new_time == when {
+				return Err(Error::<T>::RescheduleNoChange.into());
+			}
+
+			Agenda::<T>::try_mutate(when, |agenda| -> DispatchResult {
+				let task = agenda.get_mut(index as usize).ok_or(Error::<T>::NotFound)?;
+				let task = task.take().ok_or(Error::<T>::NotFound)?;
+				Agenda::<T>::append(new_time, Some(task));
+
+				Ok(())
+			})?;
+
+			let new_index = Agenda::<T>::decode_len(new_time).unwrap_or(1) as u32 - 1;
+			Self::deposit_event(RawEvent::Canceled(when, index));
+			Self::deposit_event(RawEvent::Scheduled(new_time, new_index));
+
+			*lookup = Some((new_time, new_index));
+
+			Ok((new_time, new_index))
+		})
+	}
 }
 
 impl<T: Trait> schedule::Anon<T::BlockNumber, <T as Trait>::Call, T::PalletsOrigin> for Module<T> {
@@ -623,6 +677,17 @@ impl<T: Trait> schedule::Anon<T::BlockNumber, <T as Trait>::Call, T::PalletsOrig
 	fn cancel((when, index): Self::Address) -> Result<(), ()> {
 		Self::do_cancel(None, (when, index)).map_err(|_| ())
 	}
+
+	fn reschedule(
+		address: Self::Address,
+		when: DispatchTime<T::BlockNumber>,
+	) -> Result<Self::Address, DispatchError> {
+		Self::do_reschedule(address, when)
+	}
+
+	fn next_dispatch_time((when, index): Self::Address) -> Result<T::BlockNumber, ()> {
+		Agenda::<T>::get(when).get(index as usize).ok_or(()).map(|_| when)
+	}
 }
 
 impl<T: Trait> schedule::Named<T::BlockNumber, <T as Trait>::Call, T::PalletsOrigin> for Module<T> {
@@ -642,6 +707,17 @@ impl<T: Trait> schedule::Named<T::BlockNumber, <T as Trait>::Call, T::PalletsOri
 	fn cancel_named(id: Vec<u8>) -> Result<(), ()> {
 		Self::do_cancel_named(None, id).map_err(|_| ())
 	}
+
+	fn reschedule_named(
+		id: Vec<u8>,
+		when: DispatchTime<T::BlockNumber>,
+	) -> Result<Self::Address, DispatchError> {
+		Self::do_reschedule_named(id, when)
+	}
+
+	fn next_dispatch_time(id: Vec<u8>) -> Result<T::BlockNumber, ()> {
+		Lookup::<T>::get(id).and_then(|(when, index)| Agenda::<T>::get(when).get(index as usize).map(|_| when)).ok_or(())
+	}
 }
 
 #[cfg(test)]
@@ -868,6 +944,95 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn reschedule_works() {
+		new_test_ext().execute_with(|| {
+			let call = Call::Logger(logger::Call::log(42, 1000));
+			assert!(!<Test as frame_system::Trait>::BaseCallFilter::filter(&call));
+			assert_eq!(Scheduler::do_schedule(DispatchTime::At(4), None, 127, root(), call).unwrap(), (4, 0));
+
+			run_to_block(3);
+			assert!(logger::log().is_empty());
+
+			assert_eq!(Scheduler::do_reschedule((4, 0), DispatchTime::At(6)).unwrap(), (6, 0));
+
+			assert_noop!(Scheduler::do_reschedule((6, 0), DispatchTime::At(6)), Error::<Test>::RescheduleNoChange);
+
+			run_to_block(4);
+			assert!(logger::log().is_empty());
+
+			run_to_block(6);
+			assert_eq!(logger::log(), vec![(root(), 42u32)]);
+
+			run_to_block(100);
+			assert_eq!(logger::log(), vec![(root(), 42u32)]);
+		});
+	}
+
+	#[test]
+	fn reschedule_named_works() {
+		new_test_ext().execute_with(|| {
+			let call = Call::Logger(logger::Call::log(42, 1000));
+			assert!(!<Test as frame_system::Trait>::BaseCallFilter::filter(&call));
+			assert_eq!(Scheduler::do_schedule_named(
+				1u32.encode(), DispatchTime::At(4), None, 127, root(), call
+			).unwrap(), (4, 0));
+
+			run_to_block(3);
+			assert!(logger::log().is_empty());
+
+			assert_eq!(Scheduler::do_reschedule_named(1u32.encode(), DispatchTime::At(6)).unwrap(), (6, 0));
+
+			assert_noop!(Scheduler::do_reschedule_named(1u32.encode(), DispatchTime::At(6)), Error::<Test>::RescheduleNoChange);
+
+			run_to_block(4);
+			assert!(logger::log().is_empty());
+
+			run_to_block(6);
+			assert_eq!(logger::log(), vec![(root(), 42u32)]);
+
+			run_to_block(100);
+			assert_eq!(logger::log(), vec![(root(), 42u32)]);
+		});
+	}
+
+	#[test]
+	fn reschedule_named_perodic_works() {
+		new_test_ext().execute_with(|| {
+			let call = Call::Logger(logger::Call::log(42, 1000));
+			assert!(!<Test as frame_system::Trait>::BaseCallFilter::filter(&call));
+			assert_eq!(Scheduler::do_schedule_named(
+				1u32.encode(), DispatchTime::At(4), Some((3, 3)), 127, root(), call
+			).unwrap(), (4, 0));
+
+			run_to_block(3);
+			assert!(logger::log().is_empty());
+
+			assert_eq!(Scheduler::do_reschedule_named(1u32.encode(), DispatchTime::At(5)).unwrap(), (5, 0));
+			assert_eq!(Scheduler::do_reschedule_named(1u32.encode(), DispatchTime::At(6)).unwrap(), (6, 0));
+
+			run_to_block(5);
+			assert!(logger::log().is_empty());
+
+			run_to_block(6);
+			assert_eq!(logger::log(), vec![(root(), 42u32)]);
+
+			assert_eq!(Scheduler::do_reschedule_named(1u32.encode(), DispatchTime::At(10)).unwrap(), (10, 0));
+
+			run_to_block(9);
+			assert_eq!(logger::log(), vec![(root(), 42u32)]);
+
+			run_to_block(10);
+			assert_eq!(logger::log(), vec![(root(), 42u32), (root(), 42u32)]);
+
+			run_to_block(13);
+			assert_eq!(logger::log(), vec![(root(), 42u32), (root(), 42u32), (root(), 42u32)]);
+
+			run_to_block(100);
+			assert_eq!(logger::log(), vec![(root(), 42u32), (root(), 42u32), (root(), 42u32)]);
+		});
+	}
+
 	#[test]
 	fn cancel_named_scheduling_works_with_normal_cancel() {
 		new_test_ext().execute_with(|| {

substrate/frame/support/src/traits.rs

@@ -1529,11 +1529,9 @@ pub mod schedule {
 		/// An address which can be used for removing a scheduled task.
 		type Address: Codec + Clone + Eq + EncodeLike + Debug;
 
-		/// Schedule a one-off dispatch to happen at the beginning of some block in the future.
+		/// Schedule a dispatch to happen at the beginning of some block in the future.
 		///
 		/// This is not named.
-		///
-		/// Infallible.
 		fn schedule(
 			when: DispatchTime<BlockNumber>,
 			maybe_periodic: Option<Period<BlockNumber>>,
@@ -1553,6 +1551,22 @@ pub mod schedule {
 		/// NOTE2: This will not work to cancel periodic tasks after their initial execution. For
 		/// that, you must name the task explicitly using the `Named` trait.
 		fn cancel(address: Self::Address) -> Result<(), ()>;
+
+		/// Reschedule a task. For one-off tasks, this dispatch is guaranteed to succeed
+		/// only if it is executed *before* the currently scheduled block. For periodic tasks,
+		/// this dispatch is guaranteed to succeed only before the *initial* execution; for
+		/// others, use `reschedule_named`. 
+		///
+		/// Will return an error if the `address` is invalid.
+		fn reschedule(
+			address: Self::Address,
+			when: DispatchTime<BlockNumber>,
+		) -> Result<Self::Address, DispatchError>;
+
+		/// Return the next dispatch time for a given task.
+		///
+		/// Will return an error if the `address` is invalid.
+		fn next_dispatch_time(address: Self::Address) -> Result<BlockNumber, ()>;
 	}
 
 	/// A type that can be used as a scheduler.
@@ -1560,7 +1574,7 @@ pub mod schedule {
 		/// An address which can be used for removing a scheduled task.
 		type Address: Codec + Clone + Eq + EncodeLike + sp_std::fmt::Debug;
 
-		/// Schedule a one-off dispatch to happen at the beginning of some block in the future.
+		/// Schedule a dispatch to happen at the beginning of some block in the future.
 		///
 		/// - `id`: The identity of the task. This must be unique and will return an error if not.
 		fn schedule_named(
@@ -1580,6 +1594,18 @@ pub mod schedule {
 		/// NOTE: This guaranteed to work only *before* the point that it is due to be executed.
 		/// If it ends up being delayed beyond the point of execution, then it cannot be cancelled.
 		fn cancel_named(id: Vec<u8>) -> Result<(), ()>;
+
+		/// Reschedule a task. For one-off tasks, this dispatch is guaranteed to succeed
+		/// only if it is executed *before* the currently scheduled block.
+		fn reschedule_named(
+			id: Vec<u8>,
+			when: DispatchTime<BlockNumber>,
+		) -> Result<Self::Address, DispatchError>;
+
+		/// Return the next dispatch time for a given task.
+		///
+		/// Will return an error if the `id` is invalid.
+		fn next_dispatch_time(id: Vec<u8>) -> Result<BlockNumber, ()>;
 	}
 }