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

versioned wrappers #15785

Merged
merged 1 commit into from
Dec 20, 2024
Merged

versioned wrappers #15785

merged 1 commit into from
Dec 20, 2024

Conversation

krehermann
Copy link
Contributor

@krehermann krehermann commented Dec 20, 2024

KS-492

versioned contract wrappers to support production

copies the existing wrappers and updates all imports to the new, fixed version

Requires

Supports

downstream

Copy link
Contributor

github-actions bot commented Dec 20, 2024

AER Report: CI Core

aer_workflow , commit , Detect Changes , Scheduled Run Frequency , Clean Go Tidy & Generate , Flakeguard Root Project / Get Tests To Run , GolangCI Lint (core/scripts) , GolangCI Lint (.) , 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) , Flakeguard Deployment Project / Get Tests To Run , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/services/relay/evm, ubuntu-latest) , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/capabilities/ccip/common, ubuntu-latest) , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/capabilities/ccip/launcher, ubuntu-latest) , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/capabilities/integration_tests/keystone,... , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/capabilities, ubuntu-latest) , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/services/registrysyncer, ubuntu-latest) , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/keystone/changeset, ubuntu-latest) , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/keystone/changeset/internal, ubuntu-l... , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/ccip/view/v1_6, ubuntu-latest) , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/common/view/v1_0, ubuntu-latest) , lint , Flakeguard Root Project / Report , Flakeguard Deployment Project / Report , SonarQube Scan , Flakey Test Detection

1. File is not goimports-ed with -local github.com/smartcontractkit/chainlink:[Golang Lint (deployment)]

Source of Error:
deployment/ccip/changeset/internal/deploy_home_chain.go:22: File is not `goimports`-ed with -local github.com/smartcontractkit/chainlink (goimports)
	"github.com/smartcontractkit/chainlink/v2/core/gethwrappers/keystone/generated/capabilities_registry_1_1_0"
deployment/common/view/v1_0/capreg.go:14: File is not `goimports`-ed with -local github.com/smartcontractkit/chainlink (goimports)
	"github.com/smartcontractkit/chainlink/v2/core/gethwrappers/keystone/generated/capabilities_registry_1_1_0"
deployment/keystone/changeset/internal/test/utils.go:22: File is not `goimports`-ed with -local github.com/smartcontractkit/chainlink (goimports)
	"github.com/smartcontractkit/chainlink/v2/core/gethwrappers/keystone/generated/capabilities_registry_1_1_0"
	kcr "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/keystone/generated/capabilities_registry_1_1_0"
**Why**: The files are not formatted according to the `goimports` tool with the `-local` flag set to `github.com/smartcontractkit/chainlink`.

Suggested fix: Run goimports -local github.com/smartcontractkit/chainlink -w <file> on the affected files to format them correctly.

2. Consider pre-allocating capabilities:[Golang Lint (deployment)]

Source of Error:
deployment/keystone/deploy.go:480:2: Consider pre-allocating `capabilities` (prealloc)
	var capabilities []capabilities_registry.CapabilitiesRegistryCapability
	^
**Why**: The linter suggests pre-allocating the slice `capabilities` to improve performance by reducing the number of allocations.

Suggested fix: Pre-allocate the slice with an appropriate capacity using make.

3. Consider pre-allocating uniqueNodeParams:[Golang Lint (deployment)]

Source of Error:
deployment/keystone/deploy.go:723:2: Consider pre-allocating `uniqueNodeParams` (prealloc)
	var uniqueNodeParams []capabilities_registry.CapabilitiesRegistryNodeParams
	^
**Why**: The linter suggests pre-allocating the slice `uniqueNodeParams` to improve performance by reducing the number of allocations.

Suggested fix: Pre-allocate the slice with an appropriate capacity using make.

4. var-naming: func parameter p2pIdsToDon should be p2pIDsToDon:[Golang Lint (deployment)]

Source of Error:
deployment/keystone/deploy.go:913:84: var-naming: func parameter p2pIdsToDon should be p2pIDsToDon (revive)
func containsAllDONs(donInfos []capabilities_registry.CapabilitiesRegistryDONInfo, p2pIdsToDon map[string]string) bool {
^
**Why**: The linter suggests that the function parameter `p2pIdsToDon` should follow the camelCase naming convention and be renamed to `p2pIDsToDon`.

Suggested fix: Rename the function parameter to p2pIDsToDon.

5. redefines-builtin-id: redefinition of the built-in function cap:[Golang Lint (deployment)]

Source of Error:
deployment/keystone/deploy.go:423:41: redefines-builtin-id: redefinition of the built-in function cap (revive)
func FromCapabilitiesRegistryCapability(cap *capabilities_registry.CapabilitiesRegistryCapability, e deployment.Environment, registryChainSelector uint64) (*RegisteredCapability, error) {
^
**Why**: The linter suggests that the identifier `cap` redefines the built-in function `cap`, which can lead to confusion.

Suggested fix: Rename the parameter cap to avoid redefining the built-in function.

6. Failed to CreateArtifact: Received non-retryable error:[Upload All Test Results as Artifact]

Source of Error:
Upload All Test Results as Artifact	2024-12-20T01:46:17.0395742Z ##[error]Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run
**Why**: The error occurs because an artifact with the name `all-test-results.json` already exists in the workflow run, causing a conflict.

Suggested fix: Change the artifact name to something unique or ensure that the existing artifact is deleted before attempting to create a new one.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@krehermann krehermann force-pushed the ks-492/freeze-ks-wrappers branch from 3620990 to 459df84 Compare December 20, 2024 01:36
@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
7 New Major Issues (required ≤ 5)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

Copy link
Contributor

@bolekk bolekk left a comment

Choose a reason for hiding this comment

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

bunch of lint warnings

@@ -33,7 +33,7 @@ import (
"github.com/smartcontractkit/chainlink/v2/core/chains/evm/client"
"github.com/smartcontractkit/chainlink/v2/core/chains/evm/headtracker"
"github.com/smartcontractkit/chainlink/v2/core/chains/evm/logpoller"
kcr "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/keystone/generated/capabilities_registry"
kcr "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/keystone/generated/capabilities_registry_1_1_0"
Copy link
Contributor

Choose a reason for hiding this comment

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

These package names are kind of unfortunate. Even if we didn't care to follow these rules exactly, the aliased usages are a smell. I suppose the semver makes it weird, but what about shortening capabilities_registry to capreg? And typically with go you would have capreg/v<XYZ> 🤷

Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns

https://go.dev/blog/package-names#package-names

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's generated. i don't think i have control over it

@krehermann krehermann added this pull request to the merge queue Dec 20, 2024
Merged via the queue into develop with commit cb36e64 Dec 20, 2024
194 of 199 checks passed
@krehermann krehermann deleted the ks-492/freeze-ks-wrappers branch December 20, 2024 13:29
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