Add enumerableSet and get pending functionality#17533
Closed
maurelian wants to merge 47 commits intojm/timelock-nonnestedfrom
Closed
Add enumerableSet and get pending functionality#17533maurelian wants to merge 47 commits intojm/timelock-nonnestedfrom
maurelian wants to merge 47 commits intojm/timelock-nonnestedfrom
Conversation
- Add configureTimelockGuard function to allow Safes to set timelock delays - Validate timelock delay between 1 second and 1 year - Check that guard is properly enabled on calling Safe using getStorageAt() - Store configuration per Safe with GuardConfigured event emission - Add comprehensive test suite covering all spec requirements - Implement IGuard interface for Safe compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add clearTimelockGuard function to allow Safes to clear timelock configuration - Validate that guard is disabled before allowing configuration clearing - Check that Safe was previously configured before clearing - Delete configuration data and emit GuardCleared event - Add comprehensive test suite covering all spec requirements - Add new errors: TimelockGuard_GuardNotConfigured, TimelockGuard_GuardStillEnabled 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add internal _getGuard() helper to centralize guard address retrieval - Update configureTimelockGuard and clearTimelockGuard to use helper - Reduces code duplication and improves maintainability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add cancellationThreshold function to return current threshold for a Safe - Return 0 if guard not enabled or not configured - Initialize to 1 when Safe configures guard - Clear threshold when Safe clears guard configuration - Add comprehensive test suite covering all spec requirements - Function never reverts as per spec requirements 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…lity - Add scheduleTransaction placeholder (Function 4) - Add checkPendingTransactions placeholder (Function 6) - Add rejectTransaction placeholder (Function 7) - Add rejectTransactionWithSignature placeholder (Function 8) - Add cancelTransaction placeholder (Function 9) - Update checkTransaction placeholder (Function 5) - All placeholders have proper signatures and documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Simpler code Add cancelTransaction function relying on the Safe's internal logic undo type change Add note explaining getScheduledTransactions
This is applied to function inputs/outputs as well as mappings and events
Include transaction hash in cancellation signature data to prevent signatures from being reused across different transactions with the same nonce. Updates both contract implementation and tests.
Add CancellationThresholdUpdated event and emit it from threshold modification functions. Remove threshold parameter from TransactionCancelled event for cleaner separation of concerns.
We can simply use `timelock > 0` as an indicator of configuration
The right way to do this is now just to set timelockDelay to zero.
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Closed
maurelian
commented
Sep 19, 2025
| uint256 executionTime; | ||
| bool cancelled; | ||
| bool executed; | ||
| ExecTransactionParams params; |
Contributor
Author
There was a problem hiding this comment.
This is a big change, as previously we were not actually storing the unhashed transaction params in state. Though I think it's a necessary improvement unless we're planning on some complex event parsing.
maurelian
commented
Sep 19, 2025
| mapping(Safe => EnumerableSet.Bytes32Set) internal safePendingTxHashes; | ||
|
|
||
| /// @notice Mapping from Safe to cancellation threshold. | ||
| mapping(Safe => uint256) internal safeCancellationThreshold; |
Contributor
Author
There was a problem hiding this comment.
I usually think it's a smell when you have several mappings with the same key. I will think a bit about which of the values being accessed these mappings need to be kept separate and which can be combined into a struct.
maurelian
commented
Sep 19, 2025
Comment on lines
+115
to
+120
| /// @dev WARNING: This operation will copy the entire set of pending transactions to memory, | ||
| /// which can be quite expensive. This is designed only to be used by view accessors that are | ||
| /// queried without any gas fees. Developers should keep in mind that this function has an | ||
| /// unbounded cost, and using it as part of a state-changing function may render the function | ||
| /// uncallable if the set grows to a point where copying to memory consumes too much gas to fit | ||
| /// in a block. |
Contributor
Author
There was a problem hiding this comment.
The alternative to this is some kind of pagination like in the Safe's ModuleManager.
I would rather keep it simple and assume that this will only run offchain. If we're very worried about someone reading this on chain, then we could add require(msg.sender == address(0)) which we've done elsewhere to enable/enforce offchain usage.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## jm/timelock-nonnested #17533 +/- ##
======================================================
Coverage 95.92% 95.93%
======================================================
Files 112 113 +1
Lines 5127 5212 +85
======================================================
+ Hits 4918 5000 +82
- Misses 209 212 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
maurelian
force-pushed
the
jm/timelock-nonnested
branch
from
8f252c5 to
581cab0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a draft PR intended primarily to get feedback on the proof of concept implementation of a getter that will return all pending transactions.
It uses OZ's Enumerable set, which a well battle tested dependency, including within the LivenessGuard.