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

refactor update & append capabilities #15563

Merged
merged 4 commits into from
Dec 10, 2024
Merged

refactor update & append capabilities #15563

merged 4 commits into from
Dec 10, 2024

Conversation

krehermann
Copy link
Contributor

@krehermann krehermann commented Dec 8, 2024

KS-610

mcms refactoring for update and append capabilities to nodes. these changeset are compound - they write new capabilities and set the nodes. this requires some new layering to support MCMS

  • optionally support mcms
  • when using MCMS batch operations for efficiency
  • use ContractSet consistently rather than Registry

Requires

Supports

Copy link
Contributor

github-actions bot commented Dec 8, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@krehermann krehermann marked this pull request as ready for review December 10, 2024 00:12
@krehermann krehermann requested review from a team as code owners December 10, 2024 00:12
require.NoError(t, err)
require.Len(t, csOut.Proposals, 1)
require.Len(t, csOut.Proposals[0].Transactions, 1)
require.Len(t, csOut.Proposals[0].Transactions[0].Batch, 2) // add capabilities, update nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

ty for this comment

@krehermann krehermann requested a review from a team December 10, 2024 00:28
@@ -425,30 +425,19 @@ type RegisteredCapability struct {
ID [32]byte
}

func FromCapabilitiesRegistryCapability(cap *kcr.CapabilitiesRegistryCapability, e deployment.Environment, registryChainSelector uint64) (*RegisteredCapability, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No other consumers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. yes there are. i didn't mean to delete this

patrickhuie19
patrickhuie19 previously approved these changes Dec 10, 2024
Copy link
Contributor

@patrickhuie19 patrickhuie19 left a comment

Choose a reason for hiding this comment

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

LGTM

proposerMCMSes,
[]timelock.BatchChainOperation{*r.Ops},
"proposal to set update node capabilities",
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth parameterizing this just in case you want to add timelock delay in the future. @carte7000 had a good idea here https://github.com/smartcontractkit/chainlink/pull/15525/files#diff-f6eeb14804b5d16769abc3f48ad18187965263d5e5d29345e4079e85199de731R38 to use an MCMS struct being nil/not-nil as a way to signal that but also keep the door open for any other MCMS specific params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, will address in followup; have one more in the stack to merge

bolekk
bolekk previously approved these changes Dec 10, 2024
@@ -0,0 +1,129 @@
package changeset_test
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in file name

patrickhuie19
patrickhuie19 previously approved these changes Dec 10, 2024
@krehermann krehermann added this pull request to the merge queue Dec 10, 2024
@patrickhuie19 patrickhuie19 removed this pull request from the merge queue due to a manual request Dec 10, 2024
@patrickhuie19 patrickhuie19 added this pull request to the merge queue Dec 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 10, 2024
@bolekk bolekk added this pull request to the merge queue Dec 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 10, 2024
@krehermann krehermann requested a review from bolekk December 10, 2024 18:25
@krehermann krehermann enabled auto-merge December 10, 2024 18:25
@krehermann krehermann added this pull request to the merge queue Dec 10, 2024
Merged via the queue into develop with commit 100d87d Dec 10, 2024
173 checks passed
@krehermann krehermann deleted the ks/cap-cs-refactor branch December 10, 2024 22:16
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