Dev comments (#64)

Co-authored-by: Matt Yang <[email protected]>

contracts/gas-snapshots/ccip.gas-snapshot

@@ -147,7 +147,7 @@ EVM2EVMOffRamp_getDestinationToken:testGetDestinationTokenSuccess() (gas: 32772)
 EVM2EVMOffRamp_getDestinationToken:testUnsupportedTokenReverts() (gas: 13772)
 EVM2EVMOffRamp_getDestinationTokens:testGetDestinationTokensSuccess() (gas: 26043)
 EVM2EVMOffRamp_getExecutionState:testFillExecutionStateSuccess() (gas: 3081264)
-EVM2EVMOffRamp_getExecutionState:testFuzz_DifferentialSuccess(uint16[500],uint8[500]) (runs: 32, μ: 6733738, ~: 5866656)
+EVM2EVMOffRamp_getExecutionState:testFuzz_DifferentialSuccess(uint16[500],uint8[500]) (runs: 32, μ: 6689201, ~: 5866656)
 EVM2EVMOffRamp_getExecutionState:test_GetExecutionStateSuccess() (gas: 76529)
 EVM2EVMOffRamp_manuallyExecute:testLowGasLimitManualExecSuccess() (gas: 490545)
 EVM2EVMOffRamp_manuallyExecute:testManualExecForkedChainReverts() (gas: 24483)
@@ -214,7 +214,7 @@ EVM2EVMOnRamp_getSupportedTokens:testGetSupportedTokensSuccess() (gas: 51505)
 EVM2EVMOnRamp_getTokenPool:testGetTokenPoolSuccess() (gas: 41113)
 EVM2EVMOnRamp_getTokenTransferCost:testCustomTokenBpsFeeSuccess() (gas: 39609)
 EVM2EVMOnRamp_getTokenTransferCost:testFeeTokenBpsFeeSuccess() (gas: 29898)
-EVM2EVMOnRamp_getTokenTransferCost:testFuzz_TokenTransferFeeDuplicateTokensSuccess(uint256,uint256) (runs: 256, μ: 66506, ~: 66573)
+EVM2EVMOnRamp_getTokenTransferCost:testFuzz_TokenTransferFeeDuplicateTokensSuccess(uint256,uint256) (runs: 256, μ: 66625, ~: 66573)
 EVM2EVMOnRamp_getTokenTransferCost:testLargeTokenTransferChargesMaxFeeAndGasSuccess() (gas: 25246)
 EVM2EVMOnRamp_getTokenTransferCost:testMixedTokenTransferFeeSuccess() (gas: 101530)
 EVM2EVMOnRamp_getTokenTransferCost:testNoTokenTransferChargesMinFeeSuccess() (gas: 16999)
@@ -234,7 +234,7 @@ EVM2EVMOnRamp_payNops:testNopPayNopsSuccess() (gas: 145815)
 EVM2EVMOnRamp_payNops:testOwnerPayNopsSuccess() (gas: 140496)
 EVM2EVMOnRamp_payNops:testPayNopsSuccessAfterSetNops() (gas: 294833)
 EVM2EVMOnRamp_payNops:testWrongPermissionsReverts() (gas: 15705)
-EVM2EVMOnRamp_payNops_fuzz:testFuzz_NopPayNopsSuccess(uint96) (runs: 256, μ: 293332, ~: 293332)
+EVM2EVMOnRamp_payNops_fuzz:testFuzz_NopPayNopsSuccess(uint96) (runs: 256, μ: 293333, ~: 293332)
 EVM2EVMOnRamp_setAllowListEnabled:testOnlyOwnerReverts() (gas: 15605)
 EVM2EVMOnRamp_setAllowListEnabled:testSetAllowListEnabledSuccess() (gas: 16325)
 EVM2EVMOnRamp_setDynamicConfig:testSetConfigInvalidConfigReverts() (gas: 31616)
@@ -312,8 +312,8 @@ OCR2Base_transmit:testWrongNumberOfSignaturesReverts() (gas: 20501)
 OnRampTokenPoolReentrancy:testSuccess() (gas: 316710)
 PingPong_ccipReceive:testCcipReceiveSuccess() (gas: 142369)
 PingPong_plumbing:testFuzz_CounterPartAddressSuccess(address) (runs: 256, μ: 13806, ~: 13844)
-PingPong_plumbing:testFuzz_CounterPartAddressSuccess(uint64,address) (runs: 256, μ: 19762, ~: 20119)
-PingPong_plumbing:testFuzz_CounterPartChainSelectorSuccess(uint64) (runs: 256, μ: 13595, ~: 13863)
+PingPong_plumbing:testFuzz_CounterPartAddressSuccess(uint64,address) (runs: 256, μ: 19744, ~: 20119)
+PingPong_plumbing:testFuzz_CounterPartChainSelectorSuccess(uint64) (runs: 256, μ: 13577, ~: 13863)
 PingPong_plumbing:testPausingSuccess() (gas: 14471)
 PingPong_startPingPong:testStartPingPongSuccess() (gas: 168162)
 PriceRegistry_applyFeeTokensUpdates:testApplyFeeTokensUpdatesSuccess() (gas: 77674)
@@ -323,7 +323,7 @@ PriceRegistry_applyPriceUpdatersUpdates:testOnlyCallableByOwnerReverts() (gas: 1
 PriceRegistry_constructor:testInvalidStalenessThresholdReverts() (gas: 66088)
 PriceRegistry_constructor:testSetupSuccess() (gas: 1604044)
 PriceRegistry_convertTokenAmount:testConvertTokenAmountSuccess() (gas: 60235)
-PriceRegistry_convertTokenAmount:testFuzz_ConvertTokenAmountSuccess(uint256,uint192,uint160,uint192) (runs: 256, μ: 130453, ~: 130177)
+PriceRegistry_convertTokenAmount:testFuzz_ConvertTokenAmountSuccess(uint256,uint192,uint160,uint192) (runs: 256, μ: 130440, ~: 130177)
 PriceRegistry_convertTokenAmount:testLinkTokenNotSupportedReverts() (gas: 23567)
 PriceRegistry_convertTokenAmount:testStaleFeeTokenReverts() (gas: 31612)
 PriceRegistry_convertTokenAmount:testStaleLinkTokenReverts() (gas: 31126)
@@ -392,7 +392,7 @@ Router_routeMessage:testWhenNotHealthyReverts() (gas: 47157)
 Router_setWrappedNative:testFuzz_SetWrappedNativeSuccess(address) (runs: 256, μ: 13837, ~: 13894)
 Router_setWrappedNative:testOnlyOwnerReverts() (gas: 10976)
 ThirdPartyBurnMintTokenPool_applyRampUpdates:testInvalidOffRampReverts() (gas: 23413)
-ThirdPartyBurnMintTokenPool_lockOrBurn:testFuzz_LockOrBurnNoAllowListSuccess(uint256) (runs: 256, μ: 75255, ~: 74968)
+ThirdPartyBurnMintTokenPool_lockOrBurn:testFuzz_LockOrBurnNoAllowListSuccess(uint256) (runs: 256, μ: 75253, ~: 74968)
 ThirdPartyBurnMintTokenPool_lockOrBurn:testLockOrBurnWithAllowListReverts() (gas: 19542)
 ThirdPartyBurnMintTokenPool_lockOrBurn:testLockOrBurnWithAllowListSuccess() (gas: 96126)
 TokenPoolWithAllowList_applyAllowListUpdates:testOnlyOwnerReverts() (gas: 12072)
@@ -425,7 +425,7 @@ USDCTokenPool_lockOrBurn:testFuzz_LockOrBurnWithAllowListSuccess(bytes32,uint256
 USDCTokenPool_lockOrBurn:testLockOrBurnWithAllowListReverts() (gas: 19639)
 USDCTokenPool_lockOrBurn:testPermissionsErrorReverts() (gas: 13982)
 USDCTokenPool_lockOrBurn:testUnknownDomainReverts() (gas: 181529)
-USDCTokenPool_releaseOrMint:testFuzz_ReleaseOrMintSuccess(address,uint256) (runs: 256, μ: 47117, ~: 47130)
+USDCTokenPool_releaseOrMint:testFuzz_ReleaseOrMintSuccess(address,uint256) (runs: 256, μ: 47112, ~: 47130)
 USDCTokenPool_releaseOrMint:testTokenMaxCapacityExceededReverts() (gas: 24059)
 USDCTokenPool_releaseOrMint:testUnlockingUSDCFailedReverts() (gas: 40409)
 USDCTokenPool_setConfig:testInvalidConfigReverts() (gas: 14994)

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

@@ -5,6 +5,11 @@ import {TypeAndVersionInterface} from "../interfaces/TypeAndVersionInterface.sol
 
 import {OwnerIsCreator} from "./../shared/access/OwnerIsCreator.sol";
 
+/// @notice The ARMProxy serves to allow CCIP contracts
+/// to point to a static address for ARM queries, which saves gas
+/// since each contract need not store an ARM address in storage. That way
+/// we can add ARM queries along many code paths for increased defense in depth
+/// with minimal additional cost.
 contract ARMProxy is OwnerIsCreator, TypeAndVersionInterface {
   error ZeroAddressNotAllowed();
 

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

@@ -8,6 +8,9 @@ import {Client} from "./libraries/Client.sol";
 import {RateLimiter} from "./libraries/RateLimiter.sol";
 import {USDPriceWith18Decimals} from "./libraries/USDPriceWith18Decimals.sol";
 
+/// @notice The aggregate rate limiter is a wrapper of the token bucket rate limiter
+/// which permits rate limiting based on the aggregate value of a group of
+/// token transfers, using a price registry to convert to a numeraire asset (e.g. USD).
 contract AggregateRateLimiter is OwnerIsCreator {
   using RateLimiter for RateLimiter.TokenBucket;
   using USDPriceWith18Decimals for uint192;

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

@@ -170,7 +170,7 @@ contract CommitStore is ICommitStore, TypeAndVersionInterface, OCR2Base {
   }
 
   /// @inheritdoc OCR2Base
-  /// @dev A commitReport can have two distinct parts:
+  /// @dev A commitReport can have two distinct parts (batched together to amortize the cost of checking sigs):
   /// 1. Price updates
   /// 2. A merkle root and sequence number interval
   /// Both have their own, separate, staleness checks, with price updates using the epoch and round

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

@@ -23,10 +23,11 @@ import {ERC165Checker} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts
 
 /// @notice EVM2EVMOffRamp enables OCR networks to execute multiple messages
 /// in an OffRamp in a single transaction.
-/// @dev We will always deploy an onRamp, commitStore, and offRamp at the same time
-/// and we will never do partial updates where e.g. only an offRamp gets replaced.
-/// If we would replace only the offRamp and connect it with an existing commitStore,
-/// a replay attack would be possible.
+/// @dev The EVM2EVMOnRamp, CommitStore and EVM2EVMOffRamp form an xchain upgradeable unit. Any change to one of them
+/// results an onchain upgrade of all 3.
+/// @dev OCR2BaseNoChecks is used to save gas, signatures are not required as the offramp can only execute
+/// messages which are committed in the commitStore. We still make use of OCR2 as an executor whitelist
+/// and turn-taking mechanism.
 contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, TypeAndVersionInterface, OCR2BaseNoChecks {
   using Address for address;
   using ERC165Checker for address;
@@ -96,36 +97,45 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, TypeAndVersion
   // STATIC CONFIG
   // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables
   string public constant override typeAndVersion = "EVM2EVMOffRamp 1.1.0";
-  // The minimum amount of gas to perform the call with exact gas
+  /// @dev The minimum amount of gas to perform the call with exact gas.
+  /// We include this in the offramp so that we can redeploy to adjust it
+  /// should a hardfork change the gas costs of relevant opcodes in callWithExactGas.
   uint16 private constant GAS_FOR_CALL_EXACT_CHECK = 5_000;
-  // Commit store address on the destination chain
+  /// @dev Commit store address on the destination chain
   address internal immutable i_commitStore;
-  // ChainSelector of the source chain
+  /// @dev ChainSelector of the source chain
   uint64 internal immutable i_sourceChainSelector;
-  // ChainSelector of this chain
+  /// @dev ChainSelector of this chain
   uint64 internal immutable i_chainSelector;
-  // OnRamp address on the source chain
+  /// @dev OnRamp address on the source chain
   address internal immutable i_onRamp;
-  // metadataHash is a prefix for a message hash preimage to ensure uniqueness.
+  /// @dev metadataHash is a lane-specific prefix for a message hash preimage which ensures global uniqueness.
+  /// Ensures that 2 identical messages sent to 2 different lanes will have a distinct hash.
+  /// Must match the metadataHash used in computing leaf hashes offchain for the root committed in
+  /// the commitStore and i_metadataHash in the onRamp.
   bytes32 internal immutable i_metadataHash;
-  /// @dev The address of previous-version OffRamp for this lane
+  /// @dev The address of previous-version OffRamp for this lane.
+  /// Used to be able to provide sequencing continuity during a zero downtime upgrade.
   address internal immutable i_prevOffRamp;
   /// @dev The address of the arm proxy
   address internal immutable i_armProxy;
 
   // DYNAMIC CONFIG
   DynamicConfig internal s_dynamicConfig;
-  // source token => token pool
+  /// @dev source token => token pool
   EnumerableMapAddresses.AddressToAddressMap private s_poolsBySourceToken;
-  // dest token => token pool
+  /// @dev dest token => token pool
   EnumerableMapAddresses.AddressToAddressMap private s_poolsByDestToken;
 
   // STATE
-  // The expected nonce for a given sender.
+  /// @dev The expected nonce for a given sender.
+  /// Corresponds to s_senderNonce in the OnRamp, used to enforce that messages are
+  /// executed in the same order they are sent (assuming they are DON). Note that re-execution
+  /// of FAILED messages however, can be out of order.
   mapping(address sender => uint64 nonce) internal s_senderNonce;
-  // A mapping of sequence numbers to execution state using a bitmap with each execution
-  // state only taking up 2 bits of the uint256, packing 128 states into a single slot.
-  // This state makes sure we never execute a message twice.
+  /// @dev A mapping of sequence numbers to execution state using a bitmap with each execution
+  /// state only taking up 2 bits of the uint256, packing 128 states into a single slot.
+  /// Message state is tracked to ensure message can only be executed successfully once.
   mapping(uint64 seqNum => uint256 executionStateBitmap) internal s_executionStates;
 
   constructor(
@@ -207,6 +217,9 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, TypeAndVersion
 
   /// @notice Manually execute a message.
   /// @param report Internal.ExecutionReport.
+  /// @param gasLimitOverrides New gasLimit for each message in the report.
+  /// @dev We permit gas limit overrides so that users may manually execute messages which failed due to
+  /// insufficient gas provided.
   function manuallyExecute(Internal.ExecutionReport memory report, uint256[] memory gasLimitOverrides) external {
     // We do this here because the other _execute path is already covered OCR2BaseXXX.
     if (i_chainID != block.chainid) revert OCR2BaseNoChecks.ForkedChain(i_chainID, uint64(block.chainid));
@@ -231,8 +244,8 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, TypeAndVersion
   /// @notice Executes a report, executing each message in order.
   /// @param report The execution report containing the messages and proofs.
   /// @param manualExecGasLimits An array of gas limits to use for manual execution.
-  /// If called from the DON, this array is always empty.
-  /// If called from manual execution, this array is always same length as messages.
+  /// @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.
   function _execute(Internal.ExecutionReport memory report, uint256[] memory manualExecGasLimits) internal whenHealthy {
     uint256 numMsgs = report.messages.length;
     if (numMsgs == 0) revert EmptyReport();
@@ -316,6 +329,8 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, TypeAndVersion
         }
       }
 
+      // Although we expect only valid messages will be committed, we check again
+      // when executing as a defense in depth measure.
       bytes[] memory offchainTokenData = report.offchainTokenData[i];
       _isWellFormed(message, offchainTokenData.length);
 
@@ -380,8 +395,10 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, TypeAndVersion
   /// @notice Execute a single message.
   /// @param message The message that will be executed.
   /// @param offchainTokenData Token transfer data to be passed to TokenPool.
-  /// @dev this can only be called by the contract itself. It is part of
-  /// the Execute call, as we can only try/catch on external calls.
+  /// @dev We make this external and callable by the contract itself, in order to try/catch
+  /// its execution and enforce atomicity among successful message processing and token transfer.
+  /// @dev We use 165 to check for the ccipReceive interface to permit sending tokens to contracts
+  /// (for example smart contract wallets) without an associated message.
   function executeSingleMessage(Internal.EVM2EVMMessage memory message, bytes[] memory offchainTokenData) external {
     if (msg.sender != address(this)) revert CanOnlySelfCall();
     Client.EVMTokenAmount[] memory destTokenAmounts = new Client.EVMTokenAmount[](0);

contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol

@@ -20,13 +20,10 @@ import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/
 import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/utils/structs/EnumerableSet.sol";
 import {EnumerableMap} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/utils/structs/EnumerableMap.sol";
 
-/// @notice The onRamp is a contract that handles fee logic, NOP payments,
-/// token support and an allowList. It will always be deployed 1:1:1 with a
-/// commitStore and offRamp contract. These three contracts together form a
-/// `lane`. A lane is an upgradable set of contracts within the non-upgradable
-/// routers and are always deployed as complete set, even during upgrades.
-/// This means an upgrade to an onRamp will require redeployment of the
-/// commitStore and offRamp as well.
+/// @notice The onRamp is a contract that handles lane-specific fee logic, NOP payments,
+/// bridegable token support and an allowList.
+/// @dev The EVM2EVMOnRamp, CommitStore and EVM2EVMOffRamp form an xchain upgradeable unit. Any change to one of them
+/// results an onchain upgrade of all 3.
 contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter, TypeAndVersionInterface {
   using SafeERC20 for IERC20;
   using EnumerableMap for EnumerableMap.AddressToUintMap;
@@ -140,7 +137,10 @@ contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter,
   // STATIC CONFIG
   // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables
   string public constant override typeAndVersion = "EVM2EVMOnRamp 1.1.0";
-  /// @dev The metadata hash for this contract
+  /// @dev metadataHash is a lane-specific prefix for a message hash preimage which ensures global uniqueness
+  /// Ensures that 2 identical messages sent to 2 different lanes will have a distinct hash.
+  /// Must match the metadataHash used in computing leaf hashes offchain for the root committed in
+  /// the commitStore and i_metadataHash in the offRamp.
   bytes32 internal immutable i_metadataHash;
   /// @dev Default gas limit for a transactions that did not specify
   /// a gas limit in the extraArgs.
@@ -154,10 +154,12 @@ contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter,
   /// @dev The chain ID of the destination chain
   uint64 internal immutable i_destChainSelector;
   /// @dev The address of previous-version OnRamp for this lane
+  /// Used to be able to provide sequencing continuity during a zero downtime upgrade.
   address internal immutable i_prevOnRamp;
   /// @dev The address of the arm proxy
   address internal immutable i_armProxy;
   /// @dev the maximum number of nops that can be configured at the same time.
+  /// Used to bound gas for loops over nops.
   uint256 private constant MAX_NUMBER_OF_NOPS = 64;
 
   // DYNAMIC CONFIG
@@ -176,7 +178,9 @@ contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter,
   mapping(address token => TokenTransferFeeConfig tranferFeeConfig) internal s_tokenTransferFeeConfig;
 
   // STATE
-  /// @dev The current nonce per sender
+  /// @dev The current nonce per sender.
+  /// The offramp has a corresponding s_senderNonce mapping to ensure messages
+  /// are executed in the same order they are sent.
   mapping(address sender => uint64 nonce) internal s_senderNonce;
   /// @dev The amount of LINK available to pay NOPS
   uint96 internal s_nopFeesJuels;
@@ -534,6 +538,8 @@ contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter,
   /// A basis point fee is calculated from the USD value of each token transfer.
   /// Sum of basis point fees is confined within range [minTokenTransferFeeUSD, maxTokenTransferFeeUSD].
   /// @dev Assumes that tokenAmounts are validated to be listed tokens elsewhere.
+  /// @dev Splitting one token transfer into multiple transfers is discouraged,
+  /// as it will result in a transferFee equal or greater than the same amount aggregated/de-duped.
   function _getTokenTransferCost(
     address feeToken,
     uint192 feeTokenPrice,
@@ -673,8 +679,10 @@ contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter,
     _setNops(nopsAndWeights);
   }
 
-  /// @dev Clears existing nops, sets new nops and weights
   /// @param nopsAndWeights New set of nops and weights
+  /// @dev Clears existing nops, sets new nops and weights
+  /// @dev We permit fees to accrue before nops are configured, in which case
+  /// they will go to the first set of configured nops.
   function _setNops(NopAndWeight[] memory nopsAndWeights) internal {
     uint256 numberOfNops = nopsAndWeights.length;
     if (numberOfNops > MAX_NUMBER_OF_NOPS) revert TooManyNops();