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.

Sign up for GitHub

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

Allow only price aggregator contract to have multiple bindings [CCIP-4781] #426

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Allow only price aggregator contract to have multiple bindings [CCIP-4781] #426

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5913ebc
Allow only price aggregator contract to have multiple bindings, other…
asoliman92 Jan 9, 2025
8503f84
linting
asoliman92 Jan 9, 2025
da9d2be
unbinding old contract in contract reader when overriding it in exten…
asoliman92 Jan 9, 2025
6e0710f
inlining multiBindAllowed field
asoliman92 Jan 9, 2025
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
28 changes: 23 additions & 5 deletions pkg/contractreader/extended.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"sync"
"time"

"github.com/smartcontractkit/chainlink-ccip/pkg/consts"

"github.com/smartcontractkit/chainlink-common/pkg/services"
"github.com/smartcontractkit/chainlink-common/pkg/types"
clcommontypes "github.com/smartcontractkit/chainlink-common/pkg/types"
Expand Down Expand Up @@ -95,17 +97,25 @@ type ExtendedBoundContract struct {
type extendedContractReader struct {
reader ContractReaderFacade
contractBindingsByName map[string][]ExtendedBoundContract
mu *sync.RWMutex
// contract names that allow multiple bindings
multiBindAllowed map[string]bool
mu *sync.RWMutex
}

func NewExtendedContractReader(baseContractReader ContractReaderFacade) Extended {
// avoid double wrapping
if ecr, ok := baseContractReader.(Extended); ok {
return ecr
}
// so far this is the only contract that allows multiple bindings
// if more contracts are added, this should be moved to a config
multiBindAllowed := map[string]bool{
consts.ContractNamePriceAggregator: true,
}
asoliman92 marked this conversation as resolved.
Show resolved Hide resolved
return &extendedContractReader{
reader: baseContractReader,
contractBindingsByName: make(map[string][]ExtendedBoundContract),
multiBindAllowed: multiBindAllowed,
asoliman92 marked this conversation as resolved.
Show resolved Hide resolved
asoliman92 marked this conversation as resolved.
Show resolved Hide resolved
mu: &sync.RWMutex{},
}
}
Expand Down Expand Up @@ -261,10 +271,18 @@ func (e *extendedContractReader) Bind(ctx context.Context, allBindings []types.B
e.mu.Lock()
defer e.mu.Unlock()
for _, binding := range validBindings {
e.contractBindingsByName[binding.Name] = append(e.contractBindingsByName[binding.Name], ExtendedBoundContract{
BoundAt: time.Now(),
Binding: binding,
})
if e.multiBindAllowed[binding.Name] {
e.contractBindingsByName[binding.Name] = append(e.contractBindingsByName[binding.Name], ExtendedBoundContract{
BoundAt: time.Now(),
Binding: binding,
})
Comment on lines +271 to +275
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: I would be tempted to rewrite this to avoid the else. Something like

    // unbind previous contract if needed
    if len(e.contractBindingsByName[binding.Name]) > 0 && e.multiBindAllowed[binding.Name] {
        e.reader.Unbind(...)
        e.contractBindingsByName[binding.Name] = nil
    }

    // existing logic here
}

} else {
// Override the previous binding
asoliman92 marked this conversation as resolved.
Show resolved Hide resolved
e.contractBindingsByName[binding.Name] = []ExtendedBoundContract{{
BoundAt: time.Now(),
Binding: binding,
}}
}
}

return nil
Expand Down
36 changes: 36 additions & 0 deletions pkg/contractreader/extended_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"
"testing"

"github.com/smartcontractkit/chainlink-ccip/pkg/consts"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -47,6 +49,40 @@ func TestExtendedContractReader(t *testing.T) {
err = extCr.Bind(context.Background(), []types.BoundContract{{Name: contractName, Address: "0x125"}})
assert.Error(t, err)

bindings = extCr.GetBindings(contractName)
assert.Len(t, bindings, 1)
assert.Equal(t, "0x124", bindings[0].Binding.Address)
}

func TestExtendedContractReader_AllowMultiBindingForAggregator(t *testing.T) {
const contractName = consts.ContractNamePriceAggregator
cr := chainreadermocks.NewMockContractReaderFacade(t)
extCr := contractreader.NewExtendedContractReader(cr)

bindings := extCr.GetBindings(contractName)
assert.Len(t, bindings, 0)

cr.On("Bind", context.Background(),
[]types.BoundContract{{Name: contractName, Address: "0x123"}}).Return(nil)
cr.On("Bind", context.Background(),
[]types.BoundContract{{Name: contractName, Address: "0x124"}}).Return(nil)
cr.On("Bind", context.Background(),
[]types.BoundContract{{Name: contractName, Address: "0x125"}}).Return(fmt.Errorf("some err"))

err := extCr.Bind(context.Background(), []types.BoundContract{{Name: contractName, Address: "0x123"}})
assert.NoError(t, err)

// ignored since 0x123 already exists
err = extCr.Bind(context.Background(), []types.BoundContract{{Name: contractName, Address: "0x123"}})
assert.NoError(t, err)

err = extCr.Bind(context.Background(), []types.BoundContract{{Name: contractName, Address: "0x124"}})
assert.NoError(t, err)

// Bind fails
err = extCr.Bind(context.Background(), []types.BoundContract{{Name: contractName, Address: "0x125"}})
assert.Error(t, err)

bindings = extCr.GetBindings(contractName)
assert.Len(t, bindings, 2)
assert.Equal(t, "0x123", bindings[0].Binding.Address)
Expand Down
Loading