Conversation
tt-cll
requested review from
AmrMohamedRezk,
agusaldasoro,
asoliman92,
b-gopalswami,
makramkd,
ogtownsend and
winder
There was a problem hiding this comment.
Pull request overview
Updates lane-connection logic and tests to support EVM allowlists (outbound sender restriction) and make FeeQuoter price seeding idempotent during re-applies.
Changes:
- Adjust allowlist semantics (outbound from source chain) across lane config docs, EVM/Solana sequences, and connectivity assertions.
- Add idempotent behavior for FeeQuoter gas/token price seeding (1.6 and 2.0), plus idempotent disconnect offRamp removal.
- Refactor and expand integration tests to cover lifecycle scenarios (connect → reconnect/idempotency → disconnect).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/deployment/update_to_FeeQuoter_2_0_test.go | Documents why the test cannot run in parallel due to shared mutable singleton state. |
| integration-tests/deployment/lane_migrator_test.go | Enables parallel execution for the lane migrator integration test. |
| integration-tests/deployment/connect_chains_test.go | Refactors EVM↔Solana setup, tightens router expectation, and adds EVM↔EVM lifecycle test covering allowlists + price idempotency. |
| deployment/lanes/lane_update.go | Updates allowlist field comments to reflect outbound-sender restriction semantics. |
| deployment/docs/types.md | Updates ChainDefinition docs for allowlist outbound semantics. |
| chains/solana/deployment/v1_6_0/sequences/connect_chains.go | Stops applying allowlist on the destination leg (Solana), consistent with outbound semantics. |
| chains/evm/deployment/v1_6_0/sequences/update_lanes.go | Implements allowlist application on OnRamp and FeeQuoter price seeding idempotency (v1.6 + v2) and idempotent offRamp removal. |
| chains/evm/deployment/v1_6_0/operations/onramp/onramp.go | Adds OnRamp ops for allowlist updates and reading configured allowed senders. |
| chains/evm/deployment/v1_6_0/operations/fee_quoter/fee_quoter.go | Adds FeeQuoter read ops for token price and destination gas price (timestamped). |
| chains/evm/deployment/v1_6_0/changesets/connect_chains_test.go | Removes an older connect-chains changeset test (coverage shifts to integration tests). |
| chains/evm/deployment/operations_gen_config.yaml | Updates ops generator config to include new OnRamp/FeeQuoter functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // applyOnRampAllowlist writes the full allowlist from the input to the OnRamp. | ||
| // The input is treated as the complete desired state. | ||
| func applyOnRampAllowlist(b operations.Bundle, chain cldf_evm.Chain, input lanes.UpdateLanesInput, result *sequences.OnChainOutput) error { | ||
| if len(input.Source.AllowList) > 0 && !input.Source.AllowListEnabled { | ||
| return errors.New("OnRamp allowlist: cannot specify AllowList addresses when AllowListEnabled is false") | ||
| } | ||
| desired := make([]common.Address, 0, len(input.Source.AllowList)) | ||
| for _, h := range input.Source.AllowList { | ||
| if !common.IsHexAddress(h) { | ||
| return fmt.Errorf("allowlist address %q is not a valid hex address", h) | ||
| } | ||
| desired = append(desired, common.HexToAddress(h)) | ||
| } | ||
| writeRep, err := operations.ExecuteOperation(b, onrampops.ApplyAllowlistUpdates, chain, contract.FunctionInput[[]onrampops.AllowlistConfigArgs]{ | ||
| ChainSelector: input.Source.Selector, | ||
| Address: common.BytesToAddress(input.Source.OnRamp), | ||
| Args: []onrampops.AllowlistConfigArgs{{ | ||
| DestChainSelector: input.Dest.Selector, | ||
| AllowlistEnabled: input.Source.AllowListEnabled, | ||
| AddedAllowlistedSenders: desired, | ||
| }}, | ||
| }) |
There was a problem hiding this comment.
applyOnRampAllowlist claims the input is the complete desired state, but it only ever adds addresses (AddedAllowlistedSenders) and never removes ones that were previously configured. This means re-applying with a smaller allowlist (or with AllowListEnabled=true and an empty list) will keep stale allowed senders on-chain. Fix by reading the current allowlist (via GetAllowedSendersList), computing Added/Removed deltas, and populating RemovedAllowlistedSenders accordingly; alternatively, update the function comment/behavior so it doesn't promise full-state reconciliation.
| if input.Dest.GasPrice != nil || len(input.Source.TokenPrices) > 0 { | ||
| skip, err := feeQuoterPricesAlreadySeeded(b, evmChain, fqAddr, input.Source.Selector, input.Dest.Selector) | ||
| if err != nil { | ||
| return result, err | ||
| } | ||
| if skip { | ||
| b.Logger.Info("Skipping FeeQuoter price updates: prices already seeded for dest chain") | ||
| } else { | ||
| result, err = sequences.RunAndMergeSequence(b, chains, FeeQuoterUpdatePricesSequence, FeeQuoterUpdatePricesSequenceInput{ | ||
| Address: fqAddr, | ||
| ChainSelector: input.Source.Selector, | ||
| UpdatesByChain: fqops.PriceUpdates{ | ||
| GasPriceUpdates: translateGasPrice(input.Dest.Selector, input.Dest.GasPrice), | ||
| TokenPriceUpdates: TranslateTokenPrices(input.Source.TokenPrices), | ||
| }, | ||
| }, | ||
| }, | ||
| }, result) | ||
| if err != nil { | ||
| return result, err | ||
| }, result) | ||
| if err != nil { | ||
| return result, err | ||
| } | ||
| b.Logger.Info("FeeQuoter prices seeded") | ||
| } | ||
| } |
There was a problem hiding this comment.
The idempotency gate is based only on the destination gas price timestamp, but it controls both gas price and token price updates. If gas price is already seeded while some token prices are not (or new tokens are introduced later), this will skip seeding token prices entirely. Consider splitting the logic: gate gas price seeding on GetDestinationChainGasPrice, and gate token price seeding per-token using GetTokenPrice (which this PR adds) so missing token entries can still be seeded without overwriting existing ones.
| // feeQuoterPricesAlreadySeeded checks whether a gas price entry already exists | ||
| // for the dest chain. If so, prices have been seeded and should not be overwritten. | ||
| func feeQuoterPricesAlreadySeeded( | ||
| b operations.Bundle, | ||
| chain cldf_evm.Chain, | ||
| feeQuoter common.Address, | ||
| sourceChainSelector uint64, | ||
| destChainSelector uint64, | ||
| ) (bool, error) { | ||
| rep, err := operations.ExecuteOperation(b, fqops.GetDestinationChainGasPrice, chain, contract.FunctionInput[uint64]{ | ||
| ChainSelector: sourceChainSelector, | ||
| Address: feeQuoter, | ||
| Args: destChainSelector, | ||
| }) | ||
| if err != nil { | ||
| return false, fmt.Errorf("read destination gas price from fee quoter: %w", err) | ||
| } | ||
| return rep.Output.Timestamp > 0, nil | ||
| } |
There was a problem hiding this comment.
The idempotency gate is based only on the destination gas price timestamp, but it controls both gas price and token price updates. If gas price is already seeded while some token prices are not (or new tokens are introduced later), this will skip seeding token prices entirely. Consider splitting the logic: gate gas price seeding on GetDestinationChainGasPrice, and gate token price seeding per-token using GetTokenPrice (which this PR adds) so missing token entries can still be seeded without overwriting existing ones.
| if input.Dest.GasPrice != nil || len(input.Source.TokenPrices) > 0 { | ||
| skip, err := feeQuoterV2PricesAlreadySeeded(b, evmChain, fqAddr, input.Source.Selector, input.Dest.Selector) | ||
| if err != nil { | ||
| return result, err | ||
| } | ||
| if skip { | ||
| b.Logger.Info("Skipping FeeQuoter v2 price updates: prices already seeded for dest chain") | ||
| } else { | ||
| priceReport, err := operations.ExecuteOperation(b, fqops2.UpdatePrices, evmChain, contract.FunctionInput[fqops2.PriceUpdates]{ | ||
| ChainSelector: evmChain.Selector, | ||
| Address: fqAddr, | ||
| Args: fqops2.PriceUpdates{ | ||
| TokenPriceUpdates: TranslateTokenPricesV2(input.Source.TokenPrices), | ||
| GasPriceUpdates: []fqops2.GasPriceUpdate{ | ||
| { | ||
| DestChainSelector: input.Dest.Selector, | ||
| UsdPerUnitGas: input.Dest.GasPrice, | ||
| }, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This introduces new idempotent price-seeding behavior for the FeeQuoter v2 path (skip when already seeded). The added integration test covers idempotency for the 1.6 FeeQuoter path, but this v2-specific branch should also be exercised with a test that upgrades to 2.0 and re-applies ConnectChains while attempting to change gas/token prices, asserting the on-chain values are not overwritten.
| func applyOnRampAllowlist(b operations.Bundle, chain cldf_evm.Chain, input lanes.UpdateLanesInput, result *sequences.OnChainOutput) error { | ||
| if len(input.Source.AllowList) > 0 && !input.Source.AllowListEnabled { | ||
| return errors.New("OnRamp allowlist: cannot specify AllowList addresses when AllowListEnabled is false") | ||
| } |
There was a problem hiding this comment.
applyOnRampAllowlist is invoked on every lane configure and will proceed to submit an applyAllowlistUpdates write even when AllowListEnabled is false and the list is empty (i.e., no-op intent). Consider short-circuiting to avoid unnecessary transactions/batch ops when neither the enabled flag nor the configured addresses need to change (ideally by first reading current config with GetAllowedSendersList and comparing).
|
No description provided.