Skip to content

use _validateCancel instead of overriding cancel #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Amxx merged 10 commits into from
Jan 23, 2025
Merged

use _validateCancel instead of overriding cancel #10

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f751ec6
use `_validateCancel` instead of overriding `cancel`
arr00 Jan 10, 2025
d6f0032
fix test, move error to interface
arr00 Jan 21, 2025
5ffeab0
consistent cancellation errors
arr00 Jan 21, 2025
9d378ac
fix tests
arr00 Jan 22, 2025
13f18c5
remove `.only`
arr00 Jan 22, 2025
409e406
move caller into params
arr00 Jan 22, 2025
85c8c15
simplify
arr00 Jan 22, 2025
7a7674b
Update GovernorProposalGuardian.test.js
Amxx Jan 22, 2025
ce162a6
move internal function
arr00 Jan 22, 2025
f83d972
update docs
arr00 Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions contracts/governance/Governor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,11 +484,8 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
// changes it. The `getProposalId` duplication has a cost that is limited, and that we accept.
uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);

// public cancel restrictions (on top of existing _cancel restrictions).
_validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending));
if (_msgSender() != proposalProposer(proposalId)) {
revert GovernorOnlyProposer(_msgSender());
}
address caller = _msgSender();
if (!_validateCancel(proposalId, caller)) revert GovernorUnableToCancel(proposalId, caller);

return _cancel(targets, values, calldatas, descriptionHash);
}
Expand Down Expand Up @@ -805,6 +802,15 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
}
}

/**
* @dev Check if the `caller` can cancel the proposal with the given `proposalId`.
*
* The default implementation allows the proposal proposer to cancel the proposal during the pending state.
*/
function _validateCancel(uint256 proposalId, address caller) internal view virtual returns (bool) {
return (state(proposalId) == ProposalState.Pending) && caller == proposalProposer(proposalId);
}

/**
* @inheritdoc IERC6372
*/
Expand Down
10 changes: 5 additions & 5 deletions contracts/governance/IGovernor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ interface IGovernor is IERC165, IERC6372 {
*/
error GovernorDisabledDeposit();

/**
* @dev The `account` is not a proposer.
*/
error GovernorOnlyProposer(address account);

/**
* @dev The `account` is not the governance executor.
*/
Expand Down Expand Up @@ -112,6 +107,11 @@ interface IGovernor is IERC165, IERC6372 {
*/
error GovernorInvalidSignature(address voter);

/**
* @dev The given `account` is unable to cancel the proposal with given `proposalId`.
*/
error GovernorUnableToCancel(uint256 proposalId, address account);

/**
* @dev Emitted when a proposal is created.
*/
Expand Down
27 changes: 10 additions & 17 deletions contracts/governance/extensions/GovernorProposalGuardian.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,38 +14,31 @@ abstract contract GovernorProposalGuardian is Governor {
event ProposalGuardianSet(address oldProposalGuardian, address newProposalGuardian);

/**
* @dev Override {IGovernor-cancel} that implements the extended cancellation logic.
* @dev Override `_validateCancel` that implements the extended cancellation logic.
*
* * 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, the {proposalProposer} keeps their default rights defined in {IGovernor-cancel} (calling `super`).
* * if the proposal guardian is set, the {proposalProposer} keeps their default rights defined in {IGovernor-_validateCancel} (calling `super`).
*/
function cancel(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
bytes32 descriptionHash
) public virtual override returns (uint256) {
address caller = _msgSender();
function _validateCancel(uint256 proposalId, address caller) internal view virtual override returns (bool) {
address guardian = proposalGuardian();

if (guardian == address(0)) {
// if there is no proposal guardian
// ... only the proposer can cancel
// ... no restriction on when the proposer can cancel
uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
address proposer = proposalProposer(proposalId);
if (caller != proposer) revert GovernorOnlyProposer(caller);
return _cancel(targets, values, calldatas, descriptionHash);
if (caller == proposer) return true;
Copy link
Owner

@Amxx Amxx Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behavior we want is (no fallback)

            return caller == proposalProposer(proposalId);

Copy link
Owner

@Amxx Amxx Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if we want to make do things differently, then I would make it explicit:

            return caller == proposalProposer(proposalId) || super._validateCancel(proposalId, caller);

Copy link
Author

@arr00 arr00 Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The powerful aspect of this method of validation is that additional overrides can be explicitly more permissive. So calling super in all cases that it would be false allows for it to play nice with another possibly more permissive cancelation policy (say anyone can cancel a proposal with very low participation).

Copy link
Owner

@Amxx Amxx Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can understand the logic, but then it has to be documented accordingly.

Currently the documentation says:

if there is no proposal guardian only the proposer can cancel

But that is forgetting that the super call could add additional "permission".

Note that there is a case to make that in certain conditions, we could want to activelly disable permission. For example there would be a module that has a flag disableAllCancel

function _validateCancel(uint256 proposalId, address caller) internal view virtual override returns (bool) {
    reuturn !disableAllCancel && super._validateCancel(proposalId, caller);

In that case you don't want the super call to say yes when disableAllCancel says "no way". Not saying that is what we want here, but it should be discussed.

Copy link
Author

@arr00 arr00 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the docs accordingly. If we are going in the right direction, we should merge this PR and have further conversation on the main PR?

} 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);
} else {
// if there is a proposal guardian, and the caller is not the proposal guardian
// ... apply default behavior
return super.cancel(targets, values, calldatas, descriptionHash);
return true;
}

// if there is no guardian and the caller isn't the proposer or
// there is a proposal guardian, and the caller is not the proposal guardian
// ... apply default behavior
return super._validateCancel(proposalId, caller);
Copy link
Owner

@Amxx Amxx Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// if there is no guardian and the caller isn't the proposer or
// there is a proposal guardian, and the caller is not the proposal guardian
// ... apply default behavior
return super._validateCancel(proposalId, caller);
} else {
// if there is no guardian and the caller isn't the proposer or
// there is a proposal guardian, and the caller is not the proposal guardian
// ... apply default behavior
return super._validateCancel(proposalId, caller);
}

Copy link
Author

@arr00 arr00 Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach here was to not require having two separate calls to super. If its required for readability we can do it this way.

Copy link
Owner

@Amxx Amxx Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a consequence of this discussion: https://github.com/Amxx/openzeppelin-contracts/pull/10/files#r1925693063.

(let keep it to one thread)

}

/**
Expand Down
12 changes: 5 additions & 7 deletions contracts/mocks/governance/GovernorProposalGuardianMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,10 @@ abstract contract GovernorProposalGuardianMock is
return super.proposalThreshold();
}

function cancel(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
bytes32 descriptionHash
) public override(Governor, GovernorProposalGuardian) returns (uint256) {
return super.cancel(targets, values, calldatas, descriptionHash);
function _validateCancel(
uint256 proposalId,
address caller
) internal view override(Governor, GovernorProposalGuardian) returns (bool) {
return super._validateCancel(proposalId, caller);
}
}
36 changes: 10 additions & 26 deletions test/governance/Governor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,21 +624,17 @@ describe('Governor', function () {
await this.helper.connect(this.proposer).propose();

await expect(this.helper.connect(this.owner).cancel('external'))
.to.be.revertedWithCustomError(this.mock, 'GovernorOnlyProposer')
.withArgs(this.owner);
.to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
.withArgs(this.proposal.id, this.owner);
});

it('after vote started', async function () {
await this.helper.propose();
await this.helper.waitForSnapshot(1n); // snapshot + 1 block

await expect(this.helper.cancel('external'))
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
.withArgs(
this.proposal.id,
ProposalState.Active,
GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
);
.to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
.withArgs(this.proposal.id, this.owner);
});

it('after vote', async function () {
Expand All @@ -647,12 +643,8 @@ describe('Governor', function () {
await this.helper.connect(this.voter1).vote({ support: VoteType.For });

await expect(this.helper.cancel('external'))
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
.withArgs(
this.proposal.id,
ProposalState.Active,
GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
);
.to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
.withArgs(this.proposal.id, this.voter1);
});

it('after deadline', async function () {
Expand All @@ -662,12 +654,8 @@ describe('Governor', function () {
await this.helper.waitForDeadline();

await expect(this.helper.cancel('external'))
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
.withArgs(
this.proposal.id,
ProposalState.Succeeded,
GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
);
.to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
.withArgs(this.proposal.id, this.voter1);
});

it('after execution', async function () {
Expand All @@ -678,12 +666,8 @@ describe('Governor', function () {
await this.helper.execute();

await expect(this.helper.cancel('external'))
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
.withArgs(
this.proposal.id,
ProposalState.Executed,
GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
);
.to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
.withArgs(this.proposal.id, this.voter1);
});
});
});
Expand Down
11 changes: 3 additions & 8 deletions test/governance/extensions/GovernorProposalGuardian.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
const { impersonate } = require('../../helpers/account');

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 = 'Proposal Guardian Governor';
const version = '1';
const tokenName = 'MockToken';
Expand Down Expand Up @@ -105,12 +104,8 @@ describe('GovernorProposalGuardian', 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(
this.proposal.id,
ProposalState.Active,
GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
);
.to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
.withArgs(this.proposal.id, this.proposer);
});

it('from proposer when proposal guardian is zero', async function () {
Expand Down