Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge CCIP V1.6 [CCIP-2946] #14278

Merged
merged 55 commits into from
Aug 30, 2024
Merged

Conversation

asoliman92
Copy link
Contributor

@asoliman92 asoliman92 commented Aug 29, 2024

Porting all CCIP V1.6 code (contracts and offchain) from ccip repo to chainlink repo.

This PR is divided into 2 steps.
First step cherry-picked changes that happened to contracts and capabilities directories.
Then fixed issues in this branch, squashed fixes and merged them here.

To review this PR you need the following:

  1. Checkout the branch locally
  2. Checkout ccip @6bdc1dc039
  3. Run the given script (make sure both chainlink and ccip repos have same parent directory and that the script is with them in the parent)

./check-diff.sh contracts/src/v0.8/ccip
expected output:

ccip repo commit: 
6bdc1dc039 (HEAD -> ccip-develop, origin/ccip-develop, origin/HEAD) Bump chain-selectors => v1.0.23 (#1395)
chainlink repo commit: 
3dd8a72e92 (HEAD -> ccip/capabilities-debug-contracts-fmt) pnpm i && snapshot && forge fmt

Source files not modified:
     174
Source files modified:   

Then
./check-diff.sh core/gethwrappers/ccip
expected ouptut:

ccip repo commit: 
6bdc1dc039 (HEAD -> ccip-develop, origin/ccip-develop, origin/HEAD) Bump chain-selectors => v1.0.23 (#1395)
chainlink repo commit: 
3dd8a72e92 (HEAD -> ccip/capabilities-debug-contracts-fmt) pnpm i && snapshot && forge fmt

Source files not modified:
      64
Source files modified: 

Script:

#!/usr/bin/env bash

# Check if the directory is passed as an argument
if [ -z "$1" ]; then
    echo "Usage: $0 <directory>"
    exit 1
fi

# Directory to check for parity
CHECK_DIR="$1"

if [[ "$OSTYPE" == "darwin"* ]]; then
    # macOS
    SUM=gmd5sum
else
    # Assume Linux
    SUM=md5sum
fi

pushd ccip/$CHECK_DIR > /dev/null
echo -e "ccip repo commit: \n$(git log --max-count=1 --oneline --decorate)"
find . -type f -exec $SUM {} \;  > ../files.md5
popd > /dev/null
mv ccip/$CHECK_DIR/../files.md5 ./chainlink/$CHECK_DIR/../files.md5
pushd ./chainlink/$CHECK_DIR > /dev/null
echo -e "chainlink repo commit: \n$(git log --max-count=1 --oneline --decorate)"

echo ""

echo "Source files not modified:"
$SUM -c ../files.md5 2>/dev/null | grep OK | wc -l
echo "Source files modified:"
$SUM -c ../files.md5 2>/dev/null | grep -v OK

For this reason both forge fmt and solidity lint are failing the CI checks. We're taking identical copy and we're not interested in formatting them for now to make it easier reviewing the branch.

Once both are good to go please review the offchain changes and any other changes.

For clarity this is how I cherry-picked the PRs:

# How we got the commits:
# In CCIP repo:
# git log --since="2024-08-01" --reverse --pretty=format:"%H" -- contracts/src/v0.8/ccip >> contracts-commits.txt
# git log --since="2024-08-03" --reverse --pretty=format:"%H" -- core/capabilities/ccip >> cap-commits.txt
# cat contracts-commits.txt cap-commits.txt > combined_commits.txt
# git log --no-walk --pretty=format:"%H %at" $(cat combined_commits.txt) --date-order > temp_commits.txt
# sort -k2,2n temp_commits.txt | awk '{print $1}' > final_commits.txt
# have both ccip and chainlink repos under same parent directory and copy final_commits.txt and this script into the parent directory
# run this script

# Define the paths to the source and target repositories
TARGET_REPO="./chainlink"

# Read the commit hashes from the file
commits=$(cat $COMMITS_FILE)

# Navigate to the target repository
cd $TARGET_REPO

# Loop through each commit and cherry-pick it
for commit in $commits
do
    git cherry-pick $commit

    # Handle conflicts automatically
    if [ $? -ne 0 ]; then
        # Conflict resolution strategy
        # If you want to keep the changes from the source repo, you can run:
        # git checkout --theirs .
        # Or to keep the changes in the target repo, you can run:
        # git checkout --ours .

        # Example for keeping the changes from the source repo:
        git checkout --theirs .
        git add .
        git cherry-pick --continue --no-edit
    fi
done

RensR and others added 30 commits August 28, 2024 20:54
Various minor onchain fixes

- improve accounting for token payload data
- rm old check in usdc pool
- fix comment on precompile range
- add transferLiquidity function to LockReleaseTokenPool
- move rate limit admin logic from lockRelease pool to TokenPool

New changes

- removed defaultTokenDestBytesOverhead and used
CCIP_LOCK_OR_BURN_V1_RET_BYTES as default value
- Add uint32 destGasAmount to the SourceTokenData struct to allow us to
send the amount we billed on source to dest
- This single value is used for the releaseOrMint and what is left after
that call is used for transfer
- Removed the defaults for releaseOrMint and transfer from the offRamp

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
## Motivation
`NonceManager` and `MultiAggregateRateLimiter` contracts were missing
`typeAndVersion`.

## Solution
Add `ITypeAndVersion` inheritance to both contracts.
Since the separation of manual exec window and DON exec window, it would
theoretically be possible that two executions happen at the same time
(although unlikely).

Remove reverts to ensure other msgs in the batches are unaffected

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
## Motivation

This PR applies several fixes from static analysis findings.

## Solution

Fixes include:

- Unused state variables removal
- Unused imports removal
- Unused events removal
- Unused custom errors removal
- Add of missing zero address check
- Implementation of missing getter
- Use of constants instead of magic numbers
## Motivation
We want the ability to test new lanes using custom, test routers.
Therefore, we need a one-to-many relationship b/t routers and lanes,
rather than a singleton.

## Solution
Make routers configurable per lane, rather than per contract
## Motivation
Change the hop through the offRamp to a transferFrom so the funds never
touch CCIP contracts outside of the pool

## Solution
We want to define and use the appropriate OCR offchain config for each
plugin.

Requires smartcontractkit/chainlink-ccip#36
## Motivation
Change `typeAndVersion`of RMN contract from `RMN 1.5.0-dev` to `RMN
1.5.0` since the contract is finalized.

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: Rens Rooimans <[email protected]>
## Motivation
`EVM2EVMMultiOffRamp.executeSingleMessage` is optimised by replacing
memory to calldata, internal functions
`_releaseOrMintSingleTokens` and `_releaseOrMintSingleToken` can be
optimised
```
function executeSingleMessage(Internal.Any2EVMRampMessage memory message, bytes[] memory offchainTokenData) external
```
Tradeoff is increased contract size, which is exceeding the limit and
thus we have to decrease the optimiser runs

## Findings
After replacing memory with call data we have the following findings for
gas and contract size

| Test               | Before  | after | delta |
|
-------------------------------------------------------------------------------------------
| ------ | ------ | ------ |
|
EVM2EVMMultiOffRamp_executeSingleMessage:test_NonContractWithTokens_Success()
| 249368 | 247671 | \-1697 |
| EVM2EVMMultiOffRamp_executeSingleMessage:test_NonContract_Success() |
20672 | 19245 | \-1427 |
|
EVM2EVMMultiOffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens_Success()
| 48381 | 47265 | \-1116 |
|
EVM2EVMMultiOffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens_Success()
| 278146 | 276759 | \-1387 |
|
EVM2EVMMultiOffRamp_executeSingleMessage:test_executeSingleMessage_WithValidation_Success()
| 93615 | 92499 | \-1116 |
|
EVM2EVMMultiOffRamp__releaseOrMintSingleToken:test__releaseOrMintSingleToken_Success()
| 108343 | 107939 | \-404 |

| **Optimser Run** | **Size** | **Margin(kB)** | **With call data
optimisation** |
| ---------------- | --------------------------------- | --------------
| ------------------------------- |
| 2500 | 24.113 | 0.463 | No |
| 2500 | 24.857 (size increase of 0.744kB) | \-0.281 | Yes |
| 2400 | 24.635 | \-0.059 | Yes |
| 2200 | 24.635 | \-0.059 | |
| 2000 | 24.572 | 0.004 | Yes |

---------

Signed-off-by: 0xsuryansh <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
## Motivation
It is significantly cheaper to assert balances than do the transferFrom

## Solution
Undo recent approve changes, add balance assertions


had to undo some of the multiOfframp calldata changes to make it fit

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
currently, gas amounts are placed in the SourceTokenData. However, for
the multi-ramps, the token data should be chain-family agnostic, which
will likely require the destGasAmount lift to another field

- add a new Struct which holds the receiverExecutionGasLimit and
transferGasAmounts

```js
struct GasLimitOverride {
    uint256 receiverExecutionGasLimit;
    uint256[] tokenGasOverrides;
}
```

- `tokenGasOverrides` is an array of GasLimits to be used during the
`relaseOrMint` call for the specific tokenPool associated with token

---------

Signed-off-by: 0xsuryansh <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: 0xsuryansh <[email protected]>
Co-authored-by: Rens Rooimans <[email protected]>
Final release of all 1.5 contracts - don't merge before the other PRs
are all in


TODO
- #1258
- #1256
- #1273

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
## Motivation


## Solution
## Motivation

Recent merges to chainlink-ccip have caused the integration test to
break.

## Solution

Include the gas estimator changes required as well as some token data
reader changes.

Include fixes from
smartcontractkit/chainlink-ccip#60
## Motivation
Use the latest version of deps

## Solution
Upgrade OZ deps to latest version

## Implementation Notes
`IERC20` and `SafeERC20` are kept on `4.8.3` because of the `^0.8.20`
requirement OZ imposes on the v5 contracts. We can't upgrade these deps
until the rest of the contract codebase upgrades to 0.8.20+

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
…instead of bytes32 (#1207)

rateLimiterConfig mapping of localToRemoteTokens to take bytes instead
of bytes32

- In the MultiAggregateRateLimiter contract, the the following mapping
might be simplifiable to
- (uint64 remoteChainSelector -> address token) ,
- since it is only used as an isEnabled check (i.e. the (address token
-> bytes32 remoteToken) is never used on-chain)
- mapping being necessary off-chain, we need to convert the (address ->
bytes32) mapping to (address -> bytes) to be consistent with using bytes
for all family-agnostic addresses

```js
mapping(uint64 remoteChainSelector => EnumerableMapAddresses.AddressToBytes32Map tokensLocalToRemote) internal
    s_rateLimitedTokensLocalToRemote;
```

this has to been changed to

```js
  mapping(uint64 remoteChainSelector => EnumerableMapAddresses.AddressToBytesMap tokensLocalToRemote) internal
    s_rateLimitedTokensLocalToRemote;
```

1. New solidity library `EnumerableMapBytes32` which contains
`Bytes32ToBytes` Mapping and enumerate it
2. `EnumerableMapAddresses.sol` library has added support for the new
library cherrypicked from chainlink repo `EnumerableMapBytes32`
3. `MultiAggregateRateLimiter` with mapping for remoteSelector ->
localTokenAddress -> remoteTokenAddress is updated to contain
remoteTokenAddress as `bytes` instead of `bytes32`

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
## Motivation

Bump chainlink-ccip to the version from this PR:
smartcontractkit/chainlink-ccip#64

## Solution

Bump and fix the tests.
- Fix stale comment
- remove wrapper gen for the old mockRMN 
	- should no longer be used in offchain tests, use the real RMN instead
- Remove naming clash with RMN
There's already 1.4 pools out there of this type so we need a proxy
* setter of a bool representing the `allowOutOfOrderExecution`
* creating and passing `extraArgs` to the message
## Motivation
getPreviousPool was missing 

## Solution

Add getPreviousPool
- NOTE ~5k is generated protobuf code to unblock. That will be imported
once exposed.
- We put the deployment/configuration logic in integration-tests module
for a few reasons:
- Keeps the chain dependencies out of the core module, in particular
helpful for eventual cross family tests
- It can become the canonical deployment logic to be used for CRIB envs
as well (eventually can replace the actions + contracts dirs)
- To accomplish the lightweight tests (chainlink.Application +
simulated.Backend) we expose some test utilities in util/testutils/
- integration-tests/deployment holds product agnostic deployment
utilities including a general purpose environment structure to write
environment abstracted code against and migration output components
(address books, proposals etc)
- integration-tests/deployment/ccip holds all product specific
deployment code including
- Top level migrations and migration tests where a "migration" is
defined to be a function which operates against an environment and
outputs a MigrationOutput structure with one or more artifacts (MCMS
proposals, job specs). Notably migration tests can apply those outputs
to an ephemeral environment to ensure correctness. These migrations are
intended for export and use against real environments (testnet/mainnet).
- Re-usable product specific components of top level migrations and
associated tests

Next steps / follow up PRs:
- Port testutils export to chainlink repo
- Example solana setup
- Once cross family validated, start deeper testing and real CCIP use
cases

---------

Co-authored-by: Adam Hamrick <[email protected]>
Co-authored-by: AnieeG <[email protected]>
CCIP Config can go to larger size and any query from offchain components
via rpc call can cause timeout issues

add pagination to `getAllCCIPConfig` function which takes
- pageSize
- startIndex

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: Makram Kamaleddine <[email protected]>
#1310)

## Motivation
gasUsed for Execution to be emitted along with
ExecutionStateChangedEvent

## Solution
compute `gasUsed` for execution of a message in EVM2EVMMultiOffRamp
this change is applicable to only 1.6 version
Test Assertion must be added to assert the event body parameters
(excluding the gasUsed as it cant be hardcoded in tests)

** This is extension of the closed PR:
smartcontractkit/ccip#1297
got signature verification issue with other PR. so moving all changes
over here

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: Ryan <[email protected]>
## Motivation

Use the commit plugin state machine implementation.

## Solution

Use the commit plugin state machine implementation.
Cleanup & more realistic values for gas overheads
@asoliman92 asoliman92 changed the title Ccip/capabilities debug contracts fmt Contracts fmt [CCIP-2946] Aug 30, 2024
@asoliman92 asoliman92 force-pushed the ccip/capabilities-debug-contracts-fmt branch from a47f85e to 3e28ee6 Compare August 30, 2024 10:04
@asoliman92 asoliman92 changed the base branch from develop to ccip/capabilities-debug August 30, 2024 10:33
@asoliman92 asoliman92 marked this pull request as ready for review August 30, 2024 10:34
@asoliman92 asoliman92 requested review from bolekk and removed request for a team August 30, 2024 10:34
@asoliman92 asoliman92 changed the base branch from ccip/capabilities-debug to develop August 30, 2024 10:35
@asoliman92 asoliman92 changed the title Contracts fmt [CCIP-2946] Merge CCIP V1.6 [CCIP-2946] Aug 30, 2024
Copy link
Collaborator

@mateusz-sekara mateusz-sekara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this should be one commit
image

@securejavier securejavier merged commit 7eec696 into develop Aug 30, 2024
170 checks passed
@securejavier securejavier deleted the ccip/capabilities-debug-contracts-fmt branch August 30, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.