chore: port mcms helpers from chainlink/deployment#923
Conversation
🦋 Changeset detectedLatest commit: 7485acd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ecPablo
force-pushed
the
ecpablo/port-mcms-helpers-core
branch
from
1302167 to
4dc6446
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| family, err := chainsel.GetSelectorFamily(uint64(chainSelector)) | ||
| if err != nil { | ||
| return fmt.Errorf("[ExecuteMCMSProposalV2] failed to get chain family for selector %d: %w", chainSelector, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
ExecuteMCMSProposalV2 determines the chain family but only confirms transactions for EVM and Aptos; all other families (e.g., TON, TRON, Sui) will silently skip confirmation, which can leave tests in an incorrect state (transactions unconfirmed) while still returning nil. Consider switching on family with an explicit default error, or routing confirmation through a shared helper similar to engine/test/internal/mcmsutils/executor.go:360-392 so each supported family is handled intentionally (including NOOP where appropriate, e.g., Solana).
There was a problem hiding this comment.
this comments raises a good point: should we apply these improvements now or keep the code as close as possible to what we have in core to separate the migrations steps (i.e., move to CLDF then refactor).
| family, err := chainsel.GetSelectorFamily(uint64(op.ChainSelector)) | ||
| if err != nil { | ||
| return fmt.Errorf("[ExecuteMCMSProposalV2] failed to get chain family for selector %d: %w", op.ChainSelector, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Same issue as the SetRoot loop: after computing family, the code only has confirmation branches for EVM and Aptos. For any other chain family present in the proposal, the operation tx will not be confirmed and the helper may still report success. Add an explicit default branch (or shared confirmation helper) so non-(EVM/Aptos/Solana) families don’t get silently skipped.
gustavogama-cll
left a comment
gustavogama-cll
left a comment
There was a problem hiding this comment.
since we're moving to a different repository, should we drop the "v2" suffixes? They're kind of useless by now (though it might make the migration of client code harder...)
experimental/proposalutils/types.go
Outdated
| cldf "github.com/smartcontractkit/chainlink-deployments-framework/deployment" | ||
| ) | ||
|
|
||
| type MCMSRole string |
There was a problem hiding this comment.
nit: we should probably separate the MCMSRole type functions and constants from the rest
| type MCMSRole string | ||
|
|
||
| const ( | ||
| BypasserManyChainMultisig cldf.ContractType = "BypasserManyChainMultiSig" |
There was a problem hiding this comment.
nit/noaction: we need a better place for these mcms contract types
| // LinkToken is the burn/mint link token. It should be used everywhere for | ||
| // new deployments. Corresponds to | ||
| // https://github.com/smartcontractkit/chainlink/blob/develop/core/gethwrappers/shared/generated/link_token/link_token.go#L34 | ||
| LinkToken cldf.ContractType = "LinkToken" |
There was a problem hiding this comment.
nit/noaction: and this one
ecPablo
dismissed stale reviews from ajaskolski and DimitriosNaikopoulos
via
d36a3a3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func SingleGroupTimelockConfig(t *testing.T) proposalutils.MCMSWithTimelockConfig { | ||
| t.Helper() | ||
|
|
||
| return proposalutils.MCMSWithTimelockConfig{ | ||
| Canceller: SingleGroupMCMS(t), | ||
| Bypasser: SingleGroupMCMS(t), | ||
| Proposer: SingleGroupMCMS(t), | ||
| TimelockMinDelay: big.NewInt(0), | ||
| } |
There was a problem hiding this comment.
SingleGroupTimelockConfig returns proposalutils.MCMSWithTimelockConfig, but that type’s Canceller/Bypasser/Proposer fields are mcmstypes.Config while SingleGroupMCMS returns config.Config. This won’t compile (type mismatch). Consider returning MCMSWithTimelockConfigLegacy here, or switching to SingleGroupMCMSV2 and the non-legacy config type consistently.
| func SingleGroupTimelockConfigV2(t *testing.T) proposalutils.MCMSWithTimelockConfigV2 { | ||
| t.Helper() | ||
|
|
||
| return proposalutils.MCMSWithTimelockConfigV2{ | ||
| Canceller: SingleGroupMCMSV2(t), | ||
| Bypasser: SingleGroupMCMSV2(t), | ||
| Proposer: SingleGroupMCMSV2(t), | ||
| TimelockMinDelay: big.NewInt(0), | ||
| } |
There was a problem hiding this comment.
SingleGroupTimelockConfigV2 references proposalutils.MCMSWithTimelockConfigV2, but there is no such type in experimental/proposalutils/types.go (only MCMSWithTimelockConfigLegacy and MCMSWithTimelockConfig). This will not compile; either add/rename the type to MCMSWithTimelockConfigV2 or update this helper to return the existing MCMSWithTimelockConfig.
| // MCMSWithTimelockConfigV2 holds the configuration for an MCMS with timelock. | ||
| // Unlike the legacy MCMSWithTimelockConfig type above, this variant uses the | ||
| // newer mcmstypes.Config definitions. | ||
| type MCMSWithTimelockConfig struct { | ||
| Canceller mcmstypes.Config `json:"canceller"` | ||
| Bypasser mcmstypes.Config `json:"bypasser"` | ||
| Proposer mcmstypes.Config `json:"proposer"` | ||
| TimelockMinDelay *big.Int `json:"timelockMinDelay"` |
There was a problem hiding this comment.
The comment says MCMSWithTimelockConfigV2, but the type declared is MCMSWithTimelockConfig and the “legacy … type above” is actually named MCMSWithTimelockConfigLegacy. Please align the comment/type names (and any callers) to avoid confusion and mismatched expectations about which config struct to use.
We are porting the code in https://github.com/smartcontractkit/chainlink/blob/develop/deployment/common/proposalutils/mcms_helpers.go to CLDF in order to remove deps on core repo for some downstream consumers. The functions were splitted in separate files to keep them under semantically meaningful names. Also added unit tests and fixed some obvious issues pointed out by copilot, but in general keeping the logic as close as possible to what we have in core today.
McmsInspectorForChainin core repo -> moved toexperimental/proposalutils/inspectors.goin CLDF.BatchOperationForChainandTransactionForChainmoved toexperimental/proposalutils/operations.goin CLDFtest_helpers.goandtypes.goare moved as is from core repo to CLDFFollowup PRs in core will attempt to replace usages with these. So we can delete the
mcms_helpers.gofrom coreAI Summary
This pull request ports the
proposalutilshelpers from thechainlink/deploymentrepository into thechainlink-deployments-framework, making them part of the framework and improving proposal creation, inspection, and testing across multiple blockchain environments. The main changes include adding utility functions for proposal operations, inspectors, and test helpers, as well as defining relevant types and constants for MCMS (Many Chain Multisig) and timelock configurations.New proposal utilities and helpers:
experimental/proposalutils/inspectors.goproviding functions to build MCMS inspectors for different chains, supporting optional timelock actions and mass inspector creation.experimental/proposalutils/operations.gowith helpers to create transactions and batch operations for different chain families (EVM, Solana), abstracting over chain-specific details.Testing support:
experimental/proposalutils/test_helpers.gocontaining test helpers for signing and executing MCMS proposals and timelock proposals, as well as generating single-group configs and finding call proxy addresses for tests.Types and constants:
experimental/proposalutils/types.godefining types and constants for MCMS roles, contract types, and configuration structures (including gas boost config), supporting both legacy and new MCMS config formats.Changelog: