-
Notifications
You must be signed in to change notification settings - Fork 7
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
Schedule_at_job #270
base: main
Are you sure you want to change the base?
Schedule_at_job #270
Conversation
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 at first glance. Pretty straight forward. It just need a bit of refinement to ensure we don't break existing users.
@@ -164,6 +169,7 @@ fn execute_get_next_randomness( | |||
job_id: String, | |||
) -> Result<Response, ContractError> { | |||
let config = CONFIG.load(deps.storage)?; | |||
let callback_address = info.clone().sender; |
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.
No need to clone the whole info, you can just clone the sender:
let callback_address = info.clone().sender; | |
let callback_address = info.sender.clone(); |
@@ -10,15 +10,34 @@ pub enum InPacket { | |||
RequestBeacon { | |||
/// Beacon publish time must be > `after` | |||
after: Timestamp, | |||
/// The callback data set by the proxy in a proxy specific format. | |||
callback: Binary, |
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.
We can easily keep this enum case compatible by avoiding the rename origin -> callback. Then we only have an RequestScheduleJob
addition here.
AtJob, | ||
} | ||
impl RequestType { | ||
pub fn to_string(&self) -> &str { |
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.
Here you can implement Display which gives you a to_string()
implementation for free
/// The callback data set by the proxy in a proxy specific format. | ||
callback: Binary, | ||
}, | ||
RequestScheduleJob { |
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.
I think we should have a corresponding OutPacket::DeliverScheduleJob
case which does not contain the randomness field
This is incomplete but I would like to get a general opinion on if this refactoring is going the right path or no.