CCIP-3873: merge v2.17 into ccip-develop #1506

makramkd · 2024-10-22T10:26:04Z

Motivation

https://smartcontract-it.atlassian.net/browse/CCIP-3873

Solution

Merge v2.17

Worthy mentions of changes:

core/capabilities/ccip has been deleted in this fork to ease the merge process. Development here continues as usual in chainlink.
Due to the above, integration-tests/deployment is also deleted, since it imports a lot of stuff from core/capabilities/ccip. These are also not being developed in this fork.

Conflicts still to be resolved:

.github/workflows folder
integrationt-tests/ccip-tests folder

## 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: #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

Use new ccip tests designed with deployment in CI --------- Co-authored-by: Makram <[email protected]>

To fully rely on integration-tests/deployment/ccip

Depends on : smartcontractkit/chainlink#13878 The price registry needs to conform to the KeystoneFeedsConsumer interface in order to receive keystone price feed updates. Implemented Keystones `IReciever` interface `onReport` function to handle the following report type ``` struct ReceivedFeedReport { address Token; uint224 Price; uint32 Timestamp; } ``` and storing the reported fee in `Internal.TimestampedPackedUint224` for `s_usdPerToken` mapping --------- Signed-off-by: 0xsuryansh <[email protected]>

…mk/merge-v2.17

RensR · 2024-10-28T13:16:02Z

contracts/.gas-snapshot

@@ -0,0 +1,142 @@
+ArbitrumCrossDomainForwarder_AcceptL1Ownership:test_CallableByPendingL1Owner() (gas: 37568)


should be removed

RensR

only checked solidity. Extremely hard to review

RyanRHall and others added 30 commits August 28, 2024 20:54

use transferFrom(pool -> receiver) over hop through offRamp (#1258)

1c8d278

## Motivation Change the hop through the offRamp to a transferFrom so the funds never touch CCIP contracts outside of the pool ## Solution

core/capabilities/ccip: use OCR offchain config (#1264)

cefcf1a

We want to define and use the appropriate OCR offchain config for each plugin. Requires smartcontractkit/chainlink-ccip#36

bump version to 1.5 (#1263)

9eae858

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>

bump version to 1.5 (#1279)

de645cb

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

update pool func visibility (#1283)

bbe2795

## Motivation ## Solution

core/capabilities/ccip: bump chainlink-ccip, fix tests (#1291)

77020b8

## Motivation Bump chainlink-ccip to the version from this PR: smartcontractkit/chainlink-ccip#64 ## Solution Bump and fix the tests.

Fix stale comment (#1296)

c510f41

- 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

add BurnWithFromMintTokenPoolAndProxy (#1287)

5a94c90

There's already 1.4 pools out there of this type so we need a proxy

Adding ooo flag to the PingPong (#1239)

bcfccbc

* setter of a bool representing the `allowOutOfOrderExecution` * creating and passing `extraArgs` to the message

fix hh compile and bump publish-prod version (#1299)

5d79513

add getPreviousPool (#1312)

b866aaf

## Motivation getPreviousPool was missing ## Solution Add getPreviousPool

bump ccip version in license (#1309)

f2e4479

core/capabilities/ccip: use commit plugin w/ state machine (#1321)

e862492

## Motivation Use the commit plugin state machine implementation. ## Solution Use the commit plugin state machine implementation.

set more correct gas overheads (#1319)

e5e5e29

Cleanup & more realistic values for gas overheads

fix stale comment (#1327)

08cd77d

Rename ramps and rmn (#1323)

e7341e0

Add deployment tests in ci (#1337)

49346e4

Use new ccip tests designed with deployment in CI --------- Co-authored-by: Makram <[email protected]>

Remove old integration tests for v1.6 (#1346)

1bc98ba

To fully rely on integration-tests/deployment/ccip

mateusz-sekara force-pushed the mk/merge-v2.17 branch 2 times, most recently from 7e16199 to f01277f Compare October 24, 2024 12:53

mateusz-sekara marked this pull request as ready for review October 24, 2024 12:54

mateusz-sekara requested review from a team, elatoskinas, RayXpub, jmank88 and krehermann as code owners October 24, 2024 12:54

Post merge fixes

590bce5

mateusz-sekara force-pushed the mk/merge-v2.17 branch from e8c8a0b to 590bce5 Compare October 25, 2024 10:55

bukata-sa approved these changes Oct 25, 2024

View reviewed changes

mateusz-sekara approved these changes Oct 28, 2024

View reviewed changes

b-gopalswami added 2 commits October 28, 2024 18:14

Merge develop

ba786c2

Merge branch 'ccip-develop' of github.com:smartcontractkit/ccip into …

d4537bb

…mk/merge-v2.17

winder approved these changes Oct 28, 2024

View reviewed changes

RensR reviewed Oct 28, 2024

View reviewed changes

RensR approved these changes Oct 28, 2024

View reviewed changes

lukaszcl approved these changes Oct 28, 2024

View reviewed changes

sebawo approved these changes Oct 28, 2024

View reviewed changes

jmank88 approved these changes Oct 28, 2024

View reviewed changes

anirudhwarrier approved these changes Oct 28, 2024

View reviewed changes

amirylm approved these changes Oct 28, 2024

View reviewed changes

elatoskinas approved these changes Oct 28, 2024

View reviewed changes

mateusz-sekara merged commit d888177 into ccip-develop Oct 28, 2024
163 of 165 checks passed

mateusz-sekara deleted the mk/merge-v2.17 branch October 28, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-3873: merge v2.17 into ccip-develop #1506

CCIP-3873: merge v2.17 into ccip-develop #1506

makramkd commented Oct 22, 2024 •

edited by mateusz-sekara

Loading

RensR Oct 28, 2024

RensR left a comment

		@@ -0,0 +1,142 @@
		ArbitrumCrossDomainForwarder_AcceptL1Ownership:test_CallableByPendingL1Owner() (gas: 37568)

CCIP-3873: merge v2.17 into ccip-develop #1506

CCIP-3873: merge v2.17 into ccip-develop #1506

Conversation

makramkd commented Oct 22, 2024 • edited by mateusz-sekara Loading

Motivation

Solution

RensR Oct 28, 2024

Choose a reason for hiding this comment

RensR left a comment

Choose a reason for hiding this comment

makramkd commented Oct 22, 2024 •

edited by mateusz-sekara

Loading