Add validation for a minimum of 16 nops in ccip 2.0 testnet and mainn…#2006
Add validation for a minimum of 16 nops in ccip 2.0 testnet and mainn…#2006
Conversation
carte7000
requested review from
AmrMohamedRezk,
KodeyThomas,
agusaldasoro,
asoliman92,
chris-de-leon-cll,
tt-cll and
winder
There was a problem hiding this comment.
Pull request overview
Adds environment-aware topology validation to enforce a minimum number of unique NOPs per chain in production-like environments (CCIP 2.0 mainnet/testnet), and wires this validation into key deployment changesets.
Changes:
- Introduce
EnvironmentTopology.ValidateForEnvironment(envName)and enforce ≥ 16 unique NOP aliases per chain for committees and executor pools in production environments. - Expand
shared.IsProductionEnvironmentto recognizeprod_mainnetandprod_testnet(in addition to legacymainnet). - Update and add tests across offchain topology and changesets to satisfy the stricter validation and new required fields.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| deployment/v2_0_0/offchain/topology.go | Adds production-only validation (min 16 unique NOPs/chain) and makes nop_topology required in Validate(). |
| deployment/v2_0_0/offchain/topology_test.go | New tests covering production min-NOP validation and nil NOPTopology validation. |
| deployment/v2_0_0/offchain/shared/types.go | Updates production environment detection to include prod_mainnet and prod_testnet. |
| deployment/v2_0_0/offchain/shared/types_test.go | New unit tests for IsProductionEnvironment. |
| deployment/v2_0_0/changesets/generate_aggregator_config.go | Calls ValidateForEnvironment during precondition checks. |
| deployment/v2_0_0/changesets/generate_aggregator_config_test.go | Updates fixtures/expectations to satisfy stricter topology validation. |
| deployment/v2_0_0/changesets/deploy_chain_contracts.go | Adds ValidateForEnvironment to preconditions. |
| deployment/v2_0_0/changesets/configure_chains_for_lanes_from_topology.go | Adds ValidateForEnvironment to preconditions. |
| deployment/v2_0_0/changesets/configure_chains_for_lanes_from_topology_test.go | Updates fixtures to include required topology fields and aggregator configs. |
| deployment/v2_0_0/changesets/apply_verifier_config.go | Adds ValidateForEnvironment to preconditions. |
| deployment/v2_0_0/changesets/apply_verifier_config_test.go | Updates fixtures to meet production min-NOP requirements. |
| deployment/v2_0_0/changesets/apply_executor_config.go | Adds ValidateForEnvironment to preconditions. |
| deployment/v2_0_0/changesets/apply_executor_config_test.go | Adds production min-NOP validation coverage and updates fixtures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if cfg.Topology.NOPTopology == nil { | ||
| return nil, fmt.Errorf("NOP topology is required") | ||
| } |
There was a problem hiding this comment.
After calling ValidateForEnvironment, cfg.Topology.NOPTopology is guaranteed to be non-nil (Validate() now errors on nil). This extra nil check is therefore dead code and also returns an inconsistent error string ("NOP topology is required") compared to the new "nop_topology is required" validation path. Consider removing this check (or at least the nil branch) and relying on ValidateForEnvironment/Validate for nil handling.
| if cfg.Topology.NOPTopology == nil { | |
| return nil, fmt.Errorf("NOP topology is required") | |
| } |
| if err := cfg.Topology.ValidateForEnvironment(e.Name); err != nil { | ||
| return fmt.Errorf("topology validation failed: %w", err) | ||
| } |
There was a problem hiding this comment.
Now that ValidateForEnvironment is called here (and it calls EnvironmentTopology.Validate), subsequent validation checks in this function for Topology.NOPTopology == nil and len(Topology.IndexerAddress) == 0 become redundant/dead for those failure modes (Validate already rejects nil NOPTopology and missing indexer_address). Consider removing/simplifying the later checks to avoid unreachable/inconsistent error paths.
| return fmt.Errorf("topology validation failed: %w", err) | ||
| } | ||
|
|
||
| if cfg.Topology.NOPTopology == nil || len(cfg.Topology.NOPTopology.NOPs) == 0 { |
There was a problem hiding this comment.
With ValidateForEnvironment invoked here (which calls EnvironmentTopology.Validate), later checks for Topology.NOPTopology == nil are redundant because nil NOPTopology will already fail validation. Consider simplifying later conditions to only cover what Validate() does not enforce (e.g., empty NOP list, missing committees, etc.) to keep error paths consistent.
| if cfg.Topology.NOPTopology == nil || len(cfg.Topology.NOPTopology.NOPs) == 0 { | |
| if len(cfg.Topology.NOPTopology.NOPs) == 0 { |
| return fmt.Errorf("topology validation failed: %w", err) | ||
| } | ||
|
|
||
| if cfg.Cfg.Topology.NOPTopology == nil || len(cfg.Cfg.Topology.NOPTopology.Committees) == 0 { |
There was a problem hiding this comment.
Since ValidateForEnvironment calls EnvironmentTopology.Validate (which now rejects a nil NOPTopology), the subsequent cfg.Cfg.Topology.NOPTopology == nil check below becomes redundant. Consider dropping the nil branch and only checking len(...Committees) == 0 if you still need to enforce that at least one committee exists for this changeset.
| if cfg.Cfg.Topology.NOPTopology == nil || len(cfg.Cfg.Topology.NOPTopology.Committees) == 0 { | |
| if len(cfg.Cfg.Topology.NOPTopology.Committees) == 0 { |
| if err := cfg.Topology.ValidateForEnvironment(e.Name); err != nil { | ||
| return fmt.Errorf("topology validation failed: %w", err) | ||
| } | ||
| if cfg.Topology.NOPTopology == nil || len(cfg.Topology.NOPTopology.Committees) == 0 { |
There was a problem hiding this comment.
Calling ValidateForEnvironment here means nil NOPTopology (and missing indexer_address) will already be rejected by EnvironmentTopology.Validate. The later cfg.Topology.NOPTopology == nil branch is therefore redundant; consider simplifying it to only check for the conditions this changeset specifically needs (e.g., len(...Committees) == 0).
| if cfg.Topology.NOPTopology == nil || len(cfg.Topology.NOPTopology.Committees) == 0 { | |
| if len(cfg.Topology.NOPTopology.Committees) == 0 { |
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
…et env