From 6330d4c9a7e3ae19cc0b6f731449fe2cfa8b76c9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 19 Nov 2024 11:09:10 +0100 Subject: [PATCH 1/8] Add a governor extension that implements a security council --- .../extensions/GovernorSecurityCouncil.sol | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 contracts/governance/extensions/GovernorSecurityCouncil.sol diff --git a/contracts/governance/extensions/GovernorSecurityCouncil.sol b/contracts/governance/extensions/GovernorSecurityCouncil.sol new file mode 100644 index 00000000000..c6fe639c416 --- /dev/null +++ b/contracts/governance/extensions/GovernorSecurityCouncil.sol @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Governor} from "../Governor.sol"; + +/** + * @dev Extension of {Governor} for adds a security council that can cancel proposals at any stage of their lifecycle. + * + * Note: if the council is not configure, then proposers take over the + */ +abstract contract GovernorSecurityCouncil is Governor { + address private _council; + + event CouncilSet(address oldCouncil, address newCouncil); + + /** + * @dev Override {IGovernor-cancel} that implements the extended cancellation logic. + * * council can cancel any proposal at any point in the lifecycle. + * * if no council is set, proposer can cancel their proposals at any point in the lifecycle. + * * if the council is set, proposer keep their ability to cancel during the pending stage (default behavior). + */ + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual override returns (uint256) { + address caller = _msgSender(); + address authority = council(); + + if (authority == address(0)) { + // if there is no council + // ... only the proposer can cancel + // ... no restriction on when the proposer can cancel + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + address proposer = proposalProposer(proposalId); + if (caller != proposer) revert GovernorOnlyProposer(caller); + return _cancel(targets, values, calldatas, descriptionHash); + } else if (authority == caller) { + // if there is a council, and the caller is the council + // ... just cancel + return _cancel(targets, values, calldatas, descriptionHash); + } else { + // if there is a council, and the caller is not the council + // ... apply default behavior + return super.cancel(targets, values, calldatas, descriptionHash); + } + } + + /** + * @dev Getter that returns the address of the council + */ + function council() public view virtual returns (address) { + return _council; + } + + /** + * @dev Update the council's address. This operation can only be performed through a governance proposal. + * + * Emits a {CouncilSet} event. + */ + function setCouncil(address newCouncil) public virtual onlyGovernance { + _setCouncil(newCouncil); + } + + /** + * @dev Internal setter for the council. + * + * Emits a {CouncilSet} event. + */ + function _setCouncil(address newCouncil) internal virtual { + emit CouncilSet(_council, newCouncil); + _council = newCouncil; + } +} From 578543d0750e8f1802fac09c786d11d552b60e9a Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Wed, 27 Nov 2024 16:45:09 -0500 Subject: [PATCH 2/8] add tests --- .../GovernorSecurityCouncilMock.sol | 29 ++++ .../GovernorSecurityCouncil.test.js | 125 ++++++++++++++++++ 2 files changed, 154 insertions(+) create mode 100644 contracts/mocks/governance/GovernorSecurityCouncilMock.sol create mode 100644 test/governance/extensions/GovernorSecurityCouncil.test.js diff --git a/contracts/mocks/governance/GovernorSecurityCouncilMock.sol b/contracts/mocks/governance/GovernorSecurityCouncilMock.sol new file mode 100644 index 00000000000..9cb6d0151da --- /dev/null +++ b/contracts/mocks/governance/GovernorSecurityCouncilMock.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Governor} from "../../governance/Governor.sol"; +import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; +import {GovernorCountingSimple} from "../../governance/extensions/GovernorCountingSimple.sol"; +import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; +import {GovernorSecurityCouncil} from "../../governance/extensions/GovernorSecurityCouncil.sol"; + +abstract contract GovernorSecurityCouncilMock is + GovernorSettings, + GovernorVotesQuorumFraction, + GovernorCountingSimple, + GovernorSecurityCouncil +{ + function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { + return super.proposalThreshold(); + } + + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public override(Governor, GovernorSecurityCouncil) returns (uint256) { + return super.cancel(targets, values, calldatas, descriptionHash); + } +} diff --git a/test/governance/extensions/GovernorSecurityCouncil.test.js b/test/governance/extensions/GovernorSecurityCouncil.test.js new file mode 100644 index 00000000000..39faa9b1786 --- /dev/null +++ b/test/governance/extensions/GovernorSecurityCouncil.test.js @@ -0,0 +1,125 @@ +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { impersonate } = require('../../helpers/account'); + +const { GovernorHelper } = require('../../helpers/governance'); +const { ProposalState } = require('../../helpers/enums'); + +const TOKENS = [ + { Token: '$ERC20Votes', mode: 'blocknumber' }, + { Token: '$ERC20VotesTimestampMock', mode: 'timestamp' }, +]; + +const name = 'Security Council Governor'; +const version = '1'; +const tokenName = 'MockToken'; +const tokenSymbol = 'MTKN'; +const tokenSupply = ethers.parseEther('100'); +const votingDelay = 4n; +const votingPeriod = 16n; +const value = ethers.parseEther('1'); + +describe('GovernorSecurityCouncil', function () { + for (const { Token, mode } of TOKENS) { + const fixture = async () => { + const [owner, proposer, voter1, voter2, voter3, voter4, other] = await ethers.getSigners(); + const receiver = await ethers.deployContract('CallReceiverMock'); + + const token = await ethers.deployContract(Token, [tokenName, tokenSymbol, tokenName, version]); + const mock = await ethers.deployContract('$GovernorSecurityCouncilMock', [ + name, // name + votingDelay, // initialVotingDelay + votingPeriod, // initialVotingPeriod + 0n, // initialProposalThreshold + token, // tokenAddress + 10n, // quorumNumeratorValue + ]); + + await impersonate(mock.target); + await owner.sendTransaction({ to: mock, value }); + await token.$_mint(owner, tokenSupply); + + const helper = new GovernorHelper(mock, mode); + await helper.connect(owner).delegate({ token, to: voter1, value: ethers.parseEther('10') }); + await helper.connect(owner).delegate({ token, to: voter2, value: ethers.parseEther('7') }); + await helper.connect(owner).delegate({ token, to: voter3, value: ethers.parseEther('5') }); + await helper.connect(owner).delegate({ token, to: voter4, value: ethers.parseEther('2') }); + + return { owner, proposer, voter1, voter2, voter3, voter4, other, receiver, token, mock, helper }; + }; + + describe(`using ${Token}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + + // default proposal + this.proposal = this.helper.setProposal( + [ + { + target: this.receiver.target, + value, + data: this.receiver.interface.encodeFunctionData('mockFunction'), + }, + ], + '', + ); + }); + + it('deployment check', async function () { + expect(await this.mock.name()).to.equal(name); + expect(await this.mock.token()).to.equal(this.token); + expect(await this.mock.votingDelay()).to.equal(votingDelay); + expect(await this.mock.votingPeriod()).to.equal(votingPeriod); + }); + + describe('set council', function () { + it('from governance', async function () { + const governorSigner = await ethers.getSigner(this.mock.target); + await expect(this.mock.connect(governorSigner).setCouncil(this.voter1.address)) + .to.emit(this.mock, 'CouncilSet') + .withArgs(ethers.ZeroAddress, this.voter1.address); + expect(this.mock.council()).to.eventually.equal(this.voter1.address); + }); + + it('from non-governance', async function () { + await expect(this.mock.setCouncil(this.voter1.address)) + .to.be.revertedWithCustomError(this.mock, 'GovernorOnlyExecutor') + .withArgs(this.owner.address); + }); + }); + + describe('cancel proposal during active state', function () { + beforeEach(async function () { + await this.mock.$_setCouncil(this.voter1.address); + await this.helper.connect(this.proposer).propose(); + await this.helper.waitForSnapshot(1n); + expect(this.mock.state(this.proposal.id)).to.eventually.equal(ProposalState.Active); + }); + + it('from council', async function () { + await expect(this.helper.connect(this.voter1).cancel()) + .to.emit(this.mock, 'ProposalCanceled') + .withArgs(this.proposal.id); + }); + + it('from proposer when council is non-zero', async function () { + await expect(this.helper.connect(this.proposer).cancel()) + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs( + this.proposal.id, + ProposalState.Active, + GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]), + ); + }); + + it('from proposer when council is zero', async function () { + await this.mock.$_setCouncil(ethers.ZeroAddress); + await expect(this.helper.connect(this.proposer).cancel()) + .to.emit(this.mock, 'ProposalCanceled') + .withArgs(this.proposal.id); + }); + }); + }); + } +}); From fb6e8b7fcd10de974a8b5b071034cbcb75c2055a Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Fri, 13 Dec 2024 13:20:42 -0500 Subject: [PATCH 3/8] rename `GovernorSecurityCouncil` to `GovernorProposalGuardian` --- ...uncil.sol => GovernorProposalGuardian.sol} | 0 ...k.sol => GovernorProposalGuardianMock.sol} | 8 +++--- ...st.js => GovernorProposalGuardian.test.js} | 26 +++++++++---------- 3 files changed, 17 insertions(+), 17 deletions(-) rename contracts/governance/extensions/{GovernorSecurityCouncil.sol => GovernorProposalGuardian.sol} (100%) rename contracts/mocks/governance/{GovernorSecurityCouncilMock.sol => GovernorProposalGuardianMock.sol} (77%) rename test/governance/extensions/{GovernorSecurityCouncil.test.js => GovernorProposalGuardian.test.js} (82%) diff --git a/contracts/governance/extensions/GovernorSecurityCouncil.sol b/contracts/governance/extensions/GovernorProposalGuardian.sol similarity index 100% rename from contracts/governance/extensions/GovernorSecurityCouncil.sol rename to contracts/governance/extensions/GovernorProposalGuardian.sol diff --git a/contracts/mocks/governance/GovernorSecurityCouncilMock.sol b/contracts/mocks/governance/GovernorProposalGuardianMock.sol similarity index 77% rename from contracts/mocks/governance/GovernorSecurityCouncilMock.sol rename to contracts/mocks/governance/GovernorProposalGuardianMock.sol index 9cb6d0151da..8252a30a21a 100644 --- a/contracts/mocks/governance/GovernorSecurityCouncilMock.sol +++ b/contracts/mocks/governance/GovernorProposalGuardianMock.sol @@ -6,13 +6,13 @@ import {Governor} from "../../governance/Governor.sol"; import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; import {GovernorCountingSimple} from "../../governance/extensions/GovernorCountingSimple.sol"; import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; -import {GovernorSecurityCouncil} from "../../governance/extensions/GovernorSecurityCouncil.sol"; +import {GovernorProposalGuardian} from "../../governance/extensions/GovernorProposalGuardian.sol"; -abstract contract GovernorSecurityCouncilMock is +abstract contract GovernorProposalGuardianMock is GovernorSettings, GovernorVotesQuorumFraction, GovernorCountingSimple, - GovernorSecurityCouncil + GovernorProposalGuardian { function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { return super.proposalThreshold(); @@ -23,7 +23,7 @@ abstract contract GovernorSecurityCouncilMock is uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public override(Governor, GovernorSecurityCouncil) returns (uint256) { + ) public override(Governor, GovernorProposalGuardian) returns (uint256) { return super.cancel(targets, values, calldatas, descriptionHash); } } diff --git a/test/governance/extensions/GovernorSecurityCouncil.test.js b/test/governance/extensions/GovernorProposalGuardian.test.js similarity index 82% rename from test/governance/extensions/GovernorSecurityCouncil.test.js rename to test/governance/extensions/GovernorProposalGuardian.test.js index 39faa9b1786..00230970cf8 100644 --- a/test/governance/extensions/GovernorSecurityCouncil.test.js +++ b/test/governance/extensions/GovernorProposalGuardian.test.js @@ -11,7 +11,7 @@ const TOKENS = [ { Token: '$ERC20VotesTimestampMock', mode: 'timestamp' }, ]; -const name = 'Security Council Governor'; +const name = 'Proposal Guardian Governor'; const version = '1'; const tokenName = 'MockToken'; const tokenSymbol = 'MTKN'; @@ -20,14 +20,14 @@ const votingDelay = 4n; const votingPeriod = 16n; const value = ethers.parseEther('1'); -describe('GovernorSecurityCouncil', function () { +describe.only('GovernorProposalGuardian', function () { for (const { Token, mode } of TOKENS) { const fixture = async () => { const [owner, proposer, voter1, voter2, voter3, voter4, other] = await ethers.getSigners(); const receiver = await ethers.deployContract('CallReceiverMock'); const token = await ethers.deployContract(Token, [tokenName, tokenSymbol, tokenName, version]); - const mock = await ethers.deployContract('$GovernorSecurityCouncilMock', [ + const mock = await ethers.deployContract('$GovernorProposalGuardianMock', [ name, // name votingDelay, // initialVotingDelay votingPeriod, // initialVotingPeriod @@ -73,17 +73,17 @@ describe('GovernorSecurityCouncil', function () { expect(await this.mock.votingPeriod()).to.equal(votingPeriod); }); - describe('set council', function () { + describe('set proposal guardian', function () { it('from governance', async function () { const governorSigner = await ethers.getSigner(this.mock.target); - await expect(this.mock.connect(governorSigner).setCouncil(this.voter1.address)) - .to.emit(this.mock, 'CouncilSet') + await expect(this.mock.connect(governorSigner).setProposalGuardian(this.voter1.address)) + .to.emit(this.mock, 'ProposalGuardianSet') .withArgs(ethers.ZeroAddress, this.voter1.address); - expect(this.mock.council()).to.eventually.equal(this.voter1.address); + expect(this.mock.proposalGuardian()).to.eventually.equal(this.voter1.address); }); it('from non-governance', async function () { - await expect(this.mock.setCouncil(this.voter1.address)) + await expect(this.mock.setProposalGuardian(this.voter1.address)) .to.be.revertedWithCustomError(this.mock, 'GovernorOnlyExecutor') .withArgs(this.owner.address); }); @@ -91,19 +91,19 @@ describe('GovernorSecurityCouncil', function () { describe('cancel proposal during active state', function () { beforeEach(async function () { - await this.mock.$_setCouncil(this.voter1.address); + await this.mock.$_setProposalGuardian(this.voter1.address); await this.helper.connect(this.proposer).propose(); await this.helper.waitForSnapshot(1n); expect(this.mock.state(this.proposal.id)).to.eventually.equal(ProposalState.Active); }); - it('from council', async function () { + it('from proposal guardian', async function () { await expect(this.helper.connect(this.voter1).cancel()) .to.emit(this.mock, 'ProposalCanceled') .withArgs(this.proposal.id); }); - it('from proposer when council is non-zero', async function () { + it('from proposer when proposal guardian is non-zero', async function () { await expect(this.helper.connect(this.proposer).cancel()) .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') .withArgs( @@ -113,8 +113,8 @@ describe('GovernorSecurityCouncil', function () { ); }); - it('from proposer when council is zero', async function () { - await this.mock.$_setCouncil(ethers.ZeroAddress); + it('from proposer when proposal guardian is zero', async function () { + await this.mock.$_setProposalGuardian(ethers.ZeroAddress); await expect(this.helper.connect(this.proposer).cancel()) .to.emit(this.mock, 'ProposalCanceled') .withArgs(this.proposal.id); From d613cc896af0019f0f030bbb6557afc28ebefdd5 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Fri, 13 Dec 2024 13:20:56 -0500 Subject: [PATCH 4/8] update `GovernorProposalGuardian` --- .../extensions/GovernorProposalGuardian.sol | 49 +++++++++---------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/contracts/governance/extensions/GovernorProposalGuardian.sol b/contracts/governance/extensions/GovernorProposalGuardian.sol index c6fe639c416..d74f64c13cf 100644 --- a/contracts/governance/extensions/GovernorProposalGuardian.sol +++ b/contracts/governance/extensions/GovernorProposalGuardian.sol @@ -1,24 +1,23 @@ // SPDX-License-Identifier: MIT - pragma solidity ^0.8.20; import {Governor} from "../Governor.sol"; /** - * @dev Extension of {Governor} for adds a security council that can cancel proposals at any stage of their lifecycle. + * @dev Extension of {Governor} which adds a proposal guardian that can cancel proposals at any stage of their lifecycle. * - * Note: if the council is not configure, then proposers take over the + * Note: if the proposal guardian is not configured, then proposers take this role for their proposals. */ -abstract contract GovernorSecurityCouncil is Governor { - address private _council; +abstract contract GovernorProposalGuardian is Governor { + address private _proposalGuardian; - event CouncilSet(address oldCouncil, address newCouncil); + event ProposalGuardianSet(address oldProposalGuardian, address newProposalGuardian); /** * @dev Override {IGovernor-cancel} that implements the extended cancellation logic. - * * council can cancel any proposal at any point in the lifecycle. - * * if no council is set, proposer can cancel their proposals at any point in the lifecycle. - * * if the council is set, proposer keep their ability to cancel during the pending stage (default behavior). + * * proposal guardian can cancel any proposal at any point in the lifecycle. + * * if no proposal guardian is set, proposer can cancel their proposals at any point in the lifecycle. + * * if the proposal guardian is set, proposer keep their ability to cancel during the pending stage (default behavior). */ function cancel( address[] memory targets, @@ -27,10 +26,10 @@ abstract contract GovernorSecurityCouncil is Governor { bytes32 descriptionHash ) public virtual override returns (uint256) { address caller = _msgSender(); - address authority = council(); + address authority = proposalGuardian(); if (authority == address(0)) { - // if there is no council + // if there is no proposal guardian // ... only the proposer can cancel // ... no restriction on when the proposer can cancel uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); @@ -38,39 +37,39 @@ abstract contract GovernorSecurityCouncil is Governor { if (caller != proposer) revert GovernorOnlyProposer(caller); return _cancel(targets, values, calldatas, descriptionHash); } else if (authority == caller) { - // if there is a council, and the caller is the council + // if there is a proposal guardian, and the caller is the proposal guardian // ... just cancel return _cancel(targets, values, calldatas, descriptionHash); } else { - // if there is a council, and the caller is not the council + // if there is a proposal guardian, and the caller is not the proposal guardian // ... apply default behavior return super.cancel(targets, values, calldatas, descriptionHash); } } /** - * @dev Getter that returns the address of the council + * @dev Getter that returns the address of the proposal guardian. */ - function council() public view virtual returns (address) { - return _council; + function proposalGuardian() public view virtual returns (address) { + return _proposalGuardian; } /** - * @dev Update the council's address. This operation can only be performed through a governance proposal. + * @dev Update the proposal guardian's address. This operation can only be performed through a governance proposal. * - * Emits a {CouncilSet} event. + * Emits a {ProposalGuardianSet} event. */ - function setCouncil(address newCouncil) public virtual onlyGovernance { - _setCouncil(newCouncil); + function setProposalGuardian(address newProposalGuardian) public virtual onlyGovernance { + _setProposalGuardian(newProposalGuardian); } /** - * @dev Internal setter for the council. + * @dev Internal setter for the proposal guardian. * - * Emits a {CouncilSet} event. + * Emits a {ProposalGuardianSet} event. */ - function _setCouncil(address newCouncil) internal virtual { - emit CouncilSet(_council, newCouncil); - _council = newCouncil; + function _setProposalGuardian(address newProposalGuardian) internal virtual { + emit ProposalGuardianSet(_proposalGuardian, newProposalGuardian); + _proposalGuardian = newProposalGuardian; } } From 25eeb0287bf337ea9df792eb8a24d99a88211a86 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 16 Dec 2024 10:26:43 -0500 Subject: [PATCH 5/8] remove `.only` --- test/governance/extensions/GovernorProposalGuardian.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorProposalGuardian.test.js b/test/governance/extensions/GovernorProposalGuardian.test.js index 00230970cf8..ed4e6273ec5 100644 --- a/test/governance/extensions/GovernorProposalGuardian.test.js +++ b/test/governance/extensions/GovernorProposalGuardian.test.js @@ -20,7 +20,7 @@ const votingDelay = 4n; const votingPeriod = 16n; const value = ethers.parseEther('1'); -describe.only('GovernorProposalGuardian', function () { +describe('GovernorProposalGuardian', function () { for (const { Token, mode } of TOKENS) { const fixture = async () => { const [owner, proposer, voter1, voter2, voter3, voter4, other] = await ethers.getSigners(); From 5ba45967ab1040dfe60f8cfc83cc47270fa29c58 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Fri, 20 Dec 2024 15:38:07 -0500 Subject: [PATCH 6/8] use `getProposalId` instead of `hashProposal` --- contracts/governance/extensions/GovernorProposalGuardian.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorProposalGuardian.sol b/contracts/governance/extensions/GovernorProposalGuardian.sol index d74f64c13cf..e61ae760042 100644 --- a/contracts/governance/extensions/GovernorProposalGuardian.sol +++ b/contracts/governance/extensions/GovernorProposalGuardian.sol @@ -32,7 +32,7 @@ abstract contract GovernorProposalGuardian is Governor { // if there is no proposal guardian // ... only the proposer can cancel // ... no restriction on when the proposer can cancel - uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash); address proposer = proposalProposer(proposalId); if (caller != proposer) revert GovernorOnlyProposer(caller); return _cancel(targets, values, calldatas, descriptionHash); From 954b03cc7246ba502a5b49daeb3463c3869f4b3d Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 23 Dec 2024 15:57:17 -0500 Subject: [PATCH 7/8] Update contracts/governance/extensions/GovernorProposalGuardian.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/governance/extensions/GovernorProposalGuardian.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorProposalGuardian.sol b/contracts/governance/extensions/GovernorProposalGuardian.sol index e61ae760042..7852e12101b 100644 --- a/contracts/governance/extensions/GovernorProposalGuardian.sol +++ b/contracts/governance/extensions/GovernorProposalGuardian.sol @@ -16,7 +16,7 @@ abstract contract GovernorProposalGuardian is Governor { /** * @dev Override {IGovernor-cancel} that implements the extended cancellation logic. * * proposal guardian can cancel any proposal at any point in the lifecycle. - * * if no proposal guardian is set, proposer can cancel their proposals at any point in the lifecycle. + * * if no proposal guardian is set, the {proposalProposer} can cancel their proposals at any point in the lifecycle. * * if the proposal guardian is set, proposer keep their ability to cancel during the pending stage (default behavior). */ function cancel( From e56c8b2acee8f0bd7ac3f4d3dd77f96d505fef6a Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 23 Dec 2024 15:59:04 -0500 Subject: [PATCH 8/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- .../extensions/GovernorProposalGuardian.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/governance/extensions/GovernorProposalGuardian.sol b/contracts/governance/extensions/GovernorProposalGuardian.sol index 7852e12101b..45c548e62ce 100644 --- a/contracts/governance/extensions/GovernorProposalGuardian.sol +++ b/contracts/governance/extensions/GovernorProposalGuardian.sol @@ -6,7 +6,7 @@ import {Governor} from "../Governor.sol"; /** * @dev Extension of {Governor} which adds a proposal guardian that can cancel proposals at any stage of their lifecycle. * - * Note: if the proposal guardian is not configured, then proposers take this role for their proposals. + * NOTE: if the proposal guardian is not configured, then proposers take this role for their proposals. */ abstract contract GovernorProposalGuardian is Governor { address private _proposalGuardian; @@ -15,9 +15,9 @@ abstract contract GovernorProposalGuardian is Governor { /** * @dev Override {IGovernor-cancel} that implements the extended cancellation logic. - * * proposal guardian can cancel any proposal at any point in the lifecycle. + * * The {proposalGuardian} can cancel any proposal at any point in the lifecycle. * * if no proposal guardian is set, the {proposalProposer} can cancel their proposals at any point in the lifecycle. - * * if the proposal guardian is set, proposer keep their ability to cancel during the pending stage (default behavior). + * * if the proposal guardian is set, the {proposalProposer} keeps their default rights defined in {IGovernor-cancel} (calling `super`). */ function cancel( address[] memory targets, @@ -26,9 +26,9 @@ abstract contract GovernorProposalGuardian is Governor { bytes32 descriptionHash ) public virtual override returns (uint256) { address caller = _msgSender(); - address authority = proposalGuardian(); + address guardian = proposalGuardian(); - if (authority == address(0)) { + if (guardian == address(0)) { // if there is no proposal guardian // ... only the proposer can cancel // ... no restriction on when the proposer can cancel @@ -36,7 +36,7 @@ abstract contract GovernorProposalGuardian is Governor { address proposer = proposalProposer(proposalId); if (caller != proposer) revert GovernorOnlyProposer(caller); return _cancel(targets, values, calldatas, descriptionHash); - } else if (authority == caller) { + } else if (guardian == caller) { // if there is a proposal guardian, and the caller is the proposal guardian // ... just cancel return _cancel(targets, values, calldatas, descriptionHash);