Conversation
|
tt-cll
requested review from
0xAustinWang,
KodeyThomas,
agusaldasoro,
asoliman92,
chris-de-leon-cll,
mateusz-sekara and
winder
There was a problem hiding this comment.
Pull request overview
This PR moves FeeQuoter destination-chain default configuration (and default destination gas price) out of user-supplied ChainDefinition literals and into chain-family adapters, with an optional override hook for per-lane tweaks.
Changes:
- Extend
LaneAdapterwithGetFeeQuoterDestChainConfig()andGetDefaultGasPrice()and populate these programmatically duringConnectChains. - Replace
ChainDefinition.FeeQuoterDestChainConfiginput withFeeQuoterDestChainConfigOverrides(functional option) and populateFeeQuoterDestChainConfiginternally. - Update integration tests / changeset tests and devenv helpers to rely on adapter defaults and optional overrides.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/deployment/update_to_FeeQuoter_2_0_test.go | Stops manually setting FeeQuoter dest-chain config in test chain definitions. |
| integration-tests/deployment/lane_migrator_test.go | Same as above for lane migrator test setup. |
| integration-tests/deployment/connect_chains_test.go | Adds override helper and updates assertions to use adapter-derived defaults + overrides. |
| devenv/common/implcommon.go | Removes manual default GasPrice/FQ config setup so ConnectChains can populate defaults. |
| deployment/utils/common.go | Factors hex decoding into GetHexFromString helper used by adapters. |
| deployment/lanes/product.go | Extends LaneAdapter interface with default config/gas price methods. |
| deployment/lanes/lane_update.go | Updates ChainDefinition to accept overrides and stores populated FeeQuoter dest config. |
| deployment/lanes/connect_chains.go | Populates defaults via adapter + applies overrides during address population. |
| chains/solana/deployment/v1_6_0/sequences/adapter.go | Implements new adapter default config/gas price methods for Solana. |
| chains/evm/deployment/v1_6_0/sequences/adapter.go | Implements new adapter default config/gas price methods for EVM. |
| chains/evm/deployment/v1_6_0/changesets/connect_chains_test.go | Updates expected config assertions to use adapter defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -155,7 +164,11 @@ func checkBidirectionalLaneConnectivity( | |||
|
|
|||
| feeQuoterDestConfig, err := feeQuoterOnDest.GetDestChainConfig(nil, solanaChain.Selector) | |||
| require.NoError(t, err, "must get dest chain config from feeQuoter") | |||
| expectedConfig := convertOpsConfigToGobinding(evmsequences.TranslateFQ(solanaChain.FeeQuoterDestChainConfig)) | |||
| sa := solanasequences.SolanaAdapter{} | |||
| safq := sa.GetFeeQuoterDestChainConfig() | |||
| override := getFQOverrides() | |||
| override(&safq) | |||
| expectedConfig := convertOpsConfigToGobinding(evmsequences.TranslateFQ(safq)) | |||
There was a problem hiding this comment.
These assertions instantiate concrete adapter structs just to access the default FeeQuoterDestChainConfig. Since you already have solanaAdapter/evmAdapter as a lanesapi.LaneAdapter (and the interface now exposes GetFeeQuoterDestChainConfig), prefer calling the method on the passed-in adapter instead. This avoids depending on concrete types and keeps the expected config consistent with the registry-provided adapter implementation.
| feeQuoterDestConfig, err := feeQuoterOnSrc.GetDestChainConfig(nil, lane.Dest.Selector) | ||
| require.NoError(t, err, "must get dest chain config from feeQuoter") | ||
| expectedConfig := convertOpsConfigToGobinding(sequences.TranslateFQ(lane.Dest.FeeQuoterDestChainConfig)) | ||
| a := &sequences.EVMAdapter{} | ||
| expectedConfig := convertOpsConfigToGobinding(sequences.TranslateFQ(a.GetFeeQuoterDestChainConfig())) | ||
| require.Equal(t, expectedConfig, feeQuoterDestConfig, "feeQuoter dest chain config must equal expected") |
There was a problem hiding this comment.
Test constructs a new sequences.EVMAdapter to get the default FeeQuoterDestChainConfig. Since srcAdapter/destAdapter are already lanesapi.LaneAdapter and now expose GetFeeQuoterDestChainConfig(), prefer using the adapter under test (e.g., lane.Dest adapter) to avoid drift if defaults ever become version/state dependent.
| func GetHexFromString(hexstr string) [4]byte { | ||
| b, _ := hex.DecodeString(hexstr) | ||
| var out [4]byte | ||
| copy(out[:], b) | ||
| return out |
There was a problem hiding this comment.
GetHexFromString ignores the DecodeString error and does not validate the decoded length. Because this helper is now exported and takes arbitrary input, invalid/odd-length strings will silently produce a zero-padded [4]byte, which can hide misconfiguration. Consider validating that decoding succeeds and that the decoded byte slice is exactly 4 bytes (panic or return an error).
| // FeeQuoterDestChainConfigOverrides is a functional option that mutates a | ||
| // FeeQuoterDestChainConfig in place. Pass one or more overrides to selectively change default values. | ||
| FeeQuoterDestChainConfigOverrides *FeeQuoterDestChainConfigOverride |
There was a problem hiding this comment.
The FeeQuoterDestChainConfigOverrides doc comment suggests "one or more overrides", but the field type only allows a single override function (and it’s stored as a pointer). Either adjust the comment to reflect the actual API (single override) or change the type to support multiple overrides (e.g., slice / variadic application) if that’s the intent.
| // FeeQuoterDestChainConfigOverride is a functional option that mutates a | ||
| // FeeQuoterDestChainConfig in place. Pass one or more overrides to | ||
| // DefaultFeeQuoterDestChainConfig to selectively change default values. | ||
| type FeeQuoterDestChainConfigOverride func(*FeeQuoterDestChainConfig) |
There was a problem hiding this comment.
This comment references DefaultFeeQuoterDestChainConfig, but that function no longer exists in this package after the refactor. Please update the comment to describe where defaults come from now (LaneAdapter.GetFeeQuoterDestChainConfig + optional overrides).
| // to allow for type serialization, we apply fee quoter dest chain config overrides | ||
| // at the lane level and then set them to nil before passing to the adapters | ||
| chainA.FeeQuoterDestChainConfigOverrides = nil | ||
| chainB.FeeQuoterDestChainConfigOverrides = nil |
There was a problem hiding this comment.
In this loop, chainA/chainB are pointers to fields of the range variable lane (&lane.ChainA / &lane.ChainB). Those pointers are then embedded in UpdateLanesInput and may escape via ExecutionReports; if they’re retained beyond the iteration, subsequent loop iterations will overwrite the underlying lane storage and can corrupt report/input data. Prefer iterating by index (so you can take pointers to the slice elements) or copying ChainDefinitions into per-iteration variables before taking addresses.
No description provided.