Skip to content

Commit

Permalink
more comments to address
Browse files Browse the repository at this point in the history
  • Loading branch information
matYang committed Jul 4, 2024
1 parent 1e907e9 commit 19830dd
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 12 deletions.
40 changes: 30 additions & 10 deletions contracts/src/v0.8/ccip/applications/external/CCIPClientBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/tok
import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol";
import {Address} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/Address.sol";

// TODO how do you feel about just renaming this to `CCIPBase`
// TODO add natspec comments on top of every contract
abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
using SafeERC20 for IERC20;
using Address for address;
Expand All @@ -18,18 +20,25 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
error InvalidSender(bytes sender);
error InvalidRecipient(bytes recipient);

// TODO every field in a struct should have comment, also do spack comments alignment, for example
struct ApprovedSenderUpdate {
uint64 destChainSelector;
bytes sender; // The address which initiated source-chain message, abi-encoded in case of a non-EVM-compatible network
uint64 destChainSelector; // Chainselector for a source chain that is allowed to call this dapp
bytes sender; // The sender address on source chain that is allowed to call, ABI encoded in the case of a remote EVM chain
}

// TODO change disabled to enabled so the default value is false if it is not configured
// TODO actually, can you use recipient.length != 0 as the enabled flag? Saves 1 slot
struct ChainConfig {
bool disabled;
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
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.


// TODO important to add context on why this is a nested mapping; afaik it is to support remote one-sided dapp upgrades
mapping(bytes recipient => bool isApproved) approvedSender;
}

// TODO I recall there were some discussions about upgradable Router, was it confirmed with GTM/Devrel we can have immutable router?
address internal immutable i_ccipRouter;

mapping(uint64 destChainSelector => ChainConfig) public s_chainConfigs;
Expand All @@ -43,7 +52,7 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
// │ Router Management │
// ================================================================

/// @notice returns the address of the CCIP Router set at contract-deployment
/// @notice returns the address of the CCIP Router set at contract deployment
function getRouter() public view virtual returns (address) {
return i_ccipRouter;
}
Expand All @@ -58,8 +67,10 @@ 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
// TODO we should be consistent in our styles, "source chain" is preferred over "soure-chain", when in doubt, check other contracts for reference
/// @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
// TODO emit events here
function updateApprovedSenders(
ApprovedSenderUpdate[] calldata adds,
ApprovedSenderUpdate[] calldata removes
Expand All @@ -77,6 +88,7 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
/// @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
// TODO this function isn't being used elsewhere, this implies we are missing validation in receiver
function isApprovedSender(uint64 sourceChainSelector, bytes calldata senderAddr) external view returns (bool) {
return s_chainConfigs[sourceChainSelector].approvedSender[senderAddr];
}
Expand All @@ -85,19 +97,25 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
// │ Fee Token Management │
// ===============================================================

/// @notice function support native-fee-token pre-funding in all children implementing the ccipSend function
// TODO update comments to be explicit about what the function does.
/// @notice Accepts prefunding in native fee token.
/// @dev All the example applications accept prefunding. This function should be removed if prefunding in native fee token is not required.
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
/// TODO this is withdraw native, it isn't related to token recovery
/// @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);
}

// TODO provide context on instructions, instead of just saying what to do, also explain why
/// @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
/// @dev This should NOT be used for recoverying tokens from a failed message. Token recoveries can happen only if
/// the failed message is guaranteed to not succeed upon retry, otherwise this can lead to double spend.
/// For implementation of token recovery, see inheriting contracts.
/// @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 {
Expand All @@ -108,6 +126,8 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
// │ Chain Management │
// ================================================================

// TODO let's combine `enabledChain` and `disableChain` into a single `applyChainUpdates` that takes in an array to allow for bulk edits
// Reference similar function in TokenPool
/// @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.
Expand All @@ -124,7 +144,7 @@ abstract contract CCIPClientBase is OwnerIsCreator, ITypeAndVersion {
// 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 config was previously disabled, then re-enable it
if (currentConfig.disabled) currentConfig.disabled = false;
}

Expand Down
20 changes: 18 additions & 2 deletions contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ contract CCIPReceiver is CCIPClientBase {
// RESOLVED is first so that the default value is resolved.
RESOLVED,
FAILED,
// TODO comment
ABANDONED
}

Expand Down Expand Up @@ -56,6 +57,16 @@ contract CCIPReceiver is CCIPClientBase {
{
try this.processMessage(message) {}
catch (bytes memory err) {
// TODO kinda dangerous comments here.
// The original suggestion was:
// if you want custom retry logic, plus owner extracting tokens as a last resort for recovery, use this try-catch pattern in ccipReceiver
// this means the message will appear as success to CCIP, and you can track the actual message state within the dapp
// if you do not need custom retry logic, and you don't need owner token recovery function, then you don't need the try-catch here
// because you can use ccip manualExecution as a retry function
//
// we should not phrase it such that "Any failures should be tracked by individual Dapps", it's only the case if they want custom retry pattern
// as opposed to default manual exec, or they want token recovery feature.

// Mark the message as having failed. Any failures should be tracked by individual Dapps, since CCIP
// will mark the message as having been successfully delivered. CCIP makes no assurances about message delivery
// other than invocation with proper gas limit. Any logic/execution errors should be tracked by separately.
Expand Down Expand Up @@ -87,6 +98,7 @@ contract CCIPReceiver is CCIPClientBase {
// │ Failed Message Processing |
// ================== ==============================================

// TODO we don't do new @dev on every new line, same for @notice, when in doubt check other contracts.
/// @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
Expand All @@ -108,10 +120,14 @@ contract CCIPReceiver is CCIPClientBase {
}

/// @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
/// TODO these funtions do not exist
/// @dev If the owner wants to retrieve tokens without special logic, then abandonMessage() or recoverTokens() should be used instead
/// @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 {}
function _retryFailedMessage(Client.Any2EVMMessage memory message) internal virtual onlyOwner {
// TODO how about we add a default implementation that calls `processMessage`, and comments for overrides
}

// TODO double notices
/// @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.
/// @dev this function is only callable as the owner, and
Expand Down

0 comments on commit 19830dd

Please sign in to comment.