From 64d1ec8d554bc70188562d819189c6be5132390a Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 3 Jul 2024 20:37:51 -0400 Subject: [PATCH] additional comments --- contracts/gas-snapshots/ccip.gas-snapshot | 4 +-- .../ccip/applications/external/CCIPClient.sol | 5 +-- .../applications/external/CCIPClientBase.sol | 36 ++++++++++++++----- .../applications/external/CCIPReceiver.sol | 22 ++++++------ .../external/CCIPReceiverWithACK.sol | 11 +++--- .../ccip/applications/external/CCIPSender.sol | 3 +- .../external/CCIPReceiverTest.t.sol | 2 -- 7 files changed, 51 insertions(+), 32 deletions(-) diff --git a/contracts/gas-snapshots/ccip.gas-snapshot b/contracts/gas-snapshots/ccip.gas-snapshot index c2a26ca75f..04def4f478 100644 --- a/contracts/gas-snapshots/ccip.gas-snapshot +++ b/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) diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol b/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol index 98d5b0772e..7376a3cabf 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol +++ b/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)); diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPClientBase.sol b/contracts/src/v0.8/ccip/applications/external/CCIPClientBase.sol index 6b0844ed3f..09a532c20e 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPClientBase.sol +++ b/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); _; diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol b/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol index b95d2cfcb1..b6dc6fc8b0 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol +++ b/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]; } diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol b/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol index 310182b311..aee2b4686d 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol +++ b/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), diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPSender.sol b/contracts/src/v0.8/ccip/applications/external/CCIPSender.sol index 6590c495a3..88f7d20ff9 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPSender.sol +++ b/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); } diff --git a/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol b/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol index 375d84d501..e64a5d28ab 100644 --- a/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol +++ b/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();