Conversation
|
Still needs a test but wanted to open the PR for discussion on whether there are any edge cases that this could introduce. |
| require(!done, "spell-already-cast"); | ||
| require(eta == 0 || | ||
| pause.plans(keccak256(abi.encode(action, tag, sig, eta))) == false, | ||
| "This spell has already been scheduled"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
We could add tests and start doing another run of reviews |
3 participants
It may be possible for an exec to be scheduled by a vote, and then dropped from the pause by a competing hat.
This change allows a spell to be re-scheduled in the case where it is scheduled, dropped, and then re-raised to the hat.