improve comments (#15524)

contracts/gas-snapshots/ccip.gas-snapshot

@@ -347,28 +347,24 @@ MultiOCR3Base_transmit:test_UnAuthorizedTransmitter_Revert() (gas: 24193)
 MultiOCR3Base_transmit:test_UnauthorizedSigner_Revert() (gas: 60994)
 MultiOCR3Base_transmit:test_UnconfiguredPlugin_Revert() (gas: 39824)
 MultiOCR3Base_transmit:test_ZeroSignatures_Revert() (gas: 32920)
-NonceManager_NonceIncrementation:test_getIncrementedOutboundNonce_Success() (gas: 37956)
-NonceManager_NonceIncrementation:test_incrementInboundNonce_Skip() (gas: 23706)
-NonceManager_NonceIncrementation:test_incrementInboundNonce_Success() (gas: 38778)
-NonceManager_NonceIncrementation:test_incrementNoncesInboundAndOutbound_Success() (gas: 71901)
-NonceManager_OffRampUpgrade:test_NoPrevOffRampForChain_Success() (gas: 185810)
-NonceManager_OffRampUpgrade:test_UpgradedNonceNewSenderStartsAtZero_Success() (gas: 189263)
-NonceManager_OffRampUpgrade:test_UpgradedNonceStartsAtV1Nonce_Success() (gas: 252318)
-NonceManager_OffRampUpgrade:test_UpgradedOffRampNonceSkipsIfMsgInFlight_Success() (gas: 220605)
-NonceManager_OffRampUpgrade:test_UpgradedSenderNoncesReadsPreviousRamp_Success() (gas: 60497)
-NonceManager_OffRampUpgrade:test_Upgraded_Success() (gas: 152975)
-NonceManager_OnRampUpgrade:test_UpgradeNonceNewSenderStartsAtZero_Success() (gas: 166167)
-NonceManager_OnRampUpgrade:test_UpgradeNonceStartsAtV1Nonce_Success() (gas: 195938)
-NonceManager_OnRampUpgrade:test_UpgradeSenderNoncesReadsPreviousRamp_Success() (gas: 139164)
-NonceManager_OnRampUpgrade:test_Upgrade_Success() (gas: 105212)
-NonceManager_applyPreviousRampsUpdates:test_MultipleRampsUpdates_success() (gas: 123604)
-NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySetOffRamp_Revert() (gas: 43403)
-NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySetOnRampAndOffRamp_Revert() (gas: 64752)
-NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySetOnRamp_Revert() (gas: 43245)
-NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySet_overrideAllowed_success() (gas: 45941)
+NonceManager_applyPreviousRampsUpdates:test_MultipleRampsUpdates() (gas: 123604)
+NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySet_overrideAllowed() (gas: 45986)
 NonceManager_applyPreviousRampsUpdates:test_SingleRampUpdate_success() (gas: 66889)
-NonceManager_applyPreviousRampsUpdates:test_ZeroInput_success() (gas: 12213)
-NonceManager_typeAndVersion:test_typeAndVersion() (gas: 9705)
+NonceManager_applyPreviousRampsUpdates:test_ZeroInput() (gas: 12169)
+NonceManager_getInboundNonce:test_getInboundNonce_NoPrevOffRampForChain() (gas: 185821)
+NonceManager_getInboundNonce:test_getInboundNonce_Upgraded() (gas: 152976)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceNewSenderStartsAtZero() (gas: 189296)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceStartsAtV1Nonce() (gas: 252384)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedOffRampNonceSkipsIfMsgInFlight() (gas: 220672)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedSenderNoncesReadsPreviousRamp() (gas: 60520)
+NonceManager_getIncrementedOutboundNonce:test_getIncrementedOutboundNonce() (gas: 37979)
+NonceManager_getIncrementedOutboundNonce:test_incrementInboundNonce() (gas: 38756)
+NonceManager_getIncrementedOutboundNonce:test_incrementInboundNonce_SkippedIncorrectNonce() (gas: 23759)
+NonceManager_getIncrementedOutboundNonce:test_incrementNoncesInboundAndOutbound() (gas: 71901)
+NonceManager_getOutboundNonce:test_getOutboundNonce_Upgrade() (gas: 105300)
+NonceManager_getOutboundNonce:test_getOutboundNonce_UpgradeNonceNewSenderStartsAtZero() (gas: 166146)
+NonceManager_getOutboundNonce:test_getOutboundNonce_UpgradeNonceStartsAtV1Nonce() (gas: 195937)
+NonceManager_getOutboundNonce:test_getOutboundNonce_UpgradeSenderNoncesReadsPreviousRamp() (gas: 140158)
 OffRamp_afterOC3ConfigSet:test_afterOCR3ConfigSet_SignatureVerificationDisabled_Revert() (gas: 5903354)
 OffRamp_applySourceChainConfigUpdates:test_AddMultipleChains_Success() (gas: 626094)
 OffRamp_applySourceChainConfigUpdates:test_AddNewChain_Success() (gas: 166505)
@@ -614,10 +610,8 @@ RegistryModuleOwnerCustom_registerAdminViaGetCCIPAdmin:test_registerAdminViaGetC
 RegistryModuleOwnerCustom_registerAdminViaGetCCIPAdmin:test_registerAdminViaGetCCIPAdmin_Success() (gas: 130126)
 RegistryModuleOwnerCustom_registerAdminViaOwner:test_registerAdminViaOwner_Revert() (gas: 19602)
 RegistryModuleOwnerCustom_registerAdminViaOwner:test_registerAdminViaOwner_Success() (gas: 129930)
-Router_applyRampUpdates:test_OffRampMismatch_Revert() (gas: 89591)
-Router_applyRampUpdates:test_OffRampUpdatesWithRouting() (gas: 10750087)
-Router_applyRampUpdates:test_OnRampDisable() (gas: 56445)
-Router_applyRampUpdates:test_OnlyOwner_Revert() (gas: 12414)
+Router_applyRampUpdates:test_applyRampUpdates_OffRampUpdatesWithRouting() (gas: 10749731)
+Router_applyRampUpdates:test_applyRampUpdates_OnRampDisable() (gas: 56422)
 Router_ccipSend:test_CCIPSendLinkFeeNoTokenSuccess_gas() (gas: 131447)
 Router_ccipSend:test_CCIPSendLinkFeeOneTokenSuccess_gas() (gas: 221710)
 Router_ccipSend:test_FeeTokenAmountTooLow_Revert() (gas: 71858)

contracts/src/v0.8/ccip/FeeQuoter.sol

@@ -103,12 +103,12 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
     // The following three properties are defaults, they can be overridden by setting the TokenTransferFeeConfig for a token.
     uint16 defaultTokenFeeUSDCents; //           │ Default token fee charged per token transfer.
     uint32 defaultTokenDestGasOverhead; // ──────╯ Default gas charged to execute a token transfer on the destination chain.
-    uint32 defaultTxGasLimit; //─────────────────╮ Default gas limit for a tx.
-    uint64 gasMultiplierWeiPerEth; //            │ Multiplier for gas costs, 1e18 based so 11e17 = 10% extra cost.
-    uint32 networkFeeUSDCents; //                │ Flat network fee to charge for messages, multiples of 0.01 USD.
-    uint32 gasPriceStalenessThreshold; //        │ The amount of time a gas price can be stale before it is considered invalid (0 means disabled).
-    bool enforceOutOfOrder; //                   │ Whether to enforce the allowOutOfOrderExecution extraArg value to be true.
-    bytes4 chainFamilySelector; // ──────────────╯ Selector that identifies the destination chain's family. Used to determine the correct validations to perform for the dest chain.
+    uint32 defaultTxGasLimit; //──────────╮ Default gas limit for a tx.
+    uint64 gasMultiplierWeiPerEth; //     │ Multiplier for gas costs, 1e18 based so 11e17 = 10% extra cost.
+    uint32 networkFeeUSDCents; //         │ Flat network fee to charge for messages, multiples of 0.01 USD.
+    uint32 gasPriceStalenessThreshold; // │ The amount of time a gas price can be stale before it is considered invalid (0 means disabled).
+    bool enforceOutOfOrder; //            │ Whether to enforce the allowOutOfOrderExecution extraArg value to be true.
+    bytes4 chainFamilySelector; // ───────╯ Selector that identifies the destination chain's family. Used to determine the correct validations to perform for the dest chain.
   }
 
   /// @dev Struct to hold the configs and its destination chain selector. Same as DestChainConfig but with the
@@ -121,13 +121,13 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
 
   /// @dev Struct with transfer fee configuration for token transfers.
   struct TokenTransferFeeConfig {
-    uint32 minFeeUSDCents; // ────╮ Minimum fee to charge per token transfer, multiples of 0.01 USD.
-    uint32 maxFeeUSDCents; //     │ Maximum fee to charge per token transfer, multiples of 0.01 USD.
-    uint16 deciBps; //            │ Basis points charged on token transfers, multiples of 0.1bps, or 1e-5.
-    uint32 destGasOverhead; //    │ Gas charged to execute the token transfer on the destination chain.
-    //                            │ Extra data availability bytes that are returned from the source pool and sent to
-    uint32 destBytesOverhead; //  │ the destination pool. Must be >= Pool.CCIP_LOCK_OR_BURN_V1_RET_BYTES.
-    bool isEnabled; // ───────────╯ Whether this token has custom transfer fees.
+    uint32 minFeeUSDCents; // ───╮ Minimum fee to charge per token transfer, multiples of 0.01 USD.
+    uint32 maxFeeUSDCents; //    │ Maximum fee to charge per token transfer, multiples of 0.01 USD.
+    uint16 deciBps; //           │ Basis points charged on token transfers, multiples of 0.1bps, or 1e-5.
+    uint32 destGasOverhead; //   │ Gas charged to execute the token transfer on the destination chain.
+    //                           │ Data availability bytes that are returned from the source pool and sent to the dest
+    uint32 destBytesOverhead; // │ pool. Must be >= Pool.CCIP_LOCK_OR_BURN_V1_RET_BYTES. Set as multiple of 32 bytes.
+    bool isEnabled; // ──────────╯ Whether this token has custom transfer fees.
   }
 
   /// @dev Struct with token transfer fee configurations for a token, same as TokenTransferFeeConfig but with the token
@@ -517,6 +517,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
     for (uint256 i = 0; i < feeds.length; ++i) {
       TokenPriceFeedConfig memory feedConfig = s_usdPriceFeedsPerToken[feeds[i].token];
 
+      // If the token is not enabled we revert the entire report as that indicates some type of misconfiguration.
       if (!feedConfig.isEnabled) {
         revert TokenNotSupported(feeds[i].token);
       }
@@ -710,6 +711,8 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
         continue;
       }
 
+      // In the case where bpsFeeUSDWei, minFeeUSDWei, and maxFeeUSDWei are all 0, we skip the fee. This is intended
+      // to allow for a fee of 0 to be set.
       tokenTransferFeeUSDWei += bpsFeeUSDWei;
     }
 
@@ -863,7 +866,9 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
 
     if (evmExtraArgs.gasLimit > uint256(destChainConfig.maxPerMsgGasLimit)) revert MessageGasLimitTooHigh();
 
-    // If the chain enforces out of order execution, the extra args must allow it, otherwise revert.
+    // If the chain enforces out of order execution, the extra args must allow it, otherwise revert. We cannot assume
+    // the user intended to use OOO on any chain that requires it as it may lead to unexpected behavior. Therefore we
+    // revert instead of assuming the user intended to use OOO.
     if (destChainConfig.enforceOutOfOrder && !evmExtraArgs.allowOutOfOrderExecution) {
       revert ExtraArgOutOfOrderExecutionMustBeTrue();
     }

contracts/src/v0.8/ccip/NonceManager.sol

@@ -33,9 +33,9 @@ contract NonceManager is INonceManager, AuthorizedCallers, ITypeAndVersion {
 
   /// @dev The previous on/off ramps per chain selector.
   mapping(uint64 chainSelector => PreviousRamps previousRamps) private s_previousRamps;
-  /// @dev The current outbound nonce per sender used on the onramp.
+  /// @dev The current outbound nonce per sender used on the onRamp.
   mapping(uint64 destChainSelector => mapping(address sender => uint64 outboundNonce)) private s_outboundNonces;
-  /// @dev The current inbound nonce per sender used on the offramp.
+  /// @dev The current inbound nonce per sender used on the offRamp.
   /// Eventually in sync with the outbound nonce in the remote source chain NonceManager, used to enforce that messages
   /// are executed in the same order they are sent (assuming they are DON).
   mapping(uint64 sourceChainSelector => mapping(bytes sender => uint64 inboundNonce)) private s_inboundNonces;
@@ -71,6 +71,8 @@ contract NonceManager is INonceManager, AuthorizedCallers, ITypeAndVersion {
     if (outboundNonce == 0) {
       address prevOnRamp = s_previousRamps[destChainSelector].prevOnRamp;
       if (prevOnRamp != address(0)) {
+        // This gets the current nonce for a sender, not the already incremented nonce like getIncrementedOutboundNonce
+        // would return.
         return IEVM2AnyOnRamp(prevOnRamp).getSenderNonce(sender);
       }
     }

contracts/src/v0.8/ccip/capability/CCIPHome.sol

@@ -2,7 +2,6 @@
 pragma solidity 0.8.24;
 
 import {ICapabilityConfiguration} from "../../keystone/interfaces/ICapabilityConfiguration.sol";
-
 import {INodeInfoProvider} from "../../keystone/interfaces/INodeInfoProvider.sol";
 import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";
 
@@ -111,7 +110,7 @@ contract CCIPHome is Ownable2StepMsgSender, ITypeAndVersion, ICapabilityConfigur
     uint64 chainSelector; //               │ The (remote) chain that the configuration is for.
     uint8 FRoleDON; //                     │ The "big F" parameter for the role DON.
     uint64 offchainConfigVersion; // ──────╯ The version of the exec offchain configuration.
-    bytes offrampAddress; // The remote chain offramp address.
+    bytes offrampAddress; // The remote chain offRamp address.
     bytes rmnHomeAddress; // The home chain RMN home address.
     OCR3Node[] nodes; // Keys & IDs of nodes part of the role DON.
     bytes offchainConfig; // The offchain configuration for the OCR3 plugin. Protobuf encoded.
@@ -241,10 +240,9 @@ contract CCIPHome is Ownable2StepMsgSender, ITypeAndVersion, ICapabilityConfigur
   }
 
   /// @inheritdoc ICapabilityConfiguration
-  /// @dev The CCIP capability will fetch the configuration needed directly from this contract.
-  /// The offchain syncer will call this function, so its important that it doesn't revert.
+  /// @dev This function is not used in the CCIPHome contract but the interface requires it to be implemented.
   function getCapabilityConfiguration(
-    uint32 /* donId */
+    uint32
   ) external pure override returns (bytes memory configuration) {
     return bytes("");
   }

contracts/src/v0.8/ccip/interfaces/IEVM2AnyOnRamp.sol

@@ -8,9 +8,9 @@ interface IEVM2AnyOnRamp is IEVM2AnyOnRampClient {
   /// @return the next sequence number to be used.
   function getExpectedNextSequenceNumber() external view returns (uint64);
 
-  /// @notice Get the next nonce for a given sender.
+  /// @notice Get the current nonce for a given sender.
   /// @param sender The sender to get the nonce for.
-  /// @return nonce The next nonce for the sender.
+  /// @return nonce The current nonce for the sender.
   function getSenderNonce(
     address sender
   ) external view returns (uint64 nonce);

contracts/src/v0.8/ccip/libraries/Internal.sol

@@ -3,7 +3,11 @@ pragma solidity ^0.8.4;
 
 import {MerkleMultiProof} from "../libraries/MerkleMultiProof.sol";
 
-// Library for CCIP internal definitions common to multiple contracts.
+/// @notice Library for CCIP internal definitions common to multiple contracts.
+/// @dev The following is a non-exhaustive list of "known issues" for CCIP:
+/// - We could implement yield claiming for Blast. This is not worth the custom code path on non-blast chains.
+/// - uint32 is used for timestamps, which will overflow in 2106. This is not a concern for the current use case, as we
+/// expect to have migrated to a new version by then.
 library Internal {
   error InvalidEVMAddress(bytes encodedAddress);
 
@@ -36,8 +40,8 @@ library Internal {
 
   /// @notice A timestamped uint224 value that can contain several tightly packed fields.
   struct TimestampedPackedUint224 {
-    uint224 value; // ──────╮ Value in uint224, packed.
-    uint32 timestamp; // ───╯ Timestamp of the most recent price update.
+    uint224 value; // ────╮ Value in uint224, packed.
+    uint32 timestamp; // ─╯ Timestamp of the most recent price update.
   }
 
   /// @dev Gas price is stored in 112-bit unsigned int. uint224 can pack 2 prices.
@@ -192,10 +196,10 @@ library Internal {
   /// The messageId is not expected to match hash(message), since it may originate from another ramp family.
   struct RampMessageHeader {
     bytes32 messageId; // Unique identifier for the message, generated with the source chain's encoding scheme (i.e. not necessarily abi.encoded).
-    uint64 sourceChainSelector; // ──╮ the chain selector of the source chain, note: not chainId.
-    uint64 destChainSelector; //     │ the chain selector of the destination chain, note: not chainId.
-    uint64 sequenceNumber; //        │ sequence number, not unique across lanes.
-    uint64 nonce; // ────────────────╯ nonce for this lane for this sender, not unique across senders/lanes.
+    uint64 sourceChainSelector; // ─╮ the chain selector of the source chain, note: not chainId.
+    uint64 destChainSelector; //    │ the chain selector of the destination chain, note: not chainId.
+    uint64 sequenceNumber; //       │ sequence number, not unique across lanes.
+    uint64 nonce; // ───────────────╯ nonce for this lane for this sender, not unique across senders/lanes.
   }
 
   struct EVM2AnyTokenTransfer {
@@ -264,7 +268,7 @@ library Internal {
   // solhint-disable-next-line gas-struct-packing
   struct MerkleRoot {
     uint64 sourceChainSelector; // Remote source chain selector that the Merkle Root is scoped to
-    bytes onRampAddress; //        Generic onramp address, to support arbitrary sources; for EVM, use abi.encode
+    bytes onRampAddress; //        Generic onRamp address, to support arbitrary sources; for EVM, use abi.encode
     uint64 minSeqNr; // ─────────╮ Minimum sequence number, inclusive
     uint64 maxSeqNr; // ─────────╯ Maximum sequence number, inclusive
     bytes32 merkleRoot; //         Merkle root covering the interval & source chain messages

contracts/src/v0.8/ccip/offRamp/OffRamp.sol

@@ -362,6 +362,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
   /// @param manualExecGasExecOverrides An array of gas limits to use for manual execution.
   /// @dev If called from the DON, this array is always empty.
   /// @dev If called from manual execution, this array is always same length as messages.
+  /// @dev This function can fully revert in some cases, reverting potentially valid other reports with it. The reasons
+  /// for these reverts are so severe that we prefer to revert the entire batch instead of silently failing.
   function _executeSingleReport(
     Internal.ExecutionReport memory report,
     GasLimitOverride[] memory manualExecGasExecOverrides
@@ -583,6 +585,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
       destTokenAmounts: destTokenAmounts
     });
 
+    // The main message interceptor is the aggregate rate limiter, but we also allow for a custom interceptor. This is
+    // why we always have to call into the contract when it's enabled, even when there are no tokens in the message.
     address messageInterceptor = s_dynamicConfig.messageInterceptor;
     if (messageInterceptor != address(0)) {
       try IMessageInterceptor(messageInterceptor).onInboundMessage(any2EvmMessage) {}