Skip to content

Allow for reschedule of dropped exec #100

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow for reschedule of dropped exec #100

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
109b12b
Allow for reschedule of dropped exec
brianmcmichael Jan 12, 2023
65d2b19
Don't reschedule completed spell
brianmcmichael Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/DssExec.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pragma solidity ^0.8.16;

interface PauseAbstract {
function delay() external view returns (uint256);
function plans(bytes32) external view returns (bool);
function plot(address, bytes32, bytes calldata, uint256) external;
function exec(address, bytes32, bytes calldata, uint256) external returns (bytes memory);
}
Expand Down Expand Up @@ -78,7 +79,10 @@ contract DssExec {

function schedule() public {
require(block.timestamp <= expiration, "This contract has expired");
require(eta == 0, "This spell has already been scheduled");
require(!done, "spell-already-cast");
require(eta == 0 ||
pause.plans(keccak256(abi.encode(action, tag, sig, eta))) == false,
"This spell has already been scheduled");
Comment on lines +82 to +85
Copy link

Choose a reason for hiding this comment

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

Maybe we can only use the pause.plans(keccak256(abi.encode(action, tag, sig, eta))) == false condition and remove the conditions on done and eta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done is flipped in the spell itself. Once the spell is cast it is removed from the pause. We still want it here to prevent an executed spell from being rescheduled.

Copy link

Choose a reason for hiding this comment

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

Ok, so we can consider only removing the eta check.
Note that after eta is set it can not be set back to 0, so it will only be 0 if the spell was never scheduled. In that case the spell params will also not appear in the pause mapping (2nd condition), making the eta == 0 condition redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eta still needs to be checked here. The new eta doesn't get set until the next line, so if the ETA is not zero here it means the spell has been scheduled once. If it's not done and it's not in the pause with the old eta, then it means the spell was scheduled and dropped before the cast.

Removing the ETA check here would allow spells to be scheduled repeatedly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I see your point, re: checking the pause using eta 0, but in this case we avoid the additional cost of a bunch of memory loads, a keccak hash, and an external call on the most common path.

eta = block.timestamp + PauseAbstract(pause).delay();
pause.plot(action, tag, sig, eta);
}
Expand Down