-
Notifications
You must be signed in to change notification settings - Fork 12.4k
feat: Add GovernorDelay extension for simple proposal delays #6214
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
base: master
Are you sure you want to change the base?
feat: Add GovernorDelay extension for simple proposal delays #6214
Conversation
This contract adds a configurable delay to successful proposals, enforcing a waiting period before execution. It includes functions to set and retrieve the delay, as well as checks to ensure the delay is met before executing operations.
Added description for GovernorDelay extension in README.
|
WalkthroughA new Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/governance/extensions/GovernorDelay.test.js (1)
302-355: Consider adding edge case: proposals with identical operations.The multiple proposals test uses different descriptions (
'descr1'vs'descr2') to differentiate proposals. Consider adding a comment or an additional test case that explicitly verifies behavior when proposals have identical targets/values/calldatas but different descriptions, to document that uniqueness comes from the description hash.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/governance/README.adoc(2 hunks)contracts/governance/extensions/GovernorDelay.sol(1 hunks)contracts/mocks/governance/GovernorDelayMock.sol(1 hunks)test/governance/extensions/GovernorDelay.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
contracts/mocks/governance/GovernorDelayMock.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: halmos
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (5)
contracts/governance/extensions/GovernorDelay.sol (1)
1-105: LGTM! Well-structured governance delay extension.The implementation is clean and follows OpenZeppelin patterns:
- Event emission before state mutation follows Checks-Effects-Interactions pattern
- The
uint32delay type provides sufficient range (~136 years) while being gas-efficient- Proper use of
SafeCastandTimeutilities- Clear documentation about timestamp-based delays regardless of clock mode
contracts/governance/README.adoc (2)
39-40: LGTM! Documentation accurately describes the extension.The documentation correctly positions
GovernorDelayas a simpler alternative to external timelocks and clearly explains that the delay is enforced by the Governor itself.
91-92: API entry correctly placed.contracts/mocks/governance/GovernorDelayMock.sol (1)
11-50: LGTM! Proper resolution of multiple inheritance conflicts.The override declarations correctly resolve the diamond inheritance pattern by delegating to
super, which follows the C3 linearization order. Based on learnings, mock contracts don't require additional validation as they're only used for testing purposes.test/governance/extensions/GovernorDelay.test.js (1)
25-357: Comprehensive test coverage for GovernorDelay.The test suite thoroughly covers:
- Deployment verification
- Governance-controlled delay modification with access control check
- Zero vs non-zero delay queuing behavior
- ETA calculation and enforcement
- State transitions through the proposal lifecycle
- Multiple concurrent proposals with independent ETAs
Testing both
blocknumberandtimestamptoken modes ensures the extension works correctly regardless of clock mode, which aligns with the documented behavior.
Implements issue #6052 by adding a simple way to enforce delays on all proposals without requiring external timelock contracts.
Changes
GovernorDelayextension that enforces a configurable delay on all proposalsGovernorTimelockControlwhich uses external timelock)block.timestamp(seconds) regardless of governor's clock modeTesting