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

New rmn curse changeset #15868

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

New rmn curse changeset #15868

wants to merge 9 commits into from

Conversation

carte7000
Copy link
Contributor

@carte7000 carte7000 commented Jan 8, 2025

This PR add a new changeset to curse lanes and chains via RMNRemote

It also add a utility that allow writing changeset without caring about wether we are creating a proposal or executing with a deployer key.

Example usage of the changeset

cfg := RMNCurseConfig{
    CurseActions: []CurseAction{
        CurseChain(SEPOLIA_CHAIN_SELECTOR),
        CurseLane(SEPOLIA_CHAIN_SELECTOR, AVAX_FUJI_CHAIN_SELECTOR),
    },
    CurseReason: "test curse",
    MCMS: &MCMSConfig{MinDelay: 0},
}
output, err := NewRMNCurseChangeset(env, cfg)

@carte7000 carte7000 requested review from a team as code owners January 8, 2025 19:33
Copy link
Contributor

github-actions bot commented Jan 8, 2025

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , GolangCI Lint (integration-tests) , Core Tests (go_core_tests) , GolangCI Lint (deployment) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , test-scripts , Core Tests (go_core_fuzz) , Core Tests (go_core_race_tests) , lint , SonarQube Scan

1. Unnecessary conversion error: Golang Lint (integration-tests)

Source of Error:
integration-tests/smoke/ccip/ccip_rmn_test.go:627:78: unnecessary conversion (unconvert)
curseActions = append(curseActions, changeset.CurseLane(remoteSel, uint64((tc.pf.chainSelectors[subjectDescription]))))
integration-tests/smoke/ccip/ccip_rmn_test.go:662:78: unnecessary conversion (unconvert)
curseActions = append(curseActions, changeset.CurseLane(remoteSel, uint64((tc.pf.chainSelectors[subjectDescription]))))
**Why**: The code is performing an unnecessary type conversion to `uint64` which is not required.

Suggested fix: Remove the unnecessary uint64 conversion in the specified lines.

2. Require must only be used in the goroutine running the test function: Golang Lint (integration-tests)

Source of Error:
integration-tests/smoke/ccip/ccip_rmn_test.go:679:5: go-require: require must only be used in the goroutine running the test function (testifylint)
require.NoError(t, err)
integration-tests/smoke/ccip/ccip_rmn_test.go:681:5: go-require: require must only be used in the goroutine running the test function (testifylint)
require.NoError(t, err)
**Why**: The `require` package from `testify` should only be used in the main test goroutine to ensure proper test execution and reporting.

Suggested fix: Use assert instead of require in goroutines or move the require calls to the main test function.

AER Report: Operator UI CI

aer_workflow , commit , Breaking Changes GQL Check

1. Workflow conclusion is failure:[convictional/trigger-workflow-and-wait@f69fa9e]

Source of Error:
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2025-01-10T17:41:27.9406619Z Checking conclusion [failure]
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2025-01-10T17:41:27.9407185Z Checking status [completed]
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2025-01-10T17:41:27.9407577Z Conclusion is not success, it's [failure].
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2025-01-10T17:41:27.9408419Z Propagating failure to upstream job

Why: The triggered workflow did not complete successfully. The conclusion status was "failure," which caused the upstream job to also fail.

Suggested fix: Investigate the logs of the triggered workflow (https://github.com/smartcontractkit/operator-ui/actions/runs/12714460688) to identify the specific reason for its failure and address the underlying issue.

deployment/ccip/changeset/cs_rmn_curse_uncurse.go Outdated Show resolved Hide resolved
deployment/ccip/changeset/cs_rmn_curse_uncurse.go Outdated Show resolved Hide resolved
Comment on lines +146 to +148
// CurseChain(SEPOLIA_CHAIN_SELECTOR),
// CurseLane(SEPOLIA_CHAIN_SELECTOR, AVAX_FUJI_CHAIN_SELECTOR),
// },
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of taking CurseAction as input , can we take the chain selector to curse and SourceDestPair to curse, then form the CurseAction with it? would it be easier as changeset config input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understand, the chain to curse would be the CurseChain action and the source dest would be CurseLane in the current setup. Can you give an example of what your are proposing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something like this where either of ChainToCurse and SourceDestPairToCurse would be not-nil and the CurseAction will be formed accordingly based on the input.

type RMNCurseConfig struct {
	MCMS         *MCMSConfig
	CurseSubjects []CurseSubjects
}
type CurseSubjects struct{
   ChainToCurse *uint64
   SourceDestPairToCurse  *SourceDestPair
   Reason string
}

type SourceDestPair struct{
  Source uint64
  Dest uint64
}

Copy link
Contributor Author

@carte7000 carte7000 Jan 10, 2025

Choose a reason for hiding this comment

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

I think I get what you mean now, is that right that you would prefer the CurseSubjects to look more like a union rather than accepting the CurseAction list?

I guess overall it's a matter of preference, I personally like the CurseAction list because it allows us to easily compose them and define more CurseActionquite easily (like the CurseLaneOnlyOnSourceChain that I added)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer the CurseAction list, seems more flexible.

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