accounts: use UpsertAccountPayment in removePayment

Instead of UpdateAccount.

Author: ellemouton

accounts/errors.go

@@ -15,7 +15,9 @@ var (
 	ErrAlreadySucceeded = errors.New("payment has already succeeded")
 
 	// ErrPaymentNotAssociated indicate that the payment with the given hash
-	// has not yet been associated with the account in question.
+	// has not yet been associated with the account in question. It is also
+	// returned when the WithErrIfUnknown option is used with
+	// UpsertAccountPayment if the payment is not yet known.
 	ErrPaymentNotAssociated = errors.New(
 		"payment not associated with account",
 	)

accounts/interface.go

@@ -345,6 +345,7 @@ type upsertAcctPaymentOption struct {
 	errIfAlreadyPending   bool
 	usePendingAmount      bool
 	errIfAlreadySucceeded bool
+	errIfUnknown          bool
 }
 
 // newUpsertPaymentOption creates a new upsertAcctPaymentOption with default
@@ -355,6 +356,7 @@ func newUpsertPaymentOption() *upsertAcctPaymentOption {
 		errIfAlreadyPending:   false,
 		usePendingAmount:      false,
 		errIfAlreadySucceeded: false,
+		errIfUnknown:          false,
 	}
 }
 
@@ -394,3 +396,12 @@ func WithPendingAmount() UpsertPaymentOption {
 		o.usePendingAmount = true
 	}
 }
+
+// WithErrIfUnknown is a functional option that can be passed to the
+// UpsertAccountPayment method to indicate that the ErrPaymentNotAssociated
+// error should be returned if the payment is not associated with the account.
+func WithErrIfUnknown() UpsertPaymentOption {
+	return func(o *upsertAcctPaymentOption) {
+		o.errIfUnknown = true
+	}
+}

accounts/service.go

@@ -840,23 +840,24 @@ func (s *InterceptorService) removePayment(ctx context.Context,
 		return nil
 	}
 
-	account, err := s.store.Account(ctx, pendingPayment.accountID)
-	if err != nil {
-		return err
+	_, err := s.store.UpsertAccountPayment(
+		ctx, pendingPayment.accountID, hash, 0, status,
+		// We don't want the payment to be inserted if it isn't already
+		// known. So we pass in this option to ensure that the call
+		// exits early if the payment is unknown.
+		WithErrIfUnknown(),
+		// Otherwise, we just want to update the status of the payment
+		// and use the existing pending amount.
+		WithPendingAmount(),
+	)
+	if err != nil && !errors.Is(err, ErrPaymentNotAssociated) {
+		return fmt.Errorf("error updating account: %w", err)
 	}
 
 	pendingPayment.cancel()
 	delete(s.pendingPayments, hash)
 
-	// Have we associated the payment with the account already?
-	_, ok = account.Payments[hash]
-	if !ok {
-		return nil
-	}
-
-	// If we did, let's set the status correctly in the DB now.
-	account.Payments[hash].Status = status
-	return s.store.UpdateAccount(ctx, account)
+	return nil
 }
 
 // successState returns true if a payment was completed successfully.

accounts/store_kvdb.go

@@ -300,6 +300,8 @@ func (s *BoltStore) UpsertAccountPayment(_ context.Context, id AccountID,
 			if opts.usePendingAmount {
 				fullAmount = entry.FullAmount
 			}
+		} else if opts.errIfUnknown {
+			return ErrPaymentNotAssociated
 		}
 
 		account.Payments[paymentHash] = &PaymentEntry{

accounts/store_test.go

@@ -430,6 +430,74 @@ func TestAccountUpdateMethods(t *testing.T) {
 		// error.
 		err = store.DeleteAccountPayment(ctx, acct.ID, hash1)
 		require.ErrorIs(t, err, ErrPaymentNotAssociated)
+
+		// Try once more to insert a payment that is currently unknown
+		// but this time add the WithErrIfUnknown option. This should
+		// return the ErrPaymentNotAssociated error.
+		_, err = store.UpsertAccountPayment(
+			ctx, acct.ID, hash1, 600, lnrpc.Payment_SUCCEEDED,
+			WithErrIfUnknown(),
+		)
+		require.ErrorIs(t, err, ErrPaymentNotAssociated)
+
+		// Show that using the two options WithErrIfUnknown and
+		// WithPendingAmount together will return the
+		// ErrPaymentNotAssociated and will not successfully update
+		// the status. We call this for hash1 since it is no longer
+		// known. We do this to simulate the behaviour of
+		// removePayment.
+		_, err = store.UpsertAccountPayment(
+			ctx, acct.ID, hash1, 0, lnrpc.Payment_SUCCEEDED,
+			WithErrIfUnknown(),
+			WithPendingAmount(),
+		)
+		require.ErrorIs(t, err, ErrPaymentNotAssociated)
+
+		assertBalanceAndPayments(400, AccountPayments{
+			hash2: &PaymentEntry{
+				Status:     lnrpc.Payment_SUCCEEDED,
+				FullAmount: 100,
+			},
+		})
+
+		// Now insert hash 1 again.
+		_, err = store.UpsertAccountPayment(
+			ctx, acct.ID, hash1, 600, lnrpc.Payment_IN_FLIGHT,
+		)
+		require.NoError(t, err)
+
+		assertBalanceAndPayments(400, AccountPayments{
+			hash1: &PaymentEntry{
+				Status:     lnrpc.Payment_IN_FLIGHT,
+				FullAmount: 600,
+			},
+			hash2: &PaymentEntry{
+				Status:     lnrpc.Payment_SUCCEEDED,
+				FullAmount: 100,
+			},
+		})
+
+		// Once again call UpsertAccountPayment with both the
+		// WithErrIfUnknown and WithPendingAmount options. This time
+		// it should succeed since the payment is now known and so the
+		// status should be updated.
+		_, err = store.UpsertAccountPayment(
+			ctx, acct.ID, hash1, 0, lnrpc.Payment_SUCCEEDED,
+			WithErrIfUnknown(),
+			WithPendingAmount(),
+		)
+		require.NoError(t, err)
+
+		assertBalanceAndPayments(400, AccountPayments{
+			hash1: &PaymentEntry{
+				Status:     lnrpc.Payment_SUCCEEDED,
+				FullAmount: 600,
+			},
+			hash2: &PaymentEntry{
+				Status:     lnrpc.Payment_SUCCEEDED,
+				FullAmount: 100,
+			},
+		})
 	})
 }