-
Notifications
You must be signed in to change notification settings - Fork 28
Add TimelockControllerEnumerable
#214
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?
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds TimelockControllerEnumerable extending OpenZeppelin’s TimelockController to track and enumerate scheduled operations and batches. Introduces Operation and OperationBatch structs, not-found errors, EnumerableSet-backed indexes, and read APIs. Overrides schedule, scheduleBatch, and cancel to keep enumerable state in sync. Includes tests and README entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Proposer
participant TLE as TimelockControllerEnumerable
participant OZTL as TimelockController (base)
participant IDX as Enumerable Index (sets/maps)
rect rgba(230,245,255,0.6)
Proposer->>TLE: schedule(target,value,data,predecessor,salt,delay)
TLE->>OZTL: schedule(...)
OZTL-->>TLE: scheduled (id)
TLE->>IDX: add operation id + metadata
end
Note over Proposer,TLE: Batch flow mirrors single via scheduleBatch
rect rgba(240,255,230,0.6)
Proposer->>TLE: cancel(id)
TLE->>OZTL: cancel(id)
OZTL-->>TLE: canceled
TLE->>IDX: remove id from sets/maps
end
rect rgba(255,245,230,0.6)
participant Reader
Reader->>TLE: operations()/operation(id)/counts
TLE->>IDX: read ids/records
IDX-->>TLE: data
TLE-->>Reader: Operation/OperationBatch info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Reviewer's GuideExtend the existing TimelockController to support enumeration of scheduled operations and batches by storing operation details in enumerable sets and mappings, overriding schedule/cancel methods to maintain the data structures, exposing view functions for retrieval, and adding unit tests to verify functionality. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
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: 1
🧹 Nitpick comments (9)
contracts/governance/TimelockControllerEnumerable.sol (7)
11-29
: Expose IDs (and optionally timestamps) in returned structs or via companion views.Without returning the operation id, clients must recompute it from fields to query status; also the scheduled timestamp is not exposed. Consider adding lightweight views to return ids (and optionally scheduledAt via getTimestamp(id)).
Proposed non-breaking additions:
+ /// @dev Return operation id at index + function operationIdAt(uint256 index) public view returns (bytes32) { + if (index >= _operationsIdSet.length()) revert OperationIndexNotFound(index); + return _operationsIdSet.at(index); + } + + /// @dev Return operation batch id at index + function operationBatchIdAt(uint256 index) public view returns (bytes32) { + if (index >= _operationsBatchIdSet.length()) revert OperationBatchIndexNotFound(index); + return _operationsBatchIdSet.at(index); + } + + /// @dev Optional: getTimestamp passthrough for convenience + function operationTimestamp(bytes32 id) public view returns (uint256) { + return getTimestamp(id); + }
50-56
: Rename constructor param to match OZ (admin), update NatSpec.TimelockController’s last constructor arg is commonly referred to as admin. Rename to avoid confusion.
- address canceller - ) TimelockController(minDelay, proposers, executors, canceller) {} + address admin + ) TimelockController(minDelay, proposers, executors, admin) {}
57-78
: Minor docs phrasing.“Store the operation the mapping and set” → “in the mapping and set”.
- /// @dev Store the operation the mapping and set + /// @dev Store the operation in the mapping and set
80-101
: Minor docs phrasing.“Store the operationBatch the mapping and set” → “in the mapping and set”.
- /// @dev Store the operationBatch the mapping and set + /// @dev Store the operation batch in the mapping and set
117-126
: Add pagination or ID-only views to avoid heavy returns.Returning full bytes payloads for all ops can hit RPC return limits and be slow. Offer paginated or ID-only variants.
Example:
function operationIds(uint256 offset, uint256 limit) public view returns (bytes32[] memory ids) { uint256 n = _operationsIdSet.length(); if (offset > n) return new bytes32[](0); uint256 end = offset + limit > n ? n : offset + limit; ids = new bytes32[](end - offset); for (uint256 i = offset; i < end; i++) ids[i - offset] = _operationsIdSet.at(i); }
157-166
: Same pagination concern for batches.Mirror pagination/ID-only accessors for batches to keep calls lightweight.
135-155
: Consider convenience views that include status.Many consumers will want pending-only. Add
pendingOperationIds(offset,limit)
that filters byisOperationPending(id)
or exposestatus
enum in a compact view.Would you prefer:
- Track only pending (auto-prune on execute/cancel), or
- Track all-time and expose status?
I can implement either.Also applies to: 175-195
test/governance/TimelockControllerEnumerable.t.sol (2)
61-81
: Assert enumeration semantics post-execution (clarify intended behavior).If you adopt pruning-on-execute, assert operationsCount() == 0. If you keep history, assert operationsCount() == 1 and isOperationPending(id) == false.
Example (prune case):
timelockControllerEnumerable.execute( operation.target, operation.value, operation.data, operation.predecessor, operation.salt ); assertEq(timelockControllerEnumerable.isOperationPending(id), false); + assertEq(timelockControllerEnumerable.operationsCount(), 0);
111-133
: Mirror the post-execution enumeration assertion for batch.Same note as single execute: decide prune vs. keep-history and assert accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/governance/TimelockControllerEnumerable.sol
(1 hunks)test/governance/TimelockControllerEnumerable.t.sol
(1 hunks)
🔇 Additional comments (4)
contracts/governance/TimelockControllerEnumerable.sol (1)
7-10
: Good extension approach and clean OZ integration.Using EnumerableSet with IDs tied to OZ hashOperation/hashOperationBatch keeps the surface area minimal while adding the needed visibility.
test/governance/TimelockControllerEnumerable.t.sol (3)
28-59
: Solid coverage of schedule path and field assertions.Good checks for all struct fields and id-based lookup.
83-109
: Batch schedule assertions look good.Fields, id lookup, and counts are validated correctly.
135-176
: Good cancel-path tests with revert selectors.Nice use of explicit selectors; covers both id and index paths.
Hey @aviggiano, thanks for the PR! Would you mind adding a
After that, please verify that the NatSpec renders correctly. So far the docs engine doesn't render Aside from that this looks good to me for the community version. |
@ernestognw thanks for the review I updated the documentation as requested |
btw you can review the docs preview here: https://deploy-preview-214--community-contracts.netlify.app/community-contracts/0.0.1/api/governance 😄 |
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
Sorry @ernestognw I didn't see your comments Updated everything as requested. |
Fixes #213
Summary by Sourcery
Implement an enumerable extension of TimelockController to allow listing and retrieving scheduled operations and batches.
Enhancements:
Tests:
Summary by CodeRabbit