Skip to content

Commit

Permalink
fix(ct): correct input and return argument formatting (#12131)
Browse files Browse the repository at this point in the history 
Standardizes the input and return argument formatting everywhere.
  • Loading branch information
@smartcontracts
smartcontracts authored Sep 25, 2024
1 parent c6d3adf commit d8cb523
Show file tree
Hide file tree
Showing 56 changed files with 471 additions and 430 deletions.
18 changes: 17 additions & 1 deletion .semgrepignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,25 @@ tests/
.semgrep_logs/

op-chain-ops/script/testdata
op-chain-ops/script/testdata/scripts/ScriptExample.s.sol

packages/*/node_modules
packages/*/test

# Autogenerated solidity library
# TODO: Define these exclusions inside of the semgrep rules once those rules
# are all defined locally in the repository instead of the semgrep app.

# Contracts: autogenerated solidity library
packages/contracts-bedrock/scripts/libraries/Solarray.sol

# Contracts: vendor interfaces
packages/contracts-bedrock/scripts/interfaces/IGnosisSafe.sol
packages/contracts-bedrock/src/EAS/

# Contracts: deliberate exclusions
packages/contracts-bedrock/src/universal/WETH98.sol
packages/contracts-bedrock/src/universal/interfaces/IWETH.sol
packages/contracts-bedrock/src/L2/SuperchainWETH.sol
packages/contracts-bedrock/src/L2/interfaces/ISuperchainWETH.sol
packages/contracts-bedrock/src/governance/GovernanceToken.sol
packages/contracts-bedrock/src/governance/interfaces/IGovernanceToken.sol
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/scripts/DeployAuthSystem.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ contract DeployAuthSystemInput is CommonBase {
contract DeployAuthSystemOutput is CommonBase {
Safe internal _safe;

function set(bytes4 sel, address _address) public {
if (sel == this.safe.selector) _safe = Safe(payable(_address));
function set(bytes4 _sel, address _address) public {
if (_sel == this.safe.selector) _safe = Safe(payable(_address));
else revert("DeployAuthSystemOutput: unknown selector");
}

Expand Down
54 changes: 27 additions & 27 deletions packages/contracts-bedrock/scripts/DeployImplementations.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,41 +64,41 @@ contract DeployImplementationsInput is BaseDeployIO {

string internal _standardVersionsToml;

function set(bytes4 sel, uint256 _value) public {
function set(bytes4 _sel, uint256 _value) public {
require(_value != 0, "DeployImplementationsInput: cannot set zero value");

if (sel == this.withdrawalDelaySeconds.selector) {
if (_sel == this.withdrawalDelaySeconds.selector) {
_withdrawalDelaySeconds = _value;
} else if (sel == this.minProposalSizeBytes.selector) {
} else if (_sel == this.minProposalSizeBytes.selector) {
_minProposalSizeBytes = _value;
} else if (sel == this.challengePeriodSeconds.selector) {
} else if (_sel == this.challengePeriodSeconds.selector) {
require(_value <= type(uint64).max, "DeployImplementationsInput: challengePeriodSeconds too large");
_challengePeriodSeconds = _value;
} else if (sel == this.proofMaturityDelaySeconds.selector) {
} else if (_sel == this.proofMaturityDelaySeconds.selector) {
_proofMaturityDelaySeconds = _value;
} else if (sel == this.disputeGameFinalityDelaySeconds.selector) {
} else if (_sel == this.disputeGameFinalityDelaySeconds.selector) {
_disputeGameFinalityDelaySeconds = _value;
} else {
revert("DeployImplementationsInput: unknown selector");
}
}

function set(bytes4 sel, string memory _value) public {
function set(bytes4 _sel, string memory _value) public {
require(!LibString.eq(_value, ""), "DeployImplementationsInput: cannot set empty string");
if (sel == this.release.selector) _release = _value;
else if (sel == this.standardVersionsToml.selector) _standardVersionsToml = _value;
if (_sel == this.release.selector) _release = _value;
else if (_sel == this.standardVersionsToml.selector) _standardVersionsToml = _value;
else revert("DeployImplementationsInput: unknown selector");
}

function set(bytes4 sel, address _addr) public {
function set(bytes4 _sel, address _addr) public {
require(_addr != address(0), "DeployImplementationsInput: cannot set zero address");
if (sel == this.superchainConfigProxy.selector) _superchainConfigProxy = SuperchainConfig(_addr);
else if (sel == this.protocolVersionsProxy.selector) _protocolVersionsProxy = ProtocolVersions(_addr);
if (_sel == this.superchainConfigProxy.selector) _superchainConfigProxy = SuperchainConfig(_addr);
else if (_sel == this.protocolVersionsProxy.selector) _protocolVersionsProxy = ProtocolVersions(_addr);
else revert("DeployImplementationsInput: unknown selector");
}

function set(bytes4 sel, bytes32 _value) public {
if (sel == this.salt.selector) _salt = _value;
function set(bytes4 _sel, bytes32 _value) public {
if (_sel == this.salt.selector) _salt = _value;
else revert("DeployImplementationsInput: unknown selector");
}

Expand Down Expand Up @@ -179,22 +179,22 @@ contract DeployImplementationsOutput is BaseDeployIO {
OptimismMintableERC20Factory internal _optimismMintableERC20FactoryImpl;
DisputeGameFactory internal _disputeGameFactoryImpl;

function set(bytes4 sel, address _addr) public {
function set(bytes4 _sel, address _addr) public {
require(_addr != address(0), "DeployImplementationsOutput: cannot set zero address");

// forgefmt: disable-start
if (sel == this.opcmProxy.selector) _opcmProxy = OPContractsManager(payable(_addr));
else if (sel == this.opcmImpl.selector) _opcmImpl = OPContractsManager(payable(_addr));
else if (sel == this.optimismPortalImpl.selector) _optimismPortalImpl = OptimismPortal2(payable(_addr));
else if (sel == this.delayedWETHImpl.selector) _delayedWETHImpl = DelayedWETH(payable(_addr));
else if (sel == this.preimageOracleSingleton.selector) _preimageOracleSingleton = PreimageOracle(_addr);
else if (sel == this.mipsSingleton.selector) _mipsSingleton = MIPS(_addr);
else if (sel == this.systemConfigImpl.selector) _systemConfigImpl = SystemConfig(_addr);
else if (sel == this.l1CrossDomainMessengerImpl.selector) _l1CrossDomainMessengerImpl = L1CrossDomainMessenger(_addr);
else if (sel == this.l1ERC721BridgeImpl.selector) _l1ERC721BridgeImpl = L1ERC721Bridge(_addr);
else if (sel == this.l1StandardBridgeImpl.selector) _l1StandardBridgeImpl = L1StandardBridge(payable(_addr));
else if (sel == this.optimismMintableERC20FactoryImpl.selector) _optimismMintableERC20FactoryImpl = OptimismMintableERC20Factory(_addr);
else if (sel == this.disputeGameFactoryImpl.selector) _disputeGameFactoryImpl = DisputeGameFactory(_addr);
if (_sel == this.opcmProxy.selector) _opcmProxy = OPContractsManager(payable(_addr));
else if (_sel == this.opcmImpl.selector) _opcmImpl = OPContractsManager(payable(_addr));
else if (_sel == this.optimismPortalImpl.selector) _optimismPortalImpl = OptimismPortal2(payable(_addr));
else if (_sel == this.delayedWETHImpl.selector) _delayedWETHImpl = DelayedWETH(payable(_addr));
else if (_sel == this.preimageOracleSingleton.selector) _preimageOracleSingleton = PreimageOracle(_addr);
else if (_sel == this.mipsSingleton.selector) _mipsSingleton = MIPS(_addr);
else if (_sel == this.systemConfigImpl.selector) _systemConfigImpl = SystemConfig(_addr);
else if (_sel == this.l1CrossDomainMessengerImpl.selector) _l1CrossDomainMessengerImpl = L1CrossDomainMessenger(_addr);
else if (_sel == this.l1ERC721BridgeImpl.selector) _l1ERC721BridgeImpl = L1ERC721Bridge(_addr);
else if (_sel == this.l1StandardBridgeImpl.selector) _l1StandardBridgeImpl = L1StandardBridge(payable(_addr));
else if (_sel == this.optimismMintableERC20FactoryImpl.selector) _optimismMintableERC20FactoryImpl = OptimismMintableERC20Factory(_addr);
else if (_sel == this.disputeGameFactoryImpl.selector) _disputeGameFactoryImpl = DisputeGameFactory(_addr);
else revert("DeployImplementationsOutput: unknown selector");
// forgefmt: disable-end
}
Expand Down
32 changes: 16 additions & 16 deletions packages/contracts-bedrock/scripts/DeployOPChain.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,24 +164,24 @@ contract DeployOPChainOutput is BaseDeployIO {
DelayedWETH internal _delayedWETHPermissionedGameProxy;
DelayedWETH internal _delayedWETHPermissionlessGameProxy;

function set(bytes4 sel, address _addr) public {
function set(bytes4 _sel, address _addr) public {
require(_addr != address(0), "DeployOPChainOutput: cannot set zero address");
// forgefmt: disable-start
if (sel == this.opChainProxyAdmin.selector) _opChainProxyAdmin = ProxyAdmin(_addr) ;
else if (sel == this.addressManager.selector) _addressManager = AddressManager(_addr) ;
else if (sel == this.l1ERC721BridgeProxy.selector) _l1ERC721BridgeProxy = L1ERC721Bridge(_addr) ;
else if (sel == this.systemConfigProxy.selector) _systemConfigProxy = SystemConfig(_addr) ;
else if (sel == this.optimismMintableERC20FactoryProxy.selector) _optimismMintableERC20FactoryProxy = OptimismMintableERC20Factory(_addr) ;
else if (sel == this.l1StandardBridgeProxy.selector) _l1StandardBridgeProxy = L1StandardBridge(payable(_addr)) ;
else if (sel == this.l1CrossDomainMessengerProxy.selector) _l1CrossDomainMessengerProxy = L1CrossDomainMessenger(_addr) ;
else if (sel == this.optimismPortalProxy.selector) _optimismPortalProxy = OptimismPortal2(payable(_addr)) ;
else if (sel == this.disputeGameFactoryProxy.selector) _disputeGameFactoryProxy = DisputeGameFactory(_addr) ;
else if (sel == this.anchorStateRegistryProxy.selector) _anchorStateRegistryProxy = AnchorStateRegistry(_addr) ;
else if (sel == this.anchorStateRegistryImpl.selector) _anchorStateRegistryImpl = AnchorStateRegistry(_addr) ;
else if (sel == this.faultDisputeGame.selector) _faultDisputeGame = FaultDisputeGame(_addr) ;
else if (sel == this.permissionedDisputeGame.selector) _permissionedDisputeGame = PermissionedDisputeGame(_addr) ;
else if (sel == this.delayedWETHPermissionedGameProxy.selector) _delayedWETHPermissionedGameProxy = DelayedWETH(payable(_addr)) ;
else if (sel == this.delayedWETHPermissionlessGameProxy.selector) _delayedWETHPermissionlessGameProxy = DelayedWETH(payable(_addr)) ;
if (_sel == this.opChainProxyAdmin.selector) _opChainProxyAdmin = ProxyAdmin(_addr) ;
else if (_sel == this.addressManager.selector) _addressManager = AddressManager(_addr) ;
else if (_sel == this.l1ERC721BridgeProxy.selector) _l1ERC721BridgeProxy = L1ERC721Bridge(_addr) ;
else if (_sel == this.systemConfigProxy.selector) _systemConfigProxy = SystemConfig(_addr) ;
else if (_sel == this.optimismMintableERC20FactoryProxy.selector) _optimismMintableERC20FactoryProxy = OptimismMintableERC20Factory(_addr) ;
else if (_sel == this.l1StandardBridgeProxy.selector) _l1StandardBridgeProxy = L1StandardBridge(payable(_addr)) ;
else if (_sel == this.l1CrossDomainMessengerProxy.selector) _l1CrossDomainMessengerProxy = L1CrossDomainMessenger(_addr) ;
else if (_sel == this.optimismPortalProxy.selector) _optimismPortalProxy = OptimismPortal2(payable(_addr)) ;
else if (_sel == this.disputeGameFactoryProxy.selector) _disputeGameFactoryProxy = DisputeGameFactory(_addr) ;
else if (_sel == this.anchorStateRegistryProxy.selector) _anchorStateRegistryProxy = AnchorStateRegistry(_addr) ;
else if (_sel == this.anchorStateRegistryImpl.selector) _anchorStateRegistryImpl = AnchorStateRegistry(_addr) ;
else if (_sel == this.faultDisputeGame.selector) _faultDisputeGame = FaultDisputeGame(_addr) ;
else if (_sel == this.permissionedDisputeGame.selector) _permissionedDisputeGame = PermissionedDisputeGame(_addr) ;
else if (_sel == this.delayedWETHPermissionedGameProxy.selector) _delayedWETHPermissionedGameProxy = DelayedWETH(payable(_addr)) ;
else if (_sel == this.delayedWETHPermissionlessGameProxy.selector) _delayedWETHPermissionlessGameProxy = DelayedWETH(payable(_addr)) ;
else revert("DeployOPChainOutput: unknown selector");
// forgefmt: disable-end
}
Expand Down
12 changes: 6 additions & 6 deletions packages/contracts-bedrock/scripts/DeploySuperchain.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,13 @@ contract DeploySuperchainOutput is BaseDeployIO {

// This method lets each field be set individually. The selector of an output's getter method
// is used to determine which field to set.
function set(bytes4 sel, address _address) public {
function set(bytes4 _sel, address _address) public {
require(_address != address(0), "DeploySuperchainOutput: cannot set zero address");
if (sel == this.superchainProxyAdmin.selector) _superchainProxyAdmin = ProxyAdmin(_address);
else if (sel == this.superchainConfigImpl.selector) _superchainConfigImpl = SuperchainConfig(_address);
else if (sel == this.superchainConfigProxy.selector) _superchainConfigProxy = SuperchainConfig(_address);
else if (sel == this.protocolVersionsImpl.selector) _protocolVersionsImpl = ProtocolVersions(_address);
else if (sel == this.protocolVersionsProxy.selector) _protocolVersionsProxy = ProtocolVersions(_address);
if (_sel == this.superchainProxyAdmin.selector) _superchainProxyAdmin = ProxyAdmin(_address);
else if (_sel == this.superchainConfigImpl.selector) _superchainConfigImpl = SuperchainConfig(_address);
else if (_sel == this.superchainConfigProxy.selector) _superchainConfigProxy = SuperchainConfig(_address);
else if (_sel == this.protocolVersionsImpl.selector) _protocolVersionsImpl = ProtocolVersions(_address);
else if (_sel == this.protocolVersionsProxy.selector) _protocolVersionsProxy = ProtocolVersions(_address);
else revert("DeploySuperchainOutput: unknown selector");
}

Expand Down
7 changes: 5 additions & 2 deletions packages/contracts-bedrock/scripts/checks/check-semver-diff.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ temp_dir=$(mktemp -d)
trap 'rm -rf "$temp_dir"' EXIT

# Exit early if semver-lock.json has not changed.
if ! git diff origin/develop...HEAD --name-only | grep -q "$SEMVER_LOCK"; then
if ! { git diff origin/develop...HEAD --name-only; git diff --name-only; git diff --cached --name-only; } | grep -q "$SEMVER_LOCK"; then
echo "No changes detected in semver-lock.json"
exit 0
fi
Expand Down Expand Up @@ -71,9 +71,12 @@ for contract in $changed_contracts; do
has_errors=true
fi

# TODO: Use an existing semver comparison function since this will only
# check if the version has changed at all and not that the version has
# increased properly.
# Check if the version changed.
if [ "$old_version" = "$new_version" ]; then
echo "❌ Error: src/$contract has changes in semver-lock.json but no version change"
echo "❌ Error: $contract has changes in semver-lock.json but no version change"
echo " Old version: $old_version"
echo " New version: $new_version"
has_errors=true
Expand Down
6 changes: 3 additions & 3 deletions packages/contracts-bedrock/scripts/deploy/Deploy.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,11 @@ contract Deploy is Deployer {
/// @notice Deploy a new OP Chain using an existing SuperchainConfig and ProtocolVersions
/// @param _superchainConfigProxy Address of the existing SuperchainConfig proxy
/// @param _protocolVersionsProxy Address of the existing ProtocolVersions proxy
/// @param includeDump Whether to include a state dump after deployment
/// @param _includeDump Whether to include a state dump after deployment
function runWithSuperchain(
address payable _superchainConfigProxy,
address payable _protocolVersionsProxy,
bool includeDump
bool _includeDump
)
public
{
Expand All @@ -306,7 +306,7 @@ contract Deploy is Deployer {

_run(false);

if (includeDump) {
if (_includeDump) {
vm.dumpState(Config.stateDumpPath(""));
}
}
Expand Down
56 changes: 36 additions & 20 deletions packages/contracts-bedrock/scripts/deploy/DeployConfig.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ contract DeployConfig is Script {

function read(string memory _path) public {
console.log("DeployConfig: reading file %s", _path);
try vm.readFile(_path) returns (string memory data) {
_json = data;
try vm.readFile(_path) returns (string memory data_) {
_json = data_;
} catch {
require(false, string.concat("Cannot find deploy config file at ", _path));
}
Expand Down Expand Up @@ -191,14 +191,14 @@ contract DeployConfig is Script {
}

function l1StartingBlockTag() public returns (bytes32) {
try vm.parseJsonBytes32(_json, "$.l1StartingBlockTag") returns (bytes32 tag) {
return tag;
try vm.parseJsonBytes32(_json, "$.l1StartingBlockTag") returns (bytes32 tag_) {
return tag_;
} catch {
try vm.parseJsonString(_json, "$.l1StartingBlockTag") returns (string memory tag) {
return _getBlockByTag(tag);
try vm.parseJsonString(_json, "$.l1StartingBlockTag") returns (string memory tag_) {
return _getBlockByTag(tag_);
} catch {
try vm.parseJsonUint(_json, "$.l1StartingBlockTag") returns (uint256 tag) {
return _getBlockByTag(vm.toString(tag));
try vm.parseJsonUint(_json, "$.l1StartingBlockTag") returns (uint256 tag_) {
return _getBlockByTag(vm.toString(tag_));
} catch { }
}
}
Expand Down Expand Up @@ -266,32 +266,48 @@ contract DeployConfig is Script {
return abi.decode(res, (bytes32));
}

function _readOr(string memory json, string memory key, bool defaultValue) internal view returns (bool) {
return vm.keyExistsJson(json, key) ? json.readBool(key) : defaultValue;
function _readOr(string memory _jsonInp, string memory _key, bool _defaultValue) internal view returns (bool) {
return vm.keyExistsJson(_jsonInp, _key) ? _jsonInp.readBool(_key) : _defaultValue;
}

function _readOr(string memory json, string memory key, uint256 defaultValue) internal view returns (uint256) {
return (vm.keyExistsJson(json, key) && !_isNull(json, key)) ? json.readUint(key) : defaultValue;
function _readOr(
string memory _jsonInp,
string memory _key,
uint256 _defaultValue
)
internal
view
returns (uint256)
{
return (vm.keyExistsJson(_jsonInp, _key) && !_isNull(_json, _key)) ? _jsonInp.readUint(_key) : _defaultValue;
}

function _readOr(string memory json, string memory key, address defaultValue) internal view returns (address) {
return vm.keyExistsJson(json, key) ? json.readAddress(key) : defaultValue;
function _readOr(
string memory _jsonInp,
string memory _key,
address _defaultValue
)
internal
view
returns (address)
{
return vm.keyExistsJson(_jsonInp, _key) ? _jsonInp.readAddress(_key) : _defaultValue;
}

function _isNull(string memory json, string memory key) internal pure returns (bool) {
string memory value = json.readString(key);
function _isNull(string memory _jsonInp, string memory _key) internal pure returns (bool) {
string memory value = _jsonInp.readString(_key);
return (keccak256(bytes(value)) == keccak256(bytes("null")));
}

function _readOr(
string memory json,
string memory key,
string memory defaultValue
string memory _jsonInp,
string memory _key,
string memory _defaultValue
)
internal
view
returns (string memory)
{
return vm.keyExists(json, key) ? json.readString(key) : defaultValue;
return vm.keyExists(_jsonInp, _key) ? _jsonInp.readString(_key) : _defaultValue;
}
}
Loading

0 comments on commit d8cb523

Please sign in to comment.