-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,10 @@ import ( | |
type GetContractSetsRequest struct { | ||
Chains map[uint64]deployment.Chain | ||
AddressBook deployment.AddressBook | ||
|
||
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, this seems better (though i originally opposed change the constructSet b/c i had not forseen this problem) |
||
} | ||
|
||
type GetContractSetsResponse struct { | ||
|
@@ -62,8 +66,12 @@ func GetContractSets(lggr logger.Logger, req *GetContractSetsRequest) (*GetContr | |
resp := &GetContractSetsResponse{ | ||
ContractSets: make(map[uint64]ContractSet), | ||
} | ||
var opts []func(map[string]deployment.TypeAndVersion) | ||
if req.AddressFilterer != nil { | ||
opts = append(opts, req.AddressFilterer) | ||
} | ||
for id, chain := range req.Chains { | ||
addrs, err := req.AddressBook.AddressesForChain(id) | ||
addrs, err := req.AddressBook.AddressesForChain(id, opts...) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to get addresses for chain %d: %w", id, err) | ||
} | ||
|
There was a problem hiding this comment.
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?