Skip to content
This repository was archived by the owner on Dec 17, 2024. It is now read-only.

Conversation

kaisbaccour
Copy link
Contributor

This is incomplete but I would like to get a general opinion on if this refactoring is going the right path or no.

@kaisbaccour kaisbaccour requested a review from webmaster128 July 25, 2023 14:35
Copy link
Collaborator

@webmaster128 webmaster128 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 at first glance. Pretty straight forward. It just need a bit of refinement to ensure we don't break existing users.

job_id: String,
) -> Result<Response, ContractError> {
let config = CONFIG.load(deps.storage)?;
let callback_address = info.clone().sender;
Copy link
Collaborator

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:

Suggested change
let callback_address = info.clone().sender;
let callback_address = info.sender.clone();

/// Beacon publish time must be > `after`
after: Timestamp,
/// The callback data set by the proxy in a proxy specific format.
callback: Binary,
Copy link
Collaborator

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 {
Copy link
Collaborator

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 {
Copy link
Collaborator

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants