feat: dependency set refactor #170
Conversation
|
|
||
| /// @notice Thrown when the input chain is already added to the dependency set. | ||
| error ChainAlreadyAdded(); | ||
| error DependencyAlreadyAdded(); | ||
|
|
||
| /// @notice Semantic version. | ||
| /// @custom:semver 1.1.1-beta.5 | ||
| string public constant version = "1.1.1-beta.5"; |
There was a problem hiding this comment.
No need to because it is updated in the main PR that has not been merged to develop yet
| SET_GAS_PAYING_TOKEN, | ||
| ADD_DEPENDENCY, | ||
| REMOVE_DEPENDENCY | ||
| SET_GAS_PAYING_TOKEN |
There was a problem hiding this comment.
Why using a enum with just one member?
| @@ -109,10 +72,6 @@ contract L1BlockInterop is L1Block { | |||
|
|
|||
| if (_type == ConfigType.SET_GAS_PAYING_TOKEN) { | |||
There was a problem hiding this comment.
Same here, are we expecting to add new members to ConfigType?
There was a problem hiding this comment.
Is a good question we could ask to them
There was a problem hiding this comment.
It is how it works, OptimismPortal.setConfig() can send multiple types, so this has to be extensible.
We will be using this feature in another PR so it should stay like this
There was a problem hiding this comment.
Mm... I think that the enum is right cause it will be easier to expand if needed... but this is no needed cause if you have to add another member you have to modify the function too.
There was a problem hiding this comment.
Without the if any call to setConfig means calling _setGasPayingToken and it shouldn't be like that
There was a problem hiding this comment.
Amazing job ser, left only few small comments on the code.
Besides them, 1 more question and 1 suggestion:
- On the PoC, Mark wrote a function to remove dependencies on L1. Do we need to implement it?
- If maintaining the
dependencySetSize()function, missing its tests (the other view functions have tests as well)
| @@ -47,17 +47,18 @@ contract ManageDependenciesInput is BaseDeployIO { | |||
| } | |||
|
|
|||
| contract ManageDependencies is Script { | |||
| // TODO: Fix dependency management since SystemConfig does not handle them anymore | |||
|
|
||
| /// @notice Returns the size of the dependency set. | ||
| /// @return The size of the dependency set. | ||
| function dependencySetSize() external view returns (uint8) { |
| error DependencySetTooLarge(); | ||
|
|
||
| /// @notice Thrown when the input chain ID is the same as the current chain ID. | ||
| error InvalidChainID(); | ||
|
|
||
| /// @notice Thrown when the input chain is already added to the dependency set. |
There was a problem hiding this comment.
| /// @notice Thrown when the input chain is already added to the dependency set. | |
| /// @notice Thrown when the input dependency is already added to the set. |
| abi.encode(numberOfDependencies) | ||
| ); | ||
| /// @notice Tests that `addDependency` reverts when the chain is already in the dependency set. | ||
| function test_addDependency_chainAlreadyExists_reverts(uint256 _chainId) external { |
There was a problem hiding this comment.
NIT: For the code order, this should go below test_addDependency_sameChainID_reverts
| uint256 i; | ||
| for (i; i < type(uint8).max; i++) { | ||
| superchainConfig.addDependency(i, address(systemConfig)); | ||
| } | ||
|
|
||
| assertEq(superchainConfig.dependencySetSize(), type(uint8).max); | ||
|
|
||
| vm.prank(superchainConfig.upgrader()); | ||
| vm.expectRevert(SuperchainConfig.ChainAlreadyAdded.selector); | ||
| superchainConfig.addChain(_chainId, _systemConfig); | ||
| vm.expectRevert(SuperchainConfig.DependencySetTooLarge.selector); | ||
| superchainConfig.addDependency(i + 1, address(systemConfig)); |
There was a problem hiding this comment.
NIT: adding some comments and vars for readability
// Add the maximum number of dependencies to the dependency set
for (uint256 i; i < type(uint8).max; i++) {
superchainConfig.addDependency(i, address(systemConfig));
}
// Check that the dependency set is full and that expect the next call to revert
assertEq(superchainConfig.dependencySetSize(), type(uint8).max);
vm.expectRevert(SuperchainConfig.DependencySetTooLarge.selector);
// Try to add another dependency to the dependency set
uint256 chainId = i + 1;
superchainConfig.addDependency(i + 1, address(systemConfig));
The rationale behind this PR is found in the following PR
The
SuperchainConfigis now the only source for storing thedependencySetThe changes have impact in
L1BlockInterop,SystemConfigInterop,CrossL2InboxandL2toL2CDM