additional comments

contracts/gas-snapshots/ccip.gas-snapshot

@@ -93,12 +93,12 @@ CCIPReceiverTest:test_Recovery_from_invalid_sender() (gas: 428817)
 CCIPReceiverTest:test_Recovery_with_intentional_revert() (gas: 444784)
 CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_REVERT() (gas: 205094)
 CCIPReceiverTest:test_removeSender_from_approvedList_and_revert() (gas: 425167)
-CCIPReceiverTest:test_retryFailedMessage_Success() (gas: 423774)
+CCIPReceiverTest:test_retryFailedMessage_Success() (gas: 420799)
 CCIPReceiverTest:test_withdraw_nativeToken_to_owner() (gas: 18806)
 CCIPReceiverWithAckTest:test_ccipReceive_ack_message() (gas: 55172)
 CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack() (gas: 331323)
 CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidAckMessageHeaderBytes_REVERT() (gas: 437616)
-CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor() (gas: 2641285)
+CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor() (gas: 2643085)
 CCIPReceiverWithAckTest:test_modifyFeeToken() (gas: 72519)
 CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andDestTokens() (gas: 339182)
 CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 224256)

contracts/src/v0.8/ccip/applications/external/CCIPClient.sol

@@ -22,6 +22,7 @@ contract CCIPClient is CCIPReceiverWithACK {
     return "CCIPClient 1.0.0-dev";
   }
 
+  /// @notice sends a message through CCIP to the router
   function ccipSend(
     uint64 destChainSelector,
     Client.EVMTokenAmount[] memory tokenAmounts,
@@ -64,8 +65,8 @@ contract CCIPClient is CCIPReceiverWithACK {
     return messageId;
   }
 
-  /// CCIPReceiver processMessage to make easier to modify
-  /// @notice function requres that
+  /// @notice Implementation of arbitrary logic to be executed when a CCIP message is received
+  /// @dev is only invoked by self on CCIPReceive, and should implement arbitrary dapp-specific logic
   function processMessage(Client.Any2EVMMessage calldata message) external virtual override onlySelf {
     (MessagePayload memory payload) = abi.decode(message.data, (MessagePayload));
 

contracts/src/v0.8/ccip/applications/external/CCIPClientBase.sol

@@ -25,16 +25,14 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
 
   struct ChainConfig {
     bool disabled;
-    bytes recipient;
-    // Includes additional configs such as manual gas limit, and OOO-execution.
-    // Should not be supplied at runtime to prevent unexpected contract behavior
-    bytes extraArgsBytes;
-    mapping(bytes => bool) approvedSender;
+    bytes recipient; // The address to send messages to on the destination-chain, abi.encode(addr) if an EVM-compatible networks
+    bytes extraArgsBytes; // Includes additional configs such as manual gas limit, and out-of-order-execution. Should not be supplied at runtime to prevent unexpected contract behavior
+    mapping(bytes recipient => bool isApproved) approvedSender;
   }
 
   address internal immutable i_ccipRouter;
 
-  mapping(uint64 => ChainConfig) public s_chainConfigs;
+  mapping(uint64 destChainSelector => ChainConfig) public s_chainConfigs;
 
   constructor(address router) {
     if (router == address(0)) revert ZeroAddressNotAllowed();
@@ -60,6 +58,8 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
   // │                  Sender/Receiver Management                  │
   // ================================================================
 
+  /// @notice modify the list of approved source-chain contracts which can send messages to this contract through CCIP
+  /// @dev removes are executed before additions, so a contract present in both will be approved at the end of execution
   function updateApprovedSenders(
     ApprovedSenderUpdate[] calldata adds,
     ApprovedSenderUpdate[] calldata removes
@@ -73,6 +73,10 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
     }
   }
 
+  /// @notice Return whether a contract on the specified source chain is authorized to send messages to this contract through CCIP
+  /// @param sourceChainSelector A unique CCIP-specific identifier for the source chain
+  /// @param senderAddr The address which sent the message on the source-chain, abi-encoded if evm-compatible
+  /// @return bool Whether the address is approved or not to invoke functions on this contract
   function isApprovedSender(uint64 sourceChainSelector, bytes calldata senderAddr) external view returns (bool) {
     return s_chainConfigs[sourceChainSelector].approvedSender[senderAddr];
   }
@@ -81,14 +85,21 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
   // │                  Fee Token Management                       │
   // ===============================================================
 
-  /// @notice function is set in client base to support native-fee-token pre-funding in all children implemending CCIPSend
+  /// @notice function support native-fee-token pre-funding in all children implementing the ccipSend function
   receive() external payable {}
 
+  /// @notice Allow the owner to recover any native-tokens sent to this contract out of error.
+  /// @dev Function should not be used to recover tokens from failed-messages, abandonFailedMessage() should be used instead
+  /// @param to A payable address to send the recovered tokens to
+  /// @param amount the amount of native tokens to recover, denominated in wei
   function withdrawNativeToken(address payable to, uint256 amount) external onlyOwner {
     Address.sendValue(to, amount);
   }
 
-  /// @notice Function should NEVER be used for transfering tokens from a failed message, only for recovering tokens sent in error
+  /// @notice Allow the owner to recover any ERC-20 tokens sent to this contract out of error.
+  /// @dev Function should not be used to recover tokens from failed-messages, abandonFailedMessage() should be used instead
+  /// @param to A payable address to send the recovered tokens to
+  /// @param amount the amount of native tokens to recover, denominated in wei  function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
   function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
     IERC20(token).safeTransfer(to, amount);
   }
@@ -97,6 +108,10 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
   // │                      Chain Management                        │
   // ================================================================
 
+  /// @notice Enable a remote-chain to send and receive messages to/from this contract via CCIP
+  /// @param chainSelector A unique CCIP-specific identifier for the source chain
+  /// @param recipient The address a message should be sent to on the destination chain. There should only be one per-chain, and is abi-encoded if EVM-compatible.
+  /// @param _extraArgsBytes additional optional ccipSend parameters. Do not need to be set unless necessary based on the application-logic
   function enableChain(
     uint64 chainSelector,
     bytes calldata recipient,
@@ -106,18 +121,21 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
 
     currentConfig.recipient = recipient;
 
+    // Set any additional args such as enabling out-of-order execution or manual gas-limit
     if (_extraArgsBytes.length != 0) currentConfig.extraArgsBytes = _extraArgsBytes;
 
     // If config was previously disabled, then re-enable it;
     if (currentConfig.disabled) currentConfig.disabled = false;
   }
 
+  /// @notice Mark a chain as not supported for sending-receiving messages to/from this contract via CCIP.
+  /// @dev If a chain needs to be re-enabled after being disabled, the owner must call enableChain() to support it again.
   function disableChain(uint64 chainSelector) external onlyOwner {
     s_chainConfigs[chainSelector].disabled = true;
   }
 
   modifier isValidChain(uint64 chainSelector) {
-    // Must be storage and not memory because the struct contains a nested mapping
+    // 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];
     if (currentConfig.recipient.length == 0 || currentConfig.disabled) revert InvalidChain(chainSelector);
     _;

contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol

@@ -24,7 +24,6 @@ contract CCIPReceiver is CCIPClientBase {
   enum ErrorCode {
     // RESOLVED is first so that the default value is resolved.
     RESOLVED,
-    // Could have any number of error codes here.
     FAILED,
     ABANDONED
   }
@@ -75,11 +74,8 @@ contract CCIPReceiver is CCIPClientBase {
     emit MessageSucceeded(message.messageId);
   }
 
-  /// @notice This function the entrypoint for this contract to process messages.
-  /// @param message The message to process.
-  /// @dev This example just sends the tokens to the owner of this contracts. More
-  /// interesting functions could be implemented.
-  /// @dev It has to be external because of the try/catch.
+  /// @notice Contains arbitrary application-logic for incoming CCIP messages.
+  /// @dev It has to be external because of the try/catch of ccipReceive() which invokes it
   function processMessage(Client.Any2EVMMessage calldata message)
     external
     virtual
@@ -91,9 +87,11 @@ contract CCIPReceiver is CCIPClientBase {
   // │                  Failed Message Processing                   |
   // ================== ==============================================
 
-  /// @notice This function is called when the initial message delivery has failed but should be attempted again with different logic
-  /// @dev By default this function is callable by anyone, and should be modified if special access control is needed.
-  function retryFailedMessage(bytes32 messageId) external onlyOwner {
+  /// @notice Execute 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
+  /// @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);
 
     // Set the error code to 0 to disallow reentry and retry the same failed message
@@ -109,9 +107,10 @@ contract CCIPReceiver is CCIPClientBase {
     emit MessageRecovered(messageId);
   }
 
-  /// @notice Function should contain any special logic needed to "retry" processing of a previously failed message.
+  /// @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 abandonMessage() or recoverTokens() should be used instead
-  function _retryFailedMessage(Client.Any2EVMMessage memory message) internal virtual {}
+  /// @dev 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 {}
 
   /// @notice Should be used to recover tokens from a failed message, while ensuring the message cannot be retried
   /// @notice function will send tokens to destination, but will NOT invoke any arbitrary logic afterwards.
@@ -134,6 +133,7 @@ contract CCIPReceiver is CCIPClientBase {
   // ================================================================
 
   /// @param messageId the ID of the message delivered by the CCIP Router
+  /// @return Any2EVMMessage a standard CCIP message for EVM-compatible networks
   function getMessageContents(bytes32 messageId) public view returns (Client.Any2EVMMessage memory) {
     return s_messageContents[messageId];
   }

contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol

@@ -76,8 +76,7 @@ contract CCIPReceiverWithACK is CCIPReceiver {
     emit FeeTokenModified(oldFeeToken, token);
   }
 
-  /// @notice The entrypoint for the CCIP router to call. This function should
-  /// never revert, all errors should be handled internally in this contract.
+  /// @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.
   function ccipReceive(Client.Any2EVMMessage calldata message)
@@ -101,15 +100,16 @@ contract CCIPReceiverWithACK is CCIPReceiver {
     emit MessageSucceeded(message.messageId);
   }
 
-  /// CCIPReceiver processMessage to make easier to modify
-  /// @notice Function does NOT require the status of an incoming ACK be "sent" because this implementation does not send, only receives
+  /// @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.
   function processMessage(Client.Any2EVMMessage calldata message) external virtual override onlySelf {
     (MessagePayload memory payload) = abi.decode(message.data, (MessagePayload));
 
     if (payload.messageType == MessageType.OUTGOING) {
       // Insert Processing workflow here.
 
-      // If the message was outgoing, then send an ack response.
+      // If the message was outgoing on the source-chain, then send an ack response.
       _sendAck(message);
     } else if (payload.messageType == MessageType.ACK) {
       // Decode message into the message header and the messageId to ensure the message is encoded correctly
@@ -132,6 +132,7 @@ contract CCIPReceiverWithACK is CCIPReceiver {
   function _sendAck(Client.Any2EVMMessage calldata incomingMessage) internal {
     Client.EVMTokenAmount[] memory tokenAmounts = new Client.EVMTokenAmount[](0);
 
+    // Build the outgoing ACK message, with no tokens, with data being the concatenation of the acknowledgement header and incoming-messageId
     Client.EVM2AnyMessage memory outgoingMessage = Client.EVM2AnyMessage({
       receiver: incomingMessage.sender,
       data: abi.encode(ACK_MESSAGE_HEADER, incomingMessage.messageId),

contracts/src/v0.8/ccip/applications/external/CCIPSender.sol

@@ -37,6 +37,7 @@ contract CCIPSender is CCIPClientBase {
     return "CCIPSender 1.0.0-dev";
   }
 
+  /// @notice sends a message through CCIP to the router
   function ccipSend(
     uint64 destChainSelector,
     Client.EVMTokenAmount[] calldata tokenAmounts,
@@ -52,7 +53,7 @@ contract CCIPSender is CCIPClientBase {
     });
 
     for (uint256 i = 0; i < tokenAmounts.length; ++i) {
-      // Transfer the tokens to pay for tokens in tokenAmounts
+      // Transfer the tokens to this contract to pay the router for the tokens in tokenAmounts
       IERC20(tokenAmounts[i].token).safeTransferFrom(msg.sender, address(this), tokenAmounts[i].amount);
       IERC20(tokenAmounts[i].token).safeIncreaseAllowance(i_ccipRouter, tokenAmounts[i].amount);
     }

contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol

@@ -178,8 +178,6 @@ contract CCIPReceiverTest is EVM2EVMOnRampSetup {
     // Check that message status is failed
     assertEq(s_receiver.getMessageStatus(messageId), 1);
 
-    uint256 tokenBalanceBefore = IERC20(token).balanceOf(OWNER);
-
     vm.startPrank(OWNER);
 
     vm.expectEmit();