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

[TT-1847] ocr3 keystone por test broken #15948

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

Conversation

Tofel
Copy link
Contributor

@Tofel Tofel commented Jan 16, 2025

Requires

Supports

@Tofel Tofel requested review from a team as code owners January 16, 2025 12:38
@Tofel Tofel requested a review from krehermann January 16, 2025 12:38
Copy link
Contributor

I see you updated files related to contracts. Please run pnpm changeset in the contracts directory to add a changeset.

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Static analysis results are available

Hey @Tofel, you can view Slither reports in the job summary here or download them as artifact here.
Please check them before merging and make sure you have addressed all issues.

Copy link
Contributor

github-actions bot commented Jan 16, 2025

AER Report: CI Core

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

1. Linting errors in Go code: Golang Lint (integration-tests)

Source of Error:
##[error]integration-tests/capabilities/components/onchain/fund.go:32:10: fmt.Errorf can be replaced with errors.New (perfsprint)
		return fmt.Errorf("error casting public key to ECDSA")
		 ^
##[error]integration-tests/capabilities/components/onchain/fund.go:71:35: fmt.Sprint can be replaced with faster strconv.FormatUint (perfsprint)
		ek, err := cl.ReadPrimaryETHKey(fmt.Sprint(sc.Cfg.Network.ChainID))
		 ^
##[error]integration-tests/capabilities/components/evmcontracts/forwarder/forwarder.go:15:6: exported: type name will be used as forwarder.ForwarderInstance by other packages, and that stutters; consider calling this Instance (revive)
type ForwarderInstance struct {
^
##[error]integration-tests/capabilities/components/evmcontracts/capabilities_registry/capabilities_registry.go:1:9: var-naming: don't use an underscore in package name (revive)
package capabilities_registry
^
##[error]integration-tests/capabilities/workflow_test.go:372:29: var-naming: don't use underscores in Go names; var workflow_registryInstance should be workflowRegistryInstance (revive)
		workflowRegistryAddr, tx, workflow_registryInstance, err := workflow_registry.DeployWorkflowRegistry(sc.NewTXOpts(), sc.Client)
		 ^
##[error]integration-tests/capabilities/workflow_test.go:298:25: unnecessary conversion (unconvert)
	_, err = s.Write([]byte(config))
	 ^

Why: The errors are caused by violations of Go linting rules, such as using fmt.Errorf instead of errors.New, using fmt.Sprint instead of strconv.FormatUint, improper naming conventions, and unnecessary type conversions.

Suggested fix: Update the code to comply with the linting rules. Replace fmt.Errorf with errors.New, fmt.Sprint with strconv.FormatUint, correct naming conventions, and remove unnecessary type conversions.

2. Go generate command failed: make generate

Source of Error:
make generate	2025-01-16T16:34:04.2054824Z error: integration-tests/load: exit status 1
make generate	2025-01-16T16:34:04.2055189Z make: CHANGELOG.md GNUmakefile LICENSE README.md SECURITY.md ccip common config_docs_test.go contracts core dashboard-lib deployment docs error_reporter_actions flake.lock flake.nix fuzz go.md go.mod go.sum integration-tests internal main.go main_test.go nix-darwin-shell-hook.sh nix.conf operator_ui package.json plugins pnpm-lock.yaml rawlog.log runlog.log shell.nix sonar-project.properties testdata tools [GNUmakefile:100: generate] Error 1

Why: The make generate command failed due to an error in the integration-tests/load directory, which caused the process to exit with status 1.

Suggested fix: Investigate the specific error in the integration-tests/load directory and resolve it. Ensure all dependencies are correctly downloaded and the go generate command runs successfully.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@@ -101,6 +103,27 @@ contract KeystoneFeedsConsumer is IReceiver, OwnerIsCreator {
return (report.Price, report.Timestamp);
}

function getAllFeeds() external view returns (bytes32[] memory, uint224[] memory, uint32[] memory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the contract changing? implies many downstream changes to deployments and is unreasonable if the purpose of this PR is to fix a test, which is my understanding give the (limited) name and no description

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 is not a real PR, I created it share the code with other people. I added that method to figure out why I couldn't fetch the price, even though feedId seemed correct and even though the report was correctly sent to KeeperConsumer

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

please don't commit binary files

ExistingHashedCapabilitiesIDs [][32]byte
}

func Deploy(sc *seth.Client) (*CapabilitiesRegistryInstance, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we writing this code?

we have logic in chainlink/deployment/keystone to deploy contracts and manage them

}, nil
}

func (cr *CapabilitiesRegistryInstance) AddCapabilities(capabilities []cr.CapabilitiesRegistryCapability) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

please reuse the existing code or enhance or come up with a design were we can reuse and leverage existing work

Copy link
Contributor

@krehermann krehermann left a comment

Choose a reason for hiding this comment

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

I don't understand this change.

It seems to be re-inventing the wheel relate to work in chainlink/deployments.

And it seems to be mixing concerns with by changing contracts to fix tests rather than the other way around

@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
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

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.

2 participants