fix(deployment): Embbed ChainDefinition instead of partially copying it#1907
fix(deployment): Embbed ChainDefinition instead of partially copying it#1907rodrigombsoares wants to merge 3 commits intomainfrom
Conversation
rodrigombsoares
requested review from
0xAustinWang,
KodeyThomas,
chris-de-leon-cll,
matYang,
mateusz-sekara,
tt-cll and
winder
|
👋 rodrigombsoares, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR refactors ConfigureChainsForLanesFromTopology’s per-chain input to embed lanes.ChainDefinition instead of duplicating selected fields, updating the changeset and its tests accordingly.
Changes:
- Replace
PartialChainConfigwithChainConfigForLanesFromTopologythat embedslanes.ChainDefinition. - Rename the per-chain remote mapping from
RemoteChainstoRemoteLanesto avoid confusion withCommitteeVerifierInputConfig.RemoteChains. - Update
configure_committee_verifiers_test.goto construct inputs using the embeddedChainDefinition.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| deployment/v1_7_0/changesets/configure_committee_verifiers.go | Introduces the embedded ChainDefinition input type and updates lane construction logic accordingly. |
| deployment/v1_7_0/changesets/configure_committee_verifiers_test.go | Updates tests to use the new config type and renamed fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // embedded lanes.ChainDefinition for that chain, CommitteeVerifierInputConfig resolved via topology, | ||
| // and RemoteLanes (remote selector → remote lane endpoint), separate from CommitteeVerifierInputConfig.RemoteChains. |
There was a problem hiding this comment.
The doc comment says "CommitteeVerifierInputConfig resolved via topology", but the input CommitteeVerifiers slice is still provided by the caller; only the signature quorum config is resolved from topology inside Apply. Please reword this comment to avoid implying the entire input is derived from topology.
| // embedded lanes.ChainDefinition for that chain, CommitteeVerifierInputConfig resolved via topology, | |
| // and RemoteLanes (remote selector → remote lane endpoint), separate from CommitteeVerifierInputConfig.RemoteChains. | |
| // embedded lanes.ChainDefinition for that chain, caller-provided CommitteeVerifierInputConfig values | |
| // (with signature quorum configuration resolved via topology inside Apply), and RemoteLanes (remote | |
| // selector → remote lane endpoint), separate from CommitteeVerifierInputConfig.RemoteChains. |
| chainA := chain.ChainDefinition | ||
| chainA.CommitteeVerifiers = committeeVerifiers | ||
|
|
There was a problem hiding this comment.
chainA := chain.ChainDefinition copies any caller-provided ChainDefinition.CommitteeVerifiers, but then overwrites it with the computed committeeVerifiers. Because ChainDefinition is now part of the public input type, a caller can set ChainDefinition.CommitteeVerifiers and have it silently ignored. Consider adding validation that chain.ChainDefinition.CommitteeVerifiers is empty (and/or making the input field name explicitly different) so misconfiguration fails fast.
There was a problem hiding this comment.
We'll leave it like this for now until a bigger refactor gets in place.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Summary
Embed
lanes.ChainDefinitionin the per-chain input forConfigureChainsForLanesFromTopologyinstead of partially copying its fields.Features
ChainConfigForLanesFromTopologyembeddedChainDefinitionPartialChainConfigTesting
go test ./v1_7_0/changesets/... -run ConfigureChainsForLanesFromTopology (from deployment/).