Add state getter in TimelockController using OperationState enum

[ernestognw]

Description

We introduced an enum of OperationStates for custom errors in #4261. However, it feels weird that such enum is only used for custom errors.

We should use the same enum for retrieving the state. We might also want to replace the state getters with this new state(bytes32 id) getter.

[clauBv23]

Hi there,

I have been taking a look at this task and, have some concerns regarding it.
Currently, the states are not restrictive, I mean, an operation could be in both the pending and ready state. Due to that, there are some places where this is taken as an advantage to check the minimal needed restrictions like in

openzeppelin-contracts/contracts/governance/extensions/GovernorTimelockControl.sol

Lines 65 to 66 in cd48b3e

	} else if (_timelock.isOperationPending(queueid)) {
	return ProposalState.Queued;

where with a single check covers the pending and the ready state.

A solution for that case using a state-getter could be to check if the operation is either pending or ready like is being done in the proposed PR, but by doing it more checks will be needed to do the same.
Also to check a specific operation state, will be needed to ensure it is not in other ones (to know if is ready should check if is not pending or vice-versa).

So in general, Even though I agree it will be more descriptive and clean, it will likely increase the amount of checks needed to do the same that is being done at this time.

[Amxx]

We could rename "Pending" into "Waiting" in the enum, and consider that Pending == Waiting || Ready. We would keepn the isOperationPending getter for compatibility (governor uses that)

@ernestognw WDYT?

[clauBv23]

That will definitely work, but due to "Ready" and "Pending" being opposite it could sound contradictory. Maybe going with Active == Pending || Ready?

[Amxx]

Active == Pending || Ready sounds great, but then what would isOperationPending return ?

Remember that the governor does a isOperationPending lookup to check is the operation was queued. With the change you propose the governor should look if the operation is active...

So again, either isOperationPending returns true is the operation is Active (and that doesn't sound right) ... or the govenor does an isOperationActive on the timelock controller ... but that reverts for old instances of the timelock

an option would be for the governor to do

function isOperationActiveWithFallback(bytes32 queueId) private view returns (bool) {
  try _timelock.isOperationActive(queueId) returns (bool isActive) {
    return isActive;
  } catch {
    // assume old governor
    return _timelock.isOperationPending(queueId);
  }
}

Before any decision is made, we should discuss that with the team

[clauBv23]

Yeap, you are right, changing to Active will break the previous instances, and even though it could be solved as you mentioned, it might not be worth it. And pending being ready has worked so far.

[RenanSouza2]

The current documentation says this

** *Unset:* An operation that is not part of the timelock mechanism.
** *Pending:* An operation that has been scheduled, before the timer expires.
** *Ready:* An operation that has been scheduled, after the timer expires.
** *Done:* An operation that has been executed.

So whatever is the final decision on the names I will update the documents as wel