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 helper to use in cli in CLD #15647

Merged
merged 8 commits into from
Dec 12, 2024
Merged

Conversation

krehermann
Copy link
Contributor

@krehermann krehermann commented Dec 11, 2024

DPA-1396

refactor to support timelock runner as cli

Requires

Supports

https://github.com/smartcontractkit/chainlink-deployments/pull/333

Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and c392124 (dev-auto/mcms-exec-lib).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
Test_NewAcceptOwnershipChangeset 0.00% true false false 3 0 3 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset true 0s @smartcontractkit/ccip, @smartcontractkit/core

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

Copy link
Contributor

github-actions bot commented Dec 11, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@krehermann krehermann force-pushed the dev-auto/mcms-exec-lib branch from c392124 to 95bf07c Compare December 11, 2024 21:10
@krehermann krehermann marked this pull request as ready for review December 11, 2024 23:48
@krehermann krehermann requested review from a team as code owners December 11, 2024 23:48
deployment/common/changeset/mcms_helpers.go Outdated Show resolved Hide resolved
deployment/common/changeset/mcms_helpers.go Outdated Show resolved Hide resolved
deployment/common/changeset/mcms_helpers.go Outdated Show resolved Hide resolved
@@ -91,6 +91,7 @@ func ApplyChangesets(t *testing.T, e deployment.Environment, timelockContractsPe
NodeIDs: e.NodeIDs,
Offchain: e.Offchain,
OCRSecrets: e.OCRSecrets,
GetContext: e.GetContext,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dammit, another one. Shouldn't we be using NewEnvironment here instead of priming the struct anyway? Sigh.

(Not a review, just drive-by frustration)

}

// Validate checks that all fields are non-nil, ensuring it's ready
// for use generating views or interactions.
func (state MCMSWithTimelockState) Validate() error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mv with contracts to proposalutils

// for a MCMSWithTimelock contract deployment.
// It is public for use in product specific packages.
// Either all fields are nil or all fields are non-nil.
type MCMSWithTimelockContracts struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to use MaybeLoad.. and avoid import cycle need to defn this in this package. mv of MCMSWithTimelockState

)

func Test_NewAcceptOwnershipChangeset(t *testing.T) {
t.Parallel()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ci is timing out. parallelize many ccip tests

@krehermann krehermann force-pushed the dev-auto/mcms-exec-lib branch from f4b09c6 to ae0c33e Compare December 12, 2024 01:49
cgruber
cgruber previously approved these changes Dec 12, 2024
// If the block start is not provided, it assumes that the operations have not been scheduled yet
// and executes all the operations for the given chain.
// It is an error if there are no operations for the given chain.
func RunTimelockExecutor(env deployment.Environment, cfg RunTimelockExecutorConfig) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sigh. It makes me nervous having this all accessible to changesets, as I want to avoid changesets themselves from ever executing this logic. But I guess this is fine for now - I agree, it probably ought to go into MCMS library though. We can move it there later.

Copy link
Contributor Author

@krehermann krehermann Dec 12, 2024

Choose a reason for hiding this comment

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

do either of these make it better:

  • in a real env (with non zero timelock) it doesn't matter b/c the duration must pass while in the timelock
  • moving pkgs out of changeset

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think both help, yeah.

Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and 549b3a7 (dev-auto/mcms-exec-lib).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestInitialAddChainAppliedTwice 66.67% false false false 3 2 1 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset false 2m14.353333333s @smartcontractkit/ccip, @smartcontractkit/core

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

@@ -221,8 +221,8 @@ func ConfirmCommitForAllWithExpectedSeqNums(
return false
}
},
3*time.Minute,
1*time.Second,
tests.WaitTimeout(t),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixed a lot of flakes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it may also be related to the recent ccip smoke test nightmares

Comment on lines 39 to 43
ab, types.MCMSWithTimelockConfig{
Canceller: changeset.SingleGroupMCMS(t),
Bypasser: changeset.SingleGroupMCMS(t),
Proposer: changeset.SingleGroupMCMS(t),
Canceller: proposalutils.SingleGroupMCMS(t),
Bypasser: proposalutils.SingleGroupMCMS(t),
Proposer: proposalutils.SingleGroupMCMS(t),
TimelockMinDelay: big.NewInt(0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just proposalutils.SingleGroupTimelockConfig(t) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oversight. thanks

jmank88
jmank88 previously approved these changes Dec 12, 2024
@krehermann krehermann disabled auto-merge December 12, 2024 13:29
@krehermann krehermann added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@krehermann krehermann added this pull request to the merge queue Dec 12, 2024
Merged via the queue into develop with commit dde1751 Dec 12, 2024
193 checks passed
@krehermann krehermann deleted the dev-auto/mcms-exec-lib branch December 12, 2024 15:43
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.

6 participants