Encode extrinsic call. fix #505 #507

imstar15 · 2024-02-27T11:37:23Z

Update the TaskScheduled event by incorporating the encodedCall and excluding it from the TaskTriggered event.

Use the call parameter of scheduleDynamicDispatch as encodedCall.
Use the entire extrinsic of scheduleXcmpTask as encodedCall.

Tests:

Add schedule_xcmp_task_and_check_encoded_call_success test

Manual testing:

TaskTriggered event:

Decoded call:

imstar15 · 2024-03-01T15:48:11Z

pallets/automation-time/src/lib.rs

+			)?;
+
+			// Tranform the call into a runtime call and encode it.
+			let call: <T as crate::Config>::RuntimeCall = Call::schedule_xcmp_task {


Tranform the call into a runtime call and encode it.

imstar15 · 2024-03-01T15:50:33Z

pallets/automation-time/src/lib.rs

-				_ => None,
-			};
-
-			Self::deposit_event(Event::<T>::TaskScheduled { who: owner_id, task_id, schedule_as });


Move TaskScheduled event out.

imstar15 · 2024-03-01T15:51:05Z

pallets/automation-time/src/lib.rs

 		/// Validate and schedule task.
 		/// This will also charge the execution fee.
 		pub fn validate_and_schedule_task(
 			action: ActionOf<T>,
 			owner_id: AccountOf<T>,
 			schedule: Schedule,
 			abort_errors: Vec<Vec<u8>>,
-		) -> DispatchResult {
+		) -> Result<TaskIdV2, DispatchError> {


Return TaskIdV2 or DispatchError.

imstar15 · 2024-03-01T15:52:16Z

pallets/automation-time/src/lib.rs


-			let schedule = schedule.validated_into::<T>()?;
+			Self::deposit_event(Event::<T>::TaskScheduled {


Deposit TaskScheduled event.

Before this change validate_and_schedule_task does 2 things:

write the task into storage

generate the event

so the caller just need to call it, and never have to worry about manually make the the TaskSchedule event manually. But after this change, we have to remember to also call generate TaskSchedule event whenever we call validate_and_schedule_task

I had a feeling we will eventually make a mistake and forgot the second call deposit_event.

I think we should find a way to just need to write one task schedule function and it handle both for us.

maybe create a new helper function for that? and validate_and_schedule_task can be make private.

open to idea but i think it's very useful to just call that function and it does all the setup.

on thsi new approach you can see that we have to add the extra TaskSchedule everywhere after that.

If we need to throw an event in the validate_and_schedule_task function, we should pass the pre-constructed encoded_call into the validate_and_schedule_task function.

Because the encoded_call for schedule_xcmp_task needs to be constructed using the original function parameters and cannot be built within the validate_and_schedule_task function by using the action parameter passed to it.

That's why I opted to split Self::deposit_event(Event::<T>::TaskScheduled { ... }) out this function.

Hi @chrisli30 , what are your thoughts on this?

yes, I understand it.

What I mean is the reason the previous dev wrote it that way is to ensure we don't forgot.

Now with this approach, everytime we want to schedule a task, we will need to always make 2 call:

validated_and_schedule_task

deposit_event(TaskSchedule)

Therefore I think it make sense to introduce a new helper function that does:

take the right input

call validated_and_schedule_task

call deposit_event(TaskSchedule)

Make it work in a way where we cannot make mistake.

I wrote the helper function called schedule_task_with_event. What are your thoughts on it? @v9n

Pseudocode:

pub fn schedule_task_with_event( action: ActionOf<T>, owner_id: AccountOf<T>, schedule: Schedule, abort_errors: Vec<Vec<u8>>, encoded_call: Option<Vec<u8>>, ) -> DispatchResult { // Schedule the task. let task_id: TaskIdV2 = Self::validate_and_schedule_task( action.clone(), who.clone(), schedule_value, vec![], )?; Self::deposit_event(Event::<T>::TaskScheduled { who, task_id, schedule_as, encoded_call: Some(encoded_call), }); Ok(()) }

Looks good.

I would call it just schedule_task though. But leave that to your to decide.

One last request can you make validate_and_schedule_task to not public, we don't want anyone to call it directly. (inside they can still call it obviously) and instead everyone will just call schedule_task moving forward and this function will handle all the setup.

Okay!
However, the name "schedule_task" has already been used.

imstar15 · 2024-03-01T15:54:14Z

pallets/automation-time/src/tests.rs

@@ -1034,6 +1035,62 @@ fn schedule_xcmp_works() {
 	})
 }

+#[test]
+fn schedule_xcmp_task_and_check_encoded_call_success() {


Add schedule_xcmp_task_and_check_encoded_call_success test.

pallets/automation-time/src/lib.rs

imstar15 · 2024-03-02T01:19:09Z

pallets/automation-time/src/lib.rs

 			+ Dispatchable<RuntimeOrigin = Self::RuntimeOrigin, PostInfo = PostDispatchInfo>
 			+ GetDispatchInfo
 			+ From<frame_system::Call<Self>>
 			+ IsSubType<Call<Self>>
-			+ IsType<<Self as frame_system::Config>::RuntimeCall>;
+			+ IsType<<Self as frame_system::Config>::RuntimeCall>
+			+ From<crate::Call<Self>>;


Specify that RuntimeCall must implement the From<crate::Call<Self>> trait, enabling the conversion of Pallet::Call to RuntimeCall.

chrisli30

The screenshot of the encoded call looks legit. I have left one comment. @v9n , could you double check the code?

pallets/automation-time/src/lib.rs

imstar15 · 2024-03-04T05:47:06Z

What is the reason of this type re-definition?

In Substrate version 0.9.38, the definition of Call within the Pallet configuration has been renamed to RuntimeCall to distinguish between PalletCall and RuntimeCall.

Related discussion:
paritytech/substrate#11736 (comment)

PR:
paritytech/substrate#11981

v9n · 2024-03-05T01:47:29Z

pallets/automation-time/src/benchmarking.rs

@@ -217,18 +217,18 @@ benchmarks! {
 		let schedule = ScheduleParam::Fixed { execution_times: times };

 		let caller: T::AccountId = account("caller", 0, SEED);
-		let call: <T as Config>::Call = frame_system::Call::remark { remark: vec![] }.into();
+		let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into();


can you see if we need .into(). I think Rust might be able to auto call this already (I saw similar suggestion by Clippy a while ago).

I tried removing into() but encountered some errors.

error[E0308]: mismatched types --> pallets/automation-time/src/benchmarking.rs:220:42 | 220 | let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }; | -------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected associated type, found `Call<_>` | | | expected due to this | = note: expected associated type `<T as pallet::Config>::RuntimeCall` found enum `frame_system::Call<_>` = help: consider constraining the associated type `<T as pallet::Config>::RuntimeCall` to `frame_system::Call<_>` or calling a method that returns `<T as pallet::Config>::RuntimeCall` = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html help: call `Into::into` on this expression to convert `frame_system::Call<_>` into `<T as pallet::Config>::RuntimeCall` | 220 | let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into(); | +++++++ For more information about this error, try `rustc --explain E0308`. error: could not compile `pallet-automation-time` (lib) due to previous error

What do you think the correct way to write it should be?

That's fine. I was just asking.

Because I saw before in a similar PR https://github.com/OAK-Foundation/OAK-blockchain/pull/421/files this tool remove all the .into() so that's why I was just asking.

If Rust cannot infer it for this case we will need to explicitly call it. no problem.

v9n · 2024-03-05T02:09:23Z

pallets/automation-time/src/lib.rs

+			Self::deposit_event(Event::<T>::TaskScheduled {
+				who,
+				task_id,
+				schedule_as: Self::get_schedule_as_from_action(action),


why do we need to call get_schedule_as_from_action ? shouldn't we just access schedule_as variable on line 461 directly?

Yes, You're right. 👍
Fixed!

v9n

I think the code can be improved. it's a bit repetivei and there is something I'm not clear.

v9n

lgtm

Encode extrinsic call.

aebf390

imstar15 changed the title ~~[WIP]Encode extrinsic call~~ [WIP]Encode extrinsic call. fix #505 Feb 27, 2024

imstar15 added 3 commits February 29, 2024 14:08

Fix compiling error in benchmarking.rs.

8489a93

Fix tests

be9234c

Tranform the call into a runtime call.

a3c6000

imstar15 force-pushed the task-encoded-call branch from 16c0210 to a3c6000 Compare March 1, 2024 04:57

Add schedule_xcmp_task_and_check_encoded_call_success test

b7ba184

imstar15 changed the title ~~[WIP]Encode extrinsic call. fix #505~~ Encode extrinsic call. fix #505 Mar 1, 2024

imstar15 force-pushed the task-encoded-call branch from 25c2fc7 to b7ba184 Compare March 1, 2024 15:39

imstar15 commented Mar 1, 2024

View reviewed changes

imstar15 marked this pull request as ready for review March 1, 2024 15:54

Optimize the code related to calculating expected_encoded_call.

8aa3d7b

imstar15 force-pushed the task-encoded-call branch from c7ec487 to 8aa3d7b Compare March 1, 2024 15:56

imstar15 commented Mar 2, 2024

View reviewed changes

pallets/automation-time/src/lib.rs Show resolved Hide resolved

imstar15 commented Mar 2, 2024

View reviewed changes

Remove unused code.

db378ad

imstar15 requested review from v9n and chrisli30 March 4, 2024 01:00

chrisli30 reviewed Mar 4, 2024

View reviewed changes

pallets/automation-time/src/lib.rs Show resolved Hide resolved

v9n reviewed Mar 5, 2024

View reviewed changes

v9n requested changes Mar 5, 2024

View reviewed changes

Remove get_schedule_as_from_action function.

5f38eb8

imstar15 requested review from chrisli30 and v9n March 5, 2024 05:36

Add schedule_task_with_event function.

1789887

imstar15 force-pushed the task-encoded-call branch from a41fe75 to 1789887 Compare March 5, 2024 08:02

v9n approved these changes Mar 5, 2024

View reviewed changes

imstar15 merged commit ee601bb into master Mar 6, 2024
3 checks passed

imstar15 deleted the task-encoded-call branch April 26, 2024 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode extrinsic call. fix #505 #507

Encode extrinsic call. fix #505 #507

imstar15 commented Feb 27, 2024 •

edited

Loading

imstar15 Mar 1, 2024

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 1, 2024 •

edited

Loading

v9n Mar 5, 2024

imstar15 Mar 5, 2024 •

edited

Loading

v9n Mar 5, 2024

imstar15 Mar 5, 2024 •

edited

Loading

v9n Mar 5, 2024

imstar15 Mar 5, 2024

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 2, 2024 •

edited

Loading

chrisli30 left a comment

imstar15 commented Mar 4, 2024 •

edited

Loading

v9n Mar 5, 2024

imstar15 Mar 5, 2024

v9n Mar 5, 2024

v9n Mar 5, 2024

imstar15 Mar 5, 2024

v9n left a comment

v9n left a comment


		let schedule = schedule.validated_into::<T>()?;
		Self::deposit_event(Event::<T>::TaskScheduled {

Encode extrinsic call. fix #505 #507

Encode extrinsic call. fix #505 #507

Conversation

imstar15 commented Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

imstar15 Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

imstar15 Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

imstar15 Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imstar15 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imstar15 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imstar15 Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

imstar15 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

chrisli30 left a comment

Choose a reason for hiding this comment

imstar15 commented Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v9n left a comment

Choose a reason for hiding this comment

v9n left a comment

Choose a reason for hiding this comment

imstar15 commented Feb 27, 2024 •

edited

Loading

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 5, 2024 •

edited

Loading

imstar15 Mar 5, 2024 •

edited

Loading

imstar15 Mar 1, 2024 •

edited

Loading

imstar15 Mar 2, 2024 •

edited

Loading

imstar15 commented Mar 4, 2024 •

edited

Loading