Commit plugin factory improvements (#295)

* choose better query size

* commit factory improvements and tests

* makram's code review

* fix the value

commit/factory.go

@@ -27,7 +27,9 @@ import (
 	"github.com/smartcontractkit/chainlink-ccip/pluginconfig"
 )
 
-const maxQueryLength = 1024 * 1024 // 1MB
+// maxQueryLength is set to twice the maximum size of a theoretical merkle root processor query
+// that assumes 1,000 source chains and 256 (theoretical max) RMN nodes.
+const maxQueryLength = 615_520
 
 // PluginFactoryConstructor implements common OCR3ReportingPluginClient and is used for initializing a plugin factory
 // and a validation service.
@@ -47,14 +49,13 @@ func (p PluginFactoryConstructor) NewReportingPluginFactory(
 	keyValueStore core.KeyValueStore,
 	relayerSet core.RelayerSet,
 ) (core.OCR3ReportingPluginFactory, error) {
-	return nil, errors.New("unimplemented")
+	return nil, errors.New("this functionality should not be used")
 }
 
 func (p PluginFactoryConstructor) NewValidationService(ctx context.Context) (core.ValidationService, error) {
-	panic("implement me")
+	return nil, errors.New("this functionality should not be used")
 }
 
-// PluginFactory implements common ReportingPluginFactory and is used for (re-)initializing commit plugin instances.
 type PluginFactory struct {
 	lggr              logger.Logger
 	donID             plugintypes.DonID
@@ -69,6 +70,8 @@ type PluginFactory struct {
 	rmnCrypto         cciptypes.RMNCrypto
 }
 
+// NewPluginFactory creates a new PluginFactory instance. For commit plugin, oracle instances are not managed by the
+// factory. It is safe to assume that a factory instance will create exactly one plugin instance.
 func NewPluginFactory(
 	lggr logger.Logger,
 	donID plugintypes.DonID,
@@ -149,6 +152,10 @@ func (p *PluginFactory) NewReportingPlugin(ctx context.Context, config ocr3types
 		readers[chain] = cr
 	}
 
+	if err := validateOcrConfig(p.ocrConfig.Config); err != nil {
+		return nil, ocr3types.ReportingPluginInfo{}, fmt.Errorf("validate ocr config: %w", err)
+	}
+
 	ccipReader := readerpkg.NewCCIPChainReader(
 		ctx,
 		p.lggr,
@@ -210,24 +217,36 @@ func (p *PluginFactory) NewReportingPlugin(ctx context.Context, config ocr3types
 		}, nil
 }
 
+func validateOcrConfig(cfg readerpkg.OCR3Config) error {
+	if cfg.ChainSelector == 0 {
+		return errors.New("chain selector must be set")
+	}
+
+	if cfg.OfframpAddress == nil {
+		return errors.New("offramp address must be set")
+	}
+
+	return nil
+}
+
 func (p PluginFactory) Name() string {
-	panic("implement me")
+	panic("should not be called")
 }
 
 func (p PluginFactory) Start(ctx context.Context) error {
-	panic("implement me")
+	panic("should not be called")
 }
 
 func (p PluginFactory) Close() error {
-	panic("implement me")
+	panic("should not be called")
 }
 
 func (p PluginFactory) Ready() error {
-	panic("implement me")
+	panic("should not be called")
 }
 
 func (p PluginFactory) HealthReport() map[string]error {
-	panic("implement me")
+	panic("should not be called")
 }
 
 // Interface compatibility checks.

commit/factory_test.go

@@ -0,0 +1,114 @@
+package commit
+
+import (
+	"encoding/json"
+	"math"
+	"testing"
+
+	"github.com/smartcontractkit/chainlink-common/pkg/logger"
+	"github.com/smartcontractkit/chainlink-common/pkg/merklemulti"
+	"github.com/smartcontractkit/chainlink-common/pkg/utils/tests"
+	"github.com/smartcontractkit/libocr/offchainreporting2plus/ocr3types"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/smartcontractkit/chainlink-ccip/commit/chainfee"
+	"github.com/smartcontractkit/chainlink-ccip/commit/merkleroot"
+	"github.com/smartcontractkit/chainlink-ccip/commit/merkleroot/rmn"
+	"github.com/smartcontractkit/chainlink-ccip/commit/merkleroot/rmn/rmnpb"
+	"github.com/smartcontractkit/chainlink-ccip/commit/tokenprice"
+	reader2 "github.com/smartcontractkit/chainlink-ccip/internal/reader"
+	"github.com/smartcontractkit/chainlink-ccip/pkg/reader"
+	"github.com/smartcontractkit/chainlink-ccip/pluginconfig"
+)
+
+func Test_maxQueryLength(t *testing.T) {
+	// This test will verify that the maxQueryLength constant is set to a proper value.
+
+	// Estimate the maximum number of source chains we are going to ever have.
+	// This value should be tweaked after we are close to supporting that many chains.
+	const estimatedMaxNumberOfSourceChains = 1000
+
+	// Estimate the maximum number of RMN report signers we are going to ever have.
+	// This value is defined in RMNRemote contract as `f`.
+	// This value should be tweaked if necessary in order to define new limits.
+	const estimatedMaxRmnReportSigners = 256
+
+	sigs := make([]*rmnpb.EcdsaSignature, estimatedMaxRmnReportSigners)
+	for i := range sigs {
+		sigs[i] = &rmnpb.EcdsaSignature{R: make([]byte, 32), S: make([]byte, 32)}
+	}
+
+	laneUpdates := make([]*rmnpb.FixedDestLaneUpdate, estimatedMaxNumberOfSourceChains)
+	for i := range laneUpdates {
+		laneUpdates[i] = &rmnpb.FixedDestLaneUpdate{
+			LaneSource: &rmnpb.LaneSource{
+				SourceChainSelector: math.MaxUint64,
+				OnrampAddress:       make([]byte, 40),
+			},
+			ClosedInterval: &rmnpb.ClosedInterval{
+				MinMsgNr: math.MaxUint64,
+				MaxMsgNr: math.MaxUint64,
+			},
+			Root: make([]byte, 32),
+		}
+	}
+
+	q := Query{
+		MerkleRootQuery: merkleroot.Query{
+			RetryRMNSignatures: true,
+			RMNSignatures: &rmn.ReportSignatures{
+				Signatures:  sigs,
+				LaneUpdates: laneUpdates,
+			},
+		},
+		TokenPriceQuery: tokenprice.Query{},
+		ChainFeeQuery:   chainfee.Query{},
+	}
+	b, err := q.Encode()
+	require.NoError(t, err)
+
+	// We set twice the size, for extra safety while making breaking changes between oracle versions.
+	assert.Equal(t, 2*len(b), maxQueryLength)
+}
+
+func TestPluginFactory_NewReportingPlugin(t *testing.T) {
+	t.Run("basic checks for the happy flow", func(t *testing.T) {
+		ctx := tests.Context(t)
+		lggr := logger.Test(t)
+
+		offChainConfig := pluginconfig.CommitOffchainConfig{
+			MaxMerkleTreeSize: 123,
+		}
+		b, err := json.Marshal(offChainConfig)
+		require.NoError(t, err)
+
+		p := &PluginFactory{
+			lggr: lggr,
+			ocrConfig: reader.OCR3ConfigWithMeta{
+				Version:      1,
+				ConfigDigest: [32]byte{1, 2, 3},
+				Config: reader2.OCR3Config{
+					OfframpAddress: []byte{1, 2, 3},
+					OffchainConfig: b,
+					ChainSelector:  1,
+				},
+			},
+		}
+
+		plugin, pluginInfo, err := p.NewReportingPlugin(ctx, ocr3types.ReportingPluginConfig{
+			OffchainConfig: b,
+		})
+		require.NoError(t, err)
+
+		pluginCommit, is := plugin.(*Plugin)
+		require.True(t, is)
+		pluginOffchainConfig := pluginCommit.offchainCfg
+
+		require.Equal(t, uint(5), pluginOffchainConfig.MaxReportTransmissionCheckAttempts)          // default is used
+		require.Equal(t, merklemulti.MaxNumberTreeLeaves, pluginOffchainConfig.NewMsgScanBatchSize) // default is used
+		require.Equal(t, offChainConfig.MaxMerkleTreeSize, pluginOffchainConfig.MaxMerkleTreeSize)  // override
+
+		require.Equal(t, maxQueryLength, pluginInfo.Limits.MaxQueryLength)
+	})
+}