Skip to content

Commit

Permalink
feat(ctb): Remove setResourceConfig
Browse files Browse the repository at this point in the history 
Removes the `setResourceConfig` setter from the `SystemConfig`, only
allowing it to be set by the `ProxyAdmin` owner.
  • Loading branch information
@clabby
clabby committed May 11, 2024
1 parent d90f128 commit dc2ed7d
Show file tree
Hide file tree
Showing 8 changed files with 110 additions and 140 deletions.
18 changes: 9 additions & 9 deletions packages/contracts-bedrock/.gas-snapshot
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 369443)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 2967411)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 562055)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 4074098)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 467053)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 3512735)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 72672)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 369357)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 2967497)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 561969)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 4074012)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 466924)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 3512606)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 72629)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 92973)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 68453)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 68923)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 68410)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 68880)
GasBenchMark_OptimismPortal:test_proveWithdrawalTransaction_benchmark() (gas: 155618)
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/semver-lock.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@
"sourceCodeHash": "0xd6a894e371c2c7182b5960c507491f81c3775dda0efedd29475f7c30ca07b004"
},
"src/L1/SystemConfig.sol": {
"initCodeHash": "0x68a862089f654e951850708a48025e93231c041b862d05086e1a05be9d9ac4e9",
"sourceCodeHash": "0x3a7cd5254306a01b50c9c68d9e57a57db7ad8163326eb30dff23e368c35ff166"
"initCodeHash": "0xe937c5747e42d6701ff3fbb10471302288fa765fb40cbd9822e22547a42dfa25",
"sourceCodeHash": "0xc57b2508e009f3a0a6ee195720149a7b4fd3027123a85454c796edc7ef85cb9f"
},
"src/L2/BaseFeeVault.sol": {
"initCodeHash": "0x2744d34573be83206d1b75d049d18a7bb37f9058e68c0803e5008c46b0dc2474",
Expand Down
45 changes: 0 additions & 45 deletions packages/contracts-bedrock/snapshots/abi/SystemConfig.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,51 +591,6 @@
"stateMutability": "nonpayable",
"type": "function"
},
{
"inputs": [
{
"components": [
{
"internalType": "uint32",
"name": "maxResourceLimit",
"type": "uint32"
},
{
"internalType": "uint8",
"name": "elasticityMultiplier",
"type": "uint8"
},
{
"internalType": "uint8",
"name": "baseFeeMaxChangeDenominator",
"type": "uint8"
},
{
"internalType": "uint32",
"name": "minimumBaseFee",
"type": "uint32"
},
{
"internalType": "uint32",
"name": "systemTxMaxGas",
"type": "uint32"
},
{
"internalType": "uint128",
"name": "maximumBaseFee",
"type": "uint128"
}
],
"internalType": "struct ResourceMetering.ResourceConfig",
"name": "_config",
"type": "tuple"
}
],
"name": "setResourceConfig",
"outputs": [],
"stateMutability": "nonpayable",
"type": "function"
},
{
"inputs": [
{
Expand Down
8 changes: 0 additions & 8 deletions packages/contracts-bedrock/src/L1/SystemConfig.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,14 +402,6 @@ contract SystemConfig is OwnableUpgradeable, ISemver, IGasToken {
return _resourceConfig;
}

/// @notice An external setter for the resource config.
/// In the future, this method may emit an event that the `op-node` picks up
/// for when the resource config is changed.
/// @param _config The new resource config values.
function setResourceConfig(ResourceMetering.ResourceConfig memory _config) external onlyOwner {
_setResourceConfig(_config);
}

/// @notice An internal setter for the resource config.
/// Ensures that the config is sane before storing it by checking for invariants.
/// @param _config The new resource config.
Expand Down
168 changes: 96 additions & 72 deletions packages/contracts-bedrock/test/L1/SystemConfig.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,102 @@ contract SystemConfig_Initialize_TestFail is SystemConfig_Initialize_Test {
}
}

contract SystemConfig_Init_ResourceConfig is SystemConfig_Init {
/// @dev Tests that `setResourceConfig` reverts if the min base fee
/// is greater than the maximum allowed base fee.
function test_setResourceConfig_badMinMax_reverts() external {
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: 20_000_000,
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 8,
systemTxMaxGas: 1_000_000,
minimumBaseFee: 2 gwei,
maximumBaseFee: 1 gwei
});

vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: min base fee must be less than max base");
_initializeWithResourceConfig(config);
}

/// @dev Tests that `setResourceConfig` reverts if the baseFeeMaxChangeDenominator
/// is zero.
function test_setResourceConfig_zeroDenominator_reverts() external {
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: 20_000_000,
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 0,
systemTxMaxGas: 1_000_000,
minimumBaseFee: 1 gwei,
maximumBaseFee: 2 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: denominator must be larger than 1");
_initializeWithResourceConfig(config);
}

/// @dev Tests that `setResourceConfig` reverts if the gas limit is too low.
function test_setResourceConfig_lowGasLimit_reverts() external {
uint64 gasLimit = systemConfig.gasLimit();

ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: uint32(gasLimit),
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 8,
systemTxMaxGas: uint32(gasLimit),
minimumBaseFee: 1 gwei,
maximumBaseFee: 2 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: gas limit too low");
_initializeWithResourceConfig(config);
}

/// @dev Tests that `setResourceConfig` reverts if the elasticity multiplier
/// and max resource limit are configured such that there is a loss of precision.
function test_setResourceConfig_badPrecision_reverts() external {
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: 20_000_000,
elasticityMultiplier: 11,
baseFeeMaxChangeDenominator: 8,
systemTxMaxGas: 1_000_000,
minimumBaseFee: 1 gwei,
maximumBaseFee: 2 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: precision loss with target resource limit");
_initializeWithResourceConfig(config);
}

/// @dev Helper to initialize the system config with a resource config and default values.
function _initializeWithResourceConfig(ResourceMetering.ResourceConfig memory config) internal {
// Wipe out the initialized slot so the proxy can be initialized again
vm.store(address(systemConfig), bytes32(0), bytes32(0));

address owner = systemConfig.owner();
vm.prank(owner);
systemConfig.initialize({
_owner: owner,
_overhead: 0,
_scalar: 0,
_batcherHash: bytes32(0),
_gasLimit: 1,
_unsafeBlockSigner: address(0),
_config: config,
_batchInbox: address(0),
_addresses: SystemConfig.Addresses({
l1CrossDomainMessenger: address(0),
l1ERC721Bridge: address(0),
l1StandardBridge: address(0),
disputeGameFactory: address(0),
optimismPortal: address(0),
optimismMintableERC20Factory: address(0),
gasPayingToken: address(0)
})
});
}
}

contract SystemConfig_Init_CustomGasToken is SystemConfig_Init {
ERC20 token;

Expand Down Expand Up @@ -391,13 +487,6 @@ contract SystemConfig_Setters_TestFail is SystemConfig_Init {
systemConfig.setUnsafeBlockSigner(address(0x20));
}

/// @dev Tests that `setResourceConfig` reverts if the caller is not the owner.
function test_setResourceConfig_notOwner_reverts() external {
ResourceMetering.ResourceConfig memory config = Constants.DEFAULT_RESOURCE_CONFIG();
vm.expectRevert("Ownable: caller is not the owner");
systemConfig.setResourceConfig(config);
}

/// @dev Tests that `setGasLimit` reverts if the gas limit is too low.
function test_setGasLimit_lowGasLimit_reverts() external {
uint64 minimumGasLimit = systemConfig.minimumGasLimit();
Expand All @@ -413,71 +502,6 @@ contract SystemConfig_Setters_TestFail is SystemConfig_Init {
vm.expectRevert("SystemConfig: gas limit too high");
systemConfig.setGasLimit(maximumGasLimit + 1);
}

/// @dev Tests that `setResourceConfig` reverts if the min base fee
/// is greater than the maximum allowed base fee.
function test_setResourceConfig_badMinMax_reverts() external {
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: 20_000_000,
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 8,
systemTxMaxGas: 1_000_000,
minimumBaseFee: 2 gwei,
maximumBaseFee: 1 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: min base fee must be less than max base");
systemConfig.setResourceConfig(config);
}

/// @dev Tests that `setResourceConfig` reverts if the baseFeeMaxChangeDenominator
/// is zero.
function test_setResourceConfig_zeroDenominator_reverts() external {
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: 20_000_000,
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 0,
systemTxMaxGas: 1_000_000,
minimumBaseFee: 1 gwei,
maximumBaseFee: 2 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: denominator must be larger than 1");
systemConfig.setResourceConfig(config);
}

/// @dev Tests that `setResourceConfig` reverts if the gas limit is too low.
function test_setResourceConfig_lowGasLimit_reverts() external {
uint64 gasLimit = systemConfig.gasLimit();

ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: uint32(gasLimit),
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 8,
systemTxMaxGas: uint32(gasLimit),
minimumBaseFee: 1 gwei,
maximumBaseFee: 2 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: gas limit too low");
systemConfig.setResourceConfig(config);
}

/// @dev Tests that `setResourceConfig` reverts if the elasticity multiplier
/// and max resource limit are configured such that there is a loss of precision.
function test_setResourceConfig_badPrecision_reverts() external {
ResourceMetering.ResourceConfig memory config = ResourceMetering.ResourceConfig({
maxResourceLimit: 20_000_000,
elasticityMultiplier: 11,
baseFeeMaxChangeDenominator: 8,
systemTxMaxGas: 1_000_000,
minimumBaseFee: 1 gwei,
maximumBaseFee: 2 gwei
});
vm.prank(systemConfig.owner());
vm.expectRevert("SystemConfig: precision loss with target resource limit");
systemConfig.setResourceConfig(config);
}
}

contract SystemConfig_Setters_Test is SystemConfig_Init {
Expand Down
1 change: 0 additions & 1 deletion packages/contracts-bedrock/test/Specs.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,6 @@ contract Specification_Test is CommonTest {
_addSpec({ _name: "SystemConfig", _sel: SystemConfig.setBatcherHash.selector, _auth: Role.SYSTEMCONFIGOWNER });
_addSpec({ _name: "SystemConfig", _sel: SystemConfig.setGasConfig.selector, _auth: Role.SYSTEMCONFIGOWNER });
_addSpec({ _name: "SystemConfig", _sel: SystemConfig.setGasLimit.selector, _auth: Role.SYSTEMCONFIGOWNER });
_addSpec({ _name: "SystemConfig", _sel: SystemConfig.setResourceConfig.selector, _auth: Role.SYSTEMCONFIGOWNER });
_addSpec({
_name: "SystemConfig",
_sel: SystemConfig.setUnsafeBlockSigner.selector,
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/test/kontrol/proofs/utils/DeploymentSummary.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ contract DeploymentSummary is DeploymentSummaryCode {
address internal constant safeSingletonAddress = 0x90193C961A926261B756D1E5bb255e67ff9498A1;
address internal constant superchainConfigAddress = 0x068E44eB31e111028c41598E4535be7468674D0A;
address internal constant superchainConfigProxyAddress = 0xDEb1E9a6Be7Baf84208BB6E10aC9F9bbE1D70809;
address internal constant systemConfigAddress = 0x1Fa4ABc046c3B6e20e072df7F869D67566974301;
address internal constant systemConfigAddress = 0x19E90dC1Fb905f20BB3A7eC537B5B812d5FFA682;
address internal constant systemConfigProxyAddress = 0x1c23A6d89F95ef3148BCDA8E242cAb145bf9c0E4;
address internal constant systemOwnerSafeAddress = 0x7d039be7F9b5190147621b02e82B250e1D748e02;

Expand Down Expand Up @@ -281,7 +281,7 @@ contract DeploymentSummary is DeploymentSummaryCode {
value = hex"0000000000000000000000000000000000000000000000000000000000000003";
vm.store(systemOwnerSafeAddress, slot, value);
slot = hex"360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc";
value = hex"0000000000000000000000001fa4abc046c3b6e20e072df7f869d67566974301";
value = hex"00000000000000000000000019e90dc1fb905f20bb3a7ec537b5b812d5ffa682";
vm.store(systemConfigProxyAddress, slot, value);
slot = hex"0000000000000000000000000000000000000000000000000000000000000000";
value = hex"0000000000000000000000000000000000000000000000000000000000000001";
Expand Down

0 comments on commit dc2ed7d

Please sign in to comment.