Skip to content

Commit

Permalink
fixes based on feedback from review.
Browse files Browse the repository at this point in the history
  • Loading branch information
jhweintraub committed Jul 1, 2024
1 parent d6f5cc3 commit e391d17
Show file tree
Hide file tree
Showing 14 changed files with 178 additions and 255 deletions.
62 changes: 30 additions & 32 deletions contracts/gas-snapshots/ccip.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,10 @@ 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() (gas: 329311)
CCIPClientTest:test_ccipSendAndReceiveAck_in_return() (gas: 345011)
CCIPClientTest:test_ccipSend_withNativeFeeToken_butInsufficientMsgValue_REVERT() (gas: 83749)
CCIPClientTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 240447)
CCIPClientTest:test_send_tokens_that_are_not_feeToken() (gas: 551136)
CCIPClientTest:test_ccipReceiveAndSendAck() (gas: 331428)
CCIPClientTest:test_ccipSendAndReceiveAck_in_return() (gas: 347276)
CCIPClientTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 241501)
CCIPClientTest:test_send_tokens_that_are_not_feeToken() (gas: 552173)
CCIPConfigSetup:test_getCapabilityConfiguration_Success() (gas: 9495)
CCIPConfig_ConfigStateMachine:test__computeConfigDigest_Success() (gas: 70755)
CCIPConfig_ConfigStateMachine:test__computeNewConfigWithMeta_InitToRunning_Success() (gas: 357994)
Expand Down Expand Up @@ -89,21 +88,20 @@ 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: 191895)
CCIPReceiverTest:test_Recovery_from_invalid_sender() (gas: 426665)
CCIPReceiverTest:test_Recovery_with_intentional_revert() (gas: 430412)
CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_REVERT() (gas: 195900)
CCIPReceiverTest:test_removeSender_from_approvedList_and_revert() (gas: 422972)
CCIPReceiverTest:test_withdraw_nativeToken_to_owner() (gas: 18786)
CCIPReceiverWithAckTest:test_ccipReceive_ack_message() (gas: 53078)
CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack() (gas: 329309)
CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidMagicBytes_REVERT() (gas: 435500)
CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor() (gas: 2559441)
CCIPReceiverWithAckTest:test_modifyFeeToken() (gas: 72524)
CCIPSenderTest:test_ccipSend_withNativeFeeToken_butInsufficientMsgValue_REVERT() (gas: 81444)
CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andDestTokens() (gas: 333670)
CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 223290)
CCIPSenderTest:test_ccipSend_with_NativeFeeToken_andDestTokens() (gas: 366120)
CCIPReceiverTest:test_HappyPath_Success() (gas: 193605)
CCIPReceiverTest:test_Recovery_from_invalid_sender() (gas: 428748)
CCIPReceiverTest:test_Recovery_with_intentional_revert() (gas: 432364)
CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_REVERT() (gas: 205094)
CCIPReceiverTest:test_removeSender_from_approvedList_and_revert() (gas: 425143)
CCIPReceiverTest:test_withdraw_nativeToken_to_owner() (gas: 18785)
CCIPReceiverWithAckTest:test_ccipReceive_ack_message() (gas: 55238)
CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack() (gas: 331480)
CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidAckMessageHeaderBytes_REVERT() (gas: 437682)
CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor() (gas: 2631077)
CCIPReceiverWithAckTest:test_modifyFeeToken() (gas: 72646)
CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andDestTokens() (gas: 339182)
CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 224256)
CCIPSenderTest:test_ccipSend_with_NativeFeeToken_andDestTokens() (gas: 367940)
CommitStore_constructor:test_Constructor_Success() (gas: 3091326)
CommitStore_isUnpausedAndRMNHealthy:test_RMN_Success() (gas: 73420)
CommitStore_report:test_InvalidIntervalMinLargerThanMax_Revert() (gas: 28670)
Expand Down Expand Up @@ -210,7 +208,7 @@ EVM2EVMMultiOffRamp_executeSingleReport:test_NonExistingSourceChain_Revert() (ga
EVM2EVMMultiOffRamp_executeSingleReport:test_ReceiverError_Success() (gas: 170344)
EVM2EVMMultiOffRamp_executeSingleReport:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 182073)
EVM2EVMMultiOffRamp_executeSingleReport:test_RootNotCommitted_Revert() (gas: 47177)
EVM2EVMMultiOffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 1390005)
EVM2EVMMultiOffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 1381192)
EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain_Success() (gas: 232987)
EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered_Success() (gas: 166040)
EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokens_Success() (gas: 180585)
Expand Down Expand Up @@ -244,8 +242,8 @@ EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouche
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_FailedTx_Revert() (gas: 199773)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ForkedChain_Revert() (gas: 28246)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_GasLimitMismatchMultipleReports_Revert() (gas: 160817)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 1481951)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 3173301)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 1473138)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 3164486)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_Success() (gas: 201928)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride_Success() (gas: 202502)
EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride_Success() (gas: 651738)
Expand Down Expand Up @@ -384,7 +382,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: 1386758)
EVM2EVMOffRamp_execute:test_RouterYULCall_Revert() (gas: 1377945)
EVM2EVMOffRamp_execute:test_SingleMessageNoTokensUnordered_Success() (gas: 159863)
EVM2EVMOffRamp_execute:test_SingleMessageNoTokens_Success() (gas: 175094)
EVM2EVMOffRamp_execute:test_SingleMessageToNonCCIPReceiver_Success() (gas: 248764)
Expand Down Expand Up @@ -416,14 +414,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: 1468131)
EVM2EVMOffRamp_manuallyExecute:test_LowGasLimitManualExec_Success() (gas: 1459318)
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: 2853408)
EVM2EVMOffRamp_manuallyExecute:test_ReentrancyManualExecuteFails() (gas: 2846394)
EVM2EVMOffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched_Success() (gas: 144106)
EVM2EVMOffRamp_metadataHash:test_MetadataHash_Success() (gas: 8871)
EVM2EVMOffRamp_setDynamicConfig:test_NonOwner_Revert() (gas: 40429)
Expand Down Expand Up @@ -682,9 +680,9 @@ 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: 200530)
PingPong_example_plumbing:test_Pausing_Success() (gas: 17898)
PingPong_example_startPingPong:test_StartPingPong_Success() (gas: 223875)
PingPong_example_ccipReceive:test_CcipReceive_Success() (gas: 308219)
PingPong_example_plumbing:test_Pausing_Success() (gas: 17766)
PingPong_example_startPingPong:test_StartPingPong_Success() (gas: 234273)
PriceRegistry_applyFeeTokensUpdates:test_ApplyFeeTokensUpdates_Success() (gas: 79823)
PriceRegistry_applyFeeTokensUpdates:test_OnlyCallableByOwner_Revert() (gas: 12580)
PriceRegistry_constructor:test_InvalidStalenessThreshold_Revert() (gas: 67418)
Expand Down Expand Up @@ -831,9 +829,9 @@ 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: 288024)
SelfFundedPingPong_ccipReceive:test_Funding_Success() (gas: 448398)
SelfFundedPingPong_setCountIncrBeforeFunding:test_setCountIncrBeforeFunding() (gas: 20226)
SelfFundedPingPong_ccipReceive:test_FundingIfNotANop_Revert() (gas: 290332)
SelfFundedPingPong_ccipReceive:test_Funding_Success() (gas: 452301)
SelfFundedPingPong_setCountIncrBeforeFunding:test_setCountIncrBeforeFunding() (gas: 20203)
TokenAdminRegistry_acceptAdminRole:test_acceptAdminRole_OnlyPendingAdministrator_Revert() (gas: 51085)
TokenAdminRegistry_acceptAdminRole:test_acceptAdminRole_Success() (gas: 43947)
TokenAdminRegistry_addRegistryModule:test_addRegistryModule_OnlyOwner_Revert() (gas: 12629)
Expand Down
2 changes: 0 additions & 2 deletions contracts/scripts/native_solc_compile_all_ccip
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,6 @@ compileContract ccip/tokenAdminRegistry/TokenAdminRegistry.sol
compileContract ccip/tokenAdminRegistry/RegistryModuleOwnerCustom.sol
compileContract ccip/capability/CCIPConfig.sol



# Test helpers
compileContract ccip/test/helpers/BurnMintERC677Helper.sol
compileContract ccip/test/helpers/CommitStoreHelper.sol
Expand Down
59 changes: 22 additions & 37 deletions contracts/src/v0.8/ccip/applications/external/CCIPClient.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,71 +9,56 @@ import {CCIPReceiverWithACK} from "./CCIPReceiverWithACK.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";

/// @notice CCIPReceiver and CCIPSender cannot be simultaneously imported due to similar parents so CCIPSender functionality has been duplicated
contract CCIPClient is CCIPReceiverWithACK {
using SafeERC20 for IERC20;

error InvalidConfig();
error CannotAcknowledgeUnsentMessage(bytes32);

/// @notice You can't import CCIPReceiver and Sender due to similar parents so functionality of CCIPSender is duplicated here
constructor(address router, IERC20 feeToken) CCIPReceiverWithACK(router, feeToken) {}

function typeAndVersion() external pure virtual override returns (string memory) {
return "CCIPReceiverWithACK 1.0.0-dev";
return "CCIPClient 1.0.0-dev";
}

function ccipSend(
uint64 destChainSelector,
Client.EVMTokenAmount[] memory tokenAmounts,
bytes memory data,
address feeToken
bytes memory data
) public payable validChain(destChainSelector) returns (bytes32 messageId) {
Client.EVM2AnyMessage memory message = Client.EVM2AnyMessage({
receiver: s_chains[destChainSelector].recipient,
receiver: s_chainConfigs[destChainSelector].recipient,
data: data,
tokenAmounts: tokenAmounts,
extraArgs: s_chains[destChainSelector].extraArgsBytes,
feeToken: feeToken
extraArgs: s_chainConfigs[destChainSelector].extraArgsBytes,
feeToken: address(s_feeToken)
});

uint256 fee = IRouterClient(i_ccipRouter).getFee(destChainSelector, message);

bool sendingFeeTokenNormally;

for (uint256 i = 0; i < tokenAmounts.length; ++i) {
// Transfer the tokens to pay for tokens in tokenAmounts
IERC20(tokenAmounts[i].token).safeTransferFrom(msg.sender, address(this), tokenAmounts[i].amount);

// If they are sending the feeToken through, and its the same as the ack fee token, and also paying in it, then you don't need to approve
// it at all cause its already set as type(uint).max. You can't use safeIncreaseAllowance() either cause it will overflow the token allowance
if (tokenAmounts[i].token == feeToken && feeToken != address(0) && feeToken == address(s_feeToken)) {
sendingFeeTokenNormally = true;
IERC20(tokenAmounts[i].token).safeTransferFrom(msg.sender, address(this), fee);
}
// If they're not sending the fee token, then go ahead and approve
else {
IERC20(tokenAmounts[i].token).safeApprove(i_ccipRouter, tokenAmounts[i].amount);
// Do not approve the tokens if it is the feeToken, otherwise the approval amount may overflow
if (tokenAmounts[i].token != address(s_feeToken)) {
IERC20(tokenAmounts[i].token).safeIncreaseAllowance(i_ccipRouter, tokenAmounts[i].amount);
}
}

// Since the fee token was already set in the ReceiverWithACK parent, we don't need to approve it to spend, only to ensure we have enough
// funds for the transfer
if (!sendingFeeTokenNormally && feeToken == address(s_feeToken) && feeToken != address(0)) {
// Support pre-funding the contract with fee token by checking that not enough exists to pay the fee already
// msg.sender checks that the function was not called in a _sendACK() call or as a result of processMessage(), which would require pre-funding the contract
if (IERC20(feeToken).balanceOf(address(this)) < fee && msg.sender != address(this)) {
IERC20(feeToken).safeTransferFrom(msg.sender, address(this), fee);
}
} else if (feeToken == address(0) && msg.value < fee) {
revert IRouterClient.InsufficientFeeTokenAmount();
uint256 fee = IRouterClient(i_ccipRouter).getFee(destChainSelector, message);

// Additional tokens for fees do not need to be approved to the router since it is already handled by setting s_feeToken
if (address(s_feeToken) != address(0)) {
IERC20(s_feeToken).safeTransferFrom(msg.sender, address(this), fee);
}

messageId =
IRouterClient(i_ccipRouter).ccipSend{value: feeToken == address(0) ? fee : 0}(destChainSelector, message);
messageId = IRouterClient(i_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, use bytes32(0) to reflect this
// Since the message was outgoing, and not ACK, reflect this with bytes32(0)
emit MessageSent(messageId, bytes32(0));

return messageId;
Expand All @@ -90,11 +75,11 @@ contract CCIPClient is CCIPReceiverWithACK {
// If the message was outgoing, then send an ack response.
_sendAck(message);
} else if (payload.messageType == MessageType.ACK) {
// Decode message into the magic-bytes and the messageId to ensure the message is encoded correctly
(bytes memory magicBytes, bytes32 messageId) = abi.decode(payload.data, (bytes, bytes32));
// Decode message into the message-heacder and the messageId to ensure the message is encoded correctly
(bytes memory messageHeader, bytes32 messageId) = abi.decode(payload.data, (bytes, bytes32));

// Ensure Ack Message contains proper magic-bytes
if (keccak256(magicBytes) != keccak256(ACKMESSAGEMAGICBYTES)) revert InvalidMagicBytes();
// Ensure Ack Message contains proper message header
if (keccak256(messageHeader) != keccak256(ACK_MESSAGE_HEADER)) revert InvalidAckMessageHeader();

// Make sure the ACK message was originally sent by this contract
if (s_messageStatus[messageId] != MessageStatus.SENT) revert CannotAcknowledgeUnsentMessage(messageId);
Expand Down
Loading

0 comments on commit e391d17

Please sign in to comment.