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

chore: move governance out of core #8823

Merged
merged 2 commits into from
Sep 27, 2024
Merged

chore: move governance out of core #8823

merged 2 commits into from
Sep 27, 2024

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Sep 26, 2024

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 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

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
// 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));
  }
}
// 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);
  }
}

Copy link
Contributor Author

LHerskind commented Sep 26, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @LHerskind and the rest of your teammates on Graphite Graphite

@LHerskind LHerskind marked this pull request as ready for review September 26, 2024 13:50
Base automatically changed from lh/8324-solidity-cleanup to master September 26, 2024 18:45
@LHerskind LHerskind marked this pull request as draft September 27, 2024 13:52
Copy link
Contributor

github-actions bot commented Sep 27, 2024

Changes to public function bytecode sizes

Generated at commit: cb84a18fac5cf53d1082b2cfae82aa4f28588aa9, compared to commit: ffbad355381f9db85a8dbb339af1b190e0ced3aa

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
FPC::public_dispatch +28 ❌ +0.23%
AvmTest::public_dispatch +126 ❌ +0.15%
StatefulTest::public_dispatch -7 ✅ -0.07%
TokenBridge::public_dispatch -35 ✅ -0.10%
Token::public_dispatch -56 ✅ -0.11%
Uniswap::public_dispatch -56 ✅ -0.15%
Auth::public_dispatch -28 ✅ -0.17%
Lending::public_dispatch -108 ✅ -0.18%
CardGame::public_dispatch -40 ✅ -0.18%
NFT::public_dispatch -91 ✅ -0.34%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
FPC::public_dispatch 12,260 (+28) +0.23%
AvmTest::public_dispatch 83,164 (+126) +0.15%
StatefulTest::public_dispatch 9,641 (-7) -0.07%
TokenBridge::public_dispatch 36,365 (-35) -0.10%
Token::public_dispatch 50,096 (-56) -0.11%
Uniswap::public_dispatch 36,507 (-56) -0.15%
Auth::public_dispatch 16,516 (-28) -0.17%
Lending::public_dispatch 61,133 (-108) -0.18%
CardGame::public_dispatch 22,518 (-40) -0.18%
NFT::public_dispatch 26,495 (-91) -0.34%

@AztecBot
Copy link
Collaborator

AztecBot commented Sep 27, 2024

Docs Preview

Hey there! 👋 You can check your preview at https://66f6dafe3c30e087373391f3--aztec-docs-dev.netlify.app

@Maddiaa0
Copy link
Member

Bulloak installation should be added to ./bootstrap.sh

@@ -32,8 +31,8 @@ contract Registry is IRegistry, Ownable {
* @notice Returns the rollup contract
* @return The rollup contract (of type IRollup)
Copy link
Member

@Maddiaa0 Maddiaa0 Sep 27, 2024

Choose a reason for hiding this comment

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

return type here out of date

/**
* @title Data Structures Library
* @author Aztec Labs
* @notice Library that contains data structures used throughout the Aztec protocol
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @notice Library that contains data structures used throughout the Aztec protocol
* @notice Library that contains data structures used throughout Aztec governance

@LHerskind
Copy link
Contributor Author

Bulloak installation should be added to ./bootstrap.sh

Not adding it to the bootstrap as it is mainly something useful for us when building. Checking in ci would require bulloak which make them need cargo as well so seems like a bunch of extra things to download for something most other won't know of or need. Added a readme with information though so we can use it.

@LHerskind LHerskind merged commit 7411acc into master Sep 27, 2024
51 checks passed
@LHerskind LHerskind deleted the lh/prepare-for-gov branch September 27, 2024 16:58
Rumata888 pushed a commit that referenced this pull request Sep 27, 2024
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);
  }
}
```
Rumata888 pushed a commit that referenced this pull request Sep 27, 2024
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);
  }
}
```
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.

4 participants