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

Add schedule_as into TaskScheduled event #365

Merged
merged 8 commits into from
Jul 7, 2023

Conversation

imstar15
Copy link
Member

@imstar15 imstar15 commented Jun 6, 2023

Fixed the problem that when calling scheduleXcmpTaskThroughProxy, the who parameter of TaskScheduled was inconsistent with AccountId32 in automationTime.accountTasks in storage.

who is always the caller for extrinsic.

image image

@imstar15 imstar15 requested a review from chrisli30 June 6, 2023 08:00
@imstar15 imstar15 force-pushed the add-schedule-as-into-taskscheduled-event branch from 25baf2f to d130d45 Compare June 6, 2023 08:14
Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

I’m not confident to check this in without unit tests. When you get time, please write down the design of unit test cases to make sure event filing for this xcmp task is right with a variety of inputs.

@imstar15
Copy link
Member Author

imstar15 commented Jul 4, 2023

The design of unit test case:

  1. The proxy account Bob invokes scheduleXcmpTaskThroughProxy with scheduleAs: Alice.
  2. In the emitted TaskScheduled event data, who should be Bob, and scheduleAs should be Alice.
  3. In the ScheduledTasksV3 storage, it can be queried that the owner of this task is Bob.

@chrisli30

@imstar15 imstar15 force-pushed the add-schedule-as-into-taskscheduled-event branch from d130d45 to edc25f7 Compare July 4, 2023 03:04
@imstar15 imstar15 requested a review from chrisli30 July 4, 2023 15:37
pallets/automation-time/src/benchmarking.rs Show resolved Hide resolved
pallets/automation-time/src/tests.rs Outdated Show resolved Hide resolved
pallets/automation-time/src/tests.rs Show resolved Hide resolved
pallets/automation-time/src/tests.rs Outdated Show resolved Hide resolved
pallets/automation-time/src/tests.rs Show resolved Hide resolved
@@ -267,6 +267,49 @@ 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

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.

pallets/automation-time/src/tests.rs Outdated Show resolved Hide resolved
pallets/automation-time/src/tests.rs Outdated Show resolved Hide resolved
pallets/automation-time/src/tests.rs Show resolved Hide resolved
@imstar15 imstar15 requested a review from chrisli30 July 7, 2023 05:29
Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@imstar15 imstar15 merged commit 7f6a8b9 into master Jul 7, 2023
@imstar15 imstar15 deleted the add-schedule-as-into-taskscheduled-event branch July 7, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants