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

fix(deployment): enables multiple ocr3 contracts per chain #15767

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

MStreet3
Copy link
Contributor

it is possible to deploy multiple OCR contracts per chain for workflow dons yet the behavior of fetching a ContractSet from an address book is non-deterministic in this case. this PR allows AddressBook clients to pass an address filterer that will be applied to the addresses fetched for the chain prior to transforming them into a ContractSet.

modifies the keystone package to use an address filterer that removes all OCR3Capability contracts from an address book that don't match a specified address. the specified address should be passed as config to a changeset to configure OCR3.

Requires

Supports

@MStreet3 MStreet3 requested review from a team as code owners December 19, 2024 10:20
Copy link
Contributor

github-actions bot commented Dec 19, 2024

AER Report: CI Core

aer_workflow , commit , Detect Changes , Scheduled Run Frequency , Clean Go Tidy & Generate , Flakeguard Root Project / Get Tests To Run , test-scripts , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_fuzz) , Core Tests (go_core_race_tests) , GolangCI Lint (deployment) , Flakeguard Deployment Project / Get Tests To Run , Flakeguard Root Project / Run Tests , Flakeguard Root Project / Report , lint , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment, ubuntu-latest) , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/keystone/changeset, ubuntu-latest) , Flakeguard Deployment Project / Report , SonarQube Scan , Flakey Test Detection

1. Undefined functions and variables in test file: [go_core_ccip_deployment_tests]

Source of Error:
keystone/changeset/deploy_ocr3_test.go:90:9: undefined: SetupTestEnv
keystone/changeset/deploy_ocr3_test.go:90:25: undefined: TestConfig
keystone/changeset/deploy_ocr3_test.go:91:21: undefined: DonConfig
keystone/changeset/deploy_ocr3_test.go:92:21: undefined: DonConfig
keystone/changeset/deploy_ocr3_test.go:93:21: undefined: DonConfig
keystone/changeset/deploy_ocr3_test.go:157:9: undefined: SetupTestEnv
keystone/changeset/deploy_ocr3_test.go:157:25: undefined: TestConfig
keystone/changeset/deploy_ocr3_test.go:158:21: undefined: DonConfig
keystone/changeset/deploy_ocr3_test.go:159:21: undefined: DonConfig
keystone/changeset/deploy_ocr3_test.go:160:21: undefined: DonConfig
keystone/changeset/deploy_ocr3_test.go:160:21: too many errors

Why: The error is caused by missing or undefined functions and variables (SetupTestEnv, TestConfig, DonConfig) in the deploy_ocr3_test.go file. This indicates that these functions and variables are either not imported, not defined, or there is a typo in their names.

Suggested fix: Ensure that SetupTestEnv, TestConfig, and DonConfig are correctly defined and imported in the deploy_ocr3_test.go file. Verify that there are no typos in their names and that they are accessible in the test file.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@MStreet3 MStreet3 force-pushed the fix/enable-multiple-ocr-contracts-in-address-book branch 3 times, most recently from 5ef6aba to 768abae Compare December 19, 2024 11:10
@MStreet3 MStreet3 force-pushed the fix/enable-multiple-ocr-contracts-in-address-book branch from 768abae to 021dff7 Compare December 19, 2024 11:20

// AddressFilterer is a function that filters the addresses a given address book. Filtering
// mutates the input map so the input map should be a copy of the original map.
AddressFilterer func(map[string]deployment.TypeAndVersion)
Copy link
Contributor

@krehermann krehermann Dec 19, 2024

Choose a reason for hiding this comment

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

add a test for the case of multiple addresses of the same type and version and no filter

if i read the code correctly, that will cause the last one to be choosen (line#115)

instead we need to error if there are multiple instances of any contract.

that will be a backward incompatible change in the downstream repo: after you deploy an ocr3 contract all other invocations will be broken if they don't specify an input filter

that means we need to manage the filter configuration and plumb the filter func to all the existing config structs.

this seems error prone and burdensome if we end up will a handful of cor3 contracts

alternatively,
we can change the ContractSet to support a map[addr]*contract. and then all the call sites in this package that actually have use of ocr will have a config struct will non-optional addr/s

this seems better (though i originally opposed change the constructSet b/c i had not forseen this problem)

@@ -37,6 +37,7 @@ var _ deployment.ChangeSet[ConfigureOCR3Config] = ConfigureOCR3Contract
type ConfigureOCR3Config struct {
ChainSel uint64
NodeIDs []string
OCR3ContractAddr *string
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to provide the OCR contract address directly here. That's error prone, we already have the address book, why not use it. My suggestion was to provide some human-friendly config like DON name, why not do that?

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.

after a lengthy conversation, the best path forward is use an explicit don->address mapping in the down stream repo.

this implies:

  • no change to the address book
  • change the contractSet to store map[addr]*ocr3contract
  • change the ConfigureOCR func to take an explicit address
  • use that to get the contract from the set

when this is plumbed to deployment repo we need to

  • update ocr job distribution to take an explicit address
  • create a persitent mapping btw don name and ocr addr so we can fetch it naturally

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