design: Fee bridge on old deployment #8756
Assignees
Milestone
Comments
LHerskind
added a commit
that referenced
this issue
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 issue
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 issue
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); } } ```
|
The initial variation of this is handled by #7938, so will be closing this for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The collateral that is deposited into the fee bridge needs to decide which instance it honours in case of an upgrade.
For example, it is following along, an upgrade would mean that there is no collateral to transfer funds to the sequencer to cover the fees paid in the blocks. If this causes a failure to execute that is essentially bricking the old instance, so it should be possible to address separately.
For example, one might be able to check a value in the original bridge, if it is no longer the rollup itself, it might need to alter the fee bridge.
A separate thing to consider, if this is using multiple different bridges, it will be different types or messages that is received on the L2 side of things. Meaning that the L2 contract might not allow inserting directly unless we can let it learn that things have changed, so we might need to look into either having a contract where assets might be deposited into multiple different rollups, or passing along with the constants the address of the "current" fee bridge.
When design is made, consider #7938 for implementation.
The text was updated successfully, but these errors were encountered: