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

refactor(contracts): OZ-L1-L03 Code Redundancy, OZ-L1-N15 Unused Imports and OZ-L2-N02 Unused Imports #698

Merged
merged 5 commits into from Aug 6, 2023

Conversation

zimpha
Copy link
Member

@zimpha zimpha commented Aug 1, 2023

Purpose or design rationale of this PR

This PR fix several issues reported by reported by OpenZeppelin. The following are the details:

OZ-L1-L03 Code Redundancy

Throughout the codebase, there are multiple instances of code redundancy, which is error- prone and hinders the codebase's readability:

  • The L1ScrollMessenger and ScrollGatewayBase contracts implement a nonReentrant modifier. Instead, consider utilizing the ReentrancyGuardUpgradeable contract of the OpenZeppelin library which is a
    dependency in use. The same holds for the OwnableBase contract and OpenZeppelin's Ownable contract. The reimplementation of such safety mechanisms is generally discouraged. In both cases, make sure to initialize the contracts properly if they are used for upgradeable contracts.
  • The IERC20Metadata interface could also be used from the OpenZeppelin library.
  • The IScrollERC20 and IScrollERC20Upgradeable interfaces are the same interface - consider removing one of them.
  • The isContract function of the ScrollStandardERC20 contract could also be realized with address.code.length > 0. For better gas efficiency, it is recommended to use this with a Solidity version higher than 0.8.1.
  • The L1ScrollMessenger and L2ScrollMessenger both implement the setPause function but inherit from ScrollMessengerBase. Consider moving the setPause function to the base contract.
  • The onlyMessenger modifier of the ScrollGatewayBase contract is unused. Consider removing it.

Consider applying these code changes to improve the quality of the codebase. Make sure to have all of these changes tested with an extensive test suite.

  • The OwnableBase is still used since using Ownable in GasSwap will cause compile error.
  • The IScrollERC20 and IScrollERC20Upgradeable are both kept, since IScrollERC20 is used for non-upgradeable token and IScrollERC20Upgradeable is for upgradeable token.

OZ-L1-N15 Unused Imports

Throughout the codebase, there are imports that are unused and could be removed. For instance:

  • Import ProxyAdmin of External.sol
  • Import TransparentUpgradeableProxy of External.sol
  • Import AddressAliasHelper of L1ScrollMessenger.sol
  • Import IL2GatewayRouter of L1GatewayRouter.sol
  • Import IScrollGateway of L1GatewayRouter.sol
  • Import IL1ScrollMessenger of L1GatewayRouter.sol

Consider removing unused imports to improve the overall clarity and readability of the codebase.

  • imports in External.sol are some external contracts will be used. So all are kept.

OZ-L2-N02 Unused Imports

Throughout the codebase the following imports are unused and could be removed:

  • Import IL1GasPriceOracle of L2ScrollMessenger.sol
  • Import IERC20Upgradeable of L2CustomERC20Gateway.sol
  • Import IL2ERC20Gateway of L2CustomERC20Gateway.sol
  • Import IScrollGateway of L2CustomERC20Gateway.sol
  • Import IERC1155Upgradeable of L2ERC1155Gateway.sol
  • Import IScrollGateway of L2ERC1155Gateway.sol
  • Import IERC721Upgradeable of L2ERC721Gateway.sol
  • Import IScrollGateway of L2ERC721Gateway.sol
  • Import IL2ScrollMessenger of L2GatewayRouter.sol
  • Import IL1ETHGateway of L2GatewayRouter.sol
  • Import IScrollGateway of L2GatewayRouter.sol
  • Import IL2ERC20Gateway of L2StandardERC20Gateway.sol
  • Import IScrollGateway of L2StandardERC20Gateway.sol
  • Import IScrollGateway of L2WETHGateway.sol
  • Import IL1BlockContainer of L1GasPriceOracle.sol

Consider removing unused imports to improve the overall clarity and readability of the codebase.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance

Deployment tag versioning

Has tag in common/version.go been updated?

  • No, this PR doesn't involve a new deployment, git tag, docker image tag
  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

@zimpha zimpha self-assigned this Aug 1, 2023
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

LCOV of commit 1d5a40d during Contracts #1250

Summary coverage rate:
  lines......: 52.1% (887 of 1703 lines)
  functions..: 70.0% (203 of 290 functions)
  branches...: no data found

Files changed coverage rate: n/a

@Thegaram Thegaram merged commit e2185ff into develop Aug 6, 2023
3 checks passed
@Thegaram Thegaram deleted the fix/code_redundancy branch August 6, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants