Skip to content

feat: adds tasks to sign and execute MCMS proposals#445

Merged
jkongie merged 1 commit intomainfrom
CLD-637/mcms-sign-execute-tasks
Sep 24, 2025
Merged

feat: adds tasks to sign and execute MCMS proposals#445
jkongie merged 1 commit intomainfrom
CLD-637/mcms-sign-execute-tasks

Conversation

@jkongie
Copy link
Collaborator

@jkongie jkongie commented Sep 19, 2025

This adds two new tasks to the test engine runtime

  • SignProposalTask - Signs a MCMS or Timelock proposal
  • ExecuteProposalTask - Executes a signed MCMS or Timelock proposal

Proposals that are generated from a changeset output are stored in the
proposal and assigned an id. A user can use the proposal id to sign and
execute the proposal by calling Exec with the newly provided tasks.

Future iterations may add more tasks to support automating the signing
and execution process.

@changeset-bot
Copy link

changeset-bot bot commented Sep 19, 2025

🦋 Changeset detected

Latest commit: a53123a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
chainlink-deployments-framework Minor

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

@jkongie jkongie force-pushed the CLD-637/mcms-sign-execute-tasks branch 23 times, most recently from 0f7f2c0 to 9e35cf0 Compare September 23, 2025 17:07
@jkongie jkongie changed the title [WIP] feat: adds tasks to sign and execute MCMS proposals feat: adds tasks to sign and execute MCMS proposals Sep 23, 2025
@jkongie jkongie force-pushed the CLD-637/mcms-sign-execute-tasks branch from 9e35cf0 to ed86252 Compare September 23, 2025 17:08
Copy link
Contributor

@bytesizedroll bytesizedroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its late and took a quick first pass at this. Will dive back in tomorrow morning.


// Update the proposal state with the signed proposal
if err := state.UpdateProposalJSON(t.proposalID, propJSON); err != nil {
return fmt.Errorf("failed toupdate proposal state: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return fmt.Errorf("failed toupdate proposal state: %w", err)
return fmt.Errorf("failed to update proposal state: %w", err)

t.Parallel()

// Generate a test private key
privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
privateKey, err := ecdsa.GenerateKey(btcec.S256(), rand.Reader)

I assume mcms accepts P256 for tests but should we maybe have it be secp256k1 just to keep consistency with prod?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into the mcms lib and it uses the go ethereum crypto library to perform signing, and their tests use private keys generated from the same crypto library. I've updated the tests to use this same method

t.Parallel()

// Generate a test private key
privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been changed to use the go ethereum crypto library, same reasoning as above

if err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if propState.IsExecuted {
return fmt.Errorf("proposal already executed: %s", t.proposalID)
}

Is there benefit in adding a guard here? So that we are actually using the IsExecuted attribute and protecting against re-runs, proposals should only be ran once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add a test for this as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

This adds two new tasks to the test engine runtime

- SignProposalTask - Signs a MCMS or Timelock proposal
- ExecuteProposalTask - Executes a signed MCMS or Timelock proposal

Proposals that are generated from a changeset output are stored in the
proposal and assigned an id. A user can use the proposal id to sign and
execute the proposal by calling Exec with the newly provided tasks.

Future iterations may add more tasks to support automating the signing
and execution process.
@jkongie jkongie force-pushed the CLD-637/mcms-sign-execute-tasks branch from ed86252 to a53123a Compare September 24, 2025 05:44
@cl-sonarqube-production
Copy link

Copy link
Collaborator

@graham-chainlink graham-chainlink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkongie jkongie marked this pull request as ready for review September 24, 2025 08:00
@jkongie jkongie requested a review from a team as a code owner September 24, 2025 08:00
Copilot AI review requested due to automatic review settings September 24, 2025 08:00
@jkongie jkongie added this pull request to the merge queue Sep 24, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds new MCMS (Multi-Chain Multi-Sig) proposal signing and execution functionality to the test engine runtime, enabling automated management of blockchain proposals.

  • Implements SignProposalTask for cryptographically signing MCMS and Timelock proposals
  • Implements ExecuteProposalTask for executing signed proposals on target blockchains
  • Adds comprehensive proposal state management with JSON serialization and execution tracking

Reviewed Changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
engine/test/runtime/task_mcms.go Core implementation of MCMS signing and execution tasks
engine/test/runtime/task_mcms_test.go Comprehensive test coverage for both task types with mocking
engine/test/runtime/state.go Extended state management to track proposals with unique IDs
engine/test/runtime/state_test.go Tests for proposal state operations and management
engine/test/internal/mcmsutils/ New utilities package providing MCMS operations across blockchain families
chain/blockchain.go Added GetBySelector method for chain lookup functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// ignored as cryptographic randomness is not critical for test salt generation.
func randomHash() *common.Hash {
b := make([]byte, 32)
_, _ = rand.Read(b) // Assignment for errcheck. Only used in tests so we can ignore.
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment suggests this is acceptable because it's test-only code, but this function could be called in production contexts. Consider returning an error or using a more explicit approach like crypto/rand package that would panic on failure instead of silently ignoring errors.

Suggested change
_, _ = rand.Read(b) // Assignment for errcheck. Only used in tests so we can ignore.
if _, err := rand.Read(b); err != nil {
panic(fmt.Sprintf("failed to generate random hash: %v", err))
}

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +26
// The signer is configured to use simulated EVM backends by default, which affects
// how encoders are generated for signing EVM proposals.
func NewSigner() (*Signer, error) {
return &Signer{
isEVMSim: true, // This is always true for until we can find a way to allow the user to specify the type of EVM backend they are using.
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded true value with a TODO-style comment indicates incomplete functionality. Consider adding a parameter to NewSigner() or using an environment variable to make this configurable rather than hardcoding the value.

Suggested change
// The signer is configured to use simulated EVM backends by default, which affects
// how encoders are generated for signing EVM proposals.
func NewSigner() (*Signer, error) {
return &Signer{
isEVMSim: true, // This is always true for until we can find a way to allow the user to specify the type of EVM backend they are using.
// The isEVMSim parameter controls whether to use simulated EVM backends, which affects
// how encoders are generated for signing EVM proposals.
func NewSigner(isEVMSim bool) (*Signer, error) {
return &Signer{
isEVMSim: isEVMSim,

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +55
return nil, fmt.Errorf("invalid action [%s]: must be one of %v",
f.action, slices.Collect(maps.Keys(actionToAptosRole)),
)
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message uses %v to format the slice which will display in Go slice format like [item1 item2]. Consider joining the keys with commas or using a more user-friendly format like strings.Join() for better readability.

Copilot uses AI. Check for mistakes.
Merged via the queue into main with commit 967a01b Sep 24, 2025
15 checks passed
@jkongie jkongie deleted the CLD-637/mcms-sign-execute-tasks branch September 24, 2025 08:05
github-merge-queue bot pushed a commit that referenced this pull request Sep 25, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## chainlink-deployments-framework@0.50.0

### Minor Changes

-
[#452](#452)
[`41464d4`](41464d4)
Thanks [@jkongie](https://github.com/jkongie)! - Add `runtime.New()`
convenience function for runtime initialization

Provides a simpler way to create runtime instances using functional
options for environment configuration.

-
[#445](#445)
[`967a01b`](967a01b)
Thanks [@jkongie](https://github.com/jkongie)! - Adds tasks to the test
engine runtime to sign and execute MCMS proposals

-
[#451](#451)
[`0e64684`](0e64684)
Thanks [@jkongie](https://github.com/jkongie)! - Adds new convenience
method `environment.New` to the test engine to bring up a new test
environment

The `environment.New` method is a wrapper around the environment loading
struct and allows the user
to load a new environment without having to instantiate the `Loader`
struct themselves.

The `testing.T` argument has been removed and it's dependencies have
been replaced with:

    -   A `context.Context` argument to the `Load` and `New` functions
- A new functional option `WithLogger` which overrides the default noop
logger.

While this is a breaking change, the test environment is still in
development and is not in actual usage yet.

### Patch Changes

-
[#454](#454)
[`d87d8ef`](d87d8ef)
Thanks [@DimitriosNaikopoulos](https://github.com/DimitriosNaikopoulos)!
- Bump CTF to fix docker security dependency

-
[#455](#455)
[`4788ba4`](4788ba4)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - fix:
update ValidUntil when running "mcmsv2 reset-proposal"

---------

Co-authored-by: app-token-issuer-engops[bot] <144731339+app-token-issuer-engops[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants