rpcserver: refactor logic for ListPayments/DeleteAllPayments

This commit slightly refactors the logic for the new outgoing payment
related RPC’s to more closely match the style of the rest of the
codebase. Additionally the tests have been updated to reflect the
changes to the protos of the new RPC’s.

lnd_test.go

@@ -14,6 +14,9 @@ import (
 
 	"sync/atomic"
 
+	"encoding/hex"
+	"reflect"
+
 	"github.com/davecgh/go-spew/spew"
 	"github.com/go-errors/errors"
 	"github.com/lightningnetwork/lnd/lnrpc"
@@ -24,8 +27,6 @@ import (
 	"github.com/roasbeef/btcutil"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
-	"reflect"
-	"encoding/hex"
 )
 
 // harnessTest wraps a regular testing.T providing enhanced error detection
@@ -495,21 +496,18 @@ func testSingleHopInvoice(net *networkHarness, t *harnessTest) {
 
 func testListPayments(net *networkHarness, t *harnessTest) {
 	ctxb := context.Background()
-	timeout := time.Duration(time.Second * 2)
+	timeout := time.Duration(time.Second * 5)
 
-	// Delete all payments from Alice. DB should have no payments
+	// First start by deleting all payments that Alice knows of. This will
-	deleteAllPaymentsInitialReq := &lnrpc.DeleteAllPaymentsRequest{}
+	// allow us to execute the test with a clean state for Alice.
-	deleteAllPaymentsInitialCtxt, _ := context.WithTimeout(ctxb, timeout)
+	delPaymentsReq := &lnrpc.DeleteAllPaymentsRequest{}
-	_, err := net.Alice.DeleteAllPayments(deleteAllPaymentsInitialCtxt,
+	if _, err := net.Alice.DeleteAllPayments(ctxb, delPaymentsReq); err != nil {
-		deleteAllPaymentsInitialReq)
+		t.Fatalf("unable to delete payments: %v", err)
-	if err != nil {
-		t.Fatalf("Can't delete payments at the begining: %v", err)
 	}
 
 	// Check that there are no payments before test.
 	reqInit := &lnrpc.ListPaymentsRequest{}
-	reqInitCtxt, _ := context.WithTimeout(ctxb, timeout)
+	paymentsRespInit, err := net.Alice.ListPayments(ctxb, reqInit)
-	paymentsRespInit, err := net.Alice.ListPayments(reqInitCtxt, reqInit)
 	if err != nil {
 		t.Fatalf("error when obtaining Alice payments: %v", err)
 	}
@@ -518,18 +516,16 @@ func testListPayments(net *networkHarness, t *harnessTest) {
 			len(paymentsRespInit.Payments), 0)
 	}
 
-	// Open a channel with 100k satoshis
+	// Open a channel with 100k satoshis between Alice and Bob with Alice
-	// between Alice and Bob with Alice being
+	// being the sole funder of the channel.
-	// the sole funder of the channel.
-
 	chanAmt := btcutil.Amount(100000)
-	openChannelCtxt, _ := context.WithTimeout(ctxb, timeout)
+	ctxt, _ := context.WithTimeout(ctxb, timeout)
-	chanPoint := openChannelAndAssert(t, net, openChannelCtxt,
+	chanPoint := openChannelAndAssert(t, net, ctxt, net.Alice, net.Bob,
-		net.Alice, net.Bob, chanAmt)
+		chanAmt)
 
 	// Now that the channel is open, create an invoice for Bob which
-	// expects a payment of 1000 satoshis from Alice
+	// expects a payment of 1000 satoshis from Alice paid via a particular
-	// paid via a particular pre-image.
+	// pre-image.
 	const paymentAmt = 1000
 	preimage := bytes.Repeat([]byte("B"), 32)
 	invoice := &lnrpc.Invoice{
@@ -545,8 +541,7 @@ func testListPayments(net *networkHarness, t *harnessTest) {
 
 	// With the invoice for Bob added, send a payment towards Alice paying
 	// to the above generated invoice.
-	sendPaymentCtxt, _ := context.WithTimeout(ctxb, timeout)
+	sendStream, err := net.Alice.SendPayment(ctxb)
-	sendStream, err := net.Alice.SendPayment(sendPaymentCtxt)
 	if err != nil {
 		t.Fatalf("unable to create alice payment stream: %v", err)
 	}
@@ -562,12 +557,10 @@ func testListPayments(net *networkHarness, t *harnessTest) {
 		t.Fatalf("error when attempting recv: %v", err)
 	}
 
-	// We doesn't check different states of parties
+	// Grab Alice's list of payments, she should show the existence of
-	// like balance here because it is already checked in
+	// exactly one payment.
-	// testSingleHopInvoice
 	req := &lnrpc.ListPaymentsRequest{}
-	listPaymentsCtxt, _ := context.WithTimeout(ctxb, timeout)
+	paymentsResp, err := net.Alice.ListPayments(ctxb, req)
-	paymentsResp, err := net.Alice.ListPayments(listPaymentsCtxt, req)
 	if err != nil {
 		t.Fatalf("error when obtaining Alice payments: %v", err)
 	}
@@ -577,9 +570,9 @@ func testListPayments(net *networkHarness, t *harnessTest) {
 	}
 	p := paymentsResp.Payments[0]
 
-	// Check path.
+	// Ensure that the stored path shows a direct payment to Bob with no
+	// other nodes in-between.
 	expectedPath := []string{
-		net.Alice.PubKeyStr,
 		net.Bob.PubKeyStr,
 	}
 	if !reflect.DeepEqual(p.Path, expectedPath) {
@@ -587,52 +580,47 @@ func testListPayments(net *networkHarness, t *harnessTest) {
 			p.Path, expectedPath)
 	}
 
-	// Check amount.
+	// The payment amount should also match our previous payment directly.
 	if p.Value != paymentAmt {
 		t.Fatalf("incorrect amount, got %v, want %v",
 			p.Value, paymentAmt)
 	}
 
-	// Check RHash.
+	// The payment hash (or r-hash) should have been stored correctly.
 	correctRHash := hex.EncodeToString(invoiceResp.RHash)
-	if !reflect.DeepEqual(p.RHash, correctRHash) {
+	if !reflect.DeepEqual(p.PaymentHash, correctRHash) {
 		t.Fatalf("incorrect RHash, got %v, want %v",
-			p.RHash, correctRHash)
+			p.PaymentHash, correctRHash)
 	}
 
-	// Check Fee.
+	// Finally, as we made a single-hop direct payment, there should have
-	// Currently there is no fees so assume value 0 for fees.
+	// been no fee applied.
 	if p.Fee != 0 {
 		t.Fatalf("incorrect Fee, got %v, want %v", p.Fee, 0)
 	}
 
 	// Delete all payments from Alice. DB should have no payments.
-	deleteAllPaymentsEndReq := &lnrpc.DeleteAllPaymentsRequest{}
+	delReq := &lnrpc.DeleteAllPaymentsRequest{}
-	deleteAllPaymentsEndCtxt, _ := context.WithTimeout(ctxb, timeout)
+	_, err = net.Alice.DeleteAllPayments(ctxb, delReq)
-	_, err = net.Alice.DeleteAllPayments(deleteAllPaymentsEndCtxt,
-		deleteAllPaymentsEndReq)
 	if err != nil {
 		t.Fatalf("Can't delete payments at the end: %v", err)
 	}
 
 	// Check that there are no payments before test.
-	reqEnd := &lnrpc.ListPaymentsRequest{}
+	listReq := &lnrpc.ListPaymentsRequest{}
-	listPaymentsEndCtxt, _ := context.WithTimeout(ctxb, timeout)
+	paymentsResp, err = net.Alice.ListPayments(ctxb, listReq)
-	_, err = net.Alice.ListPayments(listPaymentsEndCtxt, reqEnd)
 	if err != nil {
 		t.Fatalf("error when obtaining Alice payments: %v", err)
 	}
-	if len(paymentsRespInit.Payments) != 0 {
+	if len(paymentsResp.Payments) != 0 {
 		t.Fatalf("incorrect number of payments, got %v, want %v",
 			len(paymentsRespInit.Payments), 0)
 	}
 
-	closeChannelCtxt, _ := context.WithTimeout(ctxb, timeout)
+	ctxt, _ = context.WithTimeout(ctxb, timeout)
-	closeChannelAndAssert(t, net, closeChannelCtxt,
+	closeChannelAndAssert(t, net, ctxt, net.Alice, chanPoint, false)
-		net.Alice, chanPoint, false)
 }
 
-
 func testMultiHopPayments(net *networkHarness, t *harnessTest) {
 	const chanAmt = btcutil.Amount(100000)
 	ctxb := context.Background()
@@ -1071,11 +1059,11 @@ func testMaxPendingChannels(net *networkHarness, t *harnessTest) {
 		chanPoints[i] = fundingChanPoint
 	}
 
-	// Finally close the channel between Alice and Carol, asserting that the
+	// Finally close the channel between Alice and Carol, asserting that
-	// channel has been properly closed on-chain.
+	// the channel has been properly closed on-chain.
 	for _, chanPoint := range chanPoints {
-		ctx, _ = context.WithTimeout(context.Background(), timeout)
+		ctxt, _ := context.WithTimeout(context.Background(), timeout)
-		closeChannelAndAssert(t, net, ctx, net.Alice, chanPoint, false)
+		closeChannelAndAssert(t, net, ctxt, net.Alice, chanPoint, false)
 	}
 }
 

routing/router.go

@@ -535,7 +535,7 @@ func (r *ChannelRouter) processNetworkAnnouncement(msg lnwire.Message) bool {
 		}
 
 		// As edges are directional edge node has a unique policy for
-		// the direction of th edge they control. Therefore we first
+		// the direction of the edge they control. Therefore we first
 		// check if we already have the most up to date information for
 		// that edge. If so, then we can exit early.
 		updateTimestamp := time.Unix(int64(msg.Timestamp), 0)

rpcserver.go

@@ -610,26 +610,31 @@ func (r *rpcServer) ListChannels(ctx context.Context,
 	return resp, nil
 }
 
-func constructPayment(route *routing.Route, amount btcutil.Amount,
+// savePayment saves a successfully completed payment to the database for
-	rHash []byte) *channeldb.OutgoingPayment {
+// historical record keeping.
+func (r *rpcServer) savePayment(route *routing.Route, amount btcutil.Amount,
+	rHash []byte) error {
 
-	payment := &channeldb.OutgoingPayment{}
+	paymentPath := make([][33]byte, len(route.Hops))
-
-	// When we create payment we do not know preImage.
-	// So we need to save rHash
-	copy(payment.RHash[:], rHash)
-
-	payment.Invoice.Terms.Value = btcutil.Amount(amount)
-	payment.Invoice.CreationDate = time.Now()
-	payment.Timestamp = time.Now()
-
-	pathBytes33 := make([][33]byte, len(route.Hops))
 	for i, hop := range route.Hops {
 		hopPub := hop.Channel.Node.PubKey.SerializeCompressed()
-		copy(pathBytes33[i][:], hopPub)
+		copy(paymentPath[i][:], hopPub)
 	}
-	payment.Path = pathBytes33
+
-	return payment
+	payment := &channeldb.OutgoingPayment{
+		Invoice: channeldb.Invoice{
+			Terms: channeldb.ContractTerm{
+				Value: btcutil.Amount(amount),
+			},
+			CreationDate: time.Now(),
+		},
+		Path:           paymentPath,
+		Fee:            route.TotalFees,
+		TimeLockLength: route.TotalTimeLock,
+	}
+	copy(payment.PaymentHash[:], rHash)
+
+	return r.server.chanDB.AddPayment(payment)
 }
 
 // SendPayment dispatches a bi-directional streaming RPC for sending payments
@@ -719,8 +724,7 @@ func (r *rpcServer) SendPayment(paymentStream lnrpc.Lightning_SendPaymentServer)
 
 				// Save the completed payment to the database
 				// for record keeping purposes.
-				payment := constructPayment(route, amt, rHash[:])
+				if err := r.savePayment(route, amt, rHash[:]); err != nil {
-				if err := r.server.chanDB.AddPayment(payment); err != nil {
 					errChan <- err
 					return
 				}
@@ -779,14 +783,15 @@ func (r *rpcServer) SendPaymentSync(ctx context.Context,
 		return nil, err
 	}
 
-	// Finally, send this next packet to the routing layer in order to
+	// Next, send this next packet to the routing layer in order to
 	// complete the next payment.
 	if err := r.server.htlcSwitch.SendHTLC(htlcPkt); err != nil {
 		return nil, err
 	}
 
-	payment := constructPayment(route, amt, rHash[:])
+	// With the payment completed successfully, we now ave the details of
-	if err := r.server.chanDB.AddPayment(payment); err != nil {
+	// the completed payment to the databse for historical record keeping.
+	if err := r.savePayment(route, amt, rHash[:]); err != nil {
 		return nil, err
 	}
 
@@ -1448,17 +1453,18 @@ func (r *rpcServer) ListPayments(context.Context,
 	paymentsResp := &lnrpc.ListPaymentsResponse{
 		Payments: make([]*lnrpc.Payment, len(payments)),
 	}
-	for i := 0; i < len(payments); i++ {
+	for i, payment := range payments {
-		p := &lnrpc.Payment{}
+		path := make([]string, len(payment.Path))
-		p.CreationDate = payments[i].CreationDate.Unix()
+		for i, hop := range payment.Path {
-		p.Value = int64(payments[i].Terms.Value)
+			path[i] = hex.EncodeToString(hop[:])
-		p.RHash = hex.EncodeToString(payments[i].RHash[:])
+		}
-		path := make([]string, len(payments[i].Path))
+
-		for j := 0; j < len(path); j++ {
+		paymentsResp.Payments[i] = &lnrpc.Payment{
-			path[j] = hex.EncodeToString(payments[i].Path[j][:])
+			PaymentHash:  hex.EncodeToString(payment.PaymentHash[:]),
+			Value:        int64(payment.Terms.Value),
+			CreationDate: payment.CreationDate.Unix(),
+			Path:         path,
 		}
-		p.Path = path
-		paymentsResp.Payments[i] = p
 	}
 
 	return paymentsResp, nil
@@ -1470,9 +1476,11 @@ func (r *rpcServer) DeleteAllPayments(context.Context,
 
 	rpcsLog.Debugf("[DeleteAllPayments]")
 
-	err := r.server.chanDB.DeleteAllPayments()
+	if err := r.server.chanDB.DeleteAllPayments(); err != nil {
-	resp := &lnrpc.DeleteAllPaymentsResponse{}
+		return nil, err
-	return resp, err
+	}
+
+	return &lnrpc.DeleteAllPaymentsResponse{}, nil
 }
 
 // SetAlias...