pair review comments

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];

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
     );

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);
 

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);