better comments and natspec

contracts/gas-snapshots/ccip.gas-snapshot

@@ -34,8 +34,8 @@ BurnWithFromMintTokenPool_lockOrBurn:test_ChainNotAllowed_Revert() (gas: 28675)
 BurnWithFromMintTokenPool_lockOrBurn:test_PoolBurnRevertNotHealthy_Revert() (gas: 55158)
 BurnWithFromMintTokenPool_lockOrBurn:test_PoolBurn_Success() (gas: 243568)
 BurnWithFromMintTokenPool_lockOrBurn:test_Setup_Success() (gas: 24260)
-CCIPClientTest:test_ccipReceiveAndSendAck_Success() (gas: 331825)
-CCIPClientTest:test_ccipSendAndReceiveAck_in_return_Success() (gas: 348272)
+CCIPClientTest:test_ccipReceiveAndSendAck_Success() (gas: 331839)
+CCIPClientTest:test_ccipSendAndReceiveAck_in_return_Success() (gas: 348275)
 CCIPClientTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens_Success() (gas: 241532)
 CCIPClientTest:test_send_tokens_that_are_not_feeToken_Success() (gas: 552173)
 CCIPConfigSetup:test_getCapabilityConfiguration_Success() (gas: 9495)
@@ -88,18 +88,18 @@ CCIPConfig_validateConfig:test__validateConfig_TooManyBootstrapP2PIds_Reverts()
 CCIPConfig_validateConfig:test__validateConfig_TooManySigners_Reverts() (gas: 1160583)
 CCIPConfig_validateConfig:test__validateConfig_TooManyTransmitters_Reverts() (gas: 1158919)
 CCIPConfig_validateConfig:test_getCapabilityConfiguration_Success() (gas: 9562)
-CCIPReceiverTest:test_HappyPath_Success() (gas: 193812)
-CCIPReceiverTest:test_Recovery_from_invalid_sender_Success() (gas: 430948)
+CCIPReceiverTest:test_HappyPath_Success() (gas: 193794)
+CCIPReceiverTest:test_Recovery_from_invalid_sender_Success() (gas: 430905)
 CCIPReceiverTest:test_Recovery_with_intentional_Revert() (gas: 445121)
-CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_Revert() (gas: 199816)
-CCIPReceiverTest:test_modifyRouter_Success() (gas: 26470)
-CCIPReceiverTest:test_removeSender_from_approvedList_and_revert_Success() (gas: 427992)
-CCIPReceiverTest:test_retryFailedMessage_Success() (gas: 421204)
-CCIPReceiverTest:test_withdraw_nativeToken_to_owner_Success() (gas: 20689)
-CCIPReceiverWithAckTest:test_ccipReceive_ack_message_Success() (gas: 56254)
-CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack_Success() (gas: 331794)
-CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidAckMessageHeaderBytes_Revert() (gas: 438731)
-CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor_Success() (gas: 2948564)
+CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_Revert() (gas: 199985)
+CCIPReceiverTest:test_modifyRouter_Success() (gas: 26446)
+CCIPReceiverTest:test_removeSender_from_approvedList_and_revert_Success() (gas: 427653)
+CCIPReceiverTest:test_retryFailedMessage_Success() (gas: 454419)
+CCIPReceiverTest:test_withdraw_nativeToken_to_owner_Success() (gas: 20667)
+CCIPReceiverWithAckTest:test_ccipReceive_ack_message_Success() (gas: 56277)
+CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack_Success() (gas: 331828)
+CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidAckMessageHeaderBytes_Revert() (gas: 438737)
+CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor_Success() (gas: 2956788)
 CCIPReceiverWithAckTest:test_modifyFeeToken_Success() (gas: 74835)
 CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andDestTokens() (gas: 339572)
 CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 224467)
@@ -208,7 +208,7 @@ EVM2EVMMultiOffRamp_executeSingleReport:test_NonExistingSourceChain_Revert() (ga
 EVM2EVMMultiOffRamp_executeSingleReport:test_ReceiverError_Success() (gas: 181628)
 EVM2EVMMultiOffRamp_executeSingleReport:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 190908)
 EVM2EVMMultiOffRamp_executeSingleReport:test_RootNotCommitted_Revert() (gas: 48050)
-EVM2EVMMultiOffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 436547)
+EVM2EVMMultiOffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 1697359)
 EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain_Success() (gas: 252012)
 EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered_Success() (gas: 174226)
 EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokens_Success() (gas: 193899)
@@ -234,8 +234,8 @@ EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouche
 EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_FailedTx_Revert() (gas: 207956)
 EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ForkedChain_Revert() (gas: 26001)
 EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_GasLimitMismatchMultipleReports_Revert() (gas: 152913)
-EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 507883)
-EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 2308282)
+EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 1768860)
+EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 3413460)
 EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_Success() (gas: 210050)
 EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride_Success() (gas: 210613)
 EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride_Success() (gas: 670625)
@@ -387,7 +387,7 @@ EVM2EVMOffRamp_execute:test_Paused_Revert() (gas: 101458)
 EVM2EVMOffRamp_execute:test_ReceiverError_Success() (gas: 165192)
 EVM2EVMOffRamp_execute:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 177948)
 EVM2EVMOffRamp_execute:test_RootNotCommitted_Revert() (gas: 41317)
-EVM2EVMOffRamp_execute:test_RouterYULCall_Revert() (gas: 1678062)
+EVM2EVMOffRamp_execute:test_RouterYULCall_Revert() (gas: 1663435)
 EVM2EVMOffRamp_execute:test_SingleMessageNoTokensUnordered_Success() (gas: 159863)
 EVM2EVMOffRamp_execute:test_SingleMessageNoTokens_Success() (gas: 175094)
 EVM2EVMOffRamp_execute:test_SingleMessageToNonCCIPReceiver_Success() (gas: 248764)
@@ -419,14 +419,14 @@ EVM2EVMOffRamp_execute_upgrade:test_V2_Success() (gas: 131906)
 EVM2EVMOffRamp_getAllRateLimitTokens:test_GetAllRateLimitTokens_Success() (gas: 38408)
 EVM2EVMOffRamp_getExecutionState:test_FillExecutionState_Success() (gas: 3213556)
 EVM2EVMOffRamp_getExecutionState:test_GetExecutionState_Success() (gas: 83091)
-EVM2EVMOffRamp_manuallyExecute:test_LowGasLimitManualExec_Success() (gas: 1759547)
+EVM2EVMOffRamp_manuallyExecute:test_LowGasLimitManualExec_Success() (gas: 1744898)
 EVM2EVMOffRamp_manuallyExecute:test_ManualExecFailedTx_Revert() (gas: 186809)
 EVM2EVMOffRamp_manuallyExecute:test_ManualExecForkedChain_Revert() (gas: 25894)
 EVM2EVMOffRamp_manuallyExecute:test_ManualExecGasLimitMismatch_Revert() (gas: 43519)
 EVM2EVMOffRamp_manuallyExecute:test_ManualExecInvalidGasLimit_Revert() (gas: 26009)
 EVM2EVMOffRamp_manuallyExecute:test_ManualExecWithGasOverride_Success() (gas: 189003)
 EVM2EVMOffRamp_manuallyExecute:test_ManualExec_Success() (gas: 188464)
-EVM2EVMOffRamp_manuallyExecute:test_ReentrancyManualExecuteFails() (gas: 3171279)
+EVM2EVMOffRamp_manuallyExecute:test_ReentrancyManualExecuteFails() (gas: 3156679)
 EVM2EVMOffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched_Success() (gas: 144106)
 EVM2EVMOffRamp_metadataHash:test_MetadataHash_Success() (gas: 8871)
 EVM2EVMOffRamp_setDynamicConfig:test_NonOwner_Revert() (gas: 40429)
@@ -693,8 +693,8 @@ OCR2Base_transmit:test_UnAuthorizedTransmitter_Revert() (gas: 23484)
 OCR2Base_transmit:test_UnauthorizedSigner_Revert() (gas: 39665)
 OCR2Base_transmit:test_WrongNumberOfSignatures_Revert() (gas: 20557)
 OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy_Success() (gas: 380711)
-PingPong_example_ccipReceive:test_CcipReceive_Success() (gas: 308708)
-PingPong_example_plumbing:test_Pausing_Success() (gas: 17766)
+PingPong_example_ccipReceive:test_CcipReceive_Success() (gas: 307350)
+PingPong_example_plumbing:test_Pausing_Success() (gas: 17898)
 PingPong_example_startPingPong:test_StartPingPong_Success() (gas: 234292)
 PriceRegistry_applyFeeTokensUpdates:test_ApplyFeeTokensUpdates_Success() (gas: 79823)
 PriceRegistry_applyFeeTokensUpdates:test_OnlyCallableByOwner_Revert() (gas: 12580)
@@ -842,8 +842,8 @@ Router_routeMessage:test_ManualExec_Success() (gas: 35381)
 Router_routeMessage:test_OnlyOffRamp_Revert() (gas: 25116)
 Router_routeMessage:test_WhenNotHealthy_Revert() (gas: 44724)
 Router_setWrappedNative:test_OnlyOwner_Revert() (gas: 10985)
-SelfFundedPingPong_ccipReceive:test_FundingIfNotANop_Revert() (gas: 290920)
-SelfFundedPingPong_ccipReceive:test_Funding_Success() (gas: 456179)
+SelfFundedPingPong_ccipReceive:test_FundingIfNotANop_Revert() (gas: 289562)
+SelfFundedPingPong_ccipReceive:test_Funding_Success() (gas: 448109)
 SelfFundedPingPong_setCountIncrBeforeFunding:test_setCountIncrBeforeFunding() (gas: 20203)
 TokenAdminRegistry_acceptAdminRole:test_acceptAdminRole_OnlyPendingAdministrator_Revert() (gas: 51085)
 TokenAdminRegistry_acceptAdminRole:test_acceptAdminRole_Success() (gas: 43947)

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

@@ -23,9 +23,11 @@ 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);
+  // Parameters are indexed to simplyify indexing of cross-chain dapps where contracts may be deployed with the same address.
+  // Since the updateApprovedSenders() function should be used sparingly by the contract owner, the additional gas cost should be negligible. If this function is needed to be used constantly, or with a large number of
+  // contracts, then an alternative and more gas-efficient method should be implemented instead, e.g. with merkle trees or indexing the parameters can be removed.
+  event ApprovedSenderAdded(uint64 indexed destChainSelector, bytes indexed recipient);
+  event ApprovedSenderRemoved(uint64 indexed destChainSelector, bytes indexed recipient);
 
   event ChainAdded(uint64 indexed remoteChainSelector, bytes indexed recipient, bytes extraArgsBytes);
   event ChainRemoved(uint64 indexed removeChainSelector);
@@ -35,16 +37,16 @@ abstract contract CCIPBase is OwnerIsCreator {
     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 { // TODO comments
-    uint64 chainSelector;
-    bool allowed;
-    bytes recipient;
-    bytes extraArgsBytes;
+  struct ChainUpdate {
+    uint64 chainSelector; // ──╮ The unique identifier for a chain to send/receive messages
+    bool allowed; // ──╯ Whether the chain should be enabled
+    bytes recipient; //     Address on the remote chain which should receive incoming messages from this. The should only be one per-chain
+    bytes extraArgsBytes; //     Additional arguments to pass with the message including manually specifying gas limit and and whether to allow out-of-order execution
   }
 
   struct ChainConfig {
     bytes recipient; //      The address to send messages to on the destination chain, ABI encoded in the case of a remote EVM chain.
-    bytes extraArgsBytes; // Specifies extraArgs to pass into ccipSend, includes configs such as gas limit, and OOO execution.
+    bytes extraArgsBytes; // Specifies extraArgs to pass into ccipSend, includes configs such as gas limit, and out-of-order execution.
     mapping(bytes recipient => bool isApproved) approvedSender; // Mapping is nested to support work-flows where Dapps may need to receive messages from one-or-more contracts on a source chain, or to support one-sided dapp upgrades.
   }
 
@@ -81,19 +83,19 @@ abstract contract CCIPBase is OwnerIsCreator {
   function updateApprovedSenders(
     ApprovedSenderUpdate[] calldata adds,
     ApprovedSenderUpdate[] calldata removes
-  ) external onlyOwner {
+  ) external virtual onlyOwner {
     for (uint256 i = 0; i < removes.length; ++i) {
       delete s_chainConfigs[removes[i].destChainSelector].approvedSender[removes[i].sender];
 
       // Third parameter is false to indicate that the sender's previous approval is being revoked, to improve off-chain event indexing
-      emit ApprovedSenderModified(removes[i].destChainSelector, removes[i].sender, false);
+      emit ApprovedSenderRemoved(removes[i].destChainSelector, removes[i].sender);
     }
 
     for (uint256 i = 0; i < adds.length; ++i) {
       s_chainConfigs[adds[i].destChainSelector].approvedSender[adds[i].sender] = true;
 
       // Third parameter is true to indicate that the sender is being approved, to improve off-chain event indexing
-      emit ApprovedSenderModified(adds[i].destChainSelector, adds[i].sender, true);
+      emit ApprovedSenderAdded(adds[i].destChainSelector, adds[i].sender);
     }
   }
 
@@ -141,9 +143,10 @@ abstract contract CCIPBase is OwnerIsCreator {
   // │                      Chain Management                        │
   // ================================================================
 
-  // TODO comments
-  // TODO name as updateRouter
-  function modifyRouter(address newRouter) external onlyOwner {
+  /// @notice Updates the address of the CCIP router to send/receive messages.
+  /// @dev function will can only be called by the owner, and should only be used in emergencies if the current CCIP Router is deprecated.
+  /// @param newRouter the address of the new router, cannot be the zero address.
+  function updateRouter(address newRouter) external onlyOwner {
     if (newRouter == address(0)) revert ZeroAddressNotAllowed();
 
     // Store the old router in memory to emit event
@@ -177,14 +180,19 @@ abstract contract CCIPBase is OwnerIsCreator {
     }
   }
 
-  // TODO comments
+  /// @notice Reverts if the specified chainSelector is not approved to send/receive messages to/from this contract
+  /// @param chainSelector the CCIP specific chain selector for a given remote-chain.
   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];
     if (currentConfig.recipient.length == 0) revert InvalidChain(chainSelector);
     _;
   }
 
+  /// @notice Ensures if the specified chain is not enabled, or if the sender of an incoming message has not been approved by contract owner
+  /// @param chainSelector the CCIP specific chain selector for a given remote-chain.
+  /// @param sender the address of the sender of the message on the source-chain.
+  /// @dev The modifier will revert if either the sender is not approved OR if the relevant chain is currently disabled.
   modifier isValidSender(uint64 chainSelector, bytes memory sender) {
     // If the chain is disabled, then short-circuit trigger a revert because no sender should be valid
     if (s_chainConfigs[chainSelector].recipient.length == 0 || !s_chainConfigs[chainSelector].approvedSender[sender]) {

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

@@ -4,22 +4,31 @@ pragma solidity ^0.8.0;
 import {IRouterClient} from "../../interfaces/IRouterClient.sol";
 
 import {Client} from "../../libraries/Client.sol";
-import {CCIPReceiverWithACK} from "./CCIPReceiverWithACK.sol";
+import {CCIPReceiver} from "./CCIPReceiver.sol";
 
 import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol";
 import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol";
 
 /// @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 {
+contract CCIPClient is CCIPReceiver {
   using SafeERC20 for IERC20;
 
-  constructor(address router, IERC20 feeToken) CCIPReceiverWithACK(router, feeToken) {}
+  IERC20 public s_feeToken;
+
+  event MessageSent(bytes32 messageId);
+
+  constructor(address router, IERC20 feeToken) CCIPReceiver(router) {
+    s_feeToken = feeToken;
+  }
 
   /// @notice sends a message through CCIP to the router
-  // TODO really beef up the comments here
+  /// @param destChainSelector The unique CCIP identifier for a destination chain
+  /// @param tokenAmounts An array of CCIP compatible tokens and their amounts to send through the bridge
+  /// @param data Arbitrary bytes to be sent to the destination address on the destination chain
+  /// @dev The recipient of the message and any extraArgs are set manually ahead of time using the applyChainUpdates() function in CCIPBase
+  /// @return messageId the unique message ID generated by the CCIP router and used to track message status.
   function ccipSend(
     uint64 destChainSelector,
     Client.EVMTokenAmount[] memory tokenAmounts,
@@ -50,17 +59,24 @@ 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 is only generated in the on-ramp, and therefore cannot be calcualated head of time.
+    // This necessitates breaking CEI but since the router is a trusted contract, any risks are negligible.
     messageId = IRouterClient(s_ccipRouter).ccipSend{value: address(s_feeToken) == address(0) ? fee : 0}(
       destChainSelector, message
     );
 
-    s_messageStatus[messageId] = CCIPReceiverWithACK.MessageStatus.SENT;
-
-    // Since the message was outgoing, and not ACK, reflect this with bytes32(0)
-    emit MessageSent(messageId, bytes32(0));
+    emit MessageSent(messageId);
 
     return messageId;
   }
+
+  /// @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
+    override
+    onlySelf
+    isValidSender(message.sourceChainSelector, message.sender)
+    isValidChain(message.sourceChainSelector)
+  {}
 }