Conversation
carte7000
requested review from
0xAustinWang,
asoliman92,
b-gopalswami,
chris-de-leon-cll,
Copilot,
mateusz-sekara,
ogtownsend and
winder
There was a problem hiding this comment.
Pull request overview
This PR centralizes fee-aggregator configuration for v1.7.0 deployments by moving it out of per-committee/per-component config and into the environment topology, then plumbing the resolved value through the deployment changeset and EVM adapter.
Changes:
- Add
fee_aggregators(chainSelector → address) toEnvironmentTopology, removeFeeAggregatorfromChainCommitteeConfig, and expose aGetFeeAggregatorhelper. - Update
DeployChainContractsto resolve a single per-chain fee aggregator (topology-preferred, importer-fallback) and pass it viaDeployChainContractsInput. - Refactor EVM deploy adapter/importer to consume the per-chain fee aggregator and apply it to OnRamp / committee verifiers / executors; update and add tests accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| deployment/v1_7_0/offchain/topology.go | Introduces topology-level fee_aggregators and validation/helper access. |
| deployment/v1_7_0/changesets/deploy_chain_contracts.go | Enforces/derives per-chain fee aggregator and passes it into deploy input. |
| deployment/v1_7_0/changesets/deploy_chain_contracts_test.go | Updates test topology builders and adds validation/apply coverage around fee_aggregators. |
| deployment/v1_7_0/adapters/deploy_chain_contracts.go | Moves fee aggregator into top-level deploy input/contract params (import-output), removes per-component fields. |
| deployment/v1_7_0/adapters/deploy_chain_contracts_test.go | Adjusts merge tests for the new FeeAggregator placement/behavior. |
| ccv/chains/evm/deployment/v1_7_0/adapters/deploy_chain_contracts_adapter.go | Makes EVM adapter parse required per-chain FeeAggregator and inject it into downstream params. |
| ccv/chains/evm/deployment/v1_7_0/adapters/deploy_chain_contracts_adapter_test.go | Updates importer/merge assertions to validate the new FeeAggregator behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, topOk := cfg.Cfg.Topology.GetFeeAggregator(sel); !topOk { | ||
| if cfg.Cfg.IgnoreImportedConfigFromPreviousVersion { | ||
| return fmt.Errorf("topology fee_aggregators entry is required for chain %d when IgnoreImportedConfigFromPreviousVersion is true", sel) | ||
| } | ||
| shouldImport, impErr := shouldImportConfigFromPreviousVersion(e, registry, sel) | ||
| if impErr != nil { | ||
| return fmt.Errorf("chain %d: %w", sel, impErr) | ||
| } | ||
| if !shouldImport { | ||
| return fmt.Errorf("topology fee_aggregators entry is required for chain %d when no v1.6.0 config import applies", sel) | ||
| } | ||
| } |
There was a problem hiding this comment.
Preconditions currently treat a missing topology fee_aggregators entry as acceptable whenever shouldImportConfigFromPreviousVersion returns true, but that function only checks lane versions and does not guarantee the config importer will actually yield a FeeAggregator (e.g., OnRamp v1.6.0 metadata might be missing for the chain). This can allow VerifyPreconditions to pass and then fail later during Apply with a generic "FeeAggregator is required" error. Consider tightening validation when topOk is false by preflighting the importer (or at least verifying the previous-version metadata/address refs exist) so a missing FeeAggregator is rejected deterministically in VerifyPreconditions with a targeted message.
| IndexerAddress []string `toml:"indexer_address"` | ||
| PyroscopeURL string `toml:"pyroscope_url"` | ||
| FeeAggregators map[string]string `toml:"fee_aggregators,omitempty"` | ||
| Monitoring MonitoringConfig `toml:"monitoring"` | ||
| NOPTopology *NOPTopology `toml:"nop_topology"` | ||
| ExecutorPools map[string]ExecutorPoolConfig `toml:"executor_pools"` |
There was a problem hiding this comment.
This struct field alignment isn’t gofmt’d (extra spacing before the struct tag on ExecutorPools). Please run gofmt on this file to keep formatting consistent and avoid CI/lint failures.
| IndexerAddress []string `toml:"indexer_address"` | |
| PyroscopeURL string `toml:"pyroscope_url"` | |
| FeeAggregators map[string]string `toml:"fee_aggregators,omitempty"` | |
| Monitoring MonitoringConfig `toml:"monitoring"` | |
| NOPTopology *NOPTopology `toml:"nop_topology"` | |
| ExecutorPools map[string]ExecutorPoolConfig `toml:"executor_pools"` | |
| IndexerAddress []string `toml:"indexer_address"` | |
| PyroscopeURL string `toml:"pyroscope_url"` | |
| FeeAggregators map[string]string `toml:"fee_aggregators,omitempty"` | |
| Monitoring MonitoringConfig `toml:"monitoring"` | |
| NOPTopology *NOPTopology `toml:"nop_topology"` | |
| ExecutorPools map[string]ExecutorPoolConfig `toml:"executor_pools"` |
|
No description provided.