Skip to content
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

Add Recurrent Script #174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Recurrent Script #174

wants to merge 1 commit into from

Conversation

hayesgm
Copy link
Contributor

@hayesgm hayesgm commented Mar 4, 2024

The Recurrent Script runs a command every so often. It uses StateManager to track the last time it executed, and it allows the script to be executed after some interval post-that. To help the user, we add a notBefore and notAfter set of functions that can be used to shape the overall time curve, e.g. to expire a transaction after some time. (Though this could be accomplished with expiration on the QuarkOperation itself, it seems like this is fairly important here).

cwang25
cwang25 previously approved these changes Mar 5, 2024
Copy link
Contributor

@cwang25 cwang25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(I sort of vaguely recalled ⛑️ has made something similar as recurring purchase in test ? 🤔 but this one is more generic and not tight to purchase)

The Recurrent Script runs a command every so often. It uses StateManager to track the last time it executed, and it allows the script to be executed after some interval post-that. To help the user, we add a `notBefore` and `notAfter` set of functions that can be used to shape the overall time curve, e.g. to expire a transaction after some time. (Though this could be accomplished with expiration on the QuarkOperation itself, it seems like this is fairly important here).
@hayesgm hayesgm force-pushed the hayesgm/recurrent branch from 192b227 to 7919476 Compare March 11, 2024 10:54
Comment on lines +43 to +44
uint256 lastExecution = readU256("lastExecution");
if (block.timestamp < lastExecution + interval) revert TooFrequent(lastExecution);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value is zero, don't we need to set it first? As i understand, this will always revert the first time unless interval is bigger than block.timestamp which i don't think makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banky I think you have that logic inversed. It shouldn't revert if lastExecution is 0 since block.timestamp will likely be greater than lastExecution + interval.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh yeah that's right haha. i was reading it as if it was require

Copy link
Collaborator

@kevincheng96 kevincheng96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat idea! Probably would want to add some unit tests before we get this merged, but the script LGTM 👍

Comment on lines +43 to +44
uint256 lastExecution = readU256("lastExecution");
if (block.timestamp < lastExecution + interval) revert TooFrequent(lastExecution);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banky I think you have that logic inversed. It shouldn't revert if lastExecution is 0 since block.timestamp will likely be greater than lastExecution + interval.

* @param notAfter Do not run this script after this time.
* @return Return data from call
*/
function run(address callContract, bytes calldata callData, uint256 interval, uint256 notBefore, uint256 notAfter) external returns (bytes memory) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of interesting thing is that you can have multiple replayable txns using the same nonce with different calldata because Quark allows you to do that now.

* @notice Core transaction script that can be used to call a function every so often
* @author Compound Labs, Inc.
*/
contract Recurrent is QuarkScript {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could consider adding a cancel() function that just cancels the nonce. See the RecurringPurchase example.

}

// Note: this starts out as zero, as in
uint256 lastExecution = readU256("lastExecution");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Minor optimization can be to use a 1 char key instead. You can store it as a constant to keep the same readability.

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

Successfully merging this pull request may close these issues.

4 participants