In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if somewhat longer comments (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where very long comment lines interfere with readability. Below are two sets of comments including extra-long lines whose readability could be improved, as shown.
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
Suggestion:
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967)
* using the https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
* Note that whitelisted senders (some protocol contracts) can include additional calldata
* for a callhook to be executed on the L2 side when the tokens are received. In this case, the L2 transaction
* can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook
* never succeeds. This requires extra care when adding contracts to the whitelist, but is necessary to ensure that
* the tickets can be retried in the case of a temporary failure, and to ensure the atomicity of callhooks
* with token transfers.
Suggestion:
* Note that whitelisted senders (some protocol contracts) can include additional calldata
* for a callhook to be executed on the L2 side when the tokens are received. In this case,
* the L2 transaction can revert if the callhook reverts, potentially locking the tokens on the bridge
* if the callhook never succeeds. This requires extra care when adding contracts to the whitelist,
* but is necessary to ensure that the tickets can be retried in the case of a temporary failure,
* and to ensure the atomicity of callhooks with token transfers.
* @param _l1Token L1 Address of GRT (needed for compatibility with Arbitrum Gateway Router)
* @param _to Recipient address on L1
* @param _amount Amount of tokens to burn
* @param _data Contains sender and additional data (always empty) to send to L1
* @return ID of the withdraw transaction
*/
function outboundTransfer(
address _l1Token,
address _to,
uint256 _amount,
uint256, // unused on L2
uint256, // unused on L2
bytes calldata _data
) public payable override notPaused nonReentrant returns (bytes memory) {
Suggestion: Remove the line uint256, // unused on L2, which occurs twice
Update sensitive terms in both comments and code
Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice. I see that "blacklist" has been eliminated by using denylist. However, the contracts still use whitelist and master, as shown in the two examples below:
require(msg.sender == _proxy.admin(), "Caller must be the proxy admin");
Suggestion: Convert ALL CAP + underscore messages to the more conversational capital + lower case without underscores style
Missing require revert string
A require message should be included to enable users to understand the reason for failure
Recommendation: Add a brief (<= 32 char) explanatory message to each of the four requires referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).
function getProxyImplementation(IGraphProxy _proxy) public view returns (address) {
// We need to manually run the static call since the getter cannot be flagged as view
// bytes4(keccak256("implementation()")) == 0x5c60da1b
(bool success, bytes memory returndata) = address(_proxy).staticcall(hex"5c60da1b");
require(success);
return abi.decode(returndata, (address));
}
* @dev Accept to be an implementation of proxy and then call a function from the new
* implementation as specified by `_data`, which should be an encoded function call. This is
* useful to initialize new storage variables in the proxied contract.
*/
function acceptProxyAndCall(IGraphProxy _proxy, bytes calldata _data)
* @dev Returns the current implementation of a proxy.
* This is needed because only the proxy admin can query it.
* @return The address of the current implementation of the proxy.
*/
function getProxyImplementation(IGraphProxy _proxy) public view returns (address) {
* @dev Returns the pending implementation of a proxy.
* This is needed because only the proxy admin can query it.
* @return The address of the pending implementation of the proxy.
*/
function getProxyPendingImplementation(IGraphProxy _proxy) public view returns (address) {
* @dev Returns the admin of a proxy. Only the admin can query it.
* @return The address of the current admin of the proxy.
*/
function getProxyAdmin(IGraphProxy _proxy) public view returns (address) {
* @dev Returns the current admin.
*
* NOTE: Only the admin and implementation can call this function.
*
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
* `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
*/
function admin() external ifAdminOrPendingImpl returns (address) {
Missing: @return. @dev should contain some of the information in the notes so that @return is not redundant
* @dev Returns the current implementation.
*
* NOTE: Only the admin can call this function.
*
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
* `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
*/
function implementation() external ifAdminOrPendingImpl returns (address) {
Missing: @return. Again, @dev should contain some of the information in the notes so that @return is not redundant
* @dev Admin function for new implementation to accept its role as implementation.
*/
function acceptUpgradeAndCall(bytes calldata data) external ifAdminOrPendingImpl {
* @dev Resolve a contract address from the cache or the Controller if not found.
* @return Address of the contract
*/
function _resolveContract(bytes32 _nameHash) internal view returns (address) {
* @notice Getter to access paused state of this contract
*/
function paused() external view returns (bool) {
Missing: @return
NatSpec statements are missing
NatSpec statements are entirely missing for the functions listed below. In some cases, a group of functions is proceeded by a comment such as // -- Configuration -- or // -- Getters --. However, such comments, which serve as explanatory headings for the functions, are not as helpful as would be @notice statements (to explain to end users what a function does) and @param and @return statements (which describe the roles of individual variables).
Typos
L1GraphTokenGateway.sol: L185
Change
tranfer
totransfer
L1GraphTokenGateway.sol: L260
Change
Recepient
toRecipient
Long comment lines
In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if somewhat longer comments (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where very long comment lines interfere with readability. Below are two sets of comments including extra-long lines whose readability could be improved, as shown.
GraphProxy.sol: L65-66
Suggestion:
Similarly for the following sets of comments:
GraphProxy.sol: L78-79
GraphProxy.sol: L91-92
L2GraphTokenGateway.sol: L214-219
Suggestion:
Similarly for the following set of comments:
L1GraphTokenGateway.sol: L177-182
Open items
Code that contains open items should be addressed and modified or removed
L2GraphTokenGateway.sol: L131-144
Suggestion: Remove the line
uint256, // unused on L2
, which occurs twiceUpdate sensitive terms in both comments and code
Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice. I see that "blacklist" has been eliminated by using
denylist
. However, the contracts still usewhitelist
andmaster
, as shown in the two examples below:L1GraphTokenGateway.sol: L152-157
ICuration.sol: L16
Suggestion: Replace
whitelist
withallowlist
, andTokenMaster
withTokenPrimary
orTokenAdmin
. Similarly for variations of these terms.Capitalization of
require
revert strings should be consistentFor the sake of readability, revert strings should have consistent punctuation throughout
The Graph L2
Some error messages are written using ALL CAPS with underscores between words, as the example below shows:
L2GraphTokenGateway.sol: L98
Other error messages are written in a more conversational style without using ALL CAPS or underscores:
GraphUpgradeable.sol: L24
Suggestion: Convert ALL CAP + underscore messages to the more conversational capital + lower case without underscores style
Missing
require
revert stringA
require
message should be included to enable users to understand the reason for failureRecommendation: Add a brief (<= 32 char) explanatory message to each of the four
requires
referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).GraphProxyAdmin.sol: L30-36
Similarly for the following:
GraphProxyAdmin.sol: L43-49
GraphProxyAdmin.sol: L55-61
GraphProxy.sol: L129-134
NatSpec is incomplete
Specific NatSpec statements are missing for the 15 functions below:
GraphUpgradeable.sol: L48-50
Missing:
@param _proxy
GraphUpgradeable.sol: L55-59
Missing:
@param _proxy
and@param _data
Governed.sol: L29-31
Missing:
@param _initGovernor
Pausable.sol: L24-26
Missing:
@param _toPause
Pausable.sol: L38-40
Missing:
@param _toPause
GraphProxyAdmin.sol: L26-30
Missing:
@param _proxy
GraphProxyAdmin.sol: L39-43
Missing:
@param _proxy
GraphProxyAdmin.sol: L52-55
Missing:
@param _proxy
GraphProxy.sol: L61-69
Missing:
@return
.@dev
should contain some of the information in the notes so that@return
is not redundantGraphProxy.sol: L74-82
Missing:
@return
. Again,@dev
should contain some of the information in the notes so that@return
is not redundantGraphProxy.sol: L100-104
Missing:
@param _newAdmin
GraphProxy.sol: L127-129
Missing:
@param data
Managed.sol: L85-87
Missing:
@param _controller
Managed.sol: L158-161
Missing:
@param _nameHash
GraphTokenGateway.sol: L52-54
Missing:
@return
NatSpec statements are missing
NatSpec statements are entirely missing for the functions listed below. In some cases, a group of functions is proceeded by a comment such as
// -- Configuration --
or// -- Getters --
. However, such comments, which serve as explanatory headings for the functions, are not as helpful as would be@notice
statements (to explain to end users what a function does) and@param
and@return
statements (which describe the roles of individual variables).Managed.sol
L43; L48; L52; L56
IGraphCurationToken.sol
L8; L10; L12
IGraphProxy.sol
L6; L8; L10; L12; L14; L16; L18
IEpochManager.sol
L8; L12; L16; L18; L20; L22; L24; L26; L28; L30
IController.sol
L6; L10; L12; L14; L16; L20; L22; L24; L26; L28
IGraphToken.sol
L10; L12; L14; L18; L20; L22; L24; L28-36; L40; L42
IRewardsManager.sol
L18; L20; L24; L26; L28; L31; L35; L37; L39-42; L44-47; L49; L53; L55; L59; L61
ICuration.sol
L10; L12; L14; L16; L20-24; L26-30; L32; L36; L38-41; L43; L45; L47-50; L52-55; L57
IStaking.sol
L30; L32; L34; L36; L38; L40; L42; L44; L46-50; L52; L54; L56; L58; L60; L64; L66; L70; L72; L74; L76-81; L83; L85; L89; L91; L93; L97-103; L105-112; L114; L116; L118-127; L129; L131; L133; L137; L139; L141; L143; L145; L147; L149-152; L154-157; L159