Repository: onflow/flow-cli (go)

Issue:
Improve Error Handling when transaction signer does not match authorizer
### Problem

When building and signing a transaction with `flow transactions build` and `flow transactions sign`, if the signer specified in `sign` does not match the authorizer, or any of the signers, specified in `build`, the signing still passes, but the next time you try to do anything with that transaction, such as signing it with another key or sending it, you get this error:
```
panic: runtime error: index out of range [-1]

goroutine 1 [running]:
github.com/onflow/flow-go-sdk.DecodeTransaction({0xc000381600, 0xc000381080, 0x512})
	github.com/onflow/flow-go-sdk@v0.20.1-0.20210623043139-533a95abf071/transaction.go:441 +0x896
github.com/onflow/flow-cli/pkg/flowkit.NewTransactionFromPayload({0xc000366800, 0x512, 0x712})
	github.com/onflow/flow-cli/pkg/flowkit/transaction.go:50 +0x165
github.com/onflow/flow-cli/pkg/flowkit/services.(*Transactions).SendSigned(0xc000218f60, {0xc000366800, 0xc00041e0c0, 0xc0001c7bb8})
	github.com/onflow/flow-cli/pkg/flowkit/services/transactions.go:180 +0x70
github.com/onflow/flow-cli/internal/transactions.sendSigned({0xc0000e5530, 0xc0004abce0, 0x4153323}, {0x51a8d28, 0xc000119ac0}, {{0x0, 0x0}, {0x4ed32c9, 0x4}, {0x0, ...}, ...}, ...)
	github.com/onflow/flow-cli/internal/transactions/send-signed.go:64 +0xbb
github.com/onflow/flow-cli/internal/command.Command.AddToParent.func1(0x5ae1760, {0xc0000e5530, 0x1, 0x3})
	github.com/onflow/flow-cli/internal/command/command.go:116 +0x315
github.com/spf13/cobra.(*Command).execute(0x5ae1760, {0xc000130150, 0x3, 0x3})
	github.com/spf13/cobra@v1.1.3/command.go:856 +0x5f8
github.com/spf13/cobra.(*Command).ExecuteC(0xc000424000)
	github.com/spf13/cobra@v1.1.3/command.go:960 +0x3ad
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/cobra@v1.1.3/command.go:897
main.main()
	github.com/onflow/flow-cli/cmd/flow/main.go:69 +0x319
```
It would be a lot more helpful if the signing operation failed at the time of signing if the address doesn't match and not later, and the error message should be more descriptive.

### Steps to Reproduce

1. Build a transaction with one or two authorizers specified
2. Sign the transaction with an account that was not specified as an authorizer
3. Try to send the transaction

### Acceptance Criteria

Some sort of better error message when signing or sending. I'm not really sure what makes the most sense.


Pull request:
Transaction signing checks
Closes #380 

## Description
Implement more strict checks when doing transaction signing:
- check for transaction authorizer mismatch
- check for signer proposer mismatch

______

For contributor use:

- [ ] Targeted PR against `master` branch
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work
- [ ] Code follows the [standards mentioned here](https://github.com/onflow/flow-cli/blob/master/CONTRIBUTING.md#styleguides)
- [ ] Updated relevant documentation
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Added appropriate labels


Patch:
diff --git a/go.sum b/go.sum
index 21c590e2..f0b30320 100644
--- a/go.sum
+++ b/go.sum
@@ -937,7 +937,6 @@ github.com/onflow/cadence/languageserver v0.18.3-0.20211005105715-3c618f0fcd10 h
 github.com/onflow/cadence/languageserver v0.18.3-0.20211005105715-3c618f0fcd10/go.mod h1:kDadEzIbn14bKqIYA3yHMM1IKm2fyfOluH07Jl/SKGc=
 github.com/onflow/flow-cli v0.20.3-0.20210512000809-474effb7e7db/go.mod h1:Yh4dgrNiZdXhbU+3UVUmo4gRM0TEBVvhW4ITKVo82dg=
 github.com/onflow/flow-cli v0.23.1-0.20210621124332-11c4cd22ffb4/go.mod h1:OD9UVPFIvktnO86dSLavf1tTVFIsKQ5By5KBBDLfC3Q=
-github.com/onflow/flow-cli v0.28.2/go.mod h1:u3ywH3krvQh1y/suStSu0q2eptLh+ugXQGaG2KmvppU=
 github.com/onflow/flow-core-contracts/lib/go/contracts v0.7.2/go.mod h1:IZ2e7UyLCYmpQ8Kd7k0A32uXqdqfiV1r2sKs5/riblo=
 github.com/onflow/flow-core-contracts/lib/go/contracts v0.7.3-0.20210527134022-58c25247091a/go.mod h1:IZ2e7UyLCYmpQ8Kd7k0A32uXqdqfiV1r2sKs5/riblo=
 github.com/onflow/flow-core-contracts/lib/go/contracts v0.7.3/go.mod h1:MSNt2rodpRXm1n0iGQWL6ltDoJCtXEzlPw9nhE/zQmk=
@@ -1598,8 +1597,11 @@ golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0=
 golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/tools v0.1.2/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/tools v0.1.3/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
-golang.org/x/tools v0.1.4 h1:cVngSRcfgyZCzys3KYOpCFa+4dqX/Oub9tAq00ttGVs=
-golang.org/x/tools v0.1.4/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
+golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0=
+golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
+golang.org/x/tools v0.1.2/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
+golang.org/x/tools v0.1.3 h1:L69ShwSZEyCsLKoAxDKeMvLDZkumEe8gXUZAjab0tX8=
+golang.org/x/tools v0.1.3/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
diff --git a/pkg/flowkit/services/transactions.go b/pkg/flowkit/services/transactions.go
index f6b29bfc..a932d09a 100644
--- a/pkg/flowkit/services/transactions.go
+++ b/pkg/flowkit/services/transactions.go
@@ -102,7 +102,6 @@ func (t *Transactions) Build(
 	tx := flowkit.NewTransaction().
 		SetPayer(payer).
 		SetProposer(proposerAccount, proposerKeyIndex).
-		AddAuthorizers(authorizers).
 		SetGasLimit(gasLimit).
 		SetBlockReference(latestBlock)
 
@@ -139,6 +138,11 @@ func (t *Transactions) Build(
 		return nil, err
 	}
 
+	tx, err = tx.AddAuthorizers(authorizers)
+	if err != nil {
+		return nil, err
+	}
+
 	return tx, nil
 }
 
diff --git a/pkg/flowkit/transaction.go b/pkg/flowkit/transaction.go
index bff78114..30d03b2d 100644
--- a/pkg/flowkit/transaction.go
+++ b/pkg/flowkit/transaction.go
@@ -23,6 +23,9 @@ import (
 	"encoding/hex"
 	"fmt"
 
+	"github.com/onflow/cadence/runtime/cmd"
+	"github.com/onflow/cadence/runtime/common"
+
 	jsoncdc "github.com/onflow/cadence/encoding/json"
 
 	"github.com/onflow/flow-go-sdk/templates"
@@ -105,15 +108,8 @@ func addAccountContractWithArgs(
 		}
 	}`
 
-	cadenceName, err := cadence.NewString(contract.Name)
-	if err != nil {
-		return nil, err
-	}
-
-	cadenceCode, err := cadence.NewString(contract.SourceHex())
-	if err != nil {
-		return nil, err
-	}
+	cadenceName := cadence.String(contract.Name)
+	cadenceCode := cadence.String(contract.SourceHex())
 
 	tx := flow.NewTransaction().
 		AddRawArgument(jsoncdc.MustEncode(cadenceName)).
@@ -136,7 +132,7 @@ func addAccountContractWithArgs(
 	tx.SetGasLimit(maxGasLimit)
 
 	t := &Transaction{tx: tx}
-	err = t.SetSigner(signer)
+	err := t.SetSigner(signer)
 	if err != nil {
 		return nil, err
 	}
@@ -205,10 +201,38 @@ func (t *Transaction) SetSigner(account *Account) error {
 		return err
 	}
 
+	if !t.validSigner(account.Address()) {
+		return fmt.Errorf(
+			"not a valid signer %s, proposer: %s, payer: %s, authorizers: %s",
+			account.Address(),
+			t.tx.ProposalKey.Address,
+			t.tx.Payer,
+			t.tx.Authorizers,
+		)
+	}
+
 	t.signer = account
 	return nil
 }
 
+// validSigner checks whether the signer is valid for transaction
+func (t *Transaction) validSigner(s flow.Address) bool {
+	return t.tx.ProposalKey.Address == s ||
+		t.tx.Payer == s ||
+		t.authorizersContains(s)
+}
+
+// authorizersContains checks whether address is in the authorizer list
+func (t *Transaction) authorizersContains(address flow.Address) bool {
+	for _, a := range t.tx.Authorizers {
+		if address == a {
+			return true
+		}
+	}
+
+	return false
+}
+
 // SetProposer sets the proposer for transaction.
 func (t *Transaction) SetProposer(proposer *flow.Account, keyIndex int) *Transaction {
 	t.proposer = proposer
@@ -259,12 +283,35 @@ func (t *Transaction) AddArgument(arg cadence.Value) error {
 }
 
 // AddAuthorizers add group of authorizers.
-func (t *Transaction) AddAuthorizers(authorizers []flow.Address) *Transaction {
+func (t *Transaction) AddAuthorizers(authorizers []flow.Address) (*Transaction, error) {
+	program, _ := cmd.PrepareProgram(
+		string(t.tx.Script),
+		common.StringLocation(""),
+		map[common.LocationID]string{},
+	)
+
+	// get authorizers param list if exists
+	if program.SoleTransactionDeclaration().Prepare != nil {
+		requiredAuths := program.SoleTransactionDeclaration().
+			Prepare.
+			FunctionDeclaration.
+			ParameterList.
+			Parameters
+
+		if len(requiredAuths) != len(authorizers) {
+			return nil, fmt.Errorf(
+				"provided authorizers length mismatch, required authorizers %d, but provided %d",
+				len(requiredAuths),
+				len(authorizers),
+			)
+		}
+	}
+
 	for _, authorizer := range authorizers {
 		t.tx.AddAuthorizer(authorizer)
 	}
 
-	return t
+	return t, nil
 }
 
 // Sign signs transaction using signer account.


Test patch:
diff --git a/pkg/flowkit/services/transactions_test.go b/pkg/flowkit/services/transactions_test.go
index a1d7eab1..776a9f60 100644
--- a/pkg/flowkit/services/transactions_test.go
+++ b/pkg/flowkit/services/transactions_test.go
@@ -241,4 +241,183 @@ func TestTransactions_Integration(t *testing.T) {
 		)
 	})
 
+	t.Run("Sign transaction", func(t *testing.T) {
+		t.Parallel()
+		state, s := setupIntegration()
+		setupAccounts(state, s)
+
+		a, _ := state.Accounts().ByName("Alice")
+
+		tx, err := s.Transactions.Build(
+			a.Address(),
+			nil,
+			a.Address(),
+			0,
+			tests.TransactionSimple.Source,
+			tests.TransactionSimple.Filename,
+			1000,
+			nil,
+			"",
+		)
+
+		assert.Nil(t, err)
+		assert.NotNil(t, tx)
+
+		txSigned, err := s.Transactions.Sign(
+			a,
+			[]byte(fmt.Sprintf("%x", tx.FlowTransaction().Encode())),
+			true,
+		)
+		assert.Nil(t, err)
+		assert.NotNil(t, txSigned)
+		assert.Equal(t, len(txSigned.FlowTransaction().Authorizers), 0)
+		assert.Equal(t, txSigned.FlowTransaction().Payer, a.Address())
+		assert.Equal(t, txSigned.FlowTransaction().ProposalKey.Address, a.Address())
+		assert.Equal(t, txSigned.FlowTransaction().ProposalKey.KeyIndex, 0)
+		assert.Equal(t, txSigned.FlowTransaction().Script, tests.TransactionSimple.Source)
+	})
+
+	t.Run("Build, Sign and Send Transaction", func(t *testing.T) {
+		t.Parallel()
+		state, s := setupIntegration()
+		setupAccounts(state, s)
+
+		a, _ := state.Accounts().ByName("Alice")
+
+		tx, err := s.Transactions.Build(
+			a.Address(),
+			[]flow.Address{a.Address()},
+			a.Address(),
+			0,
+			tests.TransactionSingleAuth.Source,
+			tests.TransactionSingleAuth.Filename,
+			1000,
+			nil,
+			"",
+		)
+
+		assert.Nil(t, err)
+		assert.NotNil(t, tx)
+
+		txSigned, err := s.Transactions.Sign(
+			a,
+			[]byte(fmt.Sprintf("%x", tx.FlowTransaction().Encode())),
+			true,
+		)
+		assert.Nil(t, err)
+		assert.NotNil(t, txSigned)
+
+		txSent, txResult, err := s.Transactions.SendSigned(
+			[]byte(fmt.Sprintf("%x", txSigned.FlowTransaction().Encode())),
+		)
+		assert.Nil(t, err)
+		assert.Equal(t, txResult.Status, flow.TransactionStatusSealed)
+		assert.NotNil(t, txSent.ID())
+
+	})
+
+	t.Run("Fails signing transaction, wrong account", func(t *testing.T) {
+		t.Parallel()
+		state, s := setupIntegration()
+		setupAccounts(state, s)
+
+		a, _ := state.Accounts().ByName("Alice")
+
+		tx, err := s.Transactions.Build(
+			a.Address(),
+			[]flow.Address{a.Address()},
+			a.Address(),
+			0,
+			tests.TransactionSingleAuth.Source,
+			tests.TransactionSingleAuth.Filename,
+			1000,
+			nil,
+			"",
+		)
+
+		assert.Nil(t, err)
+		assert.NotNil(t, tx)
+
+		// sign with wrong account
+		a, _ = state.Accounts().ByName("Bob")
+
+		txSigned, err := s.Transactions.Sign(
+			a,
+			[]byte(fmt.Sprintf("%x", tx.FlowTransaction().Encode())),
+			true,
+		)
+		assert.EqualError(t, err, "not a valid signer 179b6b1cb6755e31, proposer: 01cf0e2f2f715450, payer: 01cf0e2f2f715450, authorizers: [01cf0e2f2f715450]")
+		assert.Nil(t, txSigned)
+	})
+
+	t.Run("Fails building, authorizers mismatch", func(t *testing.T) {
+		t.Parallel()
+		state, s := setupIntegration()
+		setupAccounts(state, s)
+
+		a, _ := state.Accounts().ByName("Alice")
+
+		tx, err := s.Transactions.Build(
+			a.Address(),
+			[]flow.Address{a.Address()},
+			a.Address(),
+			0,
+			tests.TransactionTwoAuth.Source,
+			tests.TransactionTwoAuth.Filename,
+			1000,
+			nil,
+			"",
+		)
+
+		assert.EqualError(t, err, "provided authorizers length mismatch, required authorizers 2, but provided 1")
+		assert.Nil(t, tx)
+	})
+
+	t.Run("Send Transaction", func(t *testing.T) {
+		t.Parallel()
+		state, s := setupIntegration()
+		setupAccounts(state, s)
+
+		a, _ := state.Accounts().ByName("Alice")
+
+		tx, txr, err := s.Transactions.Send(
+			a,
+			tests.TransactionSingleAuth.Source,
+			tests.TransactionSingleAuth.Filename,
+			1000,
+			nil,
+			"",
+		)
+		assert.NoError(t, err)
+		assert.Equal(t, tx.Payer.String(), a.Address().String())
+		assert.Equal(t, tx.ProposalKey.KeyIndex, a.Key().Index())
+		assert.Nil(t, txr.Error)
+		assert.Equal(t, txr.Status, flow.TransactionStatusSealed)
+	})
+
+	t.Run("Send Transaction with arguments", func(t *testing.T) {
+		t.Parallel()
+		state, s := setupIntegration()
+		setupAccounts(state, s)
+
+		a, _ := state.Accounts().ByName("Alice")
+		args := []cadence.Value{
+			cadence.String("Bar"),
+		}
+
+		tx, txr, err := s.Transactions.Send(
+			a,
+			tests.TransactionArgString.Source,
+			tests.TransactionArgString.Filename,
+			1000,
+			args,
+			"",
+		)
+		assert.NoError(t, err)
+		assert.Equal(t, tx.Payer.String(), a.Address().String())
+		assert.Equal(t, tx.ProposalKey.KeyIndex, a.Key().Index())
+		assert.Equal(t, fmt.Sprintf("%x", tx.Arguments), "[7b2274797065223a22537472696e67222c2276616c7565223a22426172227d0a]")
+		assert.Nil(t, txr.Error)
+		assert.Equal(t, txr.Status, flow.TransactionStatusSealed)
+	})
 }
diff --git a/tests/resources.go b/tests/resources.go
index 23566ba1..d432d092 100644
--- a/tests/resources.go
+++ b/tests/resources.go
@@ -182,6 +182,24 @@ var TransactionSimple = resource{
 	`),
 }
 
+var TransactionSingleAuth = resource{
+	Filename: "transactionAuth1.cdc",
+	Source: []byte(`
+		transaction() {
+			prepare(authorizer: AuthAccount) {}
+		}
+	`),
+}
+
+var TransactionTwoAuth = resource{
+	Filename: "transactionAuth2.cdc",
+	Source: []byte(`
+		transaction() {
+			prepare(auth1: AuthAccount, auth2: AuthAccount) {}
+		}
+	`),
+}
+
 var ScriptWithError = resource{
 	Filename: "scriptError.cdc",
 	Source: []byte(`