skip inspector building if it needs additional fields - #1136
skip inspector building if it needs additional fields#1136FelixFan1992 wants to merge 2 commits into
Conversation
|
|
👋 FelixFan1992, 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
Adjusts MCMS inspector construction to avoid failing inspector creation across an environment when some chains (notably Sui) cannot be built with empty chain metadata.
Changes:
- Reworked
McmsInspectorsto build inspectors per-chain and skip chains that error during construction. - Updated/expanded the function’s doc comment to explain why certain chains are skipped (and how missing inspectors surface later).
- Added a unit test to validate that chains requiring extra metadata (Sui) are skipped while EVM inspectors are still returned.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| engine/cld/mcms/proposalutils/inspectors.go | Switches from bulk inspector construction to per-chain BuildInspector with skip-on-error behavior and updated docs. |
| engine/cld/mcms/proposalutils/inspectors_test.go | Adds coverage ensuring metadata-dependent chains (Sui) are skipped without failing inspector generation for supported chains. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Chains that require chain-specific metadata the framework does not hold, such | ||
| // as Sui which needs AdditionalFields populated from onchain MCMS state, are | ||
| // skipped rather than failing the whole set. Proposal building only consumes | ||
| // inspectors for chains that have batch operations, so a skipped chain that is | ||
| // later needed still surfaces as a missing-inspector error. |
| if err != nil { | ||
| continue | ||
| } |
FelixFan1992
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
engine/cld/mcms/proposalutils/inspectors.go:69
McmsInspectorscurrentlycontinues on anyBuildInspectorerror. This means genuine configuration/runtime failures (e.g., missing client, RPC issues) will be silently dropped and the function will still returnnilerror, even though the doc comment implies only chains requiring extra metadata (e.g., Sui) should be skipped. Consider restricting the skip to known cases (e.g., by chain family == Sui, or by matching a specific error frommcmschainwrappers) and returning an error for unexpected failures so callers can diagnose misconfiguration.
inspector, err := mcmschainwrappers.BuildInspector(
&chainAccessor,
mcmstypes.ChainSelector(chainSelector),
mcmstypes.TimelockActionSchedule,
mcmstypes.ChainMetadata{},
)
if err != nil {
continue
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
engine/cld/mcms/proposalutils/inspectors.go:70
McmsInspectorsnow silently ignores allBuildInspectorerrors and still returnsnilerror, which can hide real failures (e.g., misconfigured chain access / RPC issues) and make it hard to debug why an inspector is missing. At minimum, log when a chain is skipped (guarding againstenv.Logger == nil) so callers have some visibility into skipped selectors and underlying errors.
inspectors := make(map[uint64]mcmssdk.Inspector)
for chainSelector := range env.BlockChains.All() {
inspector, err := mcmschainwrappers.BuildInspector(
&chainAccessor,
mcmstypes.ChainSelector(chainSelector),
mcmstypes.TimelockActionSchedule,
mcmstypes.ChainMetadata{},
)
if err != nil {
continue
}
inspectors[chainSelector] = inspector
|
sorry @FelixFan1992 but I don't think this is the direction we should head. Let us align on the path forward on the slack thread, but I believe you have been able to execute the changeset without modifying CLDF. |
No description provided.