[Cantina Finding #4]Implemented the changes suggested as general informational issues #167
Conversation
| address _sender | ||
| ) CCIPReceiver(_router) { | ||
| mainnetChainSelector = _mainnetChainSelector; | ||
| /// @dev Constructor to initialize the BaseGovernanceReceiver contract. |
There was a problem hiding this comment.
it's still referring to the BaseGovernanceReceiver while it should say GovernanceReceiver
| * @param _receiver The address of the receiver contract on the destination chain. | ||
| * @dev Only the Timelock contract can call this function. | ||
| */ | ||
| /// @inheritdoc IGovernanceCCIPRelay | ||
| function setDestinationReceiver(address _receiver) external onlyTimeLock { |
There was a problem hiding this comment.
@voith it's not early returning as suggested in the finding https://cantina.xyz/code/8b6d2575-95bc-4380-8f2a-314e44776822/findings/4
There was a problem hiding this comment.
Lets discuss this in #169 (comment)
| /// @notice The chain selector for Ethereum Mainnet. | ||
| uint64 public mainnetChainSelector; | ||
| /// @inheritdoc IGovernanceCCIPReceiver | ||
| uint64 public constant mainnetChainSelector = 5009297550715157269; |
There was a problem hiding this comment.
@voith this is a constant, replace the name in uppercase following the Solidity style recommendations
| address public mainnetSender; | ||
| contract GovernanceCCIPReceiver is IGovernanceCCIPReceiver, CCIPReceiver { | ||
| /// @inheritdoc IGovernanceCCIPReceiver | ||
| address public immutable mainnetSender; |
There was a problem hiding this comment.
@voith this is an immutable variable, replace the name in uppercase following the Solidity style recommendations
| /// @notice The address of the Timelock contract that can call this contract. | ||
| address public timelock; | ||
| /// @inheritdoc IGovernanceCCIPRelay | ||
| address public immutable timelock; |
There was a problem hiding this comment.
@voith this is an immutable variable, replace the name in uppercase following the Solidity style recommendations
| * @param _destinationChainSelector The chain selector for the destination chain. | ||
| * @param _receiver The address of the receiver contract on the destination chain. | ||
| */ | ||
| /// @dev Constructor to initialize the GovernanceRelay contract. |
There was a problem hiding this comment.
@voith the contract name is GovernanceCCIPRelay and not GovernanceRelay
| @@ -15,7 +15,7 @@ contract GovernanceCCIPRelay is IGovernanceCCIPRelay { | |||
| IRouterClient public immutable ccipRouter; | |||
There was a problem hiding this comment.
@voith this one is also immutable so it should be renamed CCIP_ROUTER
There was a problem hiding this comment.
Lol, I thought you left that intentionally because it makes this call ccipRouter.ccipSend.
I've never seen CCIP_ROUTER .ccipSend before. But I'm no style guide expert, so I'll fix this.
There was a problem hiding this comment.
Usually, it's quite useful to follow that style guide because you visually see immediately whether something can change or not.
I don't see anything strange on my side to see constant/immutable variables (defined as contracts) that perform function's calls.
This change is