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.

Sign up for GitHub

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
Merged

Encode extrinsic call. fix #505 #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aebf390
Encode extrinsic call.
imstar15 Feb 27, 2024
8489a93
Fix compiling error in benchmarking.rs.
imstar15 Feb 29, 2024
be9234c
Fix tests
imstar15 Feb 29, 2024
a3c6000
Tranform the call into a runtime call.
imstar15 Mar 1, 2024
b7ba184
Add schedule_xcmp_task_and_check_encoded_call_success test
imstar15 Mar 1, 2024
8aa3d7b
Optimize the code related to calculating expected_encoded_call.
imstar15 Mar 1, 2024
db378ad
Remove unused code.
imstar15 Mar 2, 2024
5f38eb8
Remove get_schedule_as_from_action function.
imstar15 Mar 5, 2024
1789887
Add schedule_task_with_event function.
imstar15 Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions pallets/automation-time/src/benchmarking.rs
View file
Open in desktop
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
102 changes: 73 additions & 29 deletions pallets/automation-time/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,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 •
edited
Loading

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 +288,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 +332,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 +412,55 @@ 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(),
};

// Convert the call into a runtime call
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: schedule.clone(),
destination,
schedule_fee,
execution_fee,
encoded_call,
encoded_call_weight,
overall_weight,
schedule_as,
instruction_sequence,
};
schedule_as,
}
.into();

let schedule = schedule.validated_into::<T>()?;
// Schedule the task.
Self::schedule_task_with_event(
action,
who,
schedule.validated_into::<T>()?,
vec![],
Some(call.encode()),
)?;

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

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

Self::validate_and_schedule_task(action, who, schedule, errors)?;
Self::schedule_task_with_event(action, who, schedule, errors, None)?;

Ok(())
}

Expand All @@ -502,15 +527,16 @@ 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![])?;
Self::schedule_task_with_event(action, who, schedule, vec![], Some(encoded_call))?;

Ok(())
}

Expand Down Expand Up @@ -898,17 +924,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 +1135,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 @@ -1317,12 +1336,12 @@ pub mod pallet {

/// Validate and schedule task.
/// This will also charge the execution fee.
pub fn validate_and_schedule_task(
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 •
edited
Loading

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,12 +1370,37 @@ pub mod pallet {
Ok(task_id)
})?;

Ok(task_id)
}

/// Schedule a task with TaskScheduled event.
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(),
owner_id.clone(),
schedule,
abort_errors,
)?;

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 •
edited
Loading

Choose a reason for hiding this comment

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

Move TaskScheduled event out.

// Deposit the event.
Self::deposit_event(Event::<T>::TaskScheduled {
who: owner_id,
task_id,
schedule_as,
encoded_call,
});

Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion pallets/automation-time/src/mock.rs
View file
Open in desktop
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