Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(keystone): add capabilities and add nops as mcms proposals #15887

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

MStreet3
Copy link
Contributor

@MStreet3 MStreet3 commented Jan 9, 2025

the CapabilityRegistry for a chain is eventually transferred to a MCMS. this PR enables the creation of changesets that generate MCMS proposals for AddNOPs, AddNodes, AddDONs and AddCapabilities calls.

transaction data is generated using a simulated signer. unit tests are added yet scoped to just the MCMS proposal generation.

Requires

Supports

@MStreet3 MStreet3 requested review from a team as code owners January 9, 2025 20:53
Copy link
Contributor

@krehermann krehermann left a comment

Choose a reason for hiding this comment

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

nice, smaller than i expected

deployment/keystone/changeset/internal/deploy.go Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jan 9, 2025

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@MStreet3 MStreet3 force-pushed the cappl-344/configure-cap-reg branch from b1c81ba to c4e3e2a Compare January 10, 2025 21:27
@MStreet3 MStreet3 requested a review from krehermann January 10, 2025 21:37
krehermann
krehermann previously approved these changes Jan 10, 2025
krehermann
krehermann previously approved these changes Jan 10, 2025
if useMCMS {
txOpts = deployment.SimTransactOpts()
return addCapabilitiesMCMSProposal(registry, deduped, chain)
Copy link
Contributor

Choose a reason for hiding this comment

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

@MStreet3 Just fyi that I added some strategy helpers to handle MCMS in the workflow registry changesets I wrote:

https://github.com/smartcontractkit/chainlink/blob/a1634a1d1540b4caab96be24cffa861fb61a98f9/deployment/keystone/changeset/workflowregistry/strategies.go

Is this something that could be useful here to abstract away the complexity of MCMS?

Copy link
Contributor Author

@MStreet3 MStreet3 Jan 13, 2025

Choose a reason for hiding this comment

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

yes, these could be useful, I hadn't seen them until now. Taking a quick look, the biggest blocker to using them in this PR is that the legacy functions modified in this PR don't return deployment.ChangesetOutput. I wanted to avoid refactoring while changing the implementation. Adding the unit tests in this PR will make a refactor that changes signatures and uses these strategies simpler to verify.

Thoughts @krehermann

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of this logic that's neatly tied into the strategies is currently repeated/split into chainlink-deployments. So really to use these strategies would clean up both repos.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MStreet3 MStreet3 enabled auto-merge January 13, 2025 16:14
@MStreet3 MStreet3 disabled auto-merge January 13, 2025 16:29
Copy link
Contributor

@cedric-cordenier cedric-cordenier left a comment

Choose a reason for hiding this comment

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

+1ing since the strategies stuff requires a wider refactor that we shouldn't tackle here

@MStreet3 MStreet3 dismissed stale reviews from cedric-cordenier and krehermann via c843e26 January 13, 2025 17:38
@MStreet3 MStreet3 force-pushed the cappl-344/configure-cap-reg branch from a1634a1 to c843e26 Compare January 13, 2025 17:38
@MStreet3 MStreet3 enabled auto-merge January 13, 2025 17:45
@MStreet3 MStreet3 added this pull request to the merge queue Jan 13, 2025
Merged via the queue into develop with commit e0a5bb7 Jan 13, 2025
170 of 172 checks passed
@MStreet3 MStreet3 deleted the cappl-344/configure-cap-reg branch January 13, 2025 18:26
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.

3 participants