fix: Nits and retry client

op-program/host/host.go

@@ -27,7 +27,7 @@ import (
 	"github.com/ethereum/go-ethereum/log"
 )
 
-const maxAttempts = math.MaxInt // Succeed or die trying
+const maxRPCRetries = math.MaxInt
 
 type L2Source struct {
 	*sources.L2Client
@@ -203,8 +203,8 @@ func makePrefetcher(ctx context.Context, logger log.Logger, kv kvstore.KV, cfg *
 		return nil, fmt.Errorf("failed to setup L2 RPC: %w", err)
 	}
 
-	l1Backoff := opclient.NewBackoffClient(l1RPC, maxAttempts)
-	l2Backoff := opclient.NewBackoffClient(l2RPC, maxAttempts)
+	l1Backoff := opclient.NewRetryingClient(l1RPC, maxRPCRetries)
+	l2Backoff := opclient.NewRetryingClient(l2RPC, maxRPCRetries)
 
 	l1ClCfg := sources.L1ClientDefaultConfig(cfg.Rollup, cfg.L1TrustRPC, cfg.L1RPCKind)
 	l1Cl, err := sources.NewL1Client(l1Backoff, logger, nil, l1ClCfg)

op-service/client/mocks/InnerRPC.go

-// Code generated by mockery v2.28.1. DO NOT EDIT.
-
-package mocks
-
-import (
-	context "context"
-
-	ethereum "github.com/ethereum/go-ethereum"
-	mock "github.com/stretchr/testify/mock"
-
-	rpc "github.com/ethereum/go-ethereum/rpc"
-)
-
-// InnerRPC is an autogenerated mock type for the InnerRPC type
-type InnerRPC struct {
-	mock.Mock
-}
-
-// BatchCallContext provides a mock function with given fields: ctx, b
-func (_m *InnerRPC) BatchCallContext(ctx context.Context, b []rpc.BatchElem) error {
-	ret := _m.Called(ctx, b)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(context.Context, []rpc.BatchElem) error); ok {
-		r0 = rf(ctx, b)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
-}
-
-// CallContext provides a mock function with given fields: ctx, result, method, args
-func (_m *InnerRPC) CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error {
-	var _ca []interface{}
-	_ca = append(_ca, ctx, result, method)
-	_ca = append(_ca, args...)
-	ret := _m.Called(_ca...)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(context.Context, interface{}, string, ...interface{}) error); ok {
-		r0 = rf(ctx, result, method, args...)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
-}
-
-// Close provides a mock function with given fields:
-func (_m *InnerRPC) Close() {
-	_m.Called()
-}
-
-// EthSubscribe provides a mock function with given fields: ctx, channel, args
-func (_m *InnerRPC) EthSubscribe(ctx context.Context, channel interface{}, args ...interface{}) (ethereum.Subscription, error) {
-	var _ca []interface{}
-	_ca = append(_ca, ctx, channel)
-	_ca = append(_ca, args...)
-	ret := _m.Called(_ca...)
-
-	var r0 ethereum.Subscription
-	var r1 error
-	if rf, ok := ret.Get(0).(func(context.Context, interface{}, ...interface{}) (ethereum.Subscription, error)); ok {
-		return rf(ctx, channel, args...)
-	}
-	if rf, ok := ret.Get(0).(func(context.Context, interface{}, ...interface{}) ethereum.Subscription); ok {
-		r0 = rf(ctx, channel, args...)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(ethereum.Subscription)
-		}
-	}
-
-	if rf, ok := ret.Get(1).(func(context.Context, interface{}, ...interface{}) error); ok {
-		r1 = rf(ctx, channel, args...)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-type mockConstructorTestingTNewInnerRPC interface {
-	mock.TestingT
-	Cleanup(func())
-}
-
-// NewInnerRPC creates a new instance of InnerRPC. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
-func NewInnerRPC(t mockConstructorTestingTNewInnerRPC) *InnerRPC {
-	mock := &InnerRPC{}
-	mock.Mock.Test(t)
-
-	t.Cleanup(func() { mock.AssertExpectations(t) })
-
-	return mock
-}

op-service/client/backoff.go → op-service/client/retry.go

@@ -7,6 +7,7 @@ import (
 	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/rpc"
 
+	"github.com/ethereum-optimism/optimism/op-node/client"
 	"github.com/ethereum-optimism/optimism/op-service/backoff"
 )
 
@@ -15,30 +16,20 @@ var (
 	ExponentialBackoff = backoff.Exponential()
 )
 
-// InnerRPC is a minimal [client.RPC] interface that is used by the backoff client.
-//
-//go:generate mockery --name InnerRPC --output ./mocks/
-type InnerRPC interface {
-	Close()
-	CallContext(ctx context.Context, result any, method string, args ...any) error
-	BatchCallContext(ctx context.Context, b []rpc.BatchElem) error
-	EthSubscribe(ctx context.Context, channel any, args ...any) (ethereum.Subscription, error)
-}
-
-// backoffEthClient wraps a [InnerRPC] with a backoff strategy.
-type backoffEthClient struct {
-	c             InnerRPC
+// retryingClient wraps a [client.RPC] with a backoff strategy.
+type retryingClient struct {
+	c             client.RPC
 	retryAttempts int
 	strategy      backoff.Strategy
 }
 
-// NewBackoffClient creates a new backoff client.
+// NewRetryingClient creates a new retrying client.
 // The backoff strategy is optional, if not provided, the default exponential backoff strategy is used.
-func NewBackoffClient(c InnerRPC, retries int, strategy ...backoff.Strategy) *backoffEthClient {
+func NewRetryingClient(c client.RPC, retries int, strategy ...backoff.Strategy) *retryingClient {
 	if len(strategy) == 0 {
 		strategy = []backoff.Strategy{ExponentialBackoff}
 	}
-	return &backoffEthClient{
+	return &retryingClient{
 		c:             c,
 		retryAttempts: retries,
 		strategy:      strategy[0],
@@ -46,15 +37,15 @@ func NewBackoffClient(c InnerRPC, retries int, strategy ...backoff.Strategy) *ba
 }
 
 // BackoffStrategy returns the [backoff.Strategy] used by the client.
-func (b *backoffEthClient) BackoffStrategy() backoff.Strategy {
+func (b *retryingClient) BackoffStrategy() backoff.Strategy {
 	return b.strategy
 }
 
-func (b *backoffEthClient) Close() {
+func (b *retryingClient) Close() {
 	b.c.Close()
 }
 
-func (b *backoffEthClient) CallContext(ctx context.Context, result any, method string, args ...any) error {
+func (b *retryingClient) CallContext(ctx context.Context, result any, method string, args ...any) error {
 	return backoff.DoCtx(ctx, b.retryAttempts, b.strategy, func() error {
 		cCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
 		defer cancel()
@@ -62,7 +53,7 @@ func (b *backoffEthClient) CallContext(ctx context.Context, result any, method s
 	})
 }
 
-func (b *backoffEthClient) BatchCallContext(ctx context.Context, batch []rpc.BatchElem) error {
+func (b *retryingClient) BatchCallContext(ctx context.Context, batch []rpc.BatchElem) error {
 	return backoff.DoCtx(ctx, b.retryAttempts, b.strategy, func() error {
 		cCtx, cancel := context.WithTimeout(ctx, 20*time.Second)
 		defer cancel()
@@ -71,7 +62,7 @@ func (b *backoffEthClient) BatchCallContext(ctx context.Context, batch []rpc.Bat
 	})
 }
 
-func (b *backoffEthClient) EthSubscribe(ctx context.Context, channel any, args ...any) (ethereum.Subscription, error) {
+func (b *retryingClient) EthSubscribe(ctx context.Context, channel any, args ...any) (ethereum.Subscription, error) {
 	var sub ethereum.Subscription
 	err := backoff.DoCtx(ctx, b.retryAttempts, b.strategy, func() error {
 		var err error

op-service/client/backoff_test.go → op-service/client/retry_test.go

@@ -8,80 +8,119 @@ import (
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/rpc"
 
 	"github.com/ethereum-optimism/optimism/op-service/backoff"
 	"github.com/ethereum-optimism/optimism/op-service/client"
-	"github.com/ethereum-optimism/optimism/op-service/client/mocks"
+
+	opclient "github.com/ethereum-optimism/optimism/op-node/client"
 )
 
+type MockRPC struct {
+	mock.Mock
+}
+
+func (m *MockRPC) Close() {
+	m.Called()
+}
+
+func (m *MockRPC) CallContext(ctx context.Context, result any, method string, args ...any) error {
+	out := m.Mock.MethodCalled("CallContext", ctx, result, method, args)
+	return *out[0].(*error)
+}
+
+func (m *MockRPC) BatchCallContext(ctx context.Context, b []rpc.BatchElem) error {
+	out := m.Mock.MethodCalled("BatchCallContext", ctx, b)
+	return *out[0].(*error)
+}
+
+func (m *MockRPC) EthSubscribe(ctx context.Context, channel any, args ...any) (ethereum.Subscription, error) {
+	out := m.Mock.MethodCalled("EthSubscribe", ctx, channel, args)
+	return *out[0].(*ethereum.Subscription), *out[1].(*error)
+}
+
+func (m *MockRPC) ExpectCallContext(err error, result any, method string, arg string) {
+	m.On("CallContext", mock.Anything, result, method, []interface{}{arg}).Return(&err)
+}
+
+func (m *MockRPC) ExpectBatchCallContext(err error, b []rpc.BatchElem) {
+	m.On("BatchCallContext", mock.Anything, b).Return(&err)
+}
+
+func (m *MockRPC) ExpectEthSubscribe(sub ethereum.Subscription, err error, channel any, args ...any) {
+	m.On("EthSubscribe", mock.Anything, channel, args).Return(&sub, &err)
+}
+
+var _ opclient.RPC = (*MockRPC)(nil)
+
 func TestClient_BackoffClient_Strategy(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	backoffClient := client.NewBackoffClient(mockRpc, 0)
+	mockRpc := &MockRPC{}
+	backoffClient := client.NewRetryingClient(mockRpc, 0)
 	require.Equal(t, backoffClient.BackoffStrategy(), client.ExponentialBackoff)
 
 	fixedStrategy := &backoff.FixedStrategy{}
-	backoffClient = client.NewBackoffClient(mockRpc, 0, fixedStrategy)
+	backoffClient = client.NewRetryingClient(mockRpc, 0, fixedStrategy)
 	require.Equal(t, backoffClient.BackoffStrategy(), fixedStrategy)
 }
 
 func TestClient_BackoffClient_Close(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
+	mockRpc := &MockRPC{}
 	mockRpc.On("Close").Return()
-	backoffClient := client.NewBackoffClient(mockRpc, 0)
+	backoffClient := client.NewRetryingClient(mockRpc, 0)
 	backoffClient.Close()
 	require.True(t, mockRpc.AssertCalled(t, "Close"))
 }
 
 func TestClient_BackoffClient_CallContext(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	mockRpc.On("CallContext", mock.Anything, nil, "foo", "bar").Return(nil)
-	backoffClient := client.NewBackoffClient(mockRpc, 1)
+	mockRpc := &MockRPC{}
+	mockRpc.ExpectCallContext(nil, nil, "foo", "bar")
+	backoffClient := client.NewRetryingClient(mockRpc, 1)
 	err := backoffClient.CallContext(context.Background(), nil, "foo", "bar")
 	require.NoError(t, err)
-	require.True(t, mockRpc.AssertCalled(t, "CallContext", mock.Anything, nil, "foo", "bar"))
+	require.True(t, mockRpc.AssertCalled(t, "CallContext", mock.Anything, nil, "foo", []interface{}{"bar"}))
 }
 
 func TestClient_BackoffClient_CallContext_WithRetries(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	mockRpc.On("CallContext", mock.Anything, nil, "foo", "bar").Return(errors.New("foo"))
-	backoffClient := client.NewBackoffClient(mockRpc, 2)
+	mockRpc := &MockRPC{}
+	mockRpc.ExpectCallContext(errors.New("foo"), nil, "foo", "bar")
+	backoffClient := client.NewRetryingClient(mockRpc, 2)
 	err := backoffClient.CallContext(context.Background(), nil, "foo", "bar")
 	require.Error(t, err)
 	require.True(t, mockRpc.AssertNumberOfCalls(t, "CallContext", 2))
 }
 
 func TestClient_BackoffClient_BatchCallContext(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	mockRpc.On("BatchCallContext", mock.Anything, []rpc.BatchElem(nil)).Return(nil)
-	backoffClient := client.NewBackoffClient(mockRpc, 1)
+	mockRpc := &MockRPC{}
+	mockRpc.ExpectBatchCallContext(nil, []rpc.BatchElem(nil))
+	backoffClient := client.NewRetryingClient(mockRpc, 1)
 	err := backoffClient.BatchCallContext(context.Background(), nil)
 	require.NoError(t, err)
 	require.True(t, mockRpc.AssertCalled(t, "BatchCallContext", mock.Anything, []rpc.BatchElem(nil)))
 }
 
 func TestClient_BackoffClient_BatchCallContext_WithRetries(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	mockRpc.On("BatchCallContext", mock.Anything, []rpc.BatchElem(nil)).Return(errors.New("foo"))
-	backoffClient := client.NewBackoffClient(mockRpc, 2)
+	mockRpc := &MockRPC{}
+	mockRpc.ExpectBatchCallContext(errors.New("foo"), []rpc.BatchElem(nil))
+	backoffClient := client.NewRetryingClient(mockRpc, 2)
 	err := backoffClient.BatchCallContext(context.Background(), nil)
 	require.Error(t, err)
 	require.True(t, mockRpc.AssertNumberOfCalls(t, "BatchCallContext", 2))
 }
 
 func TestClient_BackoffClient_EthSubscribe(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	mockRpc.On("EthSubscribe", mock.Anything, nil, "foo", "bar").Return(nil, nil)
-	backoffClient := client.NewBackoffClient(mockRpc, 1)
+	mockRpc := &MockRPC{}
+	mockRpc.ExpectEthSubscribe(ethereum.Subscription(nil), nil, nil, "foo", "bar")
+	backoffClient := client.NewRetryingClient(mockRpc, 1)
 	_, err := backoffClient.EthSubscribe(context.Background(), nil, "foo", "bar")
 	require.NoError(t, err)
-	require.True(t, mockRpc.AssertCalled(t, "EthSubscribe", mock.Anything, nil, "foo", "bar"))
+	require.True(t, mockRpc.AssertCalled(t, "EthSubscribe", mock.Anything, nil, []interface{}{"foo", "bar"}))
 }
 
 func TestClient_BackoffClient_EthSubscribe_WithRetries(t *testing.T) {
-	mockRpc := mocks.NewInnerRPC(t)
-	mockRpc.On("EthSubscribe", mock.Anything, nil, "foo", "bar").Return(nil, errors.New("foo"))
-	backoffClient := client.NewBackoffClient(mockRpc, 2)
+	mockRpc := &MockRPC{}
+	mockRpc.ExpectEthSubscribe(ethereum.Subscription(nil), errors.New("foo"), nil, "foo", "bar")
+	backoffClient := client.NewRetryingClient(mockRpc, 2)
 	_, err := backoffClient.EthSubscribe(context.Background(), nil, "foo", "bar")
 	require.Error(t, err)
 	require.True(t, mockRpc.AssertNumberOfCalls(t, "EthSubscribe", 2))