Skip to content
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.

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

Add a public Governor.cancel function #3983

Merged
merged 9 commits into from
Jan 26, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 20, 2023

Fixes #3436

Note: in next breaking (5.0) we should remove the proposer GovernorCompatibilityBravo.ProposalDetails as its is now in Govenor.Proposal

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jan 20, 2023

🦋 Changeset detected

Latest commit: e9e5bfb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Amxx Amxx marked this pull request as ready for review January 20, 2023 16:14
@Amxx Amxx requested a review from frangio January 20, 2023 16:15
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This looks great! I only have a comment about the test helper.

test/helpers/governance.js Outdated Show resolved Hide resolved
@Amxx Amxx requested a review from frangio January 26, 2023 11:01
@frangio
Copy link
Contributor

frangio commented Jan 26, 2023

Looking at the gas benchmarks it seems there is a +30% increase in propose. I believe this is because even though the address proposer field is packed and does not result in a new storage slot in the layout, it is packed with bool executed and bool cancelled, both of which are false initially.

At the same time, execute (and cancel) are now 30% cheaper because the slot is already non-zero.

I think this is a reasonable tradeoff. Note that in 5.0 with better packing the cost of propose will decrease to current levels.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This is good with me if you agree with the tradeoff I explained above.

@Amxx
Copy link
Collaborator Author

Amxx commented Jan 26, 2023

Agree with your analysis and with this tradeoff.

Two things for future PRs:

  • in 5.0 we want to merge that!
  • do we want to had an accessor for the proposer ?

@Amxx Amxx merged commit 5e28952 into OpenZeppelin:master Jan 26, 2023
@Amxx Amxx deleted the feature/governor/public-cancel branch January 26, 2023 19:46
@@ -99,20 +99,15 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp
);
}

function cancel(uint256 proposalId) public virtual override {
function cancel(uint256 proposalId) public virtual override(IGovernor, Governor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs docs explaining the logic, also that it overrides and doesn't call super.cancel from the core Governor.

Comment on lines +220 to +221
* @dev Cancel a proposal. This is restricted to Pending proposal (before the vote starts) and is restricted to
* the proposal's proposer.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really accurate here. The specific logic depends on the modules, for example it's different between Governor and GovernorCompatibilityBravo, so it should be documented in Governor.

@frangio
Copy link
Contributor

frangio commented Jan 26, 2023

do we want to had an accessor for the proposer ?

I don't know. I would say no because the governor contracts are already kind of large in code size (Bravo compat module is over the limit unless compiled with optimizations). Do we know roughly how much code size / deployment cost is added by such a getter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add external cancel to OZ Governor by default
2 participants