From 96b9b3200e409c71c2e03eadb601a1b4a016c636 Mon Sep 17 00:00:00 2001 From: Matt Yang Date: Tue, 9 Jul 2024 10:37:42 -0400 Subject: [PATCH] pair review comments --- .../v0.8/ccip/applications/external/CCIPBase.sol | 12 +++++++++--- .../ccip/applications/external/CCIPClient.sol | 4 ++++ .../ccip/applications/external/CCIPReceiver.sol | 12 ++++++------ .../applications/external/CCIPReceiverWithACK.sol | 15 ++++++++++++--- 4 files changed, 31 insertions(+), 12 deletions(-) diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol b/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol index 5974e79b70..86ece0a049 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol @@ -22,17 +22,20 @@ abstract contract CCIPBase is OwnerIsCreator { event CCIPRouterModified(address indexed oldRouter, address indexed newRouter); event TokensWithdrawnByOwner(address indexed token, address indexed to, uint256 amount); + + // TODO 2 events, add/remove + // TODO comment on the tradeoffs, short comment on why prefer indexed event ApprovedSenderModified(uint64 indexed destChainSelector, bytes indexed recipient, bool isBeingApproved); - event ChainAdded(uint64 remoteChainSelector, bytes recipient, bytes extraArgsBytes); - event ChainRemoved(uint64 removeChainSelector); + event ChainAdded(uint64 indexed remoteChainSelector, bytes indexed recipient, bytes extraArgsBytes); + event ChainRemoved(uint64 indexed removeChainSelector); struct ApprovedSenderUpdate { uint64 destChainSelector; // Chainselector for a source chain that is allowed to call this dapp bytes sender; // The sender address on source chain that is allowed to call, ABI encoded in the case of a remote EVM chain } - struct ChainUpdate { + struct ChainUpdate { // TODO comments uint64 chainSelector; bool allowed; bytes recipient; @@ -138,6 +141,8 @@ abstract contract CCIPBase is OwnerIsCreator { // │ Chain Management │ // ================================================================ + // TODO comments + // TODO name as updateRouter function modifyRouter(address newRouter) external onlyOwner { if (newRouter == address(0)) revert ZeroAddressNotAllowed(); @@ -172,6 +177,7 @@ abstract contract CCIPBase is OwnerIsCreator { } } + // TODO comments modifier isValidChain(uint64 chainSelector) { // Must be storage and not memory because the struct contains a nested mapping which is not capable of being copied to memory ChainConfig storage currentConfig = s_chainConfigs[chainSelector]; diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol b/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol index f22837b995..6da2db8bd9 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol @@ -12,12 +12,14 @@ import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/ /// @title CCIPClient /// @notice This contract implements logic for sending and receiving CCIP Messages. It utilizes CCIPReceiver's defensive patterns by default. /// @dev CCIPReceiver and CCIPSender cannot be simultaneously imported due to similar parents so CCIPSender functionality has been duplicated +// TODO make CCIPClient inherit from CCIPReceiver contract CCIPClient is CCIPReceiverWithACK { using SafeERC20 for IERC20; constructor(address router, IERC20 feeToken) CCIPReceiverWithACK(router, feeToken) {} /// @notice sends a message through CCIP to the router + // TODO really beef up the comments here function ccipSend( uint64 destChainSelector, Client.EVMTokenAmount[] memory tokenAmounts, @@ -48,6 +50,8 @@ contract CCIPClient is CCIPReceiverWithACK { IERC20(s_feeToken).safeTransferFrom(msg.sender, address(this), fee); } + // TODO comment we only have messageId after calling ccipSend, so brekaing CEI is necessary + // messageId clac lives in OnRamp, which can be upgradaed, this it should be abstracted away from client impl messageId = IRouterClient(s_ccipRouter).ccipSend{value: address(s_feeToken) == address(0) ? fee : 0}( destChainSelector, message ); diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol b/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol index 4e071669f7..4b38c6a2d6 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol @@ -9,7 +9,7 @@ import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/ import {EnumerableMap} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableMap.sol"; /// @title CCIPReceiver -/// @notice This contract is capable of receiving incoming messages from the CCIP-Router. +/// @notice This contract is capable of receiving incoming messages from the CCIP Router. /// @dev This contract implements various "defensive" measures to enhance security and efficiency. These include the ability to implement custom-retry logic and ownership-based token-recovery functions. contract CCIPReceiver is CCIPBase { using SafeERC20 for IERC20; @@ -28,7 +28,6 @@ contract CCIPReceiver is CCIPBase { FAILED, // FAILED messages are messages which reverted during execution of processMessage() as part of the ccipReceive() try catch loop. ABANDONED // ABANDONED messages are ones which cannot be properly processed, but any sent tokens are recoverable, and can only be triggered by the contract owner. // Only a message that was previously marked as FAILED can be abandoned. - } // The message contents of failed messages are stored here. @@ -51,7 +50,7 @@ contract CCIPReceiver is CCIPBase { external virtual onlyRouter - isValidChain(message.sourceChainSelector) + isValidChain(message.sourceChainSelector) // TODO should it validate sender as well? { try this.processMessage(message) {} catch (bytes memory err) { @@ -90,7 +89,7 @@ contract CCIPReceiver is CCIPBase { /// @notice Executes a message that failed initial delivery, but with different logic specifically for re-execution. /// @dev Since the function invoked _retryFailedMessage(), which is marked onlyOwner, this may only be called by the Owner as well. - /// @dev Function will revert if the messageId was not already stored as having failed its initial execution + /// Function will revert if the messageId was not already stored as having failed its initial execution /// @param messageId the unique ID of the CCIP-message which failed initial-execution. function retryFailedMessage(bytes32 messageId) external { if (s_failedMessages.get(messageId) != uint256(ErrorCode.FAILED)) revert MessageNotFailed(messageId); @@ -110,14 +109,15 @@ contract CCIPReceiver is CCIPBase { /// @notice A function that should contain any special logic needed to "retry" processing of a previously failed message. /// @dev If the owner wants to retrieve tokens without special logic, then abandonFailedMessage(), withdrawNativeTokens(), or withdrawTokens() should be used instead - /// @dev This function is marked onlyOwner, but is virtual. Allowing permissionless execution is not recommended but may be allowed if function is overridden + /// This function is marked onlyOwner, but is virtual. Allowing permissionless execution is not recommended but may be allowed if function is overridden function _retryFailedMessage(Client.Any2EVMMessage memory message) internal virtual onlyOwner { // TODO how about we add a default implementation that calls `processMessage`, and comments for overrides + // The idea is to vend an example that works somewhat well out of the box } /// @notice Should be used to recover tokens from a failed message, while ensuring the message cannot be retried /// @dev function will send tokens to destination, but will NOT invoke any arbitrary logic afterwards. - /// @dev function is only callable by the contract owner + /// function is only callable by the contract owner function abandonFailedMessage(bytes32 messageId, address receiver) external onlyOwner { if (s_failedMessages.get(messageId) != uint256(ErrorCode.FAILED)) revert MessageNotFailed(messageId); diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol b/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol index 7bda3e9163..5b598a6e14 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol @@ -11,6 +11,7 @@ import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/ import {EnumerableMap} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableMap.sol"; /// @title CCIPReceiverWithACK +/// TODO Beef this up contract CCIPReceiverWithACK is CCIPReceiver { using SafeERC20 for IERC20; using EnumerableMap for EnumerableMap.Bytes32ToUintMap; @@ -20,14 +21,19 @@ contract CCIPReceiverWithACK is CCIPReceiver { event MessageAckSent(bytes32 incomingMessageId); event MessageSent(bytes32 indexed incomingMessageId, bytes32 indexed ACKMessageId); + // TODO named var event MessageAckReceived(bytes32); event FeeTokenModified(address indexed oldToken, address indexed newToken); + // TODO comments on OUTGOING vs. ACK, and OUTGOING is set in a child + // TODO consider moving this to CCIPClienWithACK because CCIPReceiver does not need to be aware of send/receives + // it is only concerned about receiving enum MessageType { OUTGOING, ACK } + // TODO comments enum MessageStatus { QUIET, SENT, @@ -73,6 +79,7 @@ contract CCIPReceiverWithACK is CCIPReceiver { emit FeeTokenModified(oldFeeToken, token); } + // TODO review if this just be inherited /// @notice The entrypoint for the CCIP router to call. This function should never revert, all errors should be handled internally in this contract. /// @param message The message to process. /// @dev Extremely important to ensure only router calls this. @@ -97,14 +104,16 @@ contract CCIPReceiverWithACK is CCIPReceiver { emit MessageSucceeded(message.messageId); } - /// @notice Application-specific logic for incoming ccip-messages. + /// @notice Application-specific logic for incoming ccip messages. /// @dev Function does NOT require the status of an incoming ACK be "sent" because this implementation does not send, only receives - /// @dev Any MessageType encoding is implemented by the sender-contract, and is not natively part of CCIP-messages. + /// Any MessageType encoding is implemented by the sender contract, and is not natively part of CCIP messages. function processMessage(Client.Any2EVMMessage calldata message) external virtual override onlySelf { (MessagePayload memory payload) = abi.decode(message.data, (MessagePayload)); + // TODO CCIReceiverWithACK can just ack without message type checks + // message type is a concept with ClientWithACK if (payload.messageType == MessageType.OUTGOING) { - // Insert Processing workflow here. + // Insert processing workflow here. // If the message was outgoing on the source chain, then send an ack response. _sendAck(message);