-
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
Fix #443 Add extra data to reason about task trigger #453
Conversation
pallets/automation-price/src/lib.rs
Outdated
TargetPriceMatched { | ||
// The target price the task set to | ||
target_price: u128, | ||
trigger_function: Vec<u8>, |
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.
Okay, I think there might be confusion in this condition
paramter, , possibly due to its ambiguity. The goal of the event is not to re-iterate the trigger condition itself, but to provide real-time value of the circumstances that triggered a task in the code. For example, the TaskTriggered event should look like
TaskTriggered {who, taskId, condition: condition_struct }
in which, the condition_struct
mirrors the same structure as {destination, exchange, asset1, asset2, amount}
with amount
being the current value on chain, as a proof of trigger.
Keep in mind the definition of condition_struct is likely to change, so we should use the definition as much as possible, rather than deconstructing it, to faciliate future updates.
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.
yeah, it's actually works that way. it bundles the current value on chains plus the task information because when we debug later the task may have been remove from chain storage and hard to see the original value we have to query data at an old block.
let me cleanup a bit.
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.
Okay, I think we can omit the task details from the event, since it’s trivial compared to the real-time on-chain price. The reason is because task detais do not change after creation, but price varies across blocks. We almost always need to leverage the Insights service to read the task details from the past.
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.
In other words, the task details already have on-chain proof and can be retrieved from Insights, but the triggering price of this moment is not recorded yet.
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.
oh, the triggered price is recorded but because I only record price
but not (chain, exchange, asset_pair), and it sit at the bottom of the struct so it's hard to see.
I just update a cleaner version. Can you check now, I think should be much more clean.
b9f8638
to
aaa3195
Compare
9145090
to
488359b
Compare
5dff442
to
bf425c0
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.
Okay, the changes look good. I’ll need to give it a try to see how accessible is the new definition of condition in client’s code, for example, maybe it’s as easy as event.condition.toJson()
.
Comparing its counter-party in automationTime’s code, I think these predefined structs provides a better framework for defining conditions. Let’s try this now, and update the code in automationTime later.
https://github.com/OAK-Foundation/OAK-blockchain/blob/23433654b2c512aed336a9ae6e4174230b74969c/pallets/automation-time/src/lib.rs#L892C2-L892C2
Fix #443 to add a new
condition
into TaskTrigger. This contains 3 pieices: