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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions deployment/address_book.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func NewTypeAndVersion(t ContractType, v semver.Version) TypeAndVersion {
type AddressBook interface {
Save(chainSelector uint64, address string, tv TypeAndVersion) error
Addresses() (map[uint64]map[string]TypeAndVersion, error)
AddressesForChain(chain uint64) (map[string]TypeAndVersion, error)
AddressesForChain(chain uint64, opts ...func(map[string]TypeAndVersion)) (map[string]TypeAndVersion, error)
// Allows for merging address books (e.g. new deployments with existing ones)
Merge(other AddressBook) error
Remove(ab AddressBook) error
Expand Down Expand Up @@ -149,7 +149,7 @@ func (m *AddressBookMap) Addresses() (map[uint64]map[string]TypeAndVersion, erro
return m.cloneAddresses(m.addressesByChain), nil
}

func (m *AddressBookMap) AddressesForChain(chainSelector uint64) (map[string]TypeAndVersion, error) {
func (m *AddressBookMap) AddressesForChain(chainSelector uint64, opts ...func(map[string]TypeAndVersion)) (map[string]TypeAndVersion, error) {
_, err := chainsel.GetChainIDFromSelector(chainSelector)
if err != nil {
return nil, errors.Wrapf(ErrInvalidChainSelector, "chain selector %d", chainSelector)
Expand All @@ -165,7 +165,11 @@ func (m *AddressBookMap) AddressesForChain(chainSelector uint64) (map[string]Typ
// maps are mutable and pass via a pointer
// creating a copy of the map to prevent concurrency
// read and changes outside object-bound
return maps.Clone(m.addressesByChain[chainSelector]), nil
cloned := maps.Clone(m.addressesByChain[chainSelector])
for _, opt := range opts {
opt(cloned)
}
return cloned, nil
}

// Merge will merge the addresses from another address book into this one.
Expand Down
29 changes: 29 additions & 0 deletions deployment/address_book_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,35 @@ func TestAddressBook_Save(t *testing.T) {
})
}

func TestAddressBook_AddressesForChain(t *testing.T) {
ab := NewMemoryAddressBook()
ocr3Cap100 := NewTypeAndVersion("OCR3Capability", Version1_0_0)
copyOCR3Cap100 := NewTypeAndVersion("OCR3Capability", Version1_0_0)

addr1 := common.HexToAddress("0x1").String()
addr2 := common.HexToAddress("0x2").String()

err := ab.Save(chainsel.TEST_90000001.Selector, addr1, ocr3Cap100)
require.NoError(t, err)

err = ab.Save(chainsel.TEST_90000001.Selector, addr2, copyOCR3Cap100)
require.NoError(t, err)

addresses, err := ab.AddressesForChain(chainsel.TEST_90000001.Selector,
func(m map[string]TypeAndVersion) {
for k, v := range m {
if v.Type == "OCR3Capability" {
if k != addr1 {
delete(m, k)
}
}
}
},
)
require.NoError(t, err)
require.Len(t, addresses, 1)
}

func TestAddressBook_Merge(t *testing.T) {
onRamp100 := NewTypeAndVersion("OnRamp", Version1_0_0)
onRamp110 := NewTypeAndVersion("OnRamp", Version1_1_0)
Expand Down
2 changes: 2 additions & 0 deletions deployment/keystone/changeset/deploy_ocr3.go
Original file line number Diff line number Diff line change
Expand Up @@ -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?

OCR3Config *kslib.OracleConfig
DryRun bool
WriteGeneratedConfig io.Writer // if not nil, write the generated config to this writer as JSON [OCR2OracleConfig]
Expand All @@ -55,6 +56,7 @@ func ConfigureOCR3Contract(env deployment.Environment, cfg ConfigureOCR3Config)
NodeIDs: cfg.NodeIDs,
OCR3Config: cfg.OCR3Config,
DryRun: cfg.DryRun,
OCR3Addr: cfg.OCR3ContractAddr,
UseMCMS: cfg.UseMCMS(),
})
if err != nil {
Expand Down
117 changes: 116 additions & 1 deletion deployment/keystone/changeset/deploy_ocr3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,122 @@ func TestConfigureOCR3(t *testing.T) {
assert.Nil(t, csOut.Proposals)
})

t.Run("success multiple OCR3 contracts", func(t *testing.T) {

te := SetupTestEnv(t, TestConfig{
WFDonConfig: DonConfig{N: 4},
AssetDonConfig: DonConfig{N: 4},
WriterDonConfig: DonConfig{N: 4},
NumChains: 1,
})

registrySel := te.Env.AllChainSelectors()[0]

existingContracts, err := te.Env.ExistingAddresses.AddressesForChain(registrySel)
require.NoError(t, err)
require.Len(t, existingContracts, 3)

// Find existing OCR3 contract
var existingOCR3Addr string
for addr, tv := range existingContracts {
if tv.Type == kslib.OCR3Capability {
existingOCR3Addr = addr
break
}
}

// Deploy a new OCR3 contract
resp, err := changeset.DeployOCR3(te.Env, registrySel)
require.NoError(t, err)
require.NotNil(t, resp)
require.NoError(t, te.Env.ExistingAddresses.Merge(resp.AddressBook))

// Verify after merge there are three original contracts plus one new one
addrs, err := te.Env.ExistingAddresses.AddressesForChain(registrySel)
require.NoError(t, err)
require.Len(t, addrs, 4)

// Find new OCR3 contract
var newOCR3Addr string
for addr, tv := range addrs {
if tv.Type == kslib.OCR3Capability && addr != existingOCR3Addr {
newOCR3Addr = addr
break
}
}

var wfNodes []string
for id, _ := range te.WFNodes {
wfNodes = append(wfNodes, id)
}

w := &bytes.Buffer{}
cfg := changeset.ConfigureOCR3Config{
ChainSel: te.RegistrySelector,
NodeIDs: wfNodes,
OCR3ContractAddr: &newOCR3Addr, // Use the new OCR3 contract to configure
OCR3Config: &c,
WriteGeneratedConfig: w,
}

csOut, err := changeset.ConfigureOCR3Contract(te.Env, cfg)
require.NoError(t, err)
var got kslib.OCR2OracleConfig
err = json.Unmarshal(w.Bytes(), &got)
require.NoError(t, err)
assert.Len(t, got.Signers, 4)
assert.Len(t, got.Transmitters, 4)
assert.Nil(t, csOut.Proposals)
})

t.Run("fails to configure with multiple OCR3 contracts because not found", func(t *testing.T) {
te := SetupTestEnv(t, TestConfig{
WFDonConfig: DonConfig{N: 4},
AssetDonConfig: DonConfig{N: 4},
WriterDonConfig: DonConfig{N: 4},
NumChains: 1,
})

registrySel := te.Env.AllChainSelectors()[0]

existingContracts, err := te.Env.ExistingAddresses.AddressesForChain(registrySel)
require.NoError(t, err)
require.Len(t, existingContracts, 3)

// Deploy a new OCR3 contract
resp, err := changeset.DeployOCR3(te.Env, registrySel)
require.NoError(t, err)
require.NotNil(t, resp)
require.NoError(t, te.Env.ExistingAddresses.Merge(resp.AddressBook))

// Verify after merge there are three original contracts plus one new one
addrs, err := te.Env.ExistingAddresses.AddressesForChain(registrySel)
require.NoError(t, err)
require.Len(t, addrs, 4)

// Specify a non-existent OCR3 contract, means all deployed contracts will be filtered
// out and the configuration will fail
notFoundAddr := "0x1234567890123456789012345678901234567890"

var wfNodes []string
for id, _ := range te.WFNodes {
wfNodes = append(wfNodes, id)
}

w := &bytes.Buffer{}
cfg := changeset.ConfigureOCR3Config{
ChainSel: te.RegistrySelector,
NodeIDs: wfNodes,
OCR3ContractAddr: &notFoundAddr,
OCR3Config: &c,
WriteGeneratedConfig: w,
}

_, err = changeset.ConfigureOCR3Contract(te.Env, cfg)
require.Error(t, err)
require.ErrorContains(t, err, "no ocr3 contract found")
})

t.Run("mcms", func(t *testing.T) {
te := SetupTestEnv(t, TestConfig{
WFDonConfig: DonConfig{N: 4},
Expand Down Expand Up @@ -137,5 +253,4 @@ func TestConfigureOCR3(t *testing.T) {
})
require.NoError(t, err)
})

}
43 changes: 40 additions & 3 deletions deployment/keystone/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,11 @@ type ConfigureOCR3Config struct {
ChainSel uint64
NodeIDs []string
OCR3Config *OracleConfig
DryRun bool

// OCR3Addr is the address of the OCR3 contract to configure. If nil, and there are more than
// one deployed contracts on the chain, then the configured contract is non-deterministic.
OCR3Addr *string
DryRun bool

UseMCMS bool
}
Expand All @@ -367,9 +371,16 @@ func ConfigureOCR3ContractFromJD(env *deployment.Environment, cfg ConfigureOCR3C
if !ok {
return nil, fmt.Errorf("chain %d not found in environment", cfg.ChainSel)
}

filterFunc := identityFilterer
if cfg.OCR3Addr != nil {
filterFunc = makeOCR3CapabilityFilterer(*cfg.OCR3Addr)
}

contractSetsResp, err := GetContractSets(env.Logger, &GetContractSetsRequest{
Chains: env.Chains,
AddressBook: env.ExistingAddresses,
Chains: env.Chains,
AddressBook: env.ExistingAddresses,
AddressFilterer: filterFunc,
})
if err != nil {
return nil, fmt.Errorf("failed to get contract sets: %w", err)
Expand All @@ -382,6 +393,13 @@ func ConfigureOCR3ContractFromJD(env *deployment.Environment, cfg ConfigureOCR3C
if contract == nil {
return nil, fmt.Errorf("no ocr3 contract found for chain %d", cfg.ChainSel)
}

if cfg.OCR3Addr != nil {
if *cfg.OCR3Addr != contract.Address().String() {
return nil, fmt.Errorf("Requested address to configure %s does not match contract address %s", *cfg.OCR3Addr, contract.Address().String())
}
}

nodes, err := deployment.NodeInfo(cfg.NodeIDs, env.Offchain)
if err != nil {
return nil, err
Expand Down Expand Up @@ -971,3 +989,22 @@ func configureForwarder(lggr logger.Logger, chain deployment.Chain, contractSet
}
return opMap, nil
}

// makeOCR3CapabilityFilterer returns a filter func that deletes any OCR3Capability contract entries
// that do not match a given OCR3 Address. If no address is given, no filtering is done.
func makeOCR3CapabilityFilterer(ocr3Addr string) func(map[string]deployment.TypeAndVersion) {
return func(m map[string]deployment.TypeAndVersion) {
for k, v := range m {
if v.Type == OCR3Capability {
if k != ocr3Addr {
delete(m, k)
}
}
}
}
}

// identityFilterer is a filter func that does nothing
func identityFilterer(m map[string]deployment.TypeAndVersion) {
// no-op
}
10 changes: 9 additions & 1 deletion deployment/keystone/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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)

}

type GetContractSetsResponse struct {
Expand Down Expand Up @@ -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)
}
Expand Down
Loading