Merge FeePaid event into CCIPMessageSent #1452
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LCOV of commit
|
RyanRHall
force-pushed
the
CCIP-3446-merge-fee-paid-event-into-ccip-message-sent
branch
from
cafc2ca
to
c53ec7d
Compare
RyanRHall
commented
Sep 19, 2024
Comment on lines
+2817
to
+2818
| [GasEstimator.FeeHistory] | ||
| CacheTimeout = '10s' |
There was a problem hiding this comment.
this seems unrelated to my PR, but the TestConfigDocs was failing 🤔
RyanRHall
requested review from
makramkd,
elatoskinas,
RayXpub and
a team
as code owners
RensR
reviewed
Sep 19, 2024
RensR
reviewed
Sep 19, 2024
RyanRHall
force-pushed
the
CCIP-3446-merge-fee-paid-event-into-ccip-message-sent
branch
from
c53ec7d
to
1fe7002
Compare
|
elatoskinas
approved these changes
Sep 20, 2024
|
Closing in favor of #1456 |
RyanRHall
deleted the
CCIP-3446-merge-fee-paid-event-into-ccip-message-sent
branch
RyanRHall
added a commit
that referenced
this pull request
Sep 23, 2024
## Motivation Not sure actually why this was needed. Request was [made here](https://smartcontract-it.atlassian.net/browse/CCIP-3446?atlOrigin=eyJpIjoiMTQ4ODMwNzU2N2I3NDI0NGE0ZDhiOTIyZTYwOTNjN2IiLCJwIjoiaiJ9). This is the 2nd version, that alters the MessageSent struct. First version [here](#1452). ## Solution Merge fields from FeePaid event into CCIPMessageSent event
RensR
pushed a commit
to smartcontractkit/chainlink
that referenced
this pull request
Oct 1, 2024
## Motivation Not sure actually why this was needed. Request was [made here](https://smartcontract-it.atlassian.net/browse/CCIP-3446?atlOrigin=eyJpIjoiMTQ4ODMwNzU2N2I3NDI0NGE0ZDhiOTIyZTYwOTNjN2IiLCJwIjoiaiJ9). This is the 2nd version, that alters the MessageSent struct. First version [here](smartcontractkit/ccip#1452). ## Solution Merge fields from FeePaid event into CCIPMessageSent event
github-merge-queue bot
pushed a commit
to smartcontractkit/chainlink
that referenced
this pull request
Oct 9, 2024
* Update .gitignore * assert fees can be set to zero (#1380) Assert we would have the option to set any of the fee components to zero if we would want. --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * pnpm i && snapshot && forge fmt * Misc golfs and fixes (#1402) Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * Increase max signers (#1405) Improve tests and error checks --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * rename CCIPSendRequested to CCIPMessageSent (#1409) * Skip report execution on curse (#1408) The goal of this PR is to remove reverts on lane cursing for DON execution transactions. If a lane is cursed, reverting will cause the whole execution transaction to fail so any execution reports for other lanes will be blocked. For DON execution transactions the behavior is now to skip the report and emit a `SkippedReportExecution`. For manual execution we still revert, otherwise the transaction will silently fail for the users. --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * remove bootstrapP2PIds (#1388) ## Motivation Bootstrap P2P IDs are no longer needed in the OCR config in CCIPConfig. See #1348's description for more details. ## Solution Remove bootstrap P2P IDs from the OCR config in CCIPConfig. ## Related PRs smartcontractkit/chainlink-ccip#89 * Integrate RMNRemote and OffRamp (#1360) Remove the requirement for self-transmitted RMN blessings Allow the commit DON to include RMN blessings at commitment time This PR has a dependency on changes to [chainlink-ccip](smartcontractkit/chainlink-ccip#84) --------- Co-authored-by: Makram <[email protected]> * Add report vs message source chain selector check (#1413) The goal of this PR is to add an extra check in the execute function to enforce that the report source chain selector matches its messages source chain selector. Extra gas cost is about 50 gas per message. Revert when `report.sourceChainSelector != (message.header.sourceChainSelector` * Enforce signature verification for commit plugin in `OffRamp` (#1416) ## Motivation The goal of this PR is to implement a check to ensure that the OCR3 signature verification is enabled for the commit plugin on config. This reduces misconfig risks and impact because previously an accidental setting of `isSignatureVerificationEnabled=false` for the commit plugin in the `OffRamp` would have required a redeploy to be fixed. ## Solution Add a check enforcing `isSignatureVerificationEnabled=false` for the commit plugin in `_afterOC3Config`. * Revert "reorder fields in MerkleRoot struct" (#1417) ## Motivation Avoid forcing breaking changes on RMN nodes ## Solution This reverts commit da570c5. * Implement getTotalChainConfigurations (#1419) ## Motivation The goal of this PR is to add a getter function to the `CCIPConfig` contract to fetch the total number of chains configured. This value is necessary to correctly call `getAllChainConfigs` and previously, the only way to access it was through events. ## Solution Implement `getTotalChainConfigurations`. * Add sanity check in applyAllowListUpdates (#1420) ## Motivation Currently, in the event of a misconfig where a destination chain's `AllowListConfigArgs` `allowListEnabled` is set to false while providing addresses to add to the allow list, the config will silently be skipped. ## Solution Add a sanity check to detect `!allowListConfigArgs.allowListEnabled && `allowListConfigArgs.addedAllowlistedSenders.length > 0` * Miscellaneous fixes and cleanups (#1421) ## Motivation The goal of this PR is to fix several miscellaneous items. ## Solution - Replaces some magic numbers with constants - Default to `private` visibility for state variables that are not used in test helpers - Standardizes some natspec tags (consistent use of `@dev` instead of `@notice` for state variables) - Add a `ConfigSet` event in `CCIPConfig` - Asserts full equality of `OCRConfigWithMeta[]` in `CCIPConfigTests` - Add clarifying comments in `NonceManager` `applyPreviousRampsUpdates` * CCIP-2815 commit report event updates (#1407) ## Motivation OffRamp contract emits commitReport event as a Struct to make the event Log filtering and searching more optimized, the report properties are emitted as a event fields rather than emitting the struct --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * GasLimitOverride for manuallyExecute (#1375) ## Motivation 1. OffRamp should now use destExecData to extract the gas used for 2. Manual execution should enable user to override the gas amount for releaseOrMint per token. ## Solution 1. Removed int32 `maxTokenTransferGas` & `maxPoolReleaseOrMintGas` and used the gas encoded in `RampTokenAmount.destExecData` 2. add a new Struct which holds the receiverExecutionGasLimit and transferGasAmounts ``` 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 Note: The gas limit can not be lowered as that could cause the message to fail. If manual execution is done from an UNTOUCHED state and we would allow lower gas limit, anyone could grief by executing the message with lower gas limit than the DON would have used. This results in the message being marked FAILURE and the DON would not attempt it with the correct gas limit. for the above we have kept a check that `tokenGasOverrides` < gas encoded in `RampTokenAmount.destExecData` --------- 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> * Change data feeds from default to fallback in `FeeQuoter` (#1430) ## Motivation Due to the gas cost of reading from data feeds, it is unlikely to be the default option, we can therefore optimize the `getTokenPrice()` function by defaulting to state instead of data feeds. ## Solution Default to state reported prices and only fall back on data feed external call if the reported prices are stale. * Zp/update mockrouter (#1427) Specifically, allow for empty message with zero gas. ## Motivation EVM2EVMOffRamp checks the message for content and gaslimit and skips calling ccip receive if they're not present. Consequently the mock errors (ReceiverError) when sending only tokens to a smart contract. This update will help chainlink-local correctly process fork-based tests too. ## Solution The MockRouter implementation of _routeMessage was last updated in smartcontractkit/ccip#669 and is out of sync with the logic in EVM2EVMOffRamp. * Hybrid Token Pools: Remove problematic liquidity functions and add incoming supply lock (#1396) ## Motivation Minor Changes to the HybridLockReleaseTokenPool to better meet Circle's bridge requirements 1. Add additional supply lock on incoming messages when a proposal has been created. I.E When a proposal for a chain selector is active, new tokens cannot be released or locked to ensure better security control on the destination-chain's token supply. 2. `withdrawLiquidity()` and `transferLiqudity()` have been removed to avoid issues with the supply lock. Allowing for withdrawing liquidity that has been provided would allow for the supply on the destination chain to be greater than the locked amount on the source chain, preventing a successful migration from occuring. 3. Comment fixes for invariants * Make onRamp mutable in OffRamp (#1422) ## Motivation The goal of this PR is to reduce the misconfig risk on the `OffRamp` with the `onRamp` address. Currently, if we accidentally configure the wrong `onRamp` address on an existing `OffRamp`, the fix would involve a full redeployment + reconfig. ## Solution - Make the `onRamp` mutable in the `OffRamp`. - Add `onRamp` address verification in the `commit()` function - Only allow `onRamp` address modification when `minSeqNr == 1` --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * update offramp layout (#1434) ## Motivation Some header are outdated. The exec logic is above the commit logic, but then after the commit logic we have the token handling logic, which is only used for exec. It has been moved to be above the commit logic * add CCIPEncodingUtils contract and gethwrapper (#1429) ## Motivation I encountered a scenario w/ RMN where offchain needs to abi.encode a [struct](https://github.com/smartcontractkit/ccip/blob/fc50683640bcfd73f0074d791fbaf7cdb6b29c71/contracts/src/v0.8/ccip/rmn/RMNRemote.sol#L90:L97) that is not included in any public contract functions (as input param or return value) and so it is not a part of the ABI or the go wrapper. Offchain is currently doing a manual encoding by re-defining the type, which I usually try to avoid at all costs. In the past, we've used a Utils contract ([ex here](https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/automation/AutomationCompatibleUtils.sol) from automation) to expose all private structs in public functions so that offchain components can use E2E type safety when encoding / decoding. ## Solution Create a `CCIPEncodingUtils` contract and accompanying gethwrapper * Remove TODO and rename messageValidator to messageInterceptor (#1438) - Replaces `CCIPConfigTypes.ConfigState` `/// TODO: explain rollbacks?` by rollbacks comment - Renames `messageValidator` to `messageInterceptor` in ramps --------- Co-authored-by: Rens Rooimans <[email protected]> * update diagram (#1439) * Make forwardFromRouter non reentrant (#1437) ## Motivation Currently the `OnRamp` performs 3 separate calls to the `FeeQuoter` on the hot path, including 2 in the `forwardFromRouter` function: 1. `getValidatedFee` 2. `processMessageArgs` 3. `processPoolReturnData` This is done to follow the CEI pattern and guard against reentrancy due to the untrusted calls to the pools. This is not optimal for gas but it also makes contract interactions more complex. ## Solution Implement a non reentrant flag and reorder the `forwardFromRouter` function layout. The calls to the pools are now performed first, relying on the reentrancy protection. We reduce the `FeeQuoter` calls in `forwardFromRouter` to a single `processMessageArgs` call. ## Notes Optimizer runs had to be decreased due to contract size which is why gas diffs are showing a gas increase. At equivalent runs diffs are: ``` test_ForwardFromRouterExtraArgsV2_Success() (gas: -65 (-0.045%)) test_ForwardFromRouterSuccessLegacyExtraArgs() (gas: -65 (-0.045%)) test_ForwardFromRouterSuccessCustomExtraArgs() (gas: -65 (-0.045%)) test_ForwardFromRouter_Success() (gas: -65 (-0.045%)) test_ForwardFromRouterSuccessEmptyExtraArgs() (gas: -66 (-0.046%)) test_ForwardFromRouterExtraArgsV2AllowOutOfOrderTrue_Success() (gas: -65 (-0.057%)) test_forwardFromRouter_WithValidation_Success() (gas: 318 (0.113%)) test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: -1139 (-0.074%)) ``` * Standardize message hashing functions (#1424) ## Motivation The goal of this PR is to standardize the message hashing functions. Currently we have equivalent but inconsistent message hashing functions: ```solidity function _hash(Any2EVMRampMessage memory original, bytes memory onRamp) internal pure returns (bytes32) ``` and ```solidity function _hash(EVM2AnyRampMessage memory original, bytes32 metadataHash) internal pure returns (bytes32) ``` ## Solution Standardize to `function _hash(message, metadataHash)` and consistently encode dynamic fields. --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * Validate transmitter lengths against F values (#1432) ## Motivation Improves validations for OCR3 and CCIPConfig contracts ## Solution Covers validations: * `fChain <= F`, since `F` represents the full role DON * Allows setting empty transmitters in CCIPConfig to allow `len(transmitters) <= len(signers)`, while still enforcing `len(transmitters) >= 3 * fChain + 1` * Validates non-zero transmitters length in MultiOCR3 * Validates `len(signers) >= len(transmitters)` in MultiOCR3 ### Tooling implications * When setting OCR configs on OffRamp contracts, the list from CCIPConfig must be filtered to exclude 0-bytes addresses. The reason for this is that the MultiOCR3Base disallows zero addresses and duplicates * Signer and transmitter duplication must be checked in tooling scripts off-chain prior to setting CCIPConfig (otherwise it will result in OffRamp breakage) * `chainConfigs` must be set before OCR3 configs due to the added `fChain == F` validation * If `fChain` becomes higher than `F` after updates, existing OCR3 configs get invalidated without an on-chain check (since it is difficult to do). There should be an off-chain validation script that prevents this (first, F should be increased for all configs, only after which fChain can be increased) * `OffRamp` transmitters.length should be validated such that it meets the `3 * fChain + 1` criteria off-chain, since `fChain` is not tracked in the `OffRamp` ### Gas | Function Name | min | avg | median | max | # calls | |---------------------------------------------------------------------------|-----------------|--------|--------|---------|---------| | validateConfig (without loop check) | 7040 | 20701 | 8436 | 122591 | 11 | | validateConfig (with loop check) | 7040 | 21006 | 9132 | 123442 | 11 | * Split RampTokenAmount into EVM2AnyTokenTransfer and Any2EVMTokenTransfer (#1435) * Productionize RMN remote contract (#1431) [Original PR](smartcontractkit/ccip#1308) - there is some discussion here that is possibly still relevant) [Open PR to add cursing](smartcontractkit/ccip#1400) - I copied the relevant parts for the `RMNRemote`. Note: we are intentionally separating `RMNRemote` and `RMNHome` since they have different timelines. Productionize the RMNRemote and include it in integration tests --------- Co-authored-by: Makram <[email protected]> * Add RMN_V1_6_ANY2EVM_REPORT to RMNRemote and make public (#1447) ## Motivation smartcontractkit/ccip#1431 (comment) ## Solution Expose the constant as public * merge FeePaid event into CCIPMessageSent (#1456) ## Motivation Not sure actually why this was needed. Request was [made here](https://smartcontract-it.atlassian.net/browse/CCIP-3446?atlOrigin=eyJpIjoiMTQ4ODMwNzU2N2I3NDI0NGE0ZDhiOTIyZTYwOTNjN2IiLCJwIjoiaiJ9). This is the 2nd version, that alters the MessageSent struct. First version [here](smartcontractkit/ccip#1452). ## Solution Merge fields from FeePaid event into CCIPMessageSent event * Add RMNHomeAddress in the OCR3Config (#1449) Adding `rmnHomeAddress` in the `OCR3Config` struct + generated wrappers. * Update pragma to most restrictive dependency or feature (#1466) ## Motivation We have several contracts using `^0.8.0` but also custom errors, or a child contract with custom errors. We also have several contracts at `^0.8.0` but a dependency locked at `0.8.24`. ## Solution Update all pragmas to the pragma of the most restrictive dep or the minimum to enable custom errors (`0.8.4`) * RMNHome (#1469) New PR to have this be standalone * ccip home (#1473) Fresh PR * fix snapshots * Update gethwrappers * [WIP] fix basic offchain logic (#14613) * fix basic offchain logic * type fixes * bump chainlink-common, general fixes * fix message hashing * small fixes * Update chainlink-common * Fix commit-codec test. * Fix error messages. * Fix ccip reader tests (#14619) * Fix exec codec. * Launcher integration tests (#14648) * CCIPHome basic usage framework * Fix homeChain usage * Fix calls in launcher test * Add encoded configs * Add more asserts * Calling right function 😢 * new assertions and logging error * Add don only once and update for all next operations * Fix typo --------- Co-authored-by: Makram Kamaleddine <[email protected]> * core/capabilities/ccip: fix usdc reader test (#14652) * fix lint * Rename bluegreen to deployment and use active candidate naming (#14662) * Fix smoke tests (#14655) * Initial commit - fix AddDon in deployment tests * Fix types and add logs * more explicit config setting * Rename bluegreen to deployment and use active candidate naming (#14662) * contracts/ccip/capability: per (donId, pluginType) active config indexes (#1488) * Add CCIPHome fixes * Commit Encoding with new structs * Update gethwrappers * Commit Encode/Decode with new structure * fix encoding, bump cl-ccip * Update gethwrappers * logging updates * add rmn home and remote configuration * fixes * working int test, still some failing tests * bump chainlink-ccip * some more fixes * lint, build * more lint --------- Co-authored-by: Makram Kamaleddine <[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: Makram Kamaleddine <[email protected]> Co-authored-by: Will Winder <[email protected]> Co-authored-by: Abdelrahman Soliman (Boda) <[email protected]> Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> * tidy * lint again * remove stale files * bump chainlink-ccip again * bump with cl-ccip from main * fix build * fix --------- 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: Suryansh <[email protected]> Co-authored-by: Ryan <[email protected]> Co-authored-by: Makram <[email protected]> Co-authored-by: Ryan Hall <[email protected]> Co-authored-by: kanth <[email protected]> Co-authored-by: Zubin <[email protected]> Co-authored-by: Josh Weintraub <[email protected]> Co-authored-by: Evaldas Latoškinas <[email protected]> Co-authored-by: nogo <[email protected]> Co-authored-by: Will Winder <[email protected]> Co-authored-by: Abdelrahman Soliman (Boda) <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Not sure actually why this was needed. Request was made here.
Solution
Merge fields from
FeePaidevent intoCCIPMessageSentevent