Fix: Ensure Done() is called on external wallets even on errors

Use defer to guarantee that Done() is called on all external wallet signers
regardless of whether errors occur during signature collection, auditing,
or approval phases. This prevents resource leaks when the endorsement
process fails early.

Previously, if requestSignaturesOnIssues or requestSignaturesOnTransfers
returned an error, any external wallets already added to the map would
not have their Done() method called, potentially leaving resources uncleaned.

Signed-off-by: AkramBitar <akram@il.ibm.com>

Author: AKRAM@il.ibm.com

token/services/ttx/collectendorsements.go

@@ -84,6 +84,10 @@ func (c *CollectEndorsementsView) Call(context view.Context) (interface{}, error
 	metrics := GetMetrics(context)
 
 	externalWallets := make(map[string]ExternalWalletSigner)
+
+	// Ensure Done() is called on all external wallets regardless of errors
+	defer c.CleanupExternalWallets(context, externalWallets)
+
 	// 1. First collect signatures on the token request
 	issueSigmas, err := c.requestSignaturesOnIssues(context, externalWallets)
 	if err != nil {
@@ -95,15 +99,6 @@ func (c *CollectEndorsementsView) Call(context view.Context) (interface{}, error
 		return nil, errors.WithMessagef(err, "failed requesting signatures on transfers")
 	}
 
-	// signal the external wallets that the process is completed
-	logger.DebugfContext(context.Context(), "Inform external wallets that endorsement is complete")
-	for id, signer := range externalWallets {
-		if err := signer.Done(); err != nil {
-			logger.ErrorfContext(context.Context(), "failed to signal done external wallet [%s]", id)
-			logger.Errorf("failed to signal done external wallet [%s]", id)
-		}
-	}
-
 	// Add the signatures to the token request
 	logger.DebugfContext(context.Context(), "Add the signatures to the token request")
 	if !c.tx.TokenRequest.SetSignatures(mergeSigmas(issueSigmas, transferSigmas)) {
@@ -665,3 +660,13 @@ func TransferDistributionList(r *token.Request) []view.Identity {
 	}
 	return distributionList
 }
+
+// CleanupExternalWallets calls Done() on all external wallets to signal completion
+func (c *CollectEndorsementsView) CleanupExternalWallets(context view.Context, externalWallets map[string]ExternalWalletSigner) {
+	logger.DebugfContext(context.Context(), "Inform external wallets that endorsement is complete")
+	for id, signer := range externalWallets {
+		if err := signer.Done(); err != nil {
+			logger.ErrorfContext(context.Context(), "failed to signal done external wallet [%s]", id)
+		}
+	}
+}

token/services/ttx/collectendorsements_test.go

@@ -0,0 +1,127 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package ttx_test
+
+import (
+	"testing"
+
+	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
+	"github.com/hyperledger-labs/fabric-smart-client/platform/view/view"
+	"github.com/hyperledger-labs/fabric-token-sdk/token/services/ttx"
+	mock2 "github.com/hyperledger-labs/fabric-token-sdk/token/services/ttx/dep/mock"
+	"github.com/stretchr/testify/assert"
+)
+
+// mockExternalWalletSigner is a mock implementation of ExternalWalletSigner for testing
+type mockExternalWalletSigner struct {
+	signFunc   func(party view.Identity, message []byte) ([]byte, error)
+	doneFunc   func() error
+	doneCalled bool
+	signCalled bool
+}
+
+func (m *mockExternalWalletSigner) Sign(party view.Identity, message []byte) ([]byte, error) {
+	m.signCalled = true
+	if m.signFunc != nil {
+		return m.signFunc(party, message)
+	}
+	return []byte("mock_signature"), nil
+}
+
+func (m *mockExternalWalletSigner) Done() error {
+	m.doneCalled = true
+	if m.doneFunc != nil {
+		return m.doneFunc()
+	}
+	return nil
+}
+
+// TestCleanupExternalWallets_Success tests that CleanupExternalWallets calls Done() on all wallets
+func TestCleanupExternalWallets_Success(t *testing.T) {
+	wallet := &mockExternalWalletSigner{}
+
+	externalWallets := map[string]ttx.ExternalWalletSigner{
+		"wallet1": wallet,
+	}
+
+	// Create a minimal view and context
+	view := &ttx.CollectEndorsementsView{}
+	ctx := &mock2.Context{}
+	ctx.ContextReturns(t.Context())
+
+	// Call the cleanup method
+	view.CleanupExternalWallets(ctx, externalWallets)
+
+	assert.True(t, wallet.doneCalled, "Done() should have been called on the wallet")
+}
+
+// TestCleanupExternalWallets_MultipleWallets tests that Done() is called on all wallets
+func TestCleanupExternalWallets_MultipleWallets(t *testing.T) {
+	wallet1 := &mockExternalWalletSigner{}
+	wallet2 := &mockExternalWalletSigner{}
+	wallet3 := &mockExternalWalletSigner{}
+
+	externalWallets := map[string]ttx.ExternalWalletSigner{
+		"wallet1": wallet1,
+		"wallet2": wallet2,
+		"wallet3": wallet3,
+	}
+
+	view := &ttx.CollectEndorsementsView{}
+	ctx := &mock2.Context{}
+	ctx.ContextReturns(t.Context())
+
+	view.CleanupExternalWallets(ctx, externalWallets)
+
+	assert.True(t, wallet1.doneCalled, "Done() should have been called on wallet1")
+	assert.True(t, wallet2.doneCalled, "Done() should have been called on wallet2")
+	assert.True(t, wallet3.doneCalled, "Done() should have been called on wallet3")
+}
+
+// TestCleanupExternalWallets_DoneError tests that errors from Done() don't stop cleanup
+func TestCleanupExternalWallets_DoneError(t *testing.T) {
+	wallet1 := &mockExternalWalletSigner{
+		doneFunc: func() error {
+			return errors.New("wallet1 done failed")
+		},
+	}
+	wallet2 := &mockExternalWalletSigner{}
+	wallet3 := &mockExternalWalletSigner{
+		doneFunc: func() error {
+			return errors.New("wallet3 done failed")
+		},
+	}
+
+	externalWallets := map[string]ttx.ExternalWalletSigner{
+		"wallet1": wallet1,
+		"wallet2": wallet2,
+		"wallet3": wallet3,
+	}
+
+	view := &ttx.CollectEndorsementsView{}
+	ctx := &mock2.Context{}
+	ctx.ContextReturns(t.Context())
+
+	// Should not panic even if Done() returns errors
+	view.CleanupExternalWallets(ctx, externalWallets)
+
+	assert.True(t, wallet1.doneCalled, "Done() should have been called on wallet1 despite error")
+	assert.True(t, wallet2.doneCalled, "Done() should have been called on wallet2")
+	assert.True(t, wallet3.doneCalled, "Done() should have been called on wallet3 despite error")
+}
+
+// TestCleanupExternalWallets_EmptyMap tests cleanup with no wallets
+func TestCleanupExternalWallets_EmptyMap(t *testing.T) {
+	externalWallets := map[string]ttx.ExternalWalletSigner{}
+
+	view := &ttx.CollectEndorsementsView{}
+	ctx := &mock2.Context{}
+	ctx.ContextReturns(t.Context())
+
+	// Should not panic with empty map
+	view.CleanupExternalWallets(ctx, externalWallets)
+}