handle price confirmation when running tasks #449
Conversation
| @@ -501,7 +501,7 @@ fn test_shift_tasks_movement_through_price_changes() { | |||
| asset2.to_vec(), | |||
| 1000u128, | |||
| "gt".as_bytes().to_vec(), | |||
| vec!(100), | |||
| vec!(10100), | |||
There was a problem hiding this comment.
update mock price so adjust these accordingly.
There was a problem hiding this comment.
Could we leverage variables to make these numbers more readable? For example, instead of 10100, we could use something like target_price_1 + 100.
Basically we just need to know how much higher or lower these numbers are compared to a target number in mock, right?
| @@ -501,7 +501,7 @@ fn test_shift_tasks_movement_through_price_changes() { | |||
| asset2.to_vec(), | |||
| 1000u128, | |||
| "gt".as_bytes().to_vec(), | |||
| vec!(100), | |||
| vec!(10100), | |||
There was a problem hiding this comment.
Could we leverage variables to make these numbers more readable? For example, instead of 10100, we could use something like target_price_1 + 100.
Basically we just need to know how much higher or lower these numbers are compared to a target number in mock, right?
The price may have changed in extreme case by the time we run the tasks, if so, we adjust accordingly and emit an event. Task won't be put back to the queue but will be removed from the queue as if it has run succesfully, but we emit the event about stale price instead of executing the task
v9n
force-pushed
the
handle-price-move-before-task-run
branch
from
5c41a00
to
d59d488
Compare
|
@chrisli30 I refactor with the new var and add more comments to explain the test idea |
| let mut consumed_task_index: usize = 0; | ||
| // Check whether a task can run or not based on its expiration and price. | ||
| // | ||
| // A task can be queued but got expired when it's about to run, in that case, we don't want |
There was a problem hiding this comment.
A task can be queued but got expired when it's about to run, in that case, we don't want it to be run.
While there may be different opinions on whether it should be executed or not, we can keep the behavior as-is. This comment is crucial for reviewing the design decision later. 👌
| } else { | ||
| Self::deposit_event(Event::PriceAlreadyMoved { | ||
| who: task.owner_id.clone(), | ||
| task_id: task.task_id.clone(), |
There was a problem hiding this comment.
The same reasoning as the above,
{ taskId, price, actual}
Another way is to wrap all conditions into a condition object, so at the top level they all look the same
{taskId, condition, value}
Then for the expiration event, the event becomes {taskId, condition: {expireAt}, value: now_value }, and for the price moved event, {taskId, condition: {price: {chain, exchange, asset1, asset2, price} }, value: new_price }. Please see which way makes more sense is Rust implementation.
There was a problem hiding this comment.
code is updated with the new condition layout. Condition can be applied for multiple type of Error to add extra information later on.
There was a problem hiding this comment.
yeah, this looks cleaner and readable in code.
The price may have changed in extreme case by the time we run the tasks, if so, we adjust accordingly and emit an event.
Task won't be put back to the queue but will be removed from the queue as if it has run succesfully, but we emit the event about stale price instead of executing the task