add validation and signature checks

also expands test cases
2024-12-26 00:05:18 +00:00 · 2018-09-01 13:16:56 -04:00 · 2018-09-01 13:16:56 -04:00 · bbe83c6ad7
commit bbe83c6ad7
parent ec105e88ad
7 changed files with 444 additions and 229 deletions
--- a/internal/x/paychan/README.md
+++ b/internal/x/paychan/README.md
@ -14,6 +14,9 @@ Simplifications:
 - Tidy up - standardise var names, method descriptions, heading comments
 - any problem in signing your own address?
 - Gas
- - Codespace
- - find nicer name for payouts
+ - find nicer name for payout
 - tags - return channel id
+ - create custom errors instead of using sdk.ErrInternal
+ - maybe split off signatures from update as with txs/msgs - testing easier, code easier to use, doesn't store sigs unecessarily on chain
+ - consider removing pubKey from UpdateSignature - instead let channel module access accountMapper
+ - remove printout from tests when app initialised
--- a/internal/x/paychan/endblocker_test.go
+++ b/internal/x/paychan/endblocker_test.go
@ -3,13 +3,13 @@ package paychan
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/assert"
-	"github.com/tendermint/tendermint/crypto"
 	"testing"
 )

 func TestEndBlocker(t *testing.T) {
 	// SETUP
-	ctx, _, channelKeeper, addrs, _ := createMockApp()
+	accountSeeds := []string{"senderSeed", "receiverSeed"}
+	ctx, _, channelKeeper, addrs, _, _, _ := createMockApp(accountSeeds)
 	sender := addrs[0]
 	receiver := addrs[1]
 	coins := sdk.Coins{sdk.NewCoin("KVA", 10)}
@ -24,14 +24,11 @@ func TestEndBlocker(t *testing.T) {
 	channelKeeper.setChannel(ctx, channel)

 	// create closing update and submittedUpdate
-	payouts := Payouts{
-		{sender, sdk.Coins{sdk.NewCoin("KVA", 3)}},
-		{receiver, sdk.Coins{sdk.NewCoin("KVA", 7)}},
-	}
+	payout := Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}
 	update := Update{
 		ChannelID: channelID,
-		Payouts:   payouts,
-		Sigs:      [1]crypto.Signature{},
+		Payout:    payout,
+		//Sigs:      [1]crypto.Signature{},
 	}
 	sUpdate := SubmittedUpdate{
 		Update:        update,
--- a/internal/x/paychan/keeper.go
+++ b/internal/x/paychan/keeper.go
@ -1,6 +1,7 @@
 package paychan

 import (
+	"bytes"
 	"fmt"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/wire"
@ -15,7 +16,6 @@ type Keeper struct {
 	cdc        *wire.Codec // needed to serialize objects before putting them in the store
 	coinKeeper bank.Keeper

-	// TODO investigate codespace
 	//codespace sdk.CodespaceType
 }

@ -34,36 +34,21 @@ func NewKeeper(cdc *wire.Codec, key sdk.StoreKey, ck bank.Keeper) Keeper {

 // Create a new payment channel and lock up sender funds.
 func (k Keeper) CreateChannel(ctx sdk.Context, sender sdk.AccAddress, receiver sdk.AccAddress, coins sdk.Coins) (sdk.Tags, sdk.Error) {
-	// TODO do validation and maybe move somewhere nicer
-	/*
-		// args present
+
+	// Check addresses valid (Technicaly don't need to check sender address is valid as SubtractCoins does that)
 	if len(sender) == 0 {
 		return nil, sdk.ErrInvalidAddress(sender.String())
 	}
 	if len(receiver) == 0 {
 		return nil, sdk.ErrInvalidAddress(receiver.String())
 	}
-		if len(amount) == 0 {
-			return nil, sdk.ErrInvalidCoins(amount.String())
+	// check coins are sorted and positive (disallow channels with zero balance)
+	if !coins.IsValid() {
+		return nil, sdk.ErrInvalidCoins(coins.String())
 	}
-		// Check if coins are sorted, non zero, positive
-		if !amount.IsValid() {
-			return nil, sdk.ErrInvalidCoins(amount.String())
+	if !coins.IsPositive() {
+		return nil, sdk.ErrInvalidCoins(coins.String())
 	}
-		if !amount.IsPositive() {
-			return nil, sdk.ErrInvalidCoins(amount.String())
-		}
-		// sender should exist already as they had to sign.
-		// receiver address exists. am is the account mapper in the coin keeper.
-		// TODO automatically create account if not present?
-		// TODO remove as account mapper not available to this pkg
-		//if k.coinKeeper.am.GetAccount(ctx, receiver) == nil {
-		//	return nil, sdk.ErrUnknownAddress(receiver.String())
-		//}
-
-		// sender has enough coins - done in Subtract method
-		// TODO check if sender and receiver different?
-	*/

 	// subtract coins from sender
 	_, tags, err := k.coinKeeper.SubtractCoins(ctx, sender, coins)
@ -89,7 +74,10 @@ func (k Keeper) CreateChannel(ctx sdk.Context, sender sdk.AccAddress, receiver s
 func (k Keeper) InitCloseChannelBySender(ctx sdk.Context, update Update) (sdk.Tags, sdk.Error) {
 	// This is roughly the default path for non unidirectional channels

-	// TODO Validate update - e.g. check signed by sender
+	err := k.validateUpdate(ctx, update)
+	if err != nil {
+		return nil, err
+	}

 	q, found := k.getSubmittedUpdatesQueue(ctx)
 	if !found {
@ -108,7 +96,7 @@ func (k Keeper) InitCloseChannelBySender(ctx sdk.Context, update Update) (sdk.Ta
 		// However in unidirectional case, only the sender can close a channel this way. No clear need for them to be able to submit an update replacing a previous one they sent, so don't allow it.
 		// TODO tags
 		// TODO custom errors return sdk.EmptyTags(), sdk.NewError("Sender can't submit an update for channel if one has already been submitted.")
-		panic("Sender can't submit an update for channel if one has already been submitted.")
+		sdk.ErrInternal("Sender can't submit an update for channel if one has already been submitted.")
 	} else {
 		// No one has tried to update channel
 		submittedUpdate := SubmittedUpdate{
@ -124,7 +112,11 @@ func (k Keeper) InitCloseChannelBySender(ctx sdk.Context, update Update) (sdk.Ta
 }

 func (k Keeper) CloseChannelByReceiver(ctx sdk.Context, update Update) (sdk.Tags, sdk.Error) {
-	// TODO Validate update
+
+	err := k.validateUpdate(ctx, update)
+	if err != nil {
+		return nil, err
+	}

 	// Check if there is an update in the queue already
 	q, found := k.getSubmittedUpdatesQueue(ctx)
@ -160,16 +152,50 @@ func (k Keeper) CloseChannelByReceiver(ctx sdk.Context, update Update) (sdk.Tags
 // 	return returnUpdate
 // }

+func (k Keeper) validateUpdate(ctx sdk.Context, update Update) sdk.Error {
+	// Check that channel exists
+	channel, found := k.getChannel(ctx, update.ChannelID)
+	if !found {
+		return sdk.ErrInternal("Channel doesn't exist")
+	}
+	// Check the num of payout participants match channel participants
+	if len(update.Payout) != len(channel.Participants) {
+		return sdk.ErrInternal("Payout doesn't match number of channel participants")
+	}
+	// Check each coins are valid
+	for _, coins := range update.Payout {
+		if !coins.IsValid() {
+			return sdk.ErrInternal("Payout coins aren't formatted correctly")
+		}
+	}
+	// Check payout coins are each not negative (can be zero though)
+	if !update.Payout.IsNotNegative() {
+		return sdk.ErrInternal("Payout cannot be negative")
+	}
+	// Check payout sums to match channel.Coins
+	if !channel.Coins.IsEqual(update.Payout.Sum()) {
+		return sdk.ErrInternal("Payout amount doesn't match channel amount")
+	}
+	// Check sender signature is OK
+	if !k.verifySignatures(ctx, channel, update) {
+		return sdk.ErrInternal("Signature on update not valid")
+	}
+	return nil
+}
+
 // unsafe close channel - doesn't check if update matches existing channel TODO make safer?
 func (k Keeper) closeChannel(ctx sdk.Context, update Update) (sdk.Tags, sdk.Error) {
 	var err sdk.Error
 	var tags sdk.Tags

+	channel, _ := k.getChannel(ctx, update.ChannelID)
+	// TODO check channel exists and participants matches update payout length
+
 	// Add coins to sender and receiver
 	// TODO check for possible errors first to avoid coins being half paid out?
-	for _, payout := range update.Payouts {
+	for i, coins := range update.Payout {
 		// TODO check somewhere if coins are not negative?
-		_, tags, err = k.coinKeeper.AddCoins(ctx, payout.Address, payout.Coins)
+		_, tags, err = k.coinKeeper.AddCoins(ctx, channel.Participants[i], coins)
 		if err != nil {
 			panic(err)
 		}
@ -180,6 +206,23 @@ func (k Keeper) closeChannel(ctx sdk.Context, update Update) (sdk.Tags, sdk.Erro
 	return tags, nil
 }

+func (k Keeper) verifySignatures(ctx sdk.Context, channel Channel, update Update) bool {
+	// In non unidirectional channels there will be more than one signature to check
+
+	signBytes := update.GetSignBytes()
+
+	address := channel.Participants[0]
+	pubKey := update.Sigs[0].PubKey
+	cryptoSig := update.Sigs[0].CryptoSignature
+
+	// Check public key submitted with update signature matches the account address
+	valid := bytes.Equal(pubKey.Address(), address) &&
+		// Check the signature is correct
+		pubKey.VerifyBytes(signBytes, cryptoSig)
+	return valid
+
+}
+
 // =========================================== QUEUE

 func (k Keeper) addToSubmittedUpdatesQueue(ctx sdk.Context, sUpdate SubmittedUpdate) {
@ -239,7 +282,7 @@ func (k Keeper) getSubmittedUpdatesQueueKey() []byte {
 }

 // ============= SUBMITTED UPDATES
-// These are keyed by the IDs of thei associated Channels
+// These are keyed by the IDs of their associated Channels
 // This section deals with only setting and getting

 func (k Keeper) getSubmittedUpdate(ctx sdk.Context, channelID ChannelID) (SubmittedUpdate, bool) {
@ -336,82 +379,3 @@ func (k Keeper) getChannelKey(channelID ChannelID) []byte {
 func (k Keeper) getLastChannelIDKey() []byte {
 	return []byte("lastChannelID")
 }
-
-/*
-// Close a payment channel and distribute funds to participants.
-func (k Keeper) ClosePaychan(ctx sdk.Context, sender sdk.Address, receiver sdk.Address, id int64, receiverAmount sdk.Coins) (sdk.Tags, sdk.Error) {
-	if len(sender) == 0 {
-		return nil, sdk.ErrInvalidAddress(sender.String())
-	}
-	if len(receiver) == 0 {
-		return nil, sdk.ErrInvalidAddress(receiver.String())
-	}
-	if len(receiverAmount) == 0 {
-		return nil, sdk.ErrInvalidCoins(receiverAmount.String())
-	}
-	// check id ≥ 0
-	if id < 0 {
-		return nil, sdk.ErrInvalidAddress(strconv.Itoa(int(id))) // TODO implement custom errors
-	}
-
-	// Check if coins are sorted, non zero, non negative
-	if !receiverAmount.IsValid() {
-		return nil, sdk.ErrInvalidCoins(receiverAmount.String())
-	}
-	if !receiverAmount.IsPositive() {
-		return nil, sdk.ErrInvalidCoins(receiverAmount.String())
-	}
-
-	store := ctx.KVStore(k.storeKey)
-
-	pych, exists := k.GetPaychan(ctx, sender, receiver, id)
-	if !exists {
-		return nil, sdk.ErrUnknownAddress("paychan not found") // TODO implement custom errors
-	}
-	// compute coin distribution
-	senderAmount := pych.Balance.Minus(receiverAmount) // Minus sdk.Coins method
-	// check that receiverAmt not greater than paychan balance
-	if !senderAmount.IsNotNegative() {
-		return nil, sdk.ErrInsufficientFunds(pych.Balance.String())
-	}
-	// add coins to sender
-	// creating account if it doesn't exist
-	k.coinKeeper.AddCoins(ctx, sender, senderAmount)
-	// add coins to receiver
-	k.coinKeeper.AddCoins(ctx, receiver, receiverAmount)
-
-	// delete paychan from db
-	pychKey := paychanKey(pych.Sender, pych.Receiver, pych.Id)
-	store.Delete(pychKey)
-
-	// TODO create tags
-	//sdk.NewTags(
-	//	"action", []byte("channel closure"),
-	//	"receiver", receiver.Bytes(),
-	//	"sender", sender.Bytes(),
-	//	"id", ??)
-	tags := sdk.NewTags()
-	return tags, nil
-}
-
-// Creates a key to reference a paychan in the blockchain store.
-func paychanKey(sender sdk.Address, receiver sdk.Address, id int64) []byte {
-
-	//sdk.Address is just a slice of bytes under a different name
-	//convert id to string then to byte slice
-	idAsBytes := []byte(strconv.Itoa(int(id)))
-	// concat sender and receiver and integer ID
-	key := append(sender.Bytes(), receiver.Bytes()...)
-	key = append(key, idAsBytes...)
-	return key
-}
-
-// Get all paychans between a given sender and receiver.
-func (k Keeper) GetPaychans(sender sdk.Address, receiver sdk.Address) []Paychan {
-	var paychans []Paychan
-	// TODO Implement this
-	return paychans
-}
-
-// maybe getAllPaychans(sender sdk.address) []Paychan
-*/
--- a/internal/x/paychan/keeper_test.go
+++ b/internal/x/paychan/keeper_test.go
@ -4,74 +4,152 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/assert"
 	//"github.com/stretchr/testify/require"
-	"github.com/tendermint/tendermint/crypto"
+	//"github.com/tendermint/tendermint/crypto"
 	"testing"
 )

 func TestKeeper(t *testing.T) {
-
 	t.Run("CreateChannel", func(t *testing.T) {

-		//
+		accountSeeds := []string{"senderSeed", "receiverSeed"}
+		const (
+			senderAccountIndex   int = 0
+			receiverAccountIndex int = 1
+		)
+		_, addrs, _, _ := createTestGenAccounts(accountSeeds, sdk.Coins{}) // pure function
+
+		testCases := []struct {
+			name                string
+			sender              sdk.AccAddress
+			receiver            sdk.AccAddress
+			coins               sdk.Coins
+			shouldCreateChannel bool
+			shouldError         bool
+		}{
+			{
+				"HappyPath",
+				addrs[senderAccountIndex],
+				addrs[receiverAccountIndex],
+				sdk.Coins{sdk.NewCoin("KVA", 10)},
+				true,
+				false,
+			},
+			{
+				"NilAddress",
+				sdk.AccAddress{},
+				sdk.AccAddress{},
+				sdk.Coins{sdk.NewCoin("KVA", 10)},
+				false,
+				true,
+			},
+			{
+				"NilCoins",
+				addrs[senderAccountIndex],
+				addrs[receiverAccountIndex],
+				sdk.Coins{},
+				false,
+				true,
+			},
+			{
+				"NegativeCoins",
+				addrs[senderAccountIndex],
+				addrs[receiverAccountIndex],
+				sdk.Coins{sdk.NewCoin("KVA", -57)},
+				false,
+				true,
+			},
+		}
+
+		for _, testCase := range testCases {
+			t.Run(testCase.name, func(t *testing.T) {
 				////// SETUP
 				// create basic mock app
-		ctx, coinKeeper, channelKeeper, addrs, genAccFunding := createMockApp()
-
-		sender := addrs[0]
-		receiver := addrs[1]
-		coins := sdk.Coins{sdk.NewCoin("KVA", 10)}
-
+				ctx, coinKeeper, channelKeeper, addrs, _, _, genAccFunding := createMockApp(accountSeeds)
 				//
 				////// ACTION
-		_, err := channelKeeper.CreateChannel(ctx, sender, receiver, coins)
+				_, err := channelKeeper.CreateChannel(ctx, testCase.sender, testCase.receiver, testCase.coins)

 				//
 				////// CHECK RESULTS
-		assert.Nil(t, err)
-		// channel exists with correct attributes
+				// Check error
+				if testCase.shouldError {
+					assert.Error(t, err)
+				} else {
+					assert.NoError(t, err)
+				}
+				// Check if channel exists and is correct
 				channelID := ChannelID(0) // should be 0 as first channel
+				createdChan, found := channelKeeper.getChannel(ctx, channelID)
+
+				if testCase.shouldCreateChannel {
 					expectedChan := Channel{
 						ID:           channelID,
-			Participants: [2]sdk.AccAddress{sender, receiver},
-			Coins:        coins,
+						Participants: [2]sdk.AccAddress{testCase.sender, testCase.receiver},
+						Coins:        testCase.coins,
 					}
-		createdChan, _ := channelKeeper.getChannel(ctx, channelID)
+
+					// channel exists and correct
+					assert.True(t, found)
 					assert.Equal(t, expectedChan, createdChan)
 					// check coins deducted from sender
-		assert.Equal(t, genAccFunding.Minus(coins), coinKeeper.GetCoins(ctx, sender))
+					assert.Equal(t, genAccFunding.Minus(testCase.coins), coinKeeper.GetCoins(ctx, testCase.sender))
 					// check no coins deducted from receiver
-		assert.Equal(t, genAccFunding, coinKeeper.GetCoins(ctx, receiver))
-		// check next chan id
+					assert.Equal(t, genAccFunding, coinKeeper.GetCoins(ctx, testCase.receiver))
+					// check next global channelID incremented
 					assert.Equal(t, ChannelID(1), channelKeeper.getNewChannelID(ctx))
+				} else {
+					// channel doesn't exist
+					assert.False(t, found)
+					assert.Equal(t, Channel{}, createdChan)
+					// check no coins deducted from sender
+					assert.Equal(t, genAccFunding, coinKeeper.GetCoins(ctx, addrs[senderAccountIndex]))
+					// check no coins deducted from receiver
+					assert.Equal(t, genAccFunding, coinKeeper.GetCoins(ctx, addrs[receiverAccountIndex]))
+					// check next global channelID not incremented
+					assert.Equal(t, ChannelID(0), channelKeeper.getNewChannelID(ctx))
+				}
+			})
+		}
 	})

-	t.Run("ReceiverCloseChannel", func(t *testing.T) {
-		// SETUP
-		ctx, coinKeeper, channelKeeper, addrs, genAccFunding := createMockApp()
+	t.Run("CloseChannelByReceiver", func(t *testing.T) {
+		// TODO convert to table driven and add more test cases
+		//		channel exists or not (assume channels correct)
+		//		various Updates
+		//		submittedUpdates existing or not (assume they are valid)
+
+		// SETUP
+		accountSeeds := []string{"senderSeed", "receiverSeed"}
+		const (
+			senderAccountIndex   int = 0
+			receiverAccountIndex int = 1
+		)
+		ctx, coinKeeper, channelKeeper, addrs, pubKeys, privKeys, genAccFunding := createMockApp(accountSeeds)

-		sender := addrs[0]
-		receiver := addrs[1]
 		coins := sdk.Coins{sdk.NewCoin("KVA", 10)}

 		// create new channel
 		channelID := ChannelID(0) // should be 0 as first channel
 		channel := Channel{
 			ID:           channelID,
-			Participants: [2]sdk.AccAddress{sender, receiver},
+			Participants: [2]sdk.AccAddress{addrs[senderAccountIndex], addrs[receiverAccountIndex]},
 			Coins:        coins,
 		}
 		channelKeeper.setChannel(ctx, channel)

 		// create closing update
-		payouts := Payouts{
-			{sender, sdk.Coins{sdk.NewCoin("KVA", 3)}},
-			{receiver, sdk.Coins{sdk.NewCoin("KVA", 7)}},
-		}
+		payout := Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}
 		update := Update{
 			ChannelID: channelID,
-			Payouts:   payouts,
-			Sigs:      [1]crypto.Signature{},
+			Payout:    payout,
+			// empty sig
 		}
+		cryptoSig, _ := privKeys[senderAccountIndex].Sign(update.GetSignBytes())
+		update.Sigs = [1]UpdateSignature{UpdateSignature{
+			PubKey:          pubKeys[senderAccountIndex],
+			CryptoSignature: cryptoSig,
+		}}
+
 		// Set empty submittedUpdatesQueue TODO work out proper genesis initialisation
 		channelKeeper.setSubmittedUpdatesQueue(ctx, SubmittedUpdatesQueue{})

@ -80,71 +158,171 @@ func TestKeeper(t *testing.T) {

 		// CHECK RESULTS
 		// no error
-		assert.Nil(t, err)
+		assert.NoError(t, err)
 		// coins paid out
-		senderPayout, _ := payouts.Get(sender)
-		assert.Equal(t, genAccFunding.Plus(senderPayout), coinKeeper.GetCoins(ctx, sender))
-		receiverPayout, _ := payouts.Get(receiver)
-		assert.Equal(t, genAccFunding.Plus(receiverPayout), coinKeeper.GetCoins(ctx, receiver))
+		senderPayout := payout[senderAccountIndex]
+		assert.Equal(t, genAccFunding.Plus(senderPayout), coinKeeper.GetCoins(ctx, addrs[senderAccountIndex]))
+		receiverPayout := payout[receiverAccountIndex]
+		assert.Equal(t, genAccFunding.Plus(receiverPayout), coinKeeper.GetCoins(ctx, addrs[receiverAccountIndex]))
 		// channel deleted
 		_, found := channelKeeper.getChannel(ctx, channelID)
 		assert.False(t, found)

 	})

-	t.Run("SenderInitCloseChannel", func(t *testing.T) {
+	t.Run("InitCloseChannelBySender", func(t *testing.T) {
+
+		// TODO do some documentation here
+		// Ideally this should mock calls to ctx.store.Get/Set - test the side effects without being dependent on implementatino details
+		// TODO test correct behaviour when a submittedUpdate already exists
+
+		accountSeeds := []string{"senderSeed", "receiverSeed", "notInChannelSeed"}
+		const (
+			senderAccountIndex   int = 0
+			receiverAccountIndex int = 1
+			otherAccountIndex    int = 2
+		)
+		chanID := ChannelID(0)
+
+		type testUpdate struct { // A parameterised version of an Update for use in specifying test cases.
+			channelID          ChannelID // channelID of submitted update
+			payout             Payout    // payout of submitted update
+			pubKeyAccountIndex int       // pubkey of signature of submitted update
+			sigAccountIndex    int       // crypto signature of signature of submitted update
+		}
+		testCases := []struct {
+			name                    string
+			setupChannel            bool
+			updateToSubmit          testUpdate
+			expectedSubmittedUpdate string // "empty" or "sameAsSubmitted"
+			shouldError             bool
+		}{
+			{
+				"HappyPath",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}, senderAccountIndex, senderAccountIndex},
+				"sameAsSubmited",
+				false,
+			},
+			{
+				"NoChannel",
+				false,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}, senderAccountIndex, senderAccountIndex},
+				"empty",
+				true,
+			},
+			{
+				"NoCoins",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{}}, senderAccountIndex, senderAccountIndex},
+				"empty",
+				true,
+			},
+			{
+				"NegativeCoins",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", -5)}, sdk.Coins{sdk.NewCoin("KVA", 15)}}, senderAccountIndex, senderAccountIndex},
+				"empty",
+				true,
+			},
+			{
+				"TooManyCoins",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", 100)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}, senderAccountIndex, senderAccountIndex},
+				"empty",
+				true,
+			},
+			{
+				"WrongSignature",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}, senderAccountIndex, otherAccountIndex},
+				"empty",
+				true,
+			},
+			{
+				"WrongPubKey",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}, otherAccountIndex, senderAccountIndex},
+				"empty",
+				true,
+			},
+			{
+				"ReceiverSigned",
+				true,
+				testUpdate{chanID, Payout{sdk.Coins{sdk.NewCoin("KVA", 3)}, sdk.Coins{sdk.NewCoin("KVA", 7)}}, receiverAccountIndex, receiverAccountIndex},
+				"empty",
+				true,
+			},
+		}
+		for _, testCase := range testCases {
+			t.Run(testCase.name, func(t *testing.T) {
+
 				// SETUP
-		ctx, _, channelKeeper, addrs, _ := createMockApp()
-
-		sender := addrs[0]
-		receiver := addrs[1]
-		coins := sdk.Coins{sdk.NewCoin("KVA", 10)}
-
-		// create new channel
-		channelID := ChannelID(0) // should be 0 as first channel
-		channel := Channel{
-			ID:           channelID,
-			Participants: [2]sdk.AccAddress{sender, receiver},
-			Coins:        coins,
-		}
-		channelKeeper.setChannel(ctx, channel)
-
-		// create closing update
-		payouts := Payouts{
-			{sender, sdk.Coins{sdk.NewCoin("KVA", 3)}},
-			{receiver, sdk.Coins{sdk.NewCoin("KVA", 7)}},
-		}
-		update := Update{
-			ChannelID: channelID,
-			Payouts:   payouts,
-			Sigs:      [1]crypto.Signature{},
-		}
+				ctx, _, channelKeeper, addrs, pubKeys, privKeys, _ := createMockApp(accountSeeds)
 				// Set empty submittedUpdatesQueue TODO work out proper genesis initialisation
 				channelKeeper.setSubmittedUpdatesQueue(ctx, SubmittedUpdatesQueue{})
+				// create new channel
+				if testCase.setupChannel {
+					channel := Channel{
+						ID:           chanID, // should be 0 as first channel
+						Participants: [2]sdk.AccAddress{addrs[senderAccountIndex], addrs[receiverAccountIndex]},
+						Coins:        sdk.Coins{sdk.NewCoin("KVA", 10)},
+					}
+					channelKeeper.setChannel(ctx, channel)
+				}
+
+				// create update
+				// basic values
+				updateToSubmit := Update{
+					ChannelID: testCase.updateToSubmit.channelID,
+					Payout:    testCase.updateToSubmit.payout,
+					// empty sig
+				}
+				// create update's signature
+				cryptoSig, _ := privKeys[testCase.updateToSubmit.sigAccountIndex].Sign(updateToSubmit.GetSignBytes())
+				updateToSubmit.Sigs = [1]UpdateSignature{UpdateSignature{
+					PubKey:          pubKeys[testCase.updateToSubmit.pubKeyAccountIndex],
+					CryptoSignature: cryptoSig,
+				}}

 				// ACTION
-		_, err := channelKeeper.InitCloseChannelBySender(ctx, update)
+				_, err := channelKeeper.InitCloseChannelBySender(ctx, updateToSubmit)

 				// CHECK RESULTS
-		// no error
-		assert.Nil(t, err)
-		// submittedupdate in queue and correct
-		suq, found := channelKeeper.getSubmittedUpdatesQueue(ctx)
-		assert.True(t, found)
-		assert.True(t, suq.Contains(channelID))
-
-		su, found := channelKeeper.getSubmittedUpdate(ctx, channelID)
-		assert.True(t, found)
-		expectedSubmittedUpdate := SubmittedUpdate{
-			Update:        update,
-			ExecutionTime: ChannelDisputeTime,
+				// Check error
+				if testCase.shouldError {
+					assert.Error(t, err)
+				} else {
+					assert.NoError(t, err)
 				}
-		assert.Equal(t, expectedSubmittedUpdate, su)
-		// TODO check channel is still in db and coins haven't changed?
+				// Check submittedUpdate
+				su, found := channelKeeper.getSubmittedUpdate(ctx, chanID)
+				switch testCase.expectedSubmittedUpdate {
+				case "empty":
+					assert.False(t, found)
+					assert.Zero(t, su)
+				case "sameAsSubmitted":
+					assert.True(t, found)
+					expectedSU := SubmittedUpdate{updateToSubmit, ChannelDisputeTime}
+					assert.Equal(t, expectedSU, su)
+				}
+
+			})
+		}
+
 	})

 }

+/*
+func privAndAddr() (crypto.PrivKey, sdk.AccAddress) {
+	priv := ed25519.GenPrivKey()
+	addr := sdk.AccAddress(priv.PubKey().Address())
+	return priv, addr
+
+sig, err := priv.Sign(signBytes)
+*/
+
 /*

 func setupMultiStore() (sdk.MultiStore, *sdk.KVStoreKey, *sdk.KVStoreKey) {
--- a/internal/x/paychan/test_common.go
+++ b/internal/x/paychan/test_common.go
@ -4,14 +4,17 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/mock"
 	//"github.com/stretchr/testify/require"
+	"github.com/cosmos/cosmos-sdk/x/auth"
 	"github.com/cosmos/cosmos-sdk/x/bank"
 	abci "github.com/tendermint/tendermint/abci/types"
+	"github.com/tendermint/tendermint/crypto"
+	"github.com/tendermint/tendermint/crypto/ed25519"
 )

 // Setup an example app with an in memory DB and the required keepers
 // Also create two accounts with 1000KVA
 // Could do with refactoring
-func createMockApp() (sdk.Context, bank.Keeper, Keeper, []sdk.AccAddress, sdk.Coins) {
+func createMockApp(accountSeeds []string) (sdk.Context, bank.Keeper, Keeper, []sdk.AccAddress, []crypto.PubKey, []crypto.PrivKey, sdk.Coins) {
 	mApp := mock.NewApp() // creates a half complete app
 	coinKeeper := bank.NewKeeper(mApp.AccountMapper)

@ -24,9 +27,8 @@ func createMockApp() (sdk.Context, bank.Keeper, Keeper, []sdk.AccAddress, sdk.Co
 	mApp.CompleteSetup([]*sdk.KVStoreKey{keyChannel}) // needs to be called I think to finish setup

 	// create some accounts
-	numGenAccs := 2 // create two initial accounts
 	genAccFunding := sdk.Coins{sdk.NewCoin("KVA", 1000)}
-	genAccs, addrs, _, _ := mock.CreateGenAccounts(numGenAccs, genAccFunding)
+	genAccs, addrs, pubKeys, privKeys := createTestGenAccounts(accountSeeds, genAccFunding)

 	// initialize the app with these accounts
 	mock.SetGenesis(mApp, genAccs)
@ -34,5 +36,27 @@ func createMockApp() (sdk.Context, bank.Keeper, Keeper, []sdk.AccAddress, sdk.Co
 	mApp.BeginBlock(abci.RequestBeginBlock{}) // going off other module tests
 	ctx := mApp.BaseApp.NewContext(false, abci.Header{})

-	return ctx, coinKeeper, channelKeeper, addrs, genAccFunding
+	return ctx, coinKeeper, channelKeeper, addrs, pubKeys, privKeys, genAccFunding
+}
+
+// CreateTestGenAccounts deterministically generates genesis accounts loaded with coins, and returns
+// their addresses, pubkeys, and privkeys.
+func createTestGenAccounts(accountSeeds []string, genCoins sdk.Coins) (genAccs []auth.Account, addrs []sdk.AccAddress, pubKeys []crypto.PubKey, privKeys []crypto.PrivKey) {
+	for _, seed := range accountSeeds {
+		privKey := ed25519.GenPrivKeyFromSecret([]byte(seed))
+		pubKey := privKey.PubKey()
+		addr := sdk.AccAddress(pubKey.Address())
+
+		genAcc := &auth.BaseAccount{
+			Address: addr,
+			Coins:   genCoins,
+		}
+
+		genAccs = append(genAccs, genAcc)
+		privKeys = append(privKeys, privKey)
+		pubKeys = append(pubKeys, pubKey)
+		addrs = append(addrs, addr)
+	}
+
+	return
 }
--- a/internal/x/paychan/types.go
+++ b/internal/x/paychan/types.go
@ -3,7 +3,6 @@ package paychan
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/tendermint/tendermint/crypto"
-	"reflect"
 )

 /*  CHANNEL TYPES  */
@ -22,25 +21,56 @@ type ChannelID int64 // TODO should this be positive only
 // The data that is passed between participants as payments, and submitted to the blockchain to close a channel.
 type Update struct {
 	ChannelID ChannelID
-	Payouts   Payouts //map[string]sdk.Coins // map of bech32 addresses to coins
+	Payout    Payout //map[string]sdk.Coins // map of bech32 addresses to coins
 	//Sequence  int64 Not needed for unidirectional channels
-	Sigs [1]crypto.Signature // only sender needs to sign in unidirectional
+	Sigs [1]UpdateSignature // only sender needs to sign in unidirectional
 }
-type Payout struct {
-	Address sdk.AccAddress
-	Coins   sdk.Coins
+
+func (u Update) GetSignBytes() []byte {
+	bz, err := msgCdc.MarshalJSON(struct {
+		ChannelID ChannelID
+		Payout    Payout
+	}{
+		ChannelID: u.ChannelID,
+		Payout:    u.Payout})
+
+	if err != nil {
+		panic(err)
+	}
+	return sdk.MustSortJSON(bz)
+}
+
+type UpdateSignature struct {
+	PubKey          crypto.PubKey
+	CryptoSignature crypto.Signature
+}
+
+type Payout []sdk.Coins // a list of coins to be paid to each of Channel.Participants
+func (p Payout) IsNotNegative() bool {
+	result := true
+	for _, coins := range p {
+		result = result && coins.IsNotNegative()
+	}
+	return result
+}
+func (p Payout) Sum() sdk.Coins {
+	var total sdk.Coins
+	for _, coins := range p {
+		total = total.Plus(coins.Sort())
+		total = total.Sort()
+	}
+	return total
 }
-type Payouts []Payout

 // Get the coins associated with payout address. TODO constrain payouts to only have one entry per address
-func (payouts Payouts) Get(addr sdk.AccAddress) (sdk.Coins, bool) {
+/*func (payouts Payouts) Get(addr sdk.AccAddress) (sdk.Coins, bool) {
 	for _, p := range payouts {
 		if reflect.DeepEqual(p.Address, addr) {
 			return p.Coins, true
 		}
 	}
 	return nil, false
-}
+}*/

 const ChannelDisputeTime = int64(2000) // measured in blocks TODO pick reasonable time

--- a/internal/x/paychan/types_test.go
+++ b/internal/x/paychan/types_test.go
@ -1,6 +1,7 @@
 package paychan

 import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/assert"
 	"testing"
 )
@ -24,3 +25,21 @@ func TestSubmittedUpdatesQueue(t *testing.T) {
 		assert.Equal(t, expectedQ, q)
 	})
 }
+
+func TestPayout(t *testing.T) {
+	t.Run("IsNotNegative", func(t *testing.T) {
+		p := Payout{sdk.Coins{sdk.NewCoin("USD", 4), sdk.NewCoin("GBP", 0)}, sdk.Coins{sdk.NewCoin("USD", 129879234), sdk.NewCoin("GBP", 1)}}
+		assert.True(t, p.IsNotNegative())
+
+		p = Payout{sdk.Coins{sdk.NewCoin("USD", -4), sdk.NewCoin("GBP", 0)}, sdk.Coins{sdk.NewCoin("USD", 129879234), sdk.NewCoin("GBP", 1)}}
+		assert.False(t, p.IsNotNegative())
+	})
+	t.Run("Sum", func(t *testing.T) {
+		p := Payout{
+			sdk.Coins{sdk.NewCoin("EUR", 1), sdk.NewCoin("USD", -5)},
+			sdk.Coins{sdk.NewCoin("EUR", 1), sdk.NewCoin("USD", 100), sdk.NewCoin("GBP", 1)},
+		}
+		expected := sdk.Coins{sdk.NewCoin("EUR", 2), sdk.NewCoin("GBP", 1), sdk.NewCoin("USD", 95)}
+		assert.Equal(t, expected, p.Sum())
+	})
+}