Skip to content

TimelockGuard#752

Closed
alcueca wants to merge 7 commits intomainfrom
acc/timelock-guard
Closed

TimelockGuard#752
alcueca wants to merge 7 commits intomainfrom
acc/timelock-guard

Conversation

@alcueca
Copy link
Copy Markdown
Contributor

@alcueca alcueca commented Aug 26, 2025

Specs for TimelockGuard


### constructor

### enable(delay_period)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just configure(TimelockGuardConfig) where TimelockGuardConfig is a struct that at least includes a delay uint

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed setDelay so that enable both enables and configures, and a module can be re-enabled to re-configure.

I also added disclaimers to both definitions and the function specification to allow the developer to choose better function and variable names if they can think of any (I don't think we need to decide on enable vs configure).

I don't think I need to require which structs should be used.


To allow for identical transactions to be scheduled more than once, but requiring different signatures for each one, a `salt` parameter can be included in `data` with the sole purpose of differentiating otherwise identical transactions.

### checkTransaction
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A function to see pending txns would be nice too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This revealed a bit of an issue. The TimelockGuard doesn't know which transactions have been executed, for that it would need to peek into the storage of the Safe. more importantly, if a module (UnorderedExecutionModule) overrides the replayability mechanism, the TimelockGuard needs to be tailored to also check in the module storage, if we want the TimelockGuard to be execution-aware.

Called by anyone, verify that the `cancellation_threshold` has been met for the Safe to cancel a given scheduled transaction.

- MUST revert if the TimelockGuard is not enabled for the safe.
- MUST revert if `scheduling_time(safe, tx) + delay_period(safe) >= block.timestamp`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should also check that each owner is actually an owner on the safe, and that the owner set is unique

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We check that the owner is an owner of a valid safe for the transaction in rejectTransaction to avoid spamming.

As for the set of owners being unique, the specs don't include the arguments for the cancelTransaction function, and don't require that a list of owners is provided as an argument.

Also, the owner set won't necessarily be unique. It is possible and valid in a nested setup for the same address to be an owner in two different safes, and to reject and be counted in both.

@alcueca
Copy link
Copy Markdown
Contributor Author

alcueca commented Sep 7, 2025

Superseded by PR761

@alcueca alcueca closed this Sep 7, 2025
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.

3 participants