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

Extract MCMS to deployment/common #15288

Merged
merged 18 commits into from
Nov 20, 2024

Conversation

connorwstein
Copy link
Contributor

@connorwstein connorwstein commented Nov 18, 2024

MCMS is not CCIP specific so extracting to the common deployment directory.

@@ -1,33 +0,0 @@
package types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate of common

"github.com/smartcontractkit/chainlink/deployment/common/view/v1_0"
)

func DeployMCMSWithConfig(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intend to follow up with the same pattern here for CCIP dir. Deployment logic can be internal to enforce changeset API usage in higher level tests

Copy link
Contributor

github-actions bot commented Nov 18, 2024

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , Find New Flaky Tests In Chainlink Project / Get Tests To Run , lint , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_race_tests) , Core Tests (go_core_fuzz) , Find New Flaky Tests In Deployment Project / Get Tests To Run , Find New Flaky Tests In Chainlink Project / Run Tests , Find New Flaky Tests In Chainlink Project / Report , Find New Flaky Tests In Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/ccip, ubuntu-latest) , Find New Flaky Tests In Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/ccip/changeset, ubuntu-latest) , Find New Flaky Tests In Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/common/changeset/internal, ubuntu-lat... , Flakey Test Detection , SonarQube Scan , Find New Flaky Tests In Deployment Project / Report

1. No space left on device:[Run tests with flakeguard]

Source of Error:
Run tests with flakeguard	2024-11-19T22:21:42.9984492Z /usr/bin/ld: final link failed: No space left on device
Run tests with flakeguard	2024-11-19T22:21:42.9985398Z collect2: error: ld returned 1 exit status

Why: The error indicates that the disk space on the device where the tests are being run is full, preventing the linker from completing its task.

Suggested fix: Free up disk space on the device by deleting unnecessary files or increasing the disk space available for the runner.

2. Failed to parse JSON test output:[Run tests with flakeguard]

Source of Error:
Run tests with flakeguard	2024-11-19T22:21:42.9978540Z Error running tests: failed to parse json test output near lines:
Run tests with flakeguard	2024-11-19T22:21:42.9979932Z # github.com/smartcontractkit/chainlink/deployment/ccip/changeset.test
Run tests with flakeguard	2024-11-19T22:21:42.9992209Z error: invalid character '#' looking for beginning of value

Why: The JSON output from the test run contains invalid characters, likely due to the previous error causing incomplete or malformed output.

Suggested fix: Address the root cause of the disk space issue first. Ensure that the test output is correctly formatted as JSON and does not contain unexpected characters.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@connorwstein connorwstein marked this pull request as ready for review November 18, 2024 19:53
@connorwstein connorwstein requested review from a team as code owners November 18, 2024 19:53
@connorwstein connorwstein requested a review from a team as a code owner November 18, 2024 20:11
@connorwstein connorwstein requested a review from a team as a code owner November 18, 2024 20:18
chain1, chain2 := selectors[0], selectors[1]

feeds := state.Chains[e.FeedChainSel].USDFeeds
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As there is now 3 changesets (prereqs, mcms + ccip contracts) you need to get a full system I just created a helper

e.Chains[destCS],
destCS,
tokenConfig.GetTokenInfo(e.Logger, state.Chains[destCS].LinkToken, state.Chains[destCS].Weth9),
state.Chains[tenv.FeedChainSel].OffRamp,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why feedChainSel instead of destCS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

destCS was feedChainSel

return state, err
}
state.CancellerMcm = mcms
case deployment.NewTypeAndVersion(commontypes.RBACTimelock, deployment.Version1_0_0).String(),
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that we can do it with commas like that :). Can we move it to the end of the switch statement for better readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we can, maybe via follow up as this PR is merge conflict prone

@@ -32,12 +36,25 @@ func TestInitialDeploy(t *testing.T) {
require.NoError(t, err)
require.NoError(t, tenv.Env.ExistingAddresses.Merge(output.AddressBook))

cfg := make(map[uint64]commontypes.MCMSWithTimelockConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason for not using the NewMemoryEnvironmentWithJobsAndContracts, I see it already does this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case we want to actually test the last step in NewMemoryEnvironmentWithJobsAndContracts

TimelockExecutors: e.Env.AllDeployerKeys(),
TimelockMinDelay: big.NewInt(0),
}
out, err := commonchangeset.DeployMCMSWithTimelock(e.Env, map[uint64]commontypes.MCMSWithTimelockConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

Use DeployMCMSWithTimelockContractsBatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats the internal impl

makramkd
makramkd previously approved these changes Nov 19, 2024
asoliman92
asoliman92 previously approved these changes Nov 19, 2024
@connorwstein connorwstein dismissed stale reviews from asoliman92 and makramkd via 20eab5e November 19, 2024 19:23
@connorwstein connorwstein added this pull request to the merge queue Nov 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2024
@connorwstein connorwstein added this pull request to the merge queue Nov 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@connorwstein connorwstein added this pull request to the merge queue Nov 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@AnieeG AnieeG added this pull request to the merge queue Nov 20, 2024
Merged via the queue into develop with commit b2de054 Nov 20, 2024
166 of 169 checks passed
@AnieeG AnieeG deleted the CCIP-4256-extract-mcms-to-common-own-changeset branch November 20, 2024 02:10
cedric-cordenier pushed a commit that referenced this pull request Nov 20, 2024
* Extract MCMS

* Use changeset

* Standardize package import names

* Fix mcms test imports

* Fix add lane

* Fix bad merge

* Extract common integration helper code

* Fix go mod

* Similar common helper for memeroy

* Comments

* Fix tests

* Fix new test

* Fix RMN test
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