Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new fee changeset to update FeeQuoter destination-chain configs with “upsert” behavior, and extends fee adapters (EVM/Solana) to support reading/applying dest-chain configs across FeeQuoter versions.
Changes:
- Introduce
UpdateFeeQuoterDestschangeset and its input models. - Extend
FeeAdapterwith dest-chain config read/default/update methods; implement/register these in EVM/Solana adapters. - Make
SetTokenTransferFeeuse global registries (via init-registered adapters) and update tests/docs accordingly.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/deployment/update_fq_dests_test.go | Adds an integration test for updating FeeQuoter dest config via the new changeset. |
| integration-tests/deployment/set_token_transfer_fee_test.go | Updates tests to rely on init-registered fee adapters and new SetTokenTransferFee() constructor. |
| deployment/fees/update_fq_dests.go | New changeset implementation for upserting FeeQuoter destination configs. |
| deployment/fees/set_token_transfer_fee.go | Changes constructor to pull registries from singletons instead of parameters. |
| deployment/fees/product.go | Extends the FeeAdapter interface with dest-chain config APIs. |
| deployment/fees/models.go | Adds models for dest-chain config update sequencing and changeset input. |
| deployment/docs/changesets.md | Updates documented signature for SetTokenTransferFee. |
| chains/solana/deployment/v1_6_0/sequences/connect_chains.go | Adds reverse translation from Solana on-chain dest config to product model. |
| chains/solana/deployment/v1_6_0/sequences/translate_fq_test.go | Adds round-trip tests for Solana translate/reverse-translate. |
| chains/solana/deployment/v1_6_0/adapters/init.go | Registers Solana fee adapter in the global fees registry via init. |
| chains/solana/deployment/v1_6_0/adapters/fees.go | Implements dest-chain config read/default/update sequence for Solana. |
| chains/evm/deployment/v1_6_0/sequences/update_lanes.go | Adds reverse translation helpers for EVM v1.6 and v2.0 dest configs. |
| chains/evm/deployment/v1_6_0/sequences/translate_fq_test.go | Adds round-trip tests for EVM translate/reverse-translate (v1 + v2). |
| chains/evm/deployment/v1_6_0/adapters/init.go | Registers EVM v1.6 fee adapter in the global fees registry via init. |
| chains/evm/deployment/v1_6_0/adapters/fees.go | Implements dest-chain config read/default/update sequence for EVM v1.6. |
| chains/evm/deployment/v2_0_0/adapters/fees.go | Implements dest-chain config read/default/update sequence for FeeQuoter v2.0. |
| chains/evm/deployment/v1_5_0/adapters/init.go | Registers EVM v1.5 fee adapter in the global fees registry via init. |
| chains/evm/deployment/v1_5_0/adapters/fees.go | Adds “not supported” stubs for new dest-chain config methods (v1.5 has no FeeQuoter). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/smartcontractkit/chainlink-deployments-framework/engine/test/environment" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| _ "github.com/smartcontractkit/chainlink-ccip/chains/evm/deployment/v1_6_0/adapters" |
There was a problem hiding this comment.
The import block includes the same package path twice (once as a blank import and once as a named import). Go does not allow importing the same path more than once in a single file, so this test will not compile. Drop the blank import (the named import already triggers init()).
| _ "github.com/smartcontractkit/chainlink-ccip/chains/evm/deployment/v1_6_0/adapters" |
| var base lanes.FeeQuoterDestChainConfig | ||
| if onchain.IsEnabled { | ||
| base = onchain | ||
| } else { | ||
| base = adapter.GetDefaultDestChainConfig(src, dst) | ||
| } |
There was a problem hiding this comment.
resolveDestChainConfig uses onchain.IsEnabled to decide whether to use on-chain values vs defaults. This will treat an existing-but-disabled dest chain config as “missing” and replace it with adapter defaults (which have IsEnabled=true), unintentionally enabling the chain and discarding the configured parameters. Consider distinguishing “not found” from “found but disabled” (e.g., have adapters return a found flag / nil config, or return a typed not-found error) and always use the on-chain config as the base when it exists, regardless of IsEnabled.
| func makeFQDestsVerify(feeRegistry *FeeAdapterRegistry) func(cldf.Environment, UpdateFeeQuoterDestsInput) error { | ||
| return func(_ cldf.Environment, cfg UpdateFeeQuoterDestsInput) error { | ||
| if cfg.Version == nil { | ||
| return fmt.Errorf("version is required") | ||
| } | ||
|
|
||
| seenSrc := utils.NewSet[uint64]() | ||
| for i, src := range cfg.Args { | ||
| if exists := seenSrc.Add(src.Selector); exists { | ||
| return fmt.Errorf("duplicate src chain selector at args[%d]: %d", i, src.Selector) | ||
| } | ||
|
|
||
| seenDst := utils.NewSet[uint64]() | ||
| for j, dst := range src.Settings { | ||
| if src.Selector == dst.Selector { | ||
| return fmt.Errorf("src and dst chain selectors cannot be the same at args[%d].settings[%d]: %d", i, j, src.Selector) | ||
| } | ||
|
|
||
| if exists := seenDst.Add(dst.Selector); exists { | ||
| return fmt.Errorf("duplicate dst chain selector at args[%d].settings[%d] (src=%d): %d", i, j, src.Selector, dst.Selector) | ||
| } | ||
| } | ||
|
|
||
| srcFamily, err := chain_selectors.GetSelectorFamily(src.Selector) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get chain family for selector %d: %w", src.Selector, err) | ||
| } | ||
|
|
||
| if _, exists := feeRegistry.GetFeeAdapter(srcFamily, cfg.Version); !exists { | ||
| return fmt.Errorf("no fee adapter found for chain family %s and version %s", srcFamily, cfg.Version.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
makeFQDestsVerify only checks that a fee adapter exists for the provided Version. Versions like EVM 1.5.0 have a fee adapter but do not support FeeQuoter dest chain config reads/updates, which will cause this changeset to fail later in Apply with a less actionable error. Add validation to reject unsupported versions up front (e.g., require >= 1.6.0, or check adapter capabilities explicitly).
| // DestChainConfigForDst represents a destination chain config override for a single destination. | ||
| // Override is a functional option that mutates the base config (read from on-chain or defaults). | ||
| // If Override is nil, the existing on-chain config is re-applied as-is (noop). | ||
| type DestChainConfigForDst struct { | ||
| Selector uint64 `json:"selector" yaml:"selector"` | ||
| Override *lanes.FeeQuoterDestChainConfigOverride `json:"-" yaml:"-"` | ||
| } |
There was a problem hiding this comment.
The comment says that when Override is nil, the existing on-chain config is re-applied as-is (noop). In practice this changeset still builds and executes update operations and may submit transactions even when no values changed. Consider clarifying the comment (e.g., “idempotent re-apply”) or implementing an explicit no-op when Override is nil and the resolved config matches on-chain.
| ### SetTokenTransferFee | ||
|
|
||
| Sets per-token transfer fee configurations on the FeeQuoter for each source-destination pair. | ||
|
|
||
| **Source:** [fees/set_token_transfer_fee.go](../fees/set_token_transfer_fee.go) | ||
|
|
||
| **Constructor:** | ||
| ```go | ||
| func SetTokenTransferFee(feeRegistry *FeeAdapterRegistry, mcmsRegistry *MCMSReaderRegistry) cldf.ChangeSetV2[SetTokenTransferFeeInput] | ||
| func SetTokenTransferFee() cldf.ChangeSetV2[SetTokenTransferFeeInput] | ||
| ``` |
There was a problem hiding this comment.
This doc updates SetTokenTransferFee’s constructor signature but doesn’t document the newly added fee changeset (UpdateFeeQuoterDests). If changesets.md is intended to enumerate available changesets, please add a corresponding section under “Fee Configuration” so users can discover and use it.
| } | ||
|
|
||
| v := fqRef.Version | ||
| lookupVersion := semver.MustParse(fmt.Sprintf("%d.%d.0", v.Major(), v.Minor())) |
There was a problem hiding this comment.
non-blocking: this can be replaced with the helper now
chainlink-ccip/deployment/utils/common.go
Lines 112 to 114 in 8d65a97
I believe set_token_transfer_fee.go would also need to be updated
| src := chainsel.TEST_90000001.Selector | ||
| dst := chainsel.TEST_90000002.Selector | ||
| chains := []uint64{src, dst} |
There was a problem hiding this comment.
non-blocking: can we add Solana in this test to improve the coverage?
| chainInput[sel] = deploy.ContractDeploymentConfigPerChain{ | ||
| Version: v1_6_0, | ||
| MaxFeeJuelsPerMsg: big.NewInt(0).Mul(big.NewInt(200), big.NewInt(1e18)), | ||
| TokenPriceStalenessThreshold: uint32(24 * 60 * 60), | ||
| LinkPremiumMultiplier: 9e17, | ||
| NativeTokenPremiumMultiplier: 1e18, | ||
| PermissionLessExecutionThresholdSeconds: uint32((20 * time.Minute).Seconds()), | ||
| GasForCallExactCheck: uint16(5000), | ||
| } |
There was a problem hiding this comment.
non-blocking:
| chainInput[sel] = deploy.ContractDeploymentConfigPerChain{ | |
| Version: v1_6_0, | |
| MaxFeeJuelsPerMsg: big.NewInt(0).Mul(big.NewInt(200), big.NewInt(1e18)), | |
| TokenPriceStalenessThreshold: uint32(24 * 60 * 60), | |
| LinkPremiumMultiplier: 9e17, | |
| NativeTokenPremiumMultiplier: 1e18, | |
| PermissionLessExecutionThresholdSeconds: uint32((20 * time.Minute).Seconds()), | |
| GasForCallExactCheck: uint16(5000), | |
| } | |
| chainInput[sel] = NewDefaultDeploymentConfigForEVM(v1_6_0) |
|
No description provided.