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

Encode extrinsic call. fix #505 #507

Merged
merged 9 commits into from
Mar 6, 2024
14 changes: 7 additions & 7 deletions pallets/automation-time/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.


let account_min = T::Currency::minimum_balance().saturating_mul(ED_MULTIPLIER.into());
T::Currency::deposit_creating(&caller, account_min.saturating_mul(DEPOSIT_MULTIPLIER.into()));

let initial_event_count = frame_system::Pallet::<T>::event_count() as u8;
}: schedule_dynamic_dispatch_task(RawOrigin::Signed(caller.clone()), schedule, Box::new(call))
}: schedule_dynamic_dispatch_task(RawOrigin::Signed(caller.clone()), schedule, Box::new(call.clone()))
verify {
{
//panic!("events count: {:?} final {:?} data:{:?}", initial_event_count, frame_system::Pallet::<T>::event_count(), frame_system::Pallet::<T>::events());
// the task id schedule on first block, first extrinsics, but depend on which path in unitest or in benchmark the initial_event_count might be different therefore we get it from the initial state
assert_last_event::<T>(Event::TaskScheduled { who: caller, schedule_as: None, task_id: format!("1-0-{:?}", initial_event_count).as_bytes().to_vec(), }.into())
assert_last_event::<T>(Event::TaskScheduled { who: caller, schedule_as: None, task_id: format!("1-0-{:?}", initial_event_count).as_bytes().to_vec(), encoded_call: Some(call.encode()) }.into())
}
}

Expand All @@ -243,18 +243,18 @@ benchmarks! {
let schedule = ScheduleParam::Fixed { execution_times: times.clone() };

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();

let account_min = T::Currency::minimum_balance().saturating_mul(ED_MULTIPLIER.into());
T::Currency::deposit_creating(&caller, account_min.saturating_mul(DEPOSIT_MULTIPLIER.into()));

// Almost fill up all time slots
schedule_notify_tasks::<T>(caller.clone(), times, T::MaxTasksPerSlot::get() - 1);
let initial_event_count = frame_system::Pallet::<T>::event_count() as u8;
}: schedule_dynamic_dispatch_task(RawOrigin::Signed(caller.clone()), schedule, Box::new(call))
}: schedule_dynamic_dispatch_task(RawOrigin::Signed(caller.clone()), schedule, Box::new(call.clone()))
verify {
// the task id schedule on first block, first extrinsics, but depend on which path in unitest or in benchmark the initial_event_count might be different therefore we get it from the initial state
assert_last_event::<T>(Event::TaskScheduled { who: caller, schedule_as: None, task_id: format!("1-0-{:?}", initial_event_count).as_bytes().to_vec(), }.into())
assert_last_event::<T>(Event::TaskScheduled { who: caller, schedule_as: None, task_id: format!("1-0-{:?}", initial_event_count).as_bytes().to_vec(), encoded_call: Some(call.encode()) }.into())
}

cancel_scheduled_task_full {
Expand Down Expand Up @@ -347,7 +347,7 @@ benchmarks! {
run_dynamic_dispatch_action {
let caller: T::AccountId = account("caller", 0, SEED);
let task_id = vec![49, 45, 48, 45, 52];
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();
let encoded_call = call.encode();
}: {
let (_, error) = AutomationTime::<T>::run_dynamic_dispatch_action(caller.clone(), encoded_call);
Expand Down
114 changes: 79 additions & 35 deletions pallets/automation-time/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ use frame_system::pallet_prelude::*;
use orml_traits::{location::Reserve, FixedConversionRateProvider, MultiCurrency};
use pallet_parachain_staking::DelegatorActions;
use pallet_timestamp::{self as timestamp};
// use crate::pallet_automation_time;
pub use pallet_xcmp_handler::InstructionSequence;
use pallet_xcmp_handler::XcmpTransactor;
use primitives::EnsureProxy;
Expand Down Expand Up @@ -173,12 +174,13 @@ pub mod pallet {
type DelegatorActions: DelegatorActions<Self::AccountId, BalanceOf<Self>>;

/// The overarching call type.
type Call: Parameter
type RuntimeCall: Parameter
chrisli30 marked this conversation as resolved.
Show resolved Hide resolved
+ 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>>;
Copy link
Member Author

@imstar15 imstar15 Mar 2, 2024

Choose a reason for hiding this comment

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

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


type ScheduleAllowList: Contains<<Self as frame_system::Config>::RuntimeCall>;

Expand Down Expand Up @@ -287,6 +289,7 @@ pub mod pallet {
who: AccountOf<T>,
task_id: TaskIdV2,
schedule_as: Option<AccountOf<T>>,
encoded_call: Option<Vec<u8>>,
},
/// Cancelled a task.
TaskCancelled {
Expand Down Expand Up @@ -330,7 +333,6 @@ pub mod pallet {
who: AccountOf<T>,
task_id: TaskIdV2,
condition: BTreeMap<Vec<u8>, Vec<u8>>,
encoded_call: Option<Vec<u8>>,
},
TaskExecuted {
who: AccountOf<T>,
Expand Down Expand Up @@ -411,32 +413,64 @@ pub mod pallet {
T::EnsureProxy::ensure_ok(schedule_as_account, who.clone())?;
}

let destination =
MultiLocation::try_from(*destination).map_err(|()| Error::<T>::BadVersion)?;
let schedule_fee =
MultiLocation::try_from(*schedule_fee).map_err(|()| Error::<T>::BadVersion)?;
let destination_location = MultiLocation::try_from(*destination.clone())
.map_err(|()| Error::<T>::BadVersion)?;
let schedule_fee_location = MultiLocation::try_from(*schedule_fee.clone())
.map_err(|()| Error::<T>::BadVersion)?;

let execution_fee: AssetPayment = *execution_fee;
let execution_fee_payment: AssetPayment = *execution_fee.clone();
let execution_fee_location =
MultiLocation::try_from(execution_fee.clone().asset_location)
MultiLocation::try_from(execution_fee_payment.clone().asset_location)
.map_err(|()| Error::<T>::BadVersion)?;

Self::ensure_supported_execution_fee_location(&execution_fee_location, &destination)?;
Self::ensure_supported_execution_fee_location(
&execution_fee_location,
&destination_location,
)?;

let action = Action::XCMP {
destination: destination_location,
schedule_fee: schedule_fee_location,
execution_fee: execution_fee_payment,
encoded_call: encoded_call.clone(),
encoded_call_weight: encoded_call_weight.clone(),
overall_weight: overall_weight.clone(),
schedule_as: schedule_as.clone(),
instruction_sequence: instruction_sequence.clone(),
};

let schedule_value = schedule.clone().validated_into::<T>()?;

// Schedule the task.
let task_id: TaskIdV2 = Self::validate_and_schedule_task(
action.clone(),
who.clone(),
schedule_value,
vec![],
)?;

// Tranform the call into a runtime call and encode it.
let call: <T as crate::Config>::RuntimeCall = Call::schedule_xcmp_task {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tranform the call into a runtime call and encode it.

schedule,
destination,
schedule_fee,
execution_fee,
encoded_call,
encoded_call_weight,
overall_weight,
schedule_as,
instruction_sequence,
};
schedule_as,
}
.into();
let encoded_call = call.encode();

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

@imstar15 imstar15 Mar 1, 2024

Choose a reason for hiding this comment

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

Deposit TaskScheduled event.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@imstar15 imstar15 Mar 5, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. validated_and_schedule_task
  2. deposit_event(TaskSchedule)

CleanShot 2024-03-04 at 21 34 59

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

  1. take the right input
  2. call validated_and_schedule_task
  3. call deposit_event(TaskSchedule)

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

Copy link
Member Author

@imstar15 imstar15 Mar 5, 2024

Choose a reason for hiding this comment

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

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(())
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

who,
task_id,
schedule_as: Self::get_schedule_as_from_action(action),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, You're right. 👍
Fixed!

encoded_call: Some(encoded_call),
});

Self::validate_and_schedule_task(action, who, schedule, vec![])?;
Ok(())
}

Expand Down Expand Up @@ -480,7 +514,15 @@ pub mod pallet {
.map(|&error| error.as_bytes().to_vec())
.collect();

Self::validate_and_schedule_task(action, who, schedule, errors)?;
let task_id: TaskIdV2 =
Self::validate_and_schedule_task(action.clone(), who.clone(), schedule, errors)?;
Self::deposit_event(Event::<T>::TaskScheduled {
who,
task_id,
schedule_as: Self::get_schedule_as_from_action(action),
encoded_call: None,
});

Ok(())
}

Expand All @@ -502,15 +544,23 @@ pub mod pallet {
pub fn schedule_dynamic_dispatch_task(
origin: OriginFor<T>,
schedule: ScheduleParam,
call: Box<<T as Config>::Call>,
call: Box<<T as Config>::RuntimeCall>,
) -> DispatchResult {
let who = ensure_signed(origin)?;

let encoded_call = call.encode();
let action = Action::DynamicDispatch { encoded_call };
let action = Action::DynamicDispatch { encoded_call: encoded_call.clone() };
let schedule = schedule.validated_into::<T>()?;

Self::validate_and_schedule_task(action, who, schedule, vec![])?;
let task_id: TaskIdV2 =
Self::validate_and_schedule_task(action.clone(), who.clone(), schedule, vec![])?;
Self::deposit_event(Event::<T>::TaskScheduled {
who,
task_id,
schedule_as: Self::get_schedule_as_from_action(action),
encoded_call: Some(encoded_call),
});

Ok(())
}

Expand Down Expand Up @@ -898,17 +948,10 @@ pub mod pallet {
format!("{}", time_slot).into_bytes(),
);

let encoded_call = match task.action.clone() {
Action::XCMP { encoded_call, .. } => Some(encoded_call),
Action::DynamicDispatch { encoded_call } => Some(encoded_call),
_ => None,
};

Self::deposit_event(Event::TaskTriggered {
who: account_id.clone(),
task_id: task_id.clone(),
condition,
encoded_call,
});

let (task_action_weight, dispatch_error) = match task.action.clone() {
Expand Down Expand Up @@ -1116,7 +1159,7 @@ pub mod pallet {
caller: AccountOf<T>,
encoded_call: Vec<u8>,
) -> (Weight, Option<DispatchError>) {
match <T as Config>::Call::decode(&mut &*encoded_call) {
match <T as Config>::RuntimeCall::decode(&mut &*encoded_call) {
Ok(scheduled_call) => {
let mut dispatch_origin: T::RuntimeOrigin =
frame_system::RawOrigin::Signed(caller).into();
Expand Down Expand Up @@ -1315,14 +1358,22 @@ pub mod pallet {
.map_err(|_| Error::<T>::TimeSlotFull)
}

/// Get schedule_as from action
pub fn get_schedule_as_from_action(action: ActionOf<T>) -> Option<T::AccountId> {
match action {
Action::XCMP { schedule_as, .. } => schedule_as,
_ => None,
}
}

/// 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> {
Copy link
Member Author

@imstar15 imstar15 Mar 1, 2024

Choose a reason for hiding this comment

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

Return TaskIdV2 or DispatchError.

match action.clone() {
Action::XCMP { execution_fee, .. } => {
let asset_location = MultiLocation::try_from(execution_fee.asset_location)
Expand Down Expand Up @@ -1351,14 +1402,7 @@ pub mod pallet {
Ok(task_id)
})?;

let schedule_as = match action {
Action::XCMP { schedule_as, .. } => schedule_as,
_ => None,
};

Self::deposit_event(Event::<T>::TaskScheduled { who: owner_id, task_id, schedule_as });
Copy link
Member Author

@imstar15 imstar15 Mar 1, 2024

Choose a reason for hiding this comment

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

Move TaskScheduled event out.


Ok(())
Ok(task_id)
}

fn reschedule_or_remove_task(mut task: TaskOf<T>, dispatch_error: Option<DispatchError>) {
Expand Down
2 changes: 1 addition & 1 deletion pallets/automation-time/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ impl pallet_automation_time::Config for Test {
type FeeHandler = FeeHandler<Test, ()>;
type DelegatorActions = MockDelegatorActions<Test, Balances>;
type XcmpTransactor = MockXcmpTransactor<Test, Balances>;
type Call = RuntimeCall;
type RuntimeCall = RuntimeCall;
type ScheduleAllowList = ScheduleAllowList;
type CurrencyIdConvert = MockTokenIdConvert;
type FeeConversionRateProvider = MockConversionRateProvider;
Expand Down
Loading
Loading