openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

Governor: Clarify states when it's possible to cancel a proposal

Open cygnusv opened this issue 3 years ago • 1 comments

Current code for canceling a proposal checks first that the proposal is in a state that allows it:

        require(
            status != ProposalState.Canceled && status != ProposalState.Expired && status != ProposalState.Executed,
            "Governor: proposal not active"
        );

Is it expected that a Successful, Defeated or Queued proposal that's not yet executed can be canceled? The documentation is not clear about this. To be honest, I have no idea what to expect from Governor._cancel after reading the documentation 😅 : "Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted." 🤷

Also, the Expired state seems specific to the GovernorTimelockCompound extension, so maybe that reference belongs to the cancel condition there.

cygnusv avatar Feb 08 '22 09:02 cygnusv

You're right that the documentation for this function is not clear. We should improve it.

Is it expected that a Successful, Defeated or Queued proposal that's not yet executed can be canceled?

The reasoning here was that it should be possible to cancel from any non-final state, and we identified Canceled, Expired, and Executed as the final states. Now that you point it out, Defeated could be seen as a final state as well, and Expired may not be truly final for some hypothetical custom governor module. Perhaps this subset of states we identified as final is a bit arbitrary, but I do think it makes some sense.

What you should expect of _cancel is that it will move a proposal to a Canceled final state.

frangio avatar Feb 17 '22 22:02 frangio