Merge pull request #1586 from SamMayWork/contract-manager-fix

fix: Clean up manager creation to avoid NPEs during background start

Dockerfile

@@ -75,7 +75,7 @@ ARG UI_TAG
 ARG UI_RELEASE
 RUN apk add --update --no-cache \
     sqlite=3.44.2-r0 \
-    postgresql16-client=16.3-r0 \
+    postgresql16-client=16.4-r0 \
     curl=8.9.1-r0 \
     jq=1.7.1-r0
 WORKDIR /firefly

internal/contracts/manager.go

@@ -160,7 +160,12 @@ func NewContractManager(ctx context.Context, ns string, di database.Plugin, bi b
 	// cause recreation of all the listeners (noting that listeners that were specified to start
 	// from latest, will start from the new latest rather than replaying from the block they
 	// started from before they were deleted).
-	return cm, cm.verifyListeners(ctx)
+	err = cm.verifyListeners(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	return cm, nil
 }
 
 func (cm *contractManager) Name() string {

internal/contracts/manager_test.go

@@ -1,4 +1,4 @@
-// Copyright © 2023 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -98,6 +98,33 @@ func TestName(t *testing.T) {
 	assert.Equal(t, "ContractManager", cm.Name())
 }
 
+func TestNewContractManagerVerifyListenersFails(t *testing.T) {
+	mdi := &databasemocks.Plugin{}
+	mdm := &datamocks.Manager{}
+	mbm := &broadcastmocks.Manager{}
+	mpm := &privatemessagingmocks.Manager{}
+	mbp := &batchmocks.Manager{}
+	mim := &identitymanagermocks.Manager{}
+	mbi := &blockchainmocks.Plugin{}
+	mom := &operationmocks.Manager{}
+	txw := &txwritermocks.Writer{}
+	cmi := &cachemocks.Manager{}
+	msa := &syncasyncmocks.Bridge{}
+
+	ctx := context.Background()
+
+	cmi.On("GetCache", mock.Anything).Return(cache.NewUmanagedCache(ctx, 100, 5*time.Minute), nil)
+	txHelper, _ := txcommon.NewTransactionHelper(ctx, "ns1", mdi, mdm, cmi)
+	mbi.On("GetFFIParamValidator", mock.Anything).Return(nil, nil)
+	mom.On("RegisterHandler", mock.Anything, mock.Anything, mock.Anything)
+	mbi.On("Name").Return("mockblockchain").Maybe()
+	mdi.On("GetContractListeners", mock.Anything, "ns1", mock.Anything).Return(nil, nil, fmt.Errorf("KABOOM!")).Once()
+
+	cm, err := NewContractManager(context.Background(), "ns1", mdi, mbi, mdm, mbm, mpm, mbp, mim, mom, txHelper, txw, msa, cmi)
+	assert.Nil(t, cm)
+	assert.NotNil(t, err)
+}
+
 func TestNewContractManagerFFISchemaLoaderFail(t *testing.T) {
 	mdi := &databasemocks.Plugin{}
 	mdm := &datamocks.Manager{}

internal/definitions/sender.go

@@ -1,4 +1,4 @@
-// Copyright © 2023 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -93,8 +93,12 @@ func NewDefinitionSender(ctx context.Context, ns *core.Namespace, multiparty boo
 		tokenBroadcastNames: tokenBroadcastNames,
 	}
 	dh, err := newDefinitionHandler(ctx, ns, multiparty, di, bi, dx, dm, im, am, cm, reverseMap(tokenBroadcastNames))
+	if err != nil {
+		return nil, nil, err
+	}
+
 	ds.handler = dh
-	return ds, dh, err
+	return ds, dh, nil
 }
 
 // reverseMap reverses the key/values of a given map

internal/definitions/sender_test.go

@@ -1,4 +1,4 @@
-// Copyright © 2021 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -105,6 +105,26 @@ func TestInitSenderFail(t *testing.T) {
 	assert.Regexp(t, "FF10128", err)
 }
 
+func TestNewDefinitionSenderHandlerThrows(t *testing.T) {
+	mdi := &databasemocks.Plugin{}
+	mbi := &blockchainmocks.Plugin{}
+	mdx := &dataexchangemocks.Plugin{}
+	mbm := &broadcastmocks.Manager{}
+	mim := &identitymanagermocks.Manager{}
+	mdm := &datamocks.Manager{}
+	mcm := &contractmocks.Manager{}
+
+	tokenBroadcastNames := make(map[string]string)
+	tokenBroadcastNames["connector1"] = "remote1"
+
+	ctx := context.Background()
+	ns := &core.Namespace{Name: "ns1", NetworkName: "ns1"}
+	ds, dh, err := NewDefinitionSender(ctx, ns, false, mdi, mbi, mdx, mbm, mim, mdm, nil, mcm, tokenBroadcastNames)
+	assert.Nil(t, ds)
+	assert.Nil(t, dh)
+	assert.NotNil(t, err)
+}
+
 func TestName(t *testing.T) {
 	ds := newTestDefinitionSender(t)
 	defer ds.cleanup(t)