From 1b1964b1cc18a11cd20ccb0c226586d44db74b72 Mon Sep 17 00:00:00 2001 From: Oliver Townsend <133903322+ogtownsend@users.noreply.github.com> Date: Tue, 2 Jul 2024 09:16:23 -0700 Subject: [PATCH] [LM] Fix context passing across plugin and bridge interfaces (#1081) - We were using context.Background() in most places in the bridge interfaces - This PR passes the context down from the plugin where possible --- .../liquiditymanager/bridge/arb/l1_to_l2.go | 3 +- .../liquiditymanager/bridge/arb/l2_to_l1.go | 3 +- .../plugins/liquiditymanager/bridge/bridge.go | 14 +++++-- .../bridge/mocks/bridge_factory_mock.go | 39 +++++++++++++++++-- .../bridge/opstack/l1_to_l2.go | 3 +- .../bridge/opstack/l2_to_l1.go | 3 +- .../bridge/testonlybridge/l1_to_l1.go | 2 +- .../bridge/testonlybridge/l1_to_l1_test.go | 2 +- .../ocr2/plugins/liquiditymanager/plugin.go | 6 +-- .../plugins/liquiditymanager/plugin_test.go | 7 ++-- 10 files changed, 59 insertions(+), 23 deletions(-) diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l1_to_l2.go b/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l1_to_l2.go index 072bcd4dfa..fc424d8f67 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l1_to_l2.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l1_to_l2.go @@ -56,6 +56,7 @@ type l1ToL2Bridge struct { } func NewL1ToL2Bridge( + ctx context.Context, lggr logger.Logger, localSelector, remoteSelector models.NetworkSelector, @@ -95,8 +96,6 @@ func NewL1ToL2Bridge( remoteChain.Name, "", ) - // FIXME Makram please pass the valid context - ctx := context.Background() err = l1LogPoller.RegisterFilter(ctx, logpoller.Filter{ Addresses: []common.Address{l1LiquidityManagerAddress}, Name: l1FilterName, diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l2_to_l1.go b/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l2_to_l1.go index ea06c363e0..f71a3bd40a 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l2_to_l1.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/arb/l2_to_l1.go @@ -54,6 +54,7 @@ type l2ToL1Bridge struct { } func NewL2ToL1Bridge( + ctx context.Context, lggr logger.Logger, localSelector, remoteSelector models.NetworkSelector, @@ -81,8 +82,6 @@ func NewL2ToL1Bridge( remoteChain.Name, "", ) - // FIXME Makram fix the context plax - ctx := context.Background() err := l2LogPoller.RegisterFilter( ctx, logpoller.Filter{ diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/bridge.go b/core/services/ocr2/plugins/liquiditymanager/bridge/bridge.go index c6fb25d3d1..24af25c48e 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/bridge.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/bridge.go @@ -60,7 +60,8 @@ type Bridge interface { //go:generate mockery --name Factory --output ./mocks --filename bridge_factory_mock.go --case=underscore type Factory interface { - NewBridge(source, dest models.NetworkSelector) (Bridge, error) + NewBridge(ctx context.Context, source, dest models.NetworkSelector) (Bridge, error) + GetBridge(source, dest models.NetworkSelector) (Bridge, error) } type Opt func(c *factory) @@ -106,7 +107,7 @@ func WithEvmDep( } } -func (f *factory) NewBridge(source, dest models.NetworkSelector) (Bridge, error) { +func (f *factory) NewBridge(ctx context.Context, source, dest models.NetworkSelector) (Bridge, error) { if source == dest { return nil, fmt.Errorf("no bridge between the same network and itself: %d", source) } @@ -114,12 +115,12 @@ func (f *factory) NewBridge(source, dest models.NetworkSelector) (Bridge, error) bridge, err := f.GetBridge(source, dest) if errors.Is(err, ErrBridgeNotFound) { f.lggr.Infow("Bridge not found, initializing new bridge", "source", source, "dest", dest) - return f.initBridge(source, dest) + return f.initBridge(ctx, source, dest) } return bridge, err } -func (f *factory) initBridge(source, dest models.NetworkSelector) (Bridge, error) { +func (f *factory) initBridge(ctx context.Context, source, dest models.NetworkSelector) (Bridge, error) { f.lggr.Debugw("Initializing bridge", "source", source, "dest", dest) var bridge Bridge @@ -155,6 +156,7 @@ func (f *factory) initBridge(source, dest models.NetworkSelector) (Bridge, error "l2BridgeAdapter", l2BridgeAdapter, ) bridge, err = arb.NewL2ToL1Bridge( + ctx, f.lggr, source, dest, @@ -198,6 +200,7 @@ func (f *factory) initBridge(source, dest models.NetworkSelector) (Bridge, error "l2BridgeAdapter", l2BridgeAdapter, ) bridge, err = opstack.NewL2ToL1Bridge( + ctx, f.lggr, source, dest, @@ -245,6 +248,7 @@ func (f *factory) initBridge(source, dest models.NetworkSelector) (Bridge, error "l1BridgeAdapter", l1BridgeAdapter, ) bridge, err = arb.NewL1ToL2Bridge( + ctx, f.lggr, source, dest, @@ -267,6 +271,7 @@ func (f *factory) initBridge(source, dest models.NetworkSelector) (Bridge, error "l1BridgeAdapter", l1BridgeAdapter, ) bridge, err = opstack.NewL1ToL2Bridge( + ctx, f.lggr, source, dest, @@ -310,6 +315,7 @@ func (f *factory) initBridge(source, dest models.NetworkSelector) (Bridge, error return nil, fmt.Errorf("bridge adapter not found for dest selector %d in deps for selector %d", dest, source) } bridge, err = testonlybridge.New( + ctx, source, dest, sourceDeps.liquidityManagerAddress, diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/mocks/bridge_factory_mock.go b/core/services/ocr2/plugins/liquiditymanager/bridge/mocks/bridge_factory_mock.go index ca0781b149..53f1bf86cd 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/mocks/bridge_factory_mock.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/mocks/bridge_factory_mock.go @@ -3,7 +3,10 @@ package mocks import ( + context "context" + bridge "github.com/smartcontractkit/chainlink/v2/core/services/ocr2/plugins/liquiditymanager/bridge" + mock "github.com/stretchr/testify/mock" models "github.com/smartcontractkit/chainlink/v2/core/services/ocr2/plugins/liquiditymanager/models" @@ -14,12 +17,12 @@ type Factory struct { mock.Mock } -// NewBridge provides a mock function with given fields: source, dest -func (_m *Factory) NewBridge(source models.NetworkSelector, dest models.NetworkSelector) (bridge.Bridge, error) { +// GetBridge provides a mock function with given fields: source, dest +func (_m *Factory) GetBridge(source models.NetworkSelector, dest models.NetworkSelector) (bridge.Bridge, error) { ret := _m.Called(source, dest) if len(ret) == 0 { - panic("no return value specified for NewBridge") + panic("no return value specified for GetBridge") } var r0 bridge.Bridge @@ -44,6 +47,36 @@ func (_m *Factory) NewBridge(source models.NetworkSelector, dest models.NetworkS return r0, r1 } +// NewBridge provides a mock function with given fields: ctx, source, dest +func (_m *Factory) NewBridge(ctx context.Context, source models.NetworkSelector, dest models.NetworkSelector) (bridge.Bridge, error) { + ret := _m.Called(ctx, source, dest) + + if len(ret) == 0 { + panic("no return value specified for NewBridge") + } + + var r0 bridge.Bridge + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, models.NetworkSelector, models.NetworkSelector) (bridge.Bridge, error)); ok { + return rf(ctx, source, dest) + } + if rf, ok := ret.Get(0).(func(context.Context, models.NetworkSelector, models.NetworkSelector) bridge.Bridge); ok { + r0 = rf(ctx, source, dest) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(bridge.Bridge) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, models.NetworkSelector, models.NetworkSelector) error); ok { + r1 = rf(ctx, source, dest) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // NewFactory creates a new instance of Factory. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewFactory(t interface { diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go b/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go index f80cc86634..e9cf63c98a 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go @@ -47,6 +47,7 @@ type l1ToL2Bridge struct { } func NewL1ToL2Bridge( + ctx context.Context, lggr logger.Logger, localSelector, remoteSelector models.NetworkSelector, @@ -77,8 +78,6 @@ func NewL1ToL2Bridge( "", ) - // TODO: FIXME pass valid context - ctx := context.Background() err := l1LogPoller.RegisterFilter(ctx, logpoller.Filter{ Addresses: []common.Address{l1LiquidityManagerAddress}, // emits LiquidityTransferred Name: l1FilterName, diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l2_to_l1.go b/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l2_to_l1.go index ce354593a7..ea04652e31 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l2_to_l1.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l2_to_l1.go @@ -41,6 +41,7 @@ type l2ToL1Bridge struct { } func NewL2ToL1Bridge( + ctx context.Context, lggr logger.Logger, localSelector, remoteSelector models.NetworkSelector, @@ -68,8 +69,6 @@ func NewL2ToL1Bridge( remoteChain.Name, "", ) - // TODO (ogtownsend): pass context from above - ctx := context.Background() err := l2LogPoller.RegisterFilter( ctx, logpoller.Filter{ diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1.go b/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1.go index 7bed294bbb..1ee8096df2 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1.go @@ -51,13 +51,13 @@ type testBridge struct { } func New( + ctx context.Context, sourceSelector, destSelector models.NetworkSelector, sourceLiquidityManagerAddress, destLiquidityManagerAddress, sourceAdapter, destAdapter models.Address, sourceLogPoller, destLogPoller logpoller.LogPoller, sourceClient, destClient client.Client, lggr logger.Logger, ) (*testBridge, error) { - ctx := context.Background() err := sourceLogPoller.RegisterFilter( ctx, logpoller.Filter{ diff --git a/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1_test.go b/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1_test.go index af0c5b002d..24cc849e8e 100644 --- a/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1_test.go +++ b/core/services/ocr2/plugins/liquiditymanager/bridge/testonlybridge/l1_to_l1_test.go @@ -728,7 +728,7 @@ func TestNew(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tt.expect(t, tt.args) defer tt.assert(t, tt.args) - got, err := New(tt.args.sourceSelector, tt.args.destSelector, tt.args.sourceLiquidityManagerAddress, tt.args.destLiquidityManagerAddress, tt.args.sourceAdapter, tt.args.destAdapter, tt.args.sourceLogPoller, tt.args.destLogPoller, tt.args.sourceClient, tt.args.destClient, tt.args.lggr) + got, err := New(testutils.Context(t), tt.args.sourceSelector, tt.args.destSelector, tt.args.sourceLiquidityManagerAddress, tt.args.destLiquidityManagerAddress, tt.args.sourceAdapter, tt.args.destAdapter, tt.args.sourceLogPoller, tt.args.destLogPoller, tt.args.sourceClient, tt.args.destClient, tt.args.lggr) if tt.wantErr { require.Error(t, err) } else { diff --git a/core/services/ocr2/plugins/liquiditymanager/plugin.go b/core/services/ocr2/plugins/liquiditymanager/plugin.go index e5b0d37ec3..9917196061 100644 --- a/core/services/ocr2/plugins/liquiditymanager/plugin.go +++ b/core/services/ocr2/plugins/liquiditymanager/plugin.go @@ -507,7 +507,7 @@ func (p *Plugin) loadPendingTransfers(ctx context.Context, lggr logger.Logger) ( } for _, edge := range edges { logger := lggr.With("sourceNetwork", edge.Source, "sourceChainID", edge.Source.ChainID(), "destNetwork", edge.Dest, "destChainID", edge.Dest.ChainID()) - bridge, err := p.bridgeFactory.NewBridge(edge.Source, edge.Dest) + bridge, err := p.bridgeFactory.NewBridge(ctx, edge.Source, edge.Dest) if err != nil { return nil, fmt.Errorf("init bridge: %w", err) } @@ -607,7 +607,7 @@ func (p *Plugin) computeResolvedTransfersQuorum(observations []models.Observatio } medianizedNativeFee := rebalcalc.BigIntSortedMiddle(bridgeFees) medianizedDateUnix := rebalcalc.BigIntSortedMiddle(datesUnix) - bridge, err := p.bridgeFactory.NewBridge(k.From, k.To) + bridge, err := p.bridgeFactory.GetBridge(k.From, k.To) if err != nil { return nil, fmt.Errorf("init bridge: %w", err) } @@ -650,7 +650,7 @@ func (p *Plugin) resolveProposedTransfers(ctx context.Context, lggr logger.Logge resolvedTransfers := make([]models.Transfer, 0, len(outcome.ProposedTransfers)) for _, proposedTransfer := range outcome.ProposedTransfers { - bridge, err := p.bridgeFactory.NewBridge(proposedTransfer.From, proposedTransfer.To) + bridge, err := p.bridgeFactory.NewBridge(ctx, proposedTransfer.From, proposedTransfer.To) if err != nil { return nil, fmt.Errorf("init bridge: %w", err) } diff --git a/core/services/ocr2/plugins/liquiditymanager/plugin_test.go b/core/services/ocr2/plugins/liquiditymanager/plugin_test.go index cb3af9d08f..aeb0501083 100644 --- a/core/services/ocr2/plugins/liquiditymanager/plugin_test.go +++ b/core/services/ocr2/plugins/liquiditymanager/plugin_test.go @@ -289,7 +289,7 @@ func TestPlugin_Observation(t *testing.T) { for sourceDest, bridgeFn := range tc.bridges { br, err2 := bridgeFn(t) p.bridgeFactory. - On("NewBridge", sourceDest[0], sourceDest[1]). + On("NewBridge", ctx, sourceDest[0], sourceDest[1]). Return(br, err2) } @@ -660,7 +660,7 @@ func TestPlugin_Outcome(t *testing.T) { for sourceDest, bridgeFn := range tc.bridges { br, err := bridgeFn(t) p.bridgeFactory. - On("NewBridge", sourceDest[0], sourceDest[1]). + On("GetBridge", sourceDest[0], sourceDest[1]). Return(br, err) } @@ -1135,7 +1135,8 @@ func TestPlugin_E2EWithMocks(t *testing.T) { for _, edge := range edges { br, ok := n.bridges[[2]models.NetworkSelector{edge.Source, edge.Dest}] require.True(t, ok, "the test case is wrong, bridge is not defined %d->%d", edge.Source, edge.Dest) - n.bridgeFactory.On("NewBridge", edge.Source, edge.Dest).Return(br, nil).Maybe() + n.bridgeFactory.On("NewBridge", mock.Anything /* cancelContext */, edge.Source, edge.Dest).Return(br, nil).Maybe() + n.bridgeFactory.On("GetBridge", edge.Source, edge.Dest).Return(br, nil).Maybe() pendingTransfers := make([]models.PendingTransfer, 0) for _, tr := range round.pendingTransfersPerNode[i] {