-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Add Recurrent Script #174
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.
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).
192b227
to
7919476
Compare
uint256 lastExecution = readU256("lastExecution"); | ||
if (block.timestamp < lastExecution + interval) revert TooFrequent(lastExecution); |
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.
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
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.
@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
.
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.
ohh yeah that's right haha. i was reading it as if it was require
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.
Neat idea! Probably would want to add some unit tests before we get this merged, but the script LGTM 👍
uint256 lastExecution = readU256("lastExecution"); | ||
if (block.timestamp < lastExecution + interval) revert TooFrequent(lastExecution); |
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.
@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) { |
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.
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 { |
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.
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"); |
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.
nit: Minor optimization can be to use a 1 char key instead. You can store it as a constant to keep the same readability.
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
andnotAfter
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).