From 9ae71d6382f253c2388aff6358d2b58cbc9ca516 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 14 Mar 2023 10:31:39 +0100 Subject: [PATCH 1/5] Cleanup timelockId on execution for gas refund --- .changeset/hot-plums-approve.md | 5 +++++ .../governance/extensions/GovernorTimelockControl.sol | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 .changeset/hot-plums-approve.md diff --git a/.changeset/hot-plums-approve.md b/.changeset/hot-plums-approve.md new file mode 100644 index 00000000000..5ef53d445e3 --- /dev/null +++ b/.changeset/hot-plums-approve.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`GovernorTimelockControl`: cleanup timelockId on execution for gas refund. diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 6aa2556abf1..772601278cb 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -65,6 +65,7 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { } else if (_timelock.isOperationPending(queueid)) { return ProposalState.Queued; } else { + // This can happen if the proposal is canceled directly on the timelock. return ProposalState.Canceled; } } @@ -110,12 +111,15 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { * @dev Overridden execute function that run the already queued proposal through the timelock. */ function _execute( - uint256 /* proposalId */, + uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) internal virtual override { + // cleanup for refund + delete _timelockIds[proposalId]; + // execute _timelock.executeBatch{value: msg.value}(targets, values, calldatas, 0, descriptionHash); } @@ -135,7 +139,9 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { uint256 proposalId = super._cancel(targets, values, calldatas, descriptionHash); if (_timelockIds[proposalId] != 0) { + // cancel _timelock.cancel(_timelockIds[proposalId]); + // cleanup delete _timelockIds[proposalId]; } From b6b3e39106bd8af60661ea17cbf66c825b8b6dd3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 14 Mar 2023 11:50:19 +0100 Subject: [PATCH 2/5] reorder to optimize the most common flow --- contracts/governance/extensions/GovernorTimelockControl.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 772601278cb..4f5d74a2b0f 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -60,10 +60,11 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes32 queueid = _timelockIds[proposalId]; if (queueid == bytes32(0)) { return status; - } else if (_timelock.isOperationDone(queueid)) { - return ProposalState.Executed; } else if (_timelock.isOperationPending(queueid)) { return ProposalState.Queued; + } else if (_timelock.isOperationDone(queueid)) { + // This can happen if the proposal is executed directly on the timelock. + return ProposalState.Executed; } else { // This can happen if the proposal is canceled directly on the timelock. return ProposalState.Canceled; From 184a6c21dbda44fc7863586d8adc79843b0f86fc Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 15 Jun 2023 17:13:09 -0300 Subject: [PATCH 3/5] Update hot-plums-approve.md --- .changeset/hot-plums-approve.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/hot-plums-approve.md b/.changeset/hot-plums-approve.md index 5ef53d445e3..131559027fa 100644 --- a/.changeset/hot-plums-approve.md +++ b/.changeset/hot-plums-approve.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`GovernorTimelockControl`: cleanup timelockId on execution for gas refund. +`GovernorTimelockControl`: Clean up timelock id on execution for gas refund. From 8e6d187cf2c58876c3cf1aba44430aa1bfce1d49 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 15 Jun 2023 22:19:08 +0200 Subject: [PATCH 4/5] Update GovernorTimelockControl.sol --- contracts/governance/extensions/GovernorTimelockControl.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index bde809ea0e9..3c67d771808 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -60,8 +60,6 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes32 queueid = _timelockIds[proposalId]; if (queueid == bytes32(0)) { return currentState; - } else if (_timelock.isOperationDone(queueid)) { - return ProposalState.Executed; } else if (_timelock.isOperationPending(queueid)) { return ProposalState.Queued; } else if (_timelock.isOperationDone(queueid)) { From 830920a9c22086a4957ee410a6894c4475231bb7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 20 Jun 2023 10:10:07 +0200 Subject: [PATCH 5/5] Apply suggestions from code review --- .../governance/extensions/GovernorTimelockControl.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 3c67d771808..fadbcc70152 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -125,10 +125,10 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes[] memory calldatas, bytes32 descriptionHash ) internal virtual override { - // cleanup for refund - delete _timelockIds[proposalId]; // execute _timelock.executeBatch{value: msg.value}(targets, values, calldatas, 0, descriptionHash); + // cleanup for refund + delete _timelockIds[proposalId]; } /** @@ -148,10 +148,10 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes32 timelockId = _timelockIds[proposalId]; if (timelockId != 0) { - // cleanup - delete _timelockIds[proposalId]; // cancel _timelock.cancel(timelockId); + // cleanup + delete _timelockIds[proposalId]; } return proposalId;