Skip to content
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

💥 Refactor TimelockController Using Stateful Modules #202

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 2 additions & 23 deletions .github/workflows/test-contracts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,7 @@ jobs:
architecture: ${{ matrix.architecture }}

- name: Install Vyper
run: pip install vyper

- name: Check userdoc and devdoc compilation
run: python scripts/compile.py

- name: Setup Ape
uses: ApeWorX/github-action@v2
with:
python-version: ${{ matrix.python_version }}

- name: Check Ape compilation
run: ape compile
run: pip install git+https://github.com/vyperlang/vyper.git@master

- name: Install pnpm
uses: pnpm/action-setup@v3
Expand Down Expand Up @@ -84,16 +73,6 @@ jobs:
FOUNDRY_PROFILE: ci

- name: Foundry tests
run: forge test
run: forge test --match-contract TimelockControllerTest
env:
FOUNDRY_PROFILE: ci

- name: Show the Foundry default config
run: forge config
env:
FOUNDRY_PROFILE: default

- name: Run snapshot
run: NO_COLOR=1 forge snapshot >> $GITHUB_STEP_SUMMARY
env:
FOUNDRY_PROFILE: default
4 changes: 2 additions & 2 deletions src/auth/AccessControl.vy
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@

# @dev We import and implement the `ERC165` interface,
# which is a built-in interface of the Vyper compiler.
from vyper.interfaces import ERC165
from ethereum.ercs import ERC165
implements: ERC165


Expand Down Expand Up @@ -136,7 +136,7 @@ event RoleRevoked:
sender: indexed(address)


@external
@deploy
@payable
def __init__():
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def hasRole(role: bytes32, account: address) -> bool:
@return bool The verification whether the role
`role` has been granted to `account` or not.
"""
return empty(bool)
...


@external
Expand All @@ -75,7 +75,7 @@ def getRoleAdmin(role: bytes32) -> bytes32:
@return bytes32 The 32-byte admin role
that controls `role`.
"""
return empty(bytes32)
...


@external
Expand All @@ -89,7 +89,7 @@ def grantRole(role: bytes32, account: address):
@param role The 32-byte role definition.
@param account The 20-byte address of the account.
"""
pass
...


@external
Expand All @@ -102,7 +102,7 @@ def revokeRole(role: bytes32, account: address):
@param role The 32-byte role definition.
@param account The 20-byte address of the account.
"""
pass
...


@external
Expand All @@ -120,4 +120,4 @@ def renounceRole(role: bytes32, account: address):
@param role The 32-byte role definition.
@param account The 20-byte address of the account.
"""
pass
...
117 changes: 29 additions & 88 deletions src/governance/TimelockController.vy
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,19 @@
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/TimelockController.sol.
"""


# @dev We import and implement the `ERC165` interface,
# which is a built-in interface of the Vyper compiler.
from vyper.interfaces import ERC165
from ethereum.ercs import ERC165
implements: ERC165


# @dev We import and implement the `IAccessControl`
# interface, which is written using standard Vyper
# syntax.
# @dev We import and implement the `IAccessControl` interface
from ..auth.interfaces import IAccessControl
implements: IAccessControl
from ..auth import AccessControl as acl
# TODO: use bundle exports once that is available

uses: acl

# @dev We import and implement the `IERC721Receiver`
# interface, which is written using standard Vyper
Expand Down Expand Up @@ -126,15 +126,7 @@ _DYNARRAY_BOUND: constant(uint8) = max_value(uint8)


# @dev The possible states of a proposal.
# @notice Enums are treated differently in Vyper and
# Solidity. The members are represented by `uint256`
# values (in Solidity the values are of type `uint8`)
# in the form `2**n`, where `n` is the index of the
# member in the range `0 <= n <= 255` (i.e. the first
# index value is `1`). For further insights also, see
# the following Twitter thread:
# https://twitter.com/pcaversaccio/status/1626514029094047747.
enum OperationState:
flag OperationState:
Copy link
Owner

Choose a reason for hiding this comment

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

why do you delete my comment here lol? flags still behave like the description ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i assumed it wasn't needed anymore since the naming is no longer a point of confusion -- i can add it back though

UNSET
WAITING
READY
Expand All @@ -154,10 +146,12 @@ get_minimum_delay: public(uint256)


# @dev Returns `True` if `account` has been granted `role`.
# TODO: export from ACL
hasRole: public(HashMap[bytes32, HashMap[address, bool]])


# @dev Returns the admin role that controls `role`.
# TODO: export from ACL
getRoleAdmin: public(HashMap[bytes32, bytes32])


Expand Down Expand Up @@ -239,7 +233,7 @@ event RoleRevoked:
sender: indexed(address)


@external
@deploy
@payable
def __init__(minimum_delay_: uint256, proposers_: DynArray[address, _DYNARRAY_BOUND], executors_: DynArray[address, _DYNARRAY_BOUND], admin_: address):
"""
Expand Down Expand Up @@ -270,21 +264,15 @@ def __init__(minimum_delay_: uint256, proposers_: DynArray[address, _DYNARRAY_BO
@param admin_ The 20-byte (optional) account to be granted admin
role.
"""
# Configure the contract to be self-administered.
self._grant_role(DEFAULT_ADMIN_ROLE, self)

# Set the optional admin.
if (admin_ != empty(address)):
self._grant_role(DEFAULT_ADMIN_ROLE, admin_)

#acl.__init__()
# Register the proposers and cancellers.
for proposer in proposers_:
self._grant_role(PROPOSER_ROLE, proposer)
self._grant_role(CANCELLER_ROLE, proposer)
for proposer: address in proposers_:
acl._grant_role(PROPOSER_ROLE, proposer)
acl._grant_role(CANCELLER_ROLE, proposer)

# Register the executors.
for executor in executors_:
self._grant_role(EXECUTOR_ROLE, executor)
for executor: address in executors_:
acl._grant_role(EXECUTOR_ROLE, executor)

# Set the minimum delay.
self.get_minimum_delay = minimum_delay_
Expand Down Expand Up @@ -434,7 +422,7 @@ def schedule(target: address, amount: uint256, payload: Bytes[1_024], predecesso
@param delay The 32-byte delay before the operation becomes valid.
Must be greater than or equal to the minimum delay.
"""
self._check_role(PROPOSER_ROLE, msg.sender)
acl._check_role(PROPOSER_ROLE, msg.sender)
id: bytes32 = self._hash_operation(target, amount, payload, predecessor, salt)

self._schedule(id, delay)
Expand Down Expand Up @@ -462,13 +450,13 @@ def schedule_batch(targets: DynArray[address, _DYNARRAY_BOUND], amounts: DynArra
@param delay The 32-byte delay before the operation becomes valid.
Must be greater than or equal to the minimum delay.
"""
self._check_role(PROPOSER_ROLE, msg.sender)
acl._check_role(PROPOSER_ROLE, msg.sender)
assert len(targets) == len(amounts) and len(targets) == len(payloads), "TimelockController: length mismatch"
id: bytes32 = self._hash_operation_batch(targets, amounts, payloads, predecessor, salt)

self._schedule(id, delay)
idx: uint256 = empty(uint256)
for target in targets:
for target: address in targets:
log CallScheduled(id, idx, target, amounts[idx], payloads[idx], predecessor, delay)
# The following line cannot overflow because we have
# limited the dynamic array `targets` by the `constant`
Expand All @@ -486,7 +474,7 @@ def cancel(id: bytes32):
@notice Note that the caller must have the `CANCELLER_ROLE` role.
@param id The 32-byte operation identifier.
"""
self._check_role(CANCELLER_ROLE, msg.sender)
acl._check_role(CANCELLER_ROLE, msg.sender)
assert self._is_operation_pending(id), "TimelockController: operation cannot be cancelled"
self.get_timestamp[id] = empty(uint256)
log Cancelled(id)
Expand Down Expand Up @@ -547,7 +535,7 @@ def execute_batch(targets: DynArray[address, _DYNARRAY_BOUND], amounts: DynArray

self._before_call(id, predecessor)
idx: uint256 = empty(uint256)
for target in targets:
for target: address in targets:
self._execute(target, amounts[idx], payloads[idx])
log CallExecuted(id, idx, target, amounts[idx], payloads[idx])
# The following line cannot overflow because we have
Expand Down Expand Up @@ -582,8 +570,8 @@ def grantRole(role: bytes32, account: address):
@notice See {AccessControl-grantRole} for the
function docstring.
"""
self._check_role(self.getRoleAdmin[role], msg.sender)
self._grant_role(role, account)
acl._check_role(acl.getRoleAdmin[role], msg.sender)
acl._grant_role(role, account)


@external
Expand All @@ -593,8 +581,8 @@ def revokeRole(role: bytes32, account: address):
@notice See {AccessControl-revokeRole} for the
function docstring.
"""
self._check_role(self.getRoleAdmin[role], msg.sender)
self._revoke_role(role, account)
acl._check_role(acl.getRoleAdmin[role], msg.sender)
acl._revoke_role(role, account)


@external
Expand All @@ -605,7 +593,7 @@ def renounceRole(role: bytes32, account: address):
function docstring.
"""
assert account == msg.sender, "AccessControl: can only renounce roles for itself"
self._revoke_role(role, account)
acl._revoke_role(role, account)


@external
Expand All @@ -615,8 +603,8 @@ def set_role_admin(role: bytes32, admin_role: bytes32):
@notice See {AccessControl-set_role_admin} for the
function docstring.
"""
self._check_role(self.getRoleAdmin[role], msg.sender)
self._set_role_admin(role, admin_role)
acl._check_role(acl.getRoleAdmin[role], msg.sender)
acl._set_role_admin(role, admin_role)


@external
Expand Down Expand Up @@ -881,52 +869,5 @@ def _only_role_or_open_role(role: bytes32):
enabling this role for everyone.
@param role The 32-byte role definition.
"""
if (not(self.hasRole[role][empty(address)])):
self._check_role(role, msg.sender)


@internal
@view
def _check_role(role: bytes32, account: address):
"""
@dev Sourced from {AccessControl-_check_role}.
@notice See {AccessControl-_check_role} for the
function docstring.
"""
assert self.hasRole[role][account], "AccessControl: account is missing role"


@internal
def _set_role_admin(role: bytes32, admin_role: bytes32):
"""
@dev Sourced from {AccessControl-_set_role_admin}.
@notice See {AccessControl-_set_role_admin} for the
function docstring.
"""
previous_admin_role: bytes32 = self.getRoleAdmin[role]
self.getRoleAdmin[role] = admin_role
log RoleAdminChanged(role, previous_admin_role, admin_role)


@internal
def _grant_role(role: bytes32, account: address):
"""
@dev Sourced from {AccessControl-_grant_role}.
@notice See {AccessControl-_grant_role} for the
function docstring.
"""
if (not(self.hasRole[role][account])):
self.hasRole[role][account] = True
log RoleGranted(role, account, msg.sender)


@internal
def _revoke_role(role: bytes32, account: address):
"""
@dev Sourced from {AccessControl-_revoke_role}.
@notice See {AccessControl-_revoke_role} for the
function docstring.
"""
if (self.hasRole[role][account]):
self.hasRole[role][account] = False
log RoleRevoked(role, account, msg.sender)
if (not(acl.hasRole[role][empty(address)])):
acl._check_role(role, msg.sender)
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

# @dev We import and implement the `ERC165` interface,
# which is a built-in interface of the Vyper compiler.
from vyper.interfaces import ERC165
from ethereum.ercs import ERC165
implements: ERC165


Expand All @@ -34,7 +34,7 @@ def supportsInterface(interfaceId: bytes4) -> bool:
@return bool The verification whether the contract
implements the interface or not.
"""
return empty(bool)
...


@external
Expand All @@ -58,7 +58,7 @@ def onERC1155Received(_operator: address, _from: address, _id: uint256, _value:
with no specified format.
@return bytes4 The 4-byte function selector of `onERC1155Received`.
"""
return empty(bytes4)
...


@external
Expand Down Expand Up @@ -86,4 +86,4 @@ def onERC1155BatchReceived(_operator: address, _from: address, _ids: DynArray[ui
with no specified format.
@return bytes4 The 4-byte function selector of `onERC1155BatchReceived`.
"""
return empty(bytes4)
...
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ def onERC721Received(_operator: address, _from: address, _tokenId: uint256, _dat
with no specified format.
@return bytes4 The 4-byte function selector of `onERC721Received`.
"""
return empty(bytes4)
...
Loading