swap: fix race condition in tests (#1693)

* swap: fix race condition in tests

swap/protocol_test.go

@@ -19,8 +19,8 @@ package swap
 import (
 	"context"
 	"testing"
+	"time"
 
-	"github.com/ethereum/go-ethereum/accounts/abi/bind/backends"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/p2p"
@@ -42,7 +42,10 @@ func TestHandshake(t *testing.T) {
 	defer clean()
 
 	ctx := context.Background()
-	testDeploy(ctx, swap.backend, swap)
+	err = testDeploy(ctx, swap.backend, swap)
+	if err != nil {
+		t.Fatal(err)
+	}
 	// setup the protocolTester, which will allow protocol testing by sending messages
 	protocolTester := p2ptest.NewProtocolTester(swap.owner.privateKey, 2, swap.run)
 
@@ -116,10 +119,15 @@ func TestEmitCheque(t *testing.T) {
 	ctx := context.Background()
 
 	log.Debug("deploy to simulated backend")
-	testDeploy(ctx, creditorSwap.backend, creditorSwap)
-	testDeploy(ctx, debitorSwap.backend, debitorSwap)
-	creditorSwap.backend.(*backends.SimulatedBackend).Commit()
-	debitorSwap.backend.(*backends.SimulatedBackend).Commit()
+	var err error
+	err = testDeploy(ctx, creditorSwap.backend, creditorSwap)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = testDeploy(ctx, debitorSwap.backend, debitorSwap)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	log.Debug("create peer instances")
 
@@ -137,7 +145,6 @@ func TestEmitCheque(t *testing.T) {
 	}
 
 	log.Debug("create a cheque")
-	var err error
 	cheque := &Cheque{
 		ChequeParams: ChequeParams{
 			Contract:    debitorSwap.owner.Contract,
@@ -155,11 +162,23 @@ func TestEmitCheque(t *testing.T) {
 	emitMsg := &EmitChequeMsg{
 		Cheque: cheque,
 	}
+	// setup the wait for mined transaction function for testing
+	cleanup := setupContractTest()
+	defer cleanup()
 
+	// now we need to create the channel...
+	testBackend.submitDone = make(chan struct{})
 	err = creditorSwap.handleEmitChequeMsg(ctx, debitor, emitMsg)
 	if err != nil {
 		t.Fatal(err)
 	}
+	// ...on which we wait until the submitChequeAndCash is actually terminated (ensures proper nounce count)
+	select {
+	case <-testBackend.submitDone:
+		log.Debug("submit and cash transactions completed and committed")
+	case <-time.After(4 * time.Second):
+		t.Fatalf("Timeout waiting for submit and cash transactions to complete")
+	}
 	log.Debug("balance", "balance", creditorSwap.balances[debitor.ID()])
 	// check that the balance has been reset
 	if creditorSwap.balances[debitor.ID()] != 0 {
@@ -185,6 +204,10 @@ func TestTriggerPaymentThreshold(t *testing.T) {
 	debitorSwap, clean := newTestSwap(t, ownerKey)
 	defer clean()
 
+	// setup the wait for mined transaction function for testing
+	cleanup := setupContractTest()
+	defer cleanup()
+
 	// create a dummy pper
 	cPeer := newDummyPeerWithSpec(Spec)
 	creditor := NewPeer(cPeer.Peer, debitorSwap, common.Address{}, common.Address{})

swap/swap.go

@@ -220,6 +220,8 @@ func (s *Swap) handleMsg(p *Peer) func(ctx context.Context, msg interface{}) err
 	}
 }
 
+var defaultSubmitChequeAndCash = submitChequeAndCash
+
 // handleEmitChequeMsg should be handled by the creditor when it receives
 // a cheque from a debitor
 func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitChequeMsg) error {
@@ -242,7 +244,6 @@ func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitCheque
 		return err
 	}
 
-	// cash in cheque
 	opts := bind.NewKeyedTransactor(s.owner.privateKey)
 	opts.Context = ctx
 
@@ -251,30 +252,35 @@ func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitCheque
 		return err
 	}
 
-	// submit cheque to the blockchain and cashes it directly
-	go func() {
-		// blocks here, as we are waiting for the transaction to be mined
-		receipt, err := otherSwap.SubmitChequeBeneficiary(opts, s.backend, big.NewInt(int64(cheque.Serial)), big.NewInt(int64(cheque.Amount)), big.NewInt(int64(cheque.Timeout)), cheque.Signature)
-		if err != nil {
-			// TODO: do something with the error
-			// and we actually need to log this error as we are in an async routine; nobody is handling this error for now
-			log.Error("error submitting cheque", "err", err)
-			return
-		}
-		log.Debug("submit tx mined", "receipt", receipt)
-
-		receipt, err = otherSwap.CashChequeBeneficiary(opts, s.backend, s.owner.Contract, big.NewInt(int64(actualAmount)))
-		if err != nil {
-			// TODO: do something with the error
-			// and we actually need to log this error as we are in an async routine; nobody is handling this error for now
-			log.Error("error cashing cheque", "err", err)
-			return
-		}
-		log.Info("Cheque successfully submitted and cashed")
-	}()
+	// submit cheque and cash in async, otherwise this blocks here until the TX is mined
+	go defaultSubmitChequeAndCash(s, otherSwap, opts, actualAmount, cheque)
+
 	return err
 }
 
+// submitChequeAndCash should be called async as it blocks until the transaction(s) are mined
+// The function submits the cheque to the blockchain and then cashes it in directly
+func submitChequeAndCash(s *Swap, otherSwap contract.Contract, opts *bind.TransactOpts, actualAmount uint64, cheque *Cheque) {
+	// blocks here, as we are waiting for the transaction to be mined
+	receipt, err := otherSwap.SubmitChequeBeneficiary(opts, s.backend, big.NewInt(int64(cheque.Serial)), big.NewInt(int64(cheque.Amount)), big.NewInt(int64(cheque.Timeout)), cheque.Signature)
+	if err != nil {
+		// TODO: do something with the error
+		// and we actually need to log this error as we are in an async routine; nobody is handling this error for now
+		log.Error("error submitting cheque", "err", err)
+		return
+	}
+	log.Debug("submit tx mined", "receipt", receipt)
+
+	receipt, err = otherSwap.CashChequeBeneficiary(opts, s.backend, s.owner.Contract, big.NewInt(int64(actualAmount)))
+	if err != nil {
+		// TODO: do something with the error
+		// and we actually need to log this error as we are in an async routine; nobody is handling this error for now
+		log.Error("error cashing cheque", "err", err)
+		return
+	}
+	log.Info("Cheque successfully submitted and cashed")
+}
+
 // processAndVerifyCheque verifies the cheque and compares it with the last received cheque
 // if the cheque is valid it will also be saved as the new last cheque
 func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (uint64, error) {