Merge pull request #3838 from bhandras/i1404

channeldb+invoices: fix what pending invoice means in ChannelDB

channeldb/invoice_test.go

@@ -2,7 +2,6 @@ package channeldb
 
 import (
 	"crypto/rand"
-	mrand "math/rand"
 	"reflect"
 	"testing"
 	"time"
@@ -53,6 +52,29 @@ func randInvoice(value lnwire.MilliSatoshi) (*Invoice, error) {
 	return i, nil
 }
 
+// Tests that pending invoices are those which are either in ContractOpen or
+// in ContractAccepted state.
+func TestInvoiceIsPending(t *testing.T) {
+	contractStates := []ContractState{
+		ContractOpen, ContractSettled, ContractCanceled, ContractAccepted,
+	}
+
+	for _, state := range contractStates {
+		invoice := Invoice{
+			State: state,
+		}
+
+		// We expect that an invoice is pending if it's either in ContractOpen
+		// or ContractAccepted state.
+		pending := (state == ContractOpen || state == ContractAccepted)
+
+		if invoice.IsPending() != pending {
+			t.Fatalf("expected pending: %v, got: %v, invoice: %v",
+				pending, invoice.IsPending(), invoice)
+		}
+	}
+}
+
 func TestInvoiceWorkflow(t *testing.T) {
 	t.Parallel()
 
@@ -416,26 +438,28 @@ func TestFetchAllInvoicesWithPaymentHash(t *testing.T) {
 		t.Fatalf("expected empty list as a result, got: %v", empty)
 	}
 
-	// Now populate the DB and check if we can get all invoices with their
-	// payment hashes as expected.
-	const numInvoices = 20
-	testPendingInvoices := make(map[lntypes.Hash]*Invoice)
-	testAllInvoices := make(map[lntypes.Hash]*Invoice)
-
 	states := []ContractState{
 		ContractOpen, ContractSettled, ContractCanceled, ContractAccepted,
 	}
 
-	for i := lnwire.MilliSatoshi(1); i <= numInvoices; i++ {
-		invoice, err := randInvoice(i)
+	numInvoices := len(states) * 2
+	testPendingInvoices := make(map[lntypes.Hash]*Invoice)
+	testAllInvoices := make(map[lntypes.Hash]*Invoice)
+
+	// Now populate the DB and check if we can get all invoices with their
+	// payment hashes as expected.
+	for i := 1; i <= numInvoices; i++ {
+		invoice, err := randInvoice(lnwire.MilliSatoshi(i))
 		if err != nil {
 			t.Fatalf("unable to create invoice: %v", err)
 		}
 
-		invoice.State = states[mrand.Intn(len(states))]
+		// Set the contract state of the next invoice such that there's an equal
+		// number for all possbile states.
+		invoice.State = states[i%len(states)]
 		paymentHash := invoice.Terms.PaymentPreimage.Hash()
 
-		if invoice.State != ContractSettled && invoice.State != ContractCanceled {
+		if invoice.IsPending() {
 			testPendingInvoices[paymentHash] = invoice
 		}
 
@@ -578,6 +602,69 @@ func TestDuplicateSettleInvoice(t *testing.T) {
 	}
 }
 
+// TestFetchAllInvoices tests that FetchAllInvoices works as expected.
+func TestFetchAllInvoices(t *testing.T) {
+	t.Parallel()
+
+	db, cleanUp, err := makeTestDB()
+	defer cleanUp()
+	if err != nil {
+		t.Fatalf("unable to make test db: %v", err)
+	}
+
+	contractStates := []ContractState{
+		ContractOpen, ContractSettled, ContractCanceled, ContractAccepted,
+	}
+
+	numInvoices := len(contractStates) * 2
+
+	var expectedPendingInvoices []Invoice
+	var expectedAllInvoices []Invoice
+
+	for i := 1; i <= numInvoices; i++ {
+		invoice, err := randInvoice(lnwire.MilliSatoshi(i))
+
+		if err != nil {
+			t.Fatalf("unable to create invoice: %v", err)
+		}
+
+		invoice.AddIndex = uint64(i)
+		// Set the contract state of the next invoice such that there's an equal
+		// number for all possbile states.
+		invoice.State = contractStates[i%len(contractStates)]
+
+		paymentHash := invoice.Terms.PaymentPreimage.Hash()
+		if invoice.IsPending() {
+			expectedPendingInvoices = append(expectedPendingInvoices, *invoice)
+		}
+		expectedAllInvoices = append(expectedAllInvoices, *invoice)
+
+		if _, err := db.AddInvoice(invoice, paymentHash); err != nil {
+			t.Fatalf("unable to add invoice: %v", err)
+		}
+	}
+
+	pendingInvoices, err := db.FetchAllInvoices(true)
+	if err != nil {
+		t.Fatalf("unable to fetch all pending invoices: %v", err)
+	}
+
+	allInvoices, err := db.FetchAllInvoices(false)
+	if err != nil {
+		t.Fatalf("unable to fetch all non pending invoices: %v", err)
+	}
+
+	if !reflect.DeepEqual(pendingInvoices, expectedPendingInvoices) {
+		t.Fatalf("pending invoices: %v\n != \n expected einvoices: %v",
+			spew.Sdump(pendingInvoices), spew.Sdump(expectedPendingInvoices))
+	}
+
+	if !reflect.DeepEqual(allInvoices, expectedAllInvoices) {
+		t.Fatalf("pending + non pending: %v\n != \n expected: %v",
+			spew.Sdump(allInvoices), spew.Sdump(expectedAllInvoices))
+	}
+}
+
 // TestQueryInvoices ensures that we can properly query the invoice database for
 // invoices using different types of queries.
 func TestQueryInvoices(t *testing.T) {

channeldb/invoices.go

@@ -149,15 +149,13 @@ const (
 	// ContractOpen means the invoice has only been created.
 	ContractOpen ContractState = 0
 
-	// ContractSettled means the htlc is settled and the invoice has been
-	// paid.
+	// ContractSettled means the htlc is settled and the invoice has been paid.
 	ContractSettled ContractState = 1
 
 	// ContractCanceled means the invoice has been canceled.
 	ContractCanceled ContractState = 2
 
-	// ContractAccepted means the HTLC has been accepted but not settled
-	// yet.
+	// ContractAccepted means the HTLC has been accepted but not settled yet.
 	ContractAccepted ContractState = 3
 )
 
@@ -385,6 +383,11 @@ func validateInvoice(i *Invoice) error {
 	return nil
 }
 
+// IsPending returns ture if the invoice is in ContractOpen state.
+func (i *Invoice) IsPending() bool {
+	return i.State == ContractOpen || i.State == ContractAccepted
+}
+
 // AddInvoice inserts the targeted invoice into the database. If the invoice has
 // *any* payment hashes which already exists within the database, then the
 // insertion will be aborted and rejected due to the strict policy banning any
@@ -578,9 +581,9 @@ type InvoiceWithPaymentHash struct {
 
 // FetchAllInvoicesWithPaymentHash returns all invoices and their payment hashes
 // currently stored within the database. If the pendingOnly param is true, then
-// only unsettled invoices and their payment hashes will be returned, skipping
-// all invoices that are fully settled or canceled. Note that the returned
-// array is not ordered by add index.
+// only open or accepted invoices and their payment hashes will be returned,
+// skipping all invoices that are fully settled or canceled. Note that the
+// returned array is not ordered by add index.
 func (d *DB) FetchAllInvoicesWithPaymentHash(pendingOnly bool) (
 	[]InvoiceWithPaymentHash, error) {
 
@@ -617,10 +620,7 @@ func (d *DB) FetchAllInvoicesWithPaymentHash(pendingOnly bool) (
 				return err
 			}
 
-			if pendingOnly &&
-				(invoice.State == ContractSettled ||
-					invoice.State == ContractCanceled) {
-
+			if pendingOnly && !invoice.IsPending() {
 				return nil
 			}
 
@@ -643,8 +643,9 @@ func (d *DB) FetchAllInvoicesWithPaymentHash(pendingOnly bool) (
 }
 
 // FetchAllInvoices returns all invoices currently stored within the database.
-// If the pendingOnly param is true, then only unsettled invoices will be
-// returned, skipping all invoices that are fully settled.
+// If the pendingOnly param is set to true, then only invoices in open or
+// accepted state will be returned, skipping all invoices that are fully
+// settled or canceled.
 func (d *DB) FetchAllInvoices(pendingOnly bool) ([]Invoice, error) {
 	var invoices []Invoice
 
@@ -668,9 +669,7 @@ func (d *DB) FetchAllInvoices(pendingOnly bool) ([]Invoice, error) {
 				return err
 			}
 
-			if pendingOnly &&
-				invoice.State == ContractSettled {
-
+			if pendingOnly && !invoice.IsPending() {
 				return nil
 			}
 
@@ -816,11 +815,9 @@ func (d *DB) QueryInvoices(q InvoiceQuery) (InvoiceSlice, error) {
 				return err
 			}
 
-			// Skip any settled invoices if the caller is only
-			// interested in unsettled.
-			if q.PendingOnly &&
-				invoice.State == ContractSettled {
-
+			// Skip any settled or canceled invoices if the caller is
+			// only interested in pending ones.
+			if q.PendingOnly && !invoice.IsPending() {
 				continue
 			}
 

lnrpc/rpc.pb.go

@@ -7806,7 +7806,9 @@ func (m *PaymentHash) GetRHash() []byte {
 }
 
 type ListInvoiceRequest struct {
-	/// If set, only unsettled invoices will be returned in the response.
+	//*
+	//If set, only invoices that are not settled and not canceled will be returned
+	//in the response.
 	PendingOnly bool `protobuf:"varint,1,opt,name=pending_only,proto3" json:"pending_only,omitempty"`
 	//*
 	//The index of an invoice that will be used as either the start or end of a

lnrpc/rpc.proto

@@ -2516,7 +2516,10 @@ message PaymentHash {
 }
 
 message ListInvoiceRequest {
-    /// If set, only unsettled invoices will be returned in the response.
+    /**
+    If set, only invoices that are not settled and not canceled will be returned
+    in the response.
+    */
     bool pending_only = 1 [json_name = "pending_only"];
 
     /**

lnrpc/rpc.swagger.json

@@ -896,7 +896,7 @@
         "parameters": [
           {
             "name": "pending_only",
-            "description": "/ If set, only unsettled invoices will be returned in the response.",
+            "description": "*\nIf set, only invoices that are not settled and not canceled will be returned\nin the response.",
             "in": "query",
             "required": false,
             "type": "boolean",