Skip to content

Commit

Permalink
chore: move governance out of core (#8823)
Browse files Browse the repository at this point in the history
Slight extension on #8818 to separate gov and instance more clearly. 

Gov does not have dependencies on core, but core can depend on the gov. 

Removes the registry value from the `Rollup` does not address that an
upgrade would make the fee contract unbacked, that is to be handled
separately in #8756.

Updates testing for the `Registry` contract to use the [Branching Tree
Technique](https://www.youtube.com/watch?v=0-EmbNVgFA4) from Paul. Found
it quite neat. If using `bulloak` as well quite convenient to build the
setups.

The TL;DR is, build a `.tree` file outlining the test for a function,
and then use `bulloak scaffold` to prepare a testing file and then fill
in the logic. Makes it quite nice to follow the logic.

# Example time

```.tree
UpgradeTest
├── when caller is not owner
│   └── it should revert
└── when caller is owner
    ├── when rollup already in set
    │   └── it should revert
    └── when rollup not already in set
        ├── it should add the rollup to state
        └── it should emit a {InstanceAdded} event
```

```solidity
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.27;

import {Test} from "forge-std/Test.sol";

import {Registry} from "@aztec/governance/Registry.sol";

contract RegistryBase is Test {
  Registry internal registry;

  function setUp() public {
    registry = new Registry(address(this));
  }
}
```

```solidity
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.27;

import {RegistryBase} from "./Base.t.sol";

import {Ownable} from "@oz/access/Ownable.sol";

import {IRegistry} from "@aztec/governance/interfaces/IRegistry.sol";
import {Errors} from "@aztec/governance/libraries/Errors.sol";
import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol";

contract UpgradeTest is RegistryBase {
  function test_RevertWhen_CallerIsNotOwner(address _caller) external {
    // it should revert
    vm.assume(_caller != address(this));
    vm.prank(_caller);
    vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, _caller));
    registry.upgrade(address(uint160(uint256(bytes32("new instance")))));
  }

  modifier whenCallerIsOwner() {
    _;
  }

  function test_RevertWhen_RollupAlreadyInSet() external whenCallerIsOwner {
    // it should revert
    address rollup = registry.getRollup();

    vm.expectRevert(
      abi.encodeWithSelector(Errors.Registry__RollupAlreadyRegistered.selector, rollup)
    );
    registry.upgrade(rollup);
  }

  function test_WhenRollupNotAlreadyInSet(address _rollup) external whenCallerIsOwner {
    // it should add the rollup to state
    // it should emit a {InstanceAdded} event
    vm.assume(_rollup != address(0xdead));

    {
      DataStructures.RegistrySnapshot memory snapshotBefore = registry.getCurrentSnapshot();
      assertEq(snapshotBefore.blockNumber, block.number);
      assertEq(snapshotBefore.rollup, address(0xdead));
      assertEq(registry.numberOfVersions(), 1);
    }

    vm.expectEmit(true, true, false, false, address(registry));
    emit IRegistry.InstanceAdded(_rollup, 1);
    registry.upgrade(_rollup);

    assertEq(registry.numberOfVersions(), 2);

    DataStructures.RegistrySnapshot memory snapshot = registry.getCurrentSnapshot();
    assertEq(snapshot.blockNumber, block.number);
    assertGt(snapshot.blockNumber, 0);
    assertEq(snapshot.rollup, _rollup);
  }
}
```
  • Loading branch information
LHerskind authored and Rumata888 committed Sep 27, 2024
1 parent 74025c9 commit 55e24c2
Show file tree
Hide file tree
Showing 32 changed files with 391 additions and 141 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,11 @@ A message that is sent from L2 to L1.

A snapshot of the registry values.

#include_code registry_snapshot l1-contracts/src/core/libraries/DataStructures.sol solidity
#include_code registry_snapshot l1-contracts/src/governance/libraries/DataStructures.sol solidity

| Name | Type | Description |
| -------------- | ------- | ----------- |
| `rollup` | `address` | The address of the rollup contract for the snapshot. |
| `inbox` | `address` | The address of the inbox contract for the snapshot. |
| `outbox` | `address` | The address of the outbox contract for the snapshot. |
| `blockNumber` | `uint256` | The block number at which the snapshot was created. |


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ tags: [portals, contracts]

The registry is a contract deployed on L1, that contains addresses for the `Rollup`. It also keeps track of the different versions that have been deployed and let you query prior deployments easily.

**Links**: [Interface (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/master/l1-contracts/src/core/interfaces/messagebridge/IRegistry.sol), [Implementation (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/master/l1-contracts/src/core/messagebridge/Registry.sol).
**Links**: [Interface (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/master/l1-contracts/src/governance/interfaces/IRegistry.sol), [Implementation (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/master/l1-contracts/src/governance/Registry.sol).

## `numberOfVersions()`

Retrieves the number of versions that have been deployed.

#include_code registry_number_of_versions l1-contracts/src/core/interfaces/messagebridge/IRegistry.sol solidity
#include_code registry_number_of_versions l1-contracts/src/governance/interfaces/IRegistry.sol solidity

| Name | Description |
| -------------- | ----------- |
Expand All @@ -20,17 +20,17 @@ Retrieves the number of versions that have been deployed.
## `getRollup()`
Retrieves the current rollup contract.

#include_code registry_get_rollup l1-contracts/src/core/interfaces/messagebridge/IRegistry.sol solidity
#include_code registry_get_rollup l1-contracts/src/governance/interfaces/IRegistry.sol solidity

| Name | Description |
| -------------- | ----------- |
| ReturnValue | The current rollup |

## `getVersionFor(address _rollup)`

Retrieve the version of a specific rollup contract.
Retrieve the version of a specific rollup contract.

#include_code registry_get_version_for l1-contracts/src/core/interfaces/messagebridge/IRegistry.sol solidity
#include_code registry_get_version_for l1-contracts/src/governance/interfaces/IRegistry.sol solidity

| Name | Description |
| -------------- | ----------- |
Expand All @@ -42,10 +42,10 @@ Will revert with `Registry__RollupNotRegistered(_rollup)` if the rollup have not

## `getSnapshot(uint256 _version)`

Retrieve the snapshot of a specific version.
Retrieve the snapshot of a specific version.

#include_code registry_snapshot l1-contracts/src/core/libraries/DataStructures.sol solidity
#include_code registry_get_snapshot l1-contracts/src/core/interfaces/messagebridge/IRegistry.sol solidity
#include_code registry_snapshot l1-contracts/src/governance/libraries/DataStructures.sol solidity
#include_code registry_get_snapshot l1-contracts/src/governance/interfaces/IRegistry.sol solidity

| Name | Description |
| -------------- | ----------- |
Expand All @@ -58,7 +58,7 @@ Retrieve the snapshot of a specific version.

Retrieves the snapshot for the current version.

#include_code registry_get_current_snapshot l1-contracts/src/core/interfaces/messagebridge/IRegistry.sol solidity
#include_code registry_get_current_snapshot l1-contracts/src/governance/interfaces/IRegistry.sol solidity

| Name | Description |
| -------------- | ----------- |
Expand Down
28 changes: 26 additions & 2 deletions l1-contracts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,29 @@ Currently not running any proofs _nor_ access control so blocks can be submitted

---

# Branching Tree Technique

For writing tests we will be using the [Branching Tree Technique](https://www.youtube.com/watch?v=0-EmbNVgFA4).
The approach is simple, for a function that is to be tested (all functions) you should write a `.tree` file first.
Then the tree file can be used to generate a solidity tests file using [Bulloak](https://github.com/alexfertel/bulloak) by running the `scaffold` command.

```bash
bulloak scaffold -w **/*.tree
```

To check that the tests are following the expected format, you can run the `check` command.

```bash
bulloak check **/*.tree
```

We assume that you already have `bulloak` installed, if not you can install it as `cargo install bulloak`.
Also, we suggest using [Ascii Tree Generator](https://marketplace.visualstudio.com/items?itemName=aprilandjan.ascii-tree-generator), since the pipes can be a bit of a pain otherwise.

For examples, see the tests for the registry.

---

# Linter

We use an extended version of solhint (https://github.com/LHerskind/solhint) to include custom rules. These custom rules relate to how errors should be named, using custom errors instead of reverts etc, see `.solhint.json` for more specifics about the rules.
Expand All @@ -73,12 +96,13 @@ yarn lint
# Slither & Slitherin

We use slither as an automatic way to find blunders and common vulnerabilities in our contracts. It is not part of the docker image due to its slowness, but it can be run using the following command to generate a markdown file with the results:

```bash
yarn slither
```

When this command is run in CI, it will fail if the markdown file generated in docker don't match the one in the repository.
When this command is run in CI, it will fail if the markdown file generated in docker don't match the one in the repository.

We assume that you already have slither installed. You can install it with `pip3 install slither-analyzer==0.10.0 slitherin==0.5.0`. It is kept out of the bootstrap script as it is not a requirement for people who just want to run tests or are uninterested in the contracts.

> We are not running the `naming-convention` detector because we have our own rules for naming which is enforced by the linter.
> We are not running the `naming-convention` detector because we have our own rules for naming which is enforced by the linter.
5 changes: 3 additions & 2 deletions l1-contracts/src/core/FeeJuicePortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ pragma solidity >=0.8.27;
import {IERC20} from "@oz/token/ERC20/IERC20.sol";
import {IFeeJuicePortal} from "@aztec/core/interfaces/IFeeJuicePortal.sol";
import {IInbox} from "@aztec/core/interfaces/messagebridge/IInbox.sol";
import {IRegistry} from "@aztec/core/interfaces/messagebridge/IRegistry.sol";
import {IRegistry} from "@aztec/governance/interfaces/IRegistry.sol";
import {IRollup} from "@aztec/core/interfaces/IRollup.sol";

import {Constants} from "@aztec/core/libraries/ConstantsGen.sol";
import {DataStructures} from "@aztec/core/libraries/DataStructures.sol";
Expand Down Expand Up @@ -74,7 +75,7 @@ contract FeeJuicePortal is IFeeJuicePortal, Ownable {
returns (bytes32)
{
// Preamble
IInbox inbox = registry.getRollup().INBOX();
IInbox inbox = IRollup(registry.getRollup()).INBOX();
DataStructures.L2Actor memory actor = DataStructures.L2Actor(l2TokenAddress, 1);

// Hash the message content to be reconstructed in the receiving contract
Expand Down
4 changes: 0 additions & 4 deletions l1-contracts/src/core/Rollup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {IProofCommitmentEscrow} from "@aztec/core/interfaces/IProofCommitmentEsc
import {IInbox} from "@aztec/core/interfaces/messagebridge/IInbox.sol";
import {IOutbox} from "@aztec/core/interfaces/messagebridge/IOutbox.sol";
import {IFeeJuicePortal} from "@aztec/core/interfaces/IFeeJuicePortal.sol";
import {IRegistry} from "@aztec/core/interfaces/messagebridge/IRegistry.sol";
import {IRollup, ITestRollup} from "@aztec/core/interfaces/IRollup.sol";
import {IVerifier} from "@aztec/core/interfaces/IVerifier.sol";

Expand Down Expand Up @@ -56,7 +55,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
uint256 public constant PROOF_COMMITMENT_MIN_BOND_AMOUNT_IN_TST = 1000;

uint256 public immutable L1_BLOCK_AT_GENESIS;
IRegistry public immutable REGISTRY;
IInbox public immutable INBOX;
IOutbox public immutable OUTBOX;
IProofCommitmentEscrow public immutable PROOF_COMMITMENT_ESCROW;
Expand Down Expand Up @@ -85,15 +83,13 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
IVerifier public epochProofVerifier;

constructor(
IRegistry _registry,
IFeeJuicePortal _fpcJuicePortal,
bytes32 _vkTreeRoot,
address _ares,
address[] memory _validators
) Leonidas(_ares) {
blockProofVerifier = new MockVerifier();
epochProofVerifier = new MockVerifier();
REGISTRY = _registry;
FEE_JUICE_PORTAL = _fpcJuicePortal;
PROOF_COMMITMENT_ESCROW = new MockProofCommitmentEscrow();
INBOX = IInbox(address(new Inbox(address(this), Constants.L1_TO_L2_MSG_SUBTREE_HEIGHT)));
Expand Down
12 changes: 0 additions & 12 deletions l1-contracts/src/core/libraries/DataStructures.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,6 @@ library DataStructures {
}
// docs:end:l2_to_l1_msg

// docs:start:registry_snapshot
/**
* @notice Struct for storing address of cross communication components and the block number when it was updated
* @param rollup - The address of the rollup contract
* @param blockNumber - The block number of the snapshot
*/
struct RegistrySnapshot {
address rollup;
uint256 blockNumber;
}
// docs:end:registry_snapshot

/**
* @notice Struct for storing flags for block header validation
* @param ignoreDA - True will ignore DA check, otherwise checks
Expand Down
4 changes: 0 additions & 4 deletions l1-contracts/src/core/libraries/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@ library Errors {
error Rollup__TryingToProveNonExistingBlock(); // 0x34ef4954
error Rollup__UnavailableTxs(bytes32 txsHash); // 0x414906c3

// Registry
error Registry__RollupNotRegistered(address rollup); // 0xa1fee4cf
error Registry__RollupAlreadyRegistered(address rollup); // 0x3c34eabf

//TxsDecoder
error TxsDecoder__InvalidLogsLength(uint256 expected, uint256 actual); // 0x829ca981
error TxsDecoder__TxsTooLarge(uint256 expected, uint256 actual); // 0xc7d44a62
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@
// Copyright 2023 Aztec Labs.
pragma solidity >=0.8.27;

import {IRegistry} from "@aztec/core/interfaces/messagebridge/IRegistry.sol";
import {IRollup} from "@aztec/core/interfaces/IRollup.sol";
import {IRegistry} from "@aztec/governance/interfaces/IRegistry.sol";

import {DataStructures} from "@aztec/core/libraries/DataStructures.sol";
import {Errors} from "@aztec/core/libraries/Errors.sol";
import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol";
import {Errors} from "@aztec/governance/libraries/Errors.sol";

import {Ownable} from "@oz/access/Ownable.sol";

Expand All @@ -29,11 +28,11 @@ contract Registry is IRegistry, Ownable {
}

/**
* @notice Returns the rollup contract
* @return The rollup contract (of type IRollup)
* @notice Returns the address of the rollup contract
* @return The rollup address
*/
function getRollup() external view override(IRegistry) returns (IRollup) {
return IRollup(currentSnapshot.rollup);
function getRollup() external view override(IRegistry) returns (address) {
return currentSnapshot.rollup;
}

/**
Expand Down Expand Up @@ -109,11 +108,13 @@ contract Registry is IRegistry, Ownable {
snapshots[version] = newSnapshot;
rollupToVersion[_rollup] = version;

emit InstanceAdded(_rollup, version);

return version;
}

function _getVersionFor(address _rollup) internal view returns (uint256 version, bool exists) {
version = rollupToVersion[_rollup];
return (version, version > 0);
return (version, version > 0 || snapshots[0].rollup == _rollup);
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
// SPDX-License-Identifier: Apache-2.0
pragma solidity >=0.8.27;

import {DataStructures} from "../../libraries/DataStructures.sol";
import {IRollup} from "../IRollup.sol";
import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol";

interface IRegistry {
event InstanceAdded(address indexed instance, uint256 indexed version);

// docs:start:registry_upgrade
function upgrade(address _rollup) external returns (uint256);
// docs:end:registry_upgrade

// docs:start:registry_get_rollup
function getRollup() external view returns (IRollup);
function getRollup() external view returns (address);
// docs:end:registry_get_rollup

// docs:start:registry_get_version_for
Expand Down
22 changes: 22 additions & 0 deletions l1-contracts/src/governance/libraries/DataStructures.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2023 Aztec Labs.
pragma solidity >=0.8.18;

/**
* @title Data Structures Library
* @author Aztec Labs
* @notice Library that contains data structures used throughout Aztec governance
*/
library DataStructures {
// docs:start:registry_snapshot
/**
* @notice Struct for storing address of cross communication components and the block number when it was updated
* @param rollup - The address of the rollup contract
* @param blockNumber - The block number of the snapshot
*/
struct RegistrySnapshot {
address rollup;
uint256 blockNumber;
}
// docs:end:registry_snapshot
}
16 changes: 16 additions & 0 deletions l1-contracts/src/governance/libraries/Errors.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2023 Aztec Labs.
pragma solidity >=0.8.18;

/**
* @title Errors Library
* @author Aztec Labs
* @notice Library that contains errors used throughout the Aztec governance
* Errors are prefixed with the contract name to make it easy to identify where the error originated
* when there are multiple contracts that could have thrown the error.
*/
library Errors {
// Registry
error Registry__RollupNotRegistered(address rollup); // 0xa1fee4cf
error Registry__RollupAlreadyRegistered(address rollup); // 0x3c34eabf
}
62 changes: 0 additions & 62 deletions l1-contracts/test/Registry.t.sol

This file was deleted.

Loading

0 comments on commit 55e24c2

Please sign in to comment.