-
Notifications
You must be signed in to change notification settings - Fork 70
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
deprecate schedule_notify_task #373
deprecate schedule_notify_task #373
Conversation
pallets/automation-time/src/lib.rs
Outdated
// the extrinsic is disabled so no one can schedule that kind of tasks, just old tasks | ||
// TODO: Find out turing staging number and initalize accordingly | ||
// This is the last block we have this event | ||
let target_block: u32 = 1244899; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisli30 @imstar15 this number is for staging and maybe in-correct.
But just want to get feedback on the direction Im doing is right with this? What is our strategy so far to handle removing functionality and backward compatible when re-syncing and execute code in old blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The blockchain itself will run data migration right before new runtime code during an upgrade, so the data needs to be correctly transformed. In order to achieve that, as long as there’s data change in storage, migration could needs to follow.
We write the migration code and use try-runtime script to verify the correctness.
After the release of the code, those one-time data migration needs to be removed so they don’t run in the next release. Here’s the most recent example, #369
25826ed
to
d6bd7dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@v9n I have requested to comment the purpose of multiple tests which require explanation. Reading through those test code and figuring out their purpose will be helpful in understanding the time automation implementation.
pallets/automation-time/src/tests.rs
Outdated
@@ -38,12 +38,16 @@ const LAST_BLOCK_TIME: u64 = START_BLOCK_TIME / 1_000; | |||
#[test] | |||
fn schedule_invalid_time() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all timeAutomation call has two kinds of scheduling, Fixed and Recurring, we should duplicate time related tests and test both types.
For Fixed, multiple timestamps need always to be tested since the input is an array, Vec
For Recurring, only one timestamp needs to be tested, but there’s a second parameter frequency
to test as well, { next_execution_time: UnixTime, frequency: Seconds}
.
@imstar15 could you review this PR as well? |
@chrisli30 @imstar15 I had addressed all the comments. I tried to add comments per test to a hard to understand un-clear test case. A few tests are duplicated to check recurring schedule logic as well. ready to review agian. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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!
This PR remove
schedule_notify_task
extrinsic. This extrinsic is also used to setup and run test for many scenario so i migrated them to dynamic dispatch call and assert their behaviour.