Skip to content

Commit a21294b

Browse files
maurelianclaudealcuecaalcueca
authored
Full TimelockGuard implementation (#17584)
* feat: implement configureTimelockGuard function - 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 <[email protected]> * feat: implement clearTimelockGuard function - 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 <[email protected]> * refactor: extract guard checking logic to internal helper function - 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 <[email protected]> * feat: implement cancellationThreshold function - 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 <[email protected]> * feat: add placeholder functions for remaining TimelockGuard functionality - 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 <[email protected]> * Self review fixes * Fix warnings on unimplemented functions * Fix names of test functions * Satisfy semgrep by removing revert with string * Remove arg names from unimplemented functions * Snapshots * Add interface * Simplify cancellationThreshold() getter * Replace _getGuard with isGuardEnabled * Allow a timelock delay of zero * TimelockGuard: Add scheduleTransaction() * Add todo note * Pseudocode draft of a non-nested timelock Simpler code Add cancelTransaction function relying on the Safe's internal logic undo type change Add note explaining getScheduledTransactions * Remove signatures field from ExecTransactionParams * Refactor tests with improve utils (_getDummyTx, _getSignaturesForTx) * Test for TransactionCancelled event * Further improve util functions * Add approve hash test case * fix warnings * Use correct typing for Safe addresses This is applied to function inputs/outputs as well as mappings and events * Add additional scheduleTransaction tests * Enable specifying which safeInstance in utility functions * Change cancelTransaction to accept a tx hash * Add increaseCancellationThreshold to cancelTransaction * Add configured boolean to guard config * Fix signature reuse vulnerability in cancelTransaction 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. * Move signature verification before existence check in scheduleTransaction * Remove unused console.logs * Fix increaseCancellationThreshold inputs * Separate cancellation threshold events from transaction cancellation Add CancellationThresholdUpdated event and emit it from threshold modification functions. Remove threshold parameter from TransactionCancelled event for cleaner separation of concerns. * Remove unused _txHash argument from resetCancellation function * Update ITimelockGuard to match implementation * Use configured flag instead of timelockDelay check in clearTimelockGuard * Add configuration check to scheduleTransaction and fix test names * Implement checkTransaction * Add itest placeholder contract * Add comment to checkAfterExecution body * pre-pr checks * Remove GuardConfig.configured boolean field We can simply use `timelock > 0` as an indicator of configuration * Remove clearTimelockGuard The right way to do this is now just to set timelockDelay to zero. * Refactor: Add TransactionBuilder library This library significantly reduces the amount of boilerplace required to setup a Safe transaction, then schedule, cancel, or execute it. * Add unreachable AlreadyExecuted error * Add integration tests * Add getPendingTransactions function and tests * Add tests for getScheduledTransaction * Add _ prefix in front of internal mappings * Rename viewTimelockGuard to timelockSafeConfigurationper specs * Add maxCancellationThreshold * Improve names on getter functions * Remove @dev tags with invariants Avoids duplicating logic between specs and implementation * Update configureTimelockGuard to accept and validate signatures outside of the Safe * Refactor: use a single struct to store all state for a given Safe * Do not unnecessarily reset cancellation threshold when config set to 0 * Revert "Update configureTimelockGuard to accept and validate signatures outside of the Safe" This reverts commit daad53b. * Move timelockDelay out of unnecessary struct * Add top level detail natspec, reorder functions by vis and mutability * Remove test that does not conform to spec * Add cancelTransactionOnSafe to interface as reverting function * Add many more comments * Apply suggestions from code review Co-authored-by: Alberto Cuesta Cañada <[email protected]> * Fix ITimelockGuard iface to match impl * Rename arguments for consistency * Add/fixup @param on events * Small fixes * Fix ITimelockGuard declaration * Improve names on getter functions * Move ExecTransactionParams into TimelockGuard.sol * Address comment nits * Add TimelockGuard_MaxCancellationThreshold_Test and _deploySafe helper * Fix up iface and comment typos * Fix storage lookup in test * Add enum Transaction state and remove cancelled/executed booleans * add /// @Custom:field on ScheduledTransaction struct * add /// @Custom:field on ExecTransactionParams struct * Add SemverComp to enforce minimum Safe version * Rename empty function to signCancellationForSafe * Fix location of external view functions * Add some more comments where helpful. * Further expand on the maxCancellationThreshold rationale * Clarify blocking threshold * iFace fixes * Fix iface * Move update of tx state, event emission and cancellationThreshold into checkAfterExection * Simplified comment * remove unclear comments * fix semgrep sol-style-use-abi-encodecall * snapshots * Add course of actions table * Remove unnecessary address arg from signCancellation * Fix test names * Fix test name validation * Remove enabled guard check from configureTimelockGuard * Allow <ContractName>_Integration_Test in tests * Add isExcludedTest in checkContractNameFilePath() * Update semver-lock * Fix typo * fix typo in tests --------- Co-authored-by: Claude <[email protected]> Co-authored-by: alcueca <[email protected]> Co-authored-by: Alberto Cuesta Cañada <[email protected]>
1 parent 3f2bf28 commit a21294b

File tree

8 files changed

+2234
-3
lines changed

8 files changed

+2234
-3
lines changed
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
// SPDX-License-Identifier: UNLICENSED
2+
pragma solidity ^0.8.4;
3+
4+
library Enum {
5+
type Operation is uint8;
6+
}
7+
8+
interface ITimelockGuard {
9+
enum TransactionState {
10+
NotScheduled,
11+
Pending,
12+
Cancelled,
13+
Executed
14+
}
15+
struct ScheduledTransaction {
16+
uint256 executionTime;
17+
TransactionState state;
18+
ExecTransactionParams params;
19+
}
20+
21+
struct ExecTransactionParams {
22+
address to;
23+
uint256 value;
24+
bytes data;
25+
Enum.Operation operation;
26+
uint256 safeTxGas;
27+
uint256 baseGas;
28+
uint256 gasPrice;
29+
address gasToken;
30+
address payable refundReceiver;
31+
}
32+
33+
error TimelockGuard_GuardNotConfigured();
34+
error TimelockGuard_GuardNotEnabled();
35+
error TimelockGuard_GuardStillEnabled();
36+
error TimelockGuard_InvalidTimelockDelay();
37+
error TimelockGuard_TransactionAlreadyCancelled();
38+
error TimelockGuard_TransactionAlreadyScheduled();
39+
error TimelockGuard_TransactionNotScheduled();
40+
error TimelockGuard_TransactionNotReady();
41+
error TimelockGuard_TransactionAlreadyExecuted();
42+
error TimelockGuard_InvalidVersion();
43+
44+
event CancellationThresholdUpdated(address indexed safe, uint256 oldThreshold, uint256 newThreshold);
45+
event GuardConfigured(address indexed safe, uint256 timelockDelay);
46+
event TransactionCancelled(address indexed safe, bytes32 indexed txHash);
47+
event TransactionScheduled(address indexed safe, bytes32 indexed txHash, uint256 executionTime);
48+
event TransactionExecuted(address indexed safe, bytes32 txHash);
49+
event Message(string message);
50+
51+
function cancelTransaction(address _safe, bytes32 _txHash, uint256 _nonce, bytes memory _signatures) external;
52+
function signCancellation(bytes32 _txHash) external;
53+
function cancellationThreshold(address _safe) external view returns (uint256);
54+
function checkTransaction(
55+
address _to,
56+
uint256 _value,
57+
bytes memory _data,
58+
Enum.Operation _operation,
59+
uint256 _safeTxGas,
60+
uint256 _baseGas,
61+
uint256 _gasPrice,
62+
address _gasToken,
63+
address payable _refundReceiver,
64+
bytes memory _signatures,
65+
address _msgSender
66+
)
67+
external;
68+
function checkAfterExecution(bytes32, bool) external;
69+
function configureTimelockGuard(uint256 _timelockDelay) external;
70+
function scheduledTransaction(
71+
address _safe,
72+
bytes32 _txHash
73+
)
74+
external
75+
view
76+
returns (ScheduledTransaction memory);
77+
function safeConfigs(address) external view returns (uint256 timelockDelay);
78+
function scheduleTransaction(
79+
address _safe,
80+
uint256 _nonce,
81+
ExecTransactionParams memory _params,
82+
bytes memory _signatures
83+
)
84+
external;
85+
function version() external view returns (string memory);
86+
function timelockConfiguration(address _safe) external view returns (uint256 timelockDelay);
87+
function maxCancellationThreshold(address _safe) external view returns (uint256);
88+
function pendingTransactions(address _safe)
89+
external
90+
view
91+
returns (ScheduledTransaction[] memory);
92+
}

packages/contracts-bedrock/scripts/checks/test-validation/exclusions.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,5 +92,6 @@ contracts = [
9292
"OptimismPortal2_MigrateLiquidity_Test", # Interop tests hosted in the OptimismPortal2 test file
9393
"OptimismPortal2_MigrateToSuperRoots_Test", # Interop tests hosted in the OptimismPortal2 test file
9494
"OptimismPortal2_UpgradeInterop_Test", # Interop tests hosted in the OptimismPortal2 test file
95+
"TransactionBuilder", # Transaction builder helper library in TimelockGuard test file
9596
"Constants_Test", # Invalid naming pattern - doesn't specify function or Uncategorized
9697
]

packages/contracts-bedrock/scripts/checks/test-validation/main.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,12 @@ func checkTestStructure(artifact *solc.ForgeArtifact) []error {
194194

195195
func checkTestMethodName(artifact *solc.ForgeArtifact, contractName string, functionName string, _ string) []error {
196196
// Check for uncategorized test pattern
197-
if functionName == "Uncategorized" {
198-
// Pattern: <ContractName>_Uncategorized_Test
199-
return nil
197+
allowedFunctionNames := []string{"Uncategorized", "Integration"}
198+
for _, allowed := range allowedFunctionNames {
199+
if functionName == allowed {
200+
// Pattern: <ContractName>_Uncategorized_Test or <ContractName>_Integration_Test
201+
return nil
202+
}
200203
}
201204
// Pattern: <ContractName>_<FunctionName>_Test - validate function exists
202205
if !checkFunctionExists(artifact, functionName) {
@@ -250,6 +253,11 @@ func checkSrcPath(artifact *solc.ForgeArtifact) bool {
250253
// Validates that contract name matches the file path
251254
func checkContractNameFilePath(artifact *solc.ForgeArtifact) bool {
252255
for filePath, contractName := range artifact.Metadata.Settings.CompilationTarget {
256+
257+
if isExcludedTest(contractName) {
258+
continue
259+
}
260+
253261
// Split contract name to get the base contract name (before first underscore)
254262
contractParts := strings.Split(contractName, "_")
255263
// Split file path to get individual path components

0 commit comments

Comments
 (0)