From aa0f37c34bc9b7d05ce523a2860e53c50f405b63 Mon Sep 17 00:00:00 2001
From: Leigh McCulloch <leigh@stellar.org>
Date: Mon, 16 Dec 2019 13:29:06 -0800
Subject: [PATCH] txnbuild: remove redundant ok from internal verify functions
 (#2065)

Remove the `ok` bool value that the `verifyTxSignature` function returns alongside its `error` value.

The `bool` is redundant.

The `verifyTxSignature` function returns both a `bool` and an `error` but both are communicating the same result. The bool is only ever true when there is no error, and only ever false when there is an error. I'm refactoring some of this code and plan to use this function in a new function and it makes that reuse more difficult if this function returns bool and error.

In my new function I won't be passing on the bool to the caller, and so the question remains in my new function should I handle the bool and error separately so that if bool ever returns false without an error I generate some type of new error. I'd like to not have to solve this problem since it isn't important and instead remove the redundant bool.
---
 txnbuild/transaction.go      | 28 +++++++++++++++++-----------
 txnbuild/transaction_test.go | 15 +++++----------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go
index 986b4ac06a..17c7f6c5f4 100644
--- a/txnbuild/transaction.go
+++ b/txnbuild/transaction.go
@@ -477,32 +477,38 @@ func VerifyChallengeTx(challengeTx, serverAccountID, network string) (bool, erro
 	}
 
 	// verify signature from operation source
-	ok, err = verifyTxSignature(tx, op.SourceAccount.GetAccountID())
+	err = verifyTxSignature(tx, op.SourceAccount.GetAccountID())
 	if err != nil {
-		return ok, err
+		return false, err
 	}
 
 	// verify signature from server signing key
-	return verifyTxSignature(tx, serverAccountID)
+	err = verifyTxSignature(tx, serverAccountID)
+	if err != nil {
+		return false, err
+	}
+
+	return true, nil
 }
 
 // verifyTxSignature checks if a transaction has been signed by the provided Stellar account.
-func verifyTxSignature(tx Transaction, accountID string) (bool, error) {
-	signerFound := false
+func verifyTxSignature(tx Transaction, accountID string) error {
 	txHash, err := tx.Hash()
 	if err != nil {
-		return signerFound, err
+		return err
 	}
 
 	kp, err := keypair.Parse(accountID)
 	if err != nil {
-		return signerFound, err
+		return err
 	}
 
 	// find and verify signatures
 	if tx.xdrEnvelope == nil {
-		return signerFound, errors.New("transaction has no signatures")
+		return errors.New("transaction has no signatures")
 	}
+
+	signerFound := false
 	for _, s := range tx.xdrEnvelope.Signatures {
 		e := kp.Verify(txHash[:], s.Signature)
 		if e == nil {
@@ -510,9 +516,9 @@ func verifyTxSignature(tx Transaction, accountID string) (bool, error) {
 			break
 		}
 	}
-
 	if !signerFound {
-		return signerFound, errors.Errorf("transaction not signed by %s", accountID)
+		return errors.Errorf("transaction not signed by %s", accountID)
 	}
-	return signerFound, nil
+
+	return nil
 }
diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go
index 38d4526b70..7a4f7fa296 100644
--- a/txnbuild/transaction_test.go
+++ b/txnbuild/transaction_test.go
@@ -1183,11 +1183,10 @@ func TestVerifyTxSignatureUnsignedTx(t *testing.T) {
 	// verify unsigned tx
 	err := tx.Build()
 	assert.NoError(t, err)
-	ok, err := verifyTxSignature(tx, kp0.Address())
+	err = verifyTxSignature(tx, kp0.Address())
 	if assert.Error(t, err) {
 		assert.Contains(t, err.Error(), "transaction not signed by GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3")
 	}
-	assert.Equal(t, false, ok)
 }
 
 func TestVerifyTxSignatureSingle(t *testing.T) {
@@ -1211,9 +1210,8 @@ func TestVerifyTxSignatureSingle(t *testing.T) {
 	assert.NoError(t, err)
 	err = tx.Sign(kp0)
 	assert.NoError(t, err)
-	ok, err := verifyTxSignature(tx, kp0.Address())
+	err = verifyTxSignature(tx, kp0.Address())
 	assert.NoError(t, err)
-	assert.Equal(t, true, ok)
 }
 
 func TestVerifyTxSignatureMultiple(t *testing.T) {
@@ -1237,12 +1235,10 @@ func TestVerifyTxSignatureMultiple(t *testing.T) {
 	assert.NoError(t, err)
 	err = tx.Sign(kp0, kp1)
 	assert.NoError(t, err)
-	ok, err := verifyTxSignature(tx, kp0.Address())
+	err = verifyTxSignature(tx, kp0.Address())
 	assert.NoError(t, err)
-	assert.Equal(t, true, ok)
-	ok, err = verifyTxSignature(tx, kp1.Address())
+	err = verifyTxSignature(tx, kp1.Address())
 	assert.NoError(t, err)
-	assert.Equal(t, true, ok)
 }
 func TestVerifyTxSignatureInvalid(t *testing.T) {
 	kp0 := newKeypair0()
@@ -1265,11 +1261,10 @@ func TestVerifyTxSignatureInvalid(t *testing.T) {
 	assert.NoError(t, err)
 	err = tx.Sign(kp0, kp1)
 	assert.NoError(t, err)
-	ok, err := verifyTxSignature(tx, "GATBMIXTHXYKSUZSZUEJKACZ2OS2IYUWP2AIF3CA32PIDLJ67CH6Y5UY")
+	err = verifyTxSignature(tx, "GATBMIXTHXYKSUZSZUEJKACZ2OS2IYUWP2AIF3CA32PIDLJ67CH6Y5UY")
 	if assert.Error(t, err) {
 		assert.Contains(t, err.Error(), "transaction not signed by GATBMIXTHXYKSUZSZUEJKACZ2OS2IYUWP2AIF3CA32PIDLJ67CH6Y5UY")
 	}
-	assert.Equal(t, false, ok)
 }
 
 func TestVerifyChallengeTxInvalid(t *testing.T) {