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

Jump to bottom

Add schedule_as into TaskScheduled event #365

Merged
merged 8 commits into from
Jul 7, 2023
Merged

Add schedule_as into TaskScheduled event #365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
edc25f7
Add schedule_as into TaskScheduled event
imstar15 Jun 6, 2023
c8ae6c1
Add schedule_as to TaskRescheduled event
imstar15 Jul 4, 2023
5f75496
Add schedule_xcmp_through_proxy_works test
imstar15 Jul 4, 2023
e841538
Fix schedule_xcmp_through_proxy_works test
imstar15 Jul 6, 2023
1ce9c2c
Add schedule_xcmp_through_proxy_same_as_user_account test
imstar15 Jul 6, 2023
6ff5f4d
Rename user_account to delegator_account
imstar15 Jul 7, 2023
f271544
Merge branch 'master' into add-schedule-as-into-taskscheduled-event
imstar15 Jul 7, 2023
d380f8b
Modify the expect text in schedule_xcmp_through_proxy_works test
imstar15 Jul 7, 2023
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
4 changes: 2 additions & 2 deletions pallets/automation-time/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ benchmarks! {
let task_id = Pallet::<T>::generate_task_id(caller.clone(), provided_id.clone());
}: schedule_dynamic_dispatch_task(RawOrigin::Signed(caller.clone()), provided_id, schedule, Box::new(call))
verify {
assert_last_event::<T>(Event::TaskScheduled { who: caller, task_id: task_id }.into())
assert_last_event::<T>(Event::TaskScheduled { who: caller, task_id: task_id, schedule_as: None }.into())
}

schedule_dynamic_dispatch_task_full {
Expand All @@ -285,7 +285,7 @@ benchmarks! {
schedule_notify_tasks::<T>(caller.clone(), times, T::MaxTasksPerSlot::get() - 1);
}: schedule_dynamic_dispatch_task(RawOrigin::Signed(caller.clone()), provided_id, schedule, Box::new(call))
verify {
assert_last_event::<T>(Event::TaskScheduled { who: caller, task_id: task_id }.into())
assert_last_event::<T>(Event::TaskScheduled { who: caller, task_id: task_id, schedule_as: None }.into())
chrisli30 marked this conversation as resolved.
Show resolved Hide resolved
}

cancel_scheduled_task_full {
Expand Down
34 changes: 27 additions & 7 deletions pallets/automation-time/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ pub mod pallet {
TaskScheduled {
who: AccountOf<T>,
task_id: TaskId<T>,
schedule_as: Option<AccountOf<T>>,
},
/// Cancelled a task.
TaskCancelled {
Expand Down Expand Up @@ -332,6 +333,7 @@ pub mod pallet {
TaskRescheduled {
who: AccountOf<T>,
task_id: TaskId<T>,
schedule_as: Option<AccountOf<T>>,
},
/// A recurring task was not rescheduled
TaskNotRescheduled {
Expand Down Expand Up @@ -1347,12 +1349,12 @@ pub mod pallet {
Ok(task_id)
})?;

let scheduler = match action {
Action::XCMP { schedule_as, .. } => schedule_as.unwrap_or(owner_id),
_ => owner_id,
let schedule_as = match action {
Action::XCMP { schedule_as, .. } => schedule_as,
_ => None,
};

Self::deposit_event(Event::<T>::TaskScheduled { who: scheduler, task_id });
Self::deposit_event(Event::<T>::TaskScheduled { who: owner_id, task_id, schedule_as });
Ok(())
}

Expand All @@ -1370,9 +1372,18 @@ pub mod pallet {
AccountTasks::<T>::remove(task.owner_id.clone(), task_id);
} else {
let owner_id = task.owner_id.clone();
let action = task.action.clone();
match Self::reschedule_existing_task(task_id, &mut task) {
Ok(_) => {
Self::deposit_event(Event::<T>::TaskRescheduled { who: owner_id, task_id });
let schedule_as = match action {
Action::XCMP { schedule_as, .. } => schedule_as,
_ => None,
};
Self::deposit_event(Event::<T>::TaskRescheduled {
who: owner_id,
task_id,
schedule_as,
});
},
Err(err) => {
Self::deposit_event(Event::<T>::TaskFailedToReschedule {
Expand Down Expand Up @@ -1401,9 +1412,18 @@ pub mod pallet {
})?;

let owner_id = task.owner_id.clone();
AccountTasks::<T>::insert(owner_id.clone(), task_id, task);
AccountTasks::<T>::insert(owner_id.clone(), task_id, task.clone());

Self::deposit_event(Event::<T>::TaskScheduled { who: owner_id, task_id });
let schedule_as = match task.action.clone() {
Action::XCMP { schedule_as, .. } => schedule_as.clone(),
_ => None,
};

Self::deposit_event(Event::<T>::TaskScheduled {
who: owner_id,
task_id,
schedule_as,
});
},
Schedule::Fixed { .. } => {},
}
Expand Down
9 changes: 8 additions & 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 @@ -42,6 +42,9 @@ pub type CurrencyId = u32;

pub const ALICE: [u8; 32] = [1u8; 32];
pub const BOB: [u8; 32] = [2u8; 32];
pub const DELEGATOR_ACCOUNT: [u8; 32] = [3u8; 32];
pub const PROXY_ACCOUNT: [u8; 32] = [4u8; 32];

pub const PARA_ID: u32 = 2000;
pub const NATIVE: CurrencyId = 0;

Expand Down Expand Up @@ -351,7 +354,11 @@ impl Convert<CurrencyId, Option<MultiLocation>> for MockTokenIdConvert {
pub struct MockEnsureProxy;
impl EnsureProxy<AccountId> for MockEnsureProxy {
fn ensure_ok(_delegator: AccountId, _delegatee: AccountId) -> Result<(), &'static str> {
Ok(())
if _delegator == DELEGATOR_ACCOUNT.into() && _delegatee == PROXY_ACCOUNT.into() {
Ok(())
} else {
Err("proxy error: expected `ProxyType::Any`")
}
chrisli30 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
74 changes: 73 additions & 1 deletion pallets/automation-time/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,77 @@ fn schedule_xcmp_works() {
})
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

We need to include more tests around this function. For example,

  1. schedule through an empty wallet address(empty string).
  2. schedule through an invalid Turing wallet address.
  3. schedule through a wallet address that is the same as the owner wallet.
  4. schedule with an invalid multi-location
  5. schedule with a non-existent multi-location
  6. Could verify that the task can execute with a correct proxy?

Copy link
Member Author

@imstar15 imstar15 Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

1. schedule through an empty wallet address(empty string).
The AccountId32::new(bytes) method requires a byte array parameter with 32 elements. An empty wallet address cannot be used to construct an AccountId32 variable.

let user_account = AccountId32::new([]);
    |                            ---------------- ^^ expected an array with a fixed size of 32 elements, found one with 0 elements
    |                            |
    |                            arguments to this function are incorrect
    |
    = note: expected array `[u8; 32]`
               found array `[_; 0]`

2. schedule through an invalid Turing wallet address.
The code within the automation time pallet does not perform validation checks on the account addresses.

Account address validation is a feature provided by the Substrate framework, which verifies the checksum of user addresses during actual runtime. I haven't verified this aspect yet.

However, in functional testing, incorrect account addresses will not result in an error.

schedule_as is a AccountId type parameter.

If you create an scheduleXcmpTaskThroughProxy extrinsic with polkadot.js API on Turing chain with other ss58 prefix address, polkadot.js API will convert it to a Turing address.

YFbVAJHJbn5sH523f3o6iLeropaD5CREtocrkPaGVzpJ5iz will be converted to 68TwNoCpyz1X3ygMi9WtUAaCb8Q6jWAMvAHfAByRZqMFEtJG

If the conversion fails, an error will be thrown:

Struct: failed on schedule_as: AccountId:: Decoding YFbVAJHJbn5sH523f3o6iLeropaD5CREtocrkPaGVzpJ222: Invalid decoded address checksum.

4. schedule with an invalid multi-location
5. schedule with a non-existent multi-location

The automation time pallet does not check on whether destination parameter is a valid MultiLocation, as that responsibility lies within the XCM module.

Alternatively, end-to-end (E2E) tests can be done to verify it.

The automation-time pallet does not check xcm_asset_location parameter.
Because the calculation logic of XCM instruction fee is in the xcmp-handler pallet.
In the mock implementation, we do not provide real functionality.

https://github.com/OAK-Foundation/OAK-blockchain/blob/67b4f7f5976d58dd35b080cf325eb01dff4921a7/pallets/automation-time/src/mock.rs#L285C33-L285C47

6. Could verify that the task can execute with a correct proxy?
The MockEnsureProxy is only used for automation-time testing purposes. However, it is not a fully functional proxy module in a real sense.

https://github.com/OAK-Foundation/OAK-blockchain/blob/e8415385756db9daf29cf12cb2ae49e2772d4c7d/pallets/automation-time/src/mock.rs#L352

Copy link
Member

Choose a reason for hiding this comment

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

  1. a wrong xcm_asset_location for destination doesn’t cause error on our XcmMessageSent, but causing one on relay chain instead. Therefore, we can’t test the validity in our chain’s code.

fn schedule_xcmp_through_proxy_works() {
new_test_ext(START_BLOCK_TIME).execute_with(|| {
let provided_id = vec![50];
let delegator_account = AccountId32::new(DELEGATOR_ACCOUNT);
let proxy_account = AccountId32::new(PROXY_ACCOUNT);
let call: Vec<u8> = vec![2, 4, 5];

// Funds including XCM fees
get_xcmp_funds(proxy_account.clone());

assert_ok!(AutomationTime::schedule_xcmp_task_through_proxy(
RuntimeOrigin::signed(proxy_account.clone()),
provided_id,
ScheduleParam::Fixed { execution_times: vec![SCHEDULED_TIME] },
PARA_ID.try_into().unwrap(),
NATIVE,
MultiLocation::new(1, X1(Parachain(PARA_ID.into()))).into(),
call.clone(),
Weight::from_ref_time(100_000),
delegator_account.clone(),
));
chrisli30 marked this conversation as resolved.
Show resolved Hide resolved

let tasks = AutomationTime::get_scheduled_tasks(SCHEDULED_TIME);
assert_eq!(tasks.is_some(), true);

let tasks = tasks.unwrap();
assert_eq!(tasks.tasks[0].0, proxy_account.clone());

// Find the TaskScheduled event in the event list and verify if the who within it is correct.
events()
chrisli30 marked this conversation as resolved.
Show resolved Hide resolved
.into_iter()
.find(|e| match e {
RuntimeEvent::AutomationTime(crate::Event::TaskScheduled {
who,
schedule_as,
..
}) if *who == proxy_account && *schedule_as == Some(delegator_account.clone()) => true,
_ => false,
})
.expect("TaskScheduled event should emit with 'who' being proxy_account, and 'schedule_as' being delegator_account.");
})
}

#[test]
fn schedule_xcmp_through_proxy_same_as_delegator_account() {
new_test_ext(START_BLOCK_TIME).execute_with(|| {
let provided_id = vec![50];
let delegator_account = AccountId32::new(ALICE);
let call: Vec<u8> = vec![2, 4, 5];

// Funds including XCM fees
get_xcmp_funds(delegator_account.clone());

assert_noop!(
AutomationTime::schedule_xcmp_task_through_proxy(
RuntimeOrigin::signed(delegator_account.clone()),
provided_id,
ScheduleParam::Fixed { execution_times: vec![SCHEDULED_TIME] },
PARA_ID.try_into().unwrap(),
NATIVE,
MultiLocation::new(1, X1(Parachain(PARA_ID.into()))).into(),
call.clone(),
Weight::from_ref_time(100_000),
delegator_account.clone(),
),
sp_runtime::DispatchError::Other("proxy error: expected `ProxyType::Any`"),
chrisli30 marked this conversation as resolved.
Show resolved Hide resolved
);
})
}

#[test]
fn schedule_xcmp_fails_if_not_enough_funds() {
new_test_ext(START_BLOCK_TIME).execute_with(|| {
Expand Down Expand Up @@ -1024,7 +1095,8 @@ mod extrinsics {
last_event(),
RuntimeEvent::AutomationTime(crate::Event::TaskScheduled {
who: account_id,
task_id
task_id,
schedule_as: None,
})
);
})
Expand Down
2 changes: 1 addition & 1 deletion pallets/automation-time/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ impl Schedule {
}

/// The struct that stores all information needed for a task.
#[derive(Debug, Encode, Decode, TypeInfo)]
#[derive(Debug, Encode, Decode, TypeInfo, Clone)]
#[scale_info(skip_type_params(MaxExecutionTimes))]
pub struct Task<AccountId, Balance, CurrencyId> {
pub owner_id: AccountId,
Expand Down