accounts: use UpsertAccountPayment for TrackPayment

Instead of useing UpdateAccount.

Author: ellemouton

accounts/errors.go

@@ -8,4 +8,9 @@ var (
 	ErrLabelAlreadyExists = errors.New(
 		"account label uniqueness constraint violation",
 	)
+
+	// ErrAlreadySucceeded is returned by the UpsertAccountPayment method
+	// if the WithErrAlreadySucceeded option is used and the payment has
+	// already succeeded.
+	ErrAlreadySucceeded = errors.New("payment has already succeeded")
 )

accounts/interface.go

@@ -237,11 +237,14 @@ type Store interface {
 
 	// UpsertAccountPayment updates or inserts a payment entry for the given
 	// account. Various functional options can be passed to modify the
-	// behavior of the method.
+	// behavior of the method. The returned boolean is true if the payment
+	// was already known before the update. This is to be treated as a
+	// best-effort indication if an error is also returned since the method
+	// may error before the boolean can be set correctly.
 	UpsertAccountPayment(_ context.Context, id AccountID,
 		paymentHash lntypes.Hash, fullAmount lnwire.MilliSatoshi,
 		status lnrpc.Payment_PaymentStatus,
-		options ...UpsertPaymentOption) error
+		options ...UpsertPaymentOption) (bool, error)
 
 	// RemoveAccount finds an account by its ID and removes it from the¨
 	// store.
@@ -332,16 +335,20 @@ type UpsertPaymentOption func(*upsertAcctPaymentOption)
 // upsertAcctPaymentOption is a struct that holds optional parameters for the
 // UpsertAccountPayment method.
 type upsertAcctPaymentOption struct {
-	debitAccount        bool
-	errIfAlreadyPending bool
+	debitAccount          bool
+	errIfAlreadyPending   bool
+	usePendingAmount      bool
+	errIfAlreadySucceeded bool
 }
 
 // newUpsertPaymentOption creates a new upsertAcctPaymentOption with default
 // values.
 func newUpsertPaymentOption() *upsertAcctPaymentOption {
 	return &upsertAcctPaymentOption{
-		debitAccount:        false,
-		errIfAlreadyPending: false,
+		debitAccount:          false,
+		errIfAlreadyPending:   false,
+		usePendingAmount:      false,
+		errIfAlreadySucceeded: false,
 	}
 }
 
@@ -362,3 +369,22 @@ func WithErrIfAlreadyPending() UpsertPaymentOption {
 		o.errIfAlreadyPending = true
 	}
 }
+
+// WithErrIfAlreadySucceeded is a functional option that can be passed to the
+// UpsertAccountPayment method to indicate that the ErrAlreadySucceeded error
+// should be returned if the payment is already in a succeeded state.
+func WithErrIfAlreadySucceeded() UpsertPaymentOption {
+	return func(o *upsertAcctPaymentOption) {
+		o.errIfAlreadySucceeded = true
+	}
+}
+
+// WithPendingAmount is a functional option that can be passed to the
+// UpsertAccountPayment method to indicate that if the payment already exists,
+// then the known payment amount should be used instead of the new value passed
+// to the method.
+func WithPendingAmount() UpsertPaymentOption {
+	return func(o *upsertAcctPaymentOption) {
+		o.usePendingAmount = true
+	}
+}

accounts/service.go

@@ -510,10 +510,12 @@ func (s *InterceptorService) AssociatePayment(ctx context.Context, id AccountID,
 	// If the payment is already associated with the account but not in
 	// flight, we update the payment amount in case we have a zero-amount
 	// invoice that is retried.
-	return s.store.UpsertAccountPayment(
+	_, err := s.store.UpsertAccountPayment(
 		ctx, id, paymentHash, fullAmt, lnrpc.Payment_UNKNOWN,
 		WithErrIfAlreadyPending(),
 	)
+
+	return err
 }
 
 // invoiceUpdate credits the account an invoice was registered with, in case the
@@ -607,34 +609,30 @@ func (s *InterceptorService) TrackPayment(ctx context.Context, id AccountID,
 		return nil
 	}
 
-	// Similarly, if we've already processed the payment in the past, there
-	// is a reference in the account with the given state.
-	account, err := s.store.Account(ctx, id)
-	if err != nil {
-		return fmt.Errorf("error fetching account: %w", err)
-	}
-
 	// If the account already stored a terminal state, we also don't need to
-	// track the payment again.
-	entry, ok := account.Payments[hash]
-	if ok && successState(entry.Status) {
-		return nil
+	// track the payment again. So we add the WithErrIfAlreadySucceeded
+	// option to ensure that we return an error if the payment has already
+	// succeeded. We can then match on the ErrAlreadySucceeded error and
+	// exit early if it is returned.
+	opts := []UpsertPaymentOption{
+		WithErrIfAlreadySucceeded(),
 	}
 
 	// There is a case where the passed in fullAmt is zero but the pending
 	// amount is not. In that case, we should not overwrite the pending
 	// amount.
 	if fullAmt == 0 {
-		fullAmt = entry.FullAmount
+		opts = append(opts, WithPendingAmount())
 	}
 
-	account.Payments[hash] = &PaymentEntry{
-		Status:     lnrpc.Payment_UNKNOWN,
-		FullAmount: fullAmt,
-	}
-
-	if err := s.store.UpdateAccount(ctx, account); err != nil {
-		if !ok {
+	known, err := s.store.UpsertAccountPayment(
+		ctx, id, hash, fullAmt, lnrpc.Payment_UNKNOWN, opts...,
+	)
+	if err != nil {
+		if errors.Is(err, ErrAlreadySucceeded) {
+			return nil
+		}
+		if !known {
 			// In the rare case that the payment isn't associated
 			// with an account yet, and we fail to update the
 			// account we will not be tracking the payment, even if
@@ -813,7 +811,7 @@ func (s *InterceptorService) paymentUpdate(ctx context.Context,
 	fullAmount := status.Value + status.Fee
 
 	// Update the persisted account.
-	err := s.store.UpsertAccountPayment(
+	_, err := s.store.UpsertAccountPayment(
 		ctx, pendingPayment.accountID, hash, fullAmount,
 		lnrpc.Payment_SUCCEEDED, WithDebitAccount(),
 	)

accounts/store_kvdb.go

@@ -258,22 +258,33 @@ func (s *BoltStore) IncreaseAccountBalance(_ context.Context, id AccountID,
 
 // UpsertAccountPayment updates or inserts a payment entry for the given
 // account. Various functional options can be passed to modify the behavior of
-// the method.
+// the method. The returned boolean is true if the payment was already known
+// before the update. This is to be treated as a best-effort indication if an
+// error is also returned since the method may error before the boolean can be
+// set correctly.
 //
 // NOTE: This is part of the Store interface.
 func (s *BoltStore) UpsertAccountPayment(_ context.Context, id AccountID,
 	paymentHash lntypes.Hash, fullAmount lnwire.MilliSatoshi,
 	status lnrpc.Payment_PaymentStatus,
-	options ...UpsertPaymentOption) error {
+	options ...UpsertPaymentOption) (bool, error) {
 
 	opts := newUpsertPaymentOption()
 	for _, o := range options {
 		o(opts)
 	}
 
+	var known bool
 	update := func(account *OffChainBalanceAccount) error {
-		entry, ok := account.Payments[paymentHash]
-		if ok {
+		var entry *PaymentEntry
+		entry, known = account.Payments[paymentHash]
+		if known {
+			if opts.errIfAlreadySucceeded &&
+				successState(entry.Status) {
+
+				return ErrAlreadySucceeded
+			}
+
 			// If the errIfAlreadyPending option is set, we return
 			// an error if the payment is already in-flight or
 			// succeeded.
@@ -285,6 +296,10 @@ func (s *BoltStore) UpsertAccountPayment(_ context.Context, id AccountID,
 					"(status %v)", paymentHash,
 					account.Payments[paymentHash].Status)
 			}
+
+			if opts.usePendingAmount {
+				fullAmount = entry.FullAmount
+			}
 		}
 
 		account.Payments[paymentHash] = &PaymentEntry{
@@ -299,7 +314,7 @@ func (s *BoltStore) UpsertAccountPayment(_ context.Context, id AccountID,
 		return nil
 	}
 
-	return s.updateAccount(id, update)
+	return known, s.updateAccount(id, update)
 }
 
 func (s *BoltStore) updateAccount(id AccountID,

accounts/store_test.go

@@ -284,22 +284,24 @@ func TestAccountUpdateMethods(t *testing.T) {
 
 		// Assert that calling the method for a non-existent account
 		// errors out.
-		err = store.UpsertAccountPayment(
+		_, err = store.UpsertAccountPayment(
 			ctx, AccountID{}, lntypes.Hash{}, 0,
 			lnrpc.Payment_UNKNOWN,
 		)
 		require.ErrorIs(t, err, ErrAccNotFound)
 
 		// Add a payment to the account but don't update the balance.
-		// We do add a WithErrIfAlreadyPending option here just to show
-		// that no error is returned since the payment does not exist
-		// yet.
+		// We do add a WithErrIfAlreadyPending and
+		// WithErrIfAlreadySucceeded option. here just to show that no
+		// error is returned since the payment does not exist yet.
 		hash1 := lntypes.Hash{1, 2, 3, 4}
-		err = store.UpsertAccountPayment(
+		known, err := store.UpsertAccountPayment(
 			ctx, acct.ID, hash1, 600, lnrpc.Payment_UNKNOWN,
 			WithErrIfAlreadyPending(),
+			WithErrIfAlreadySucceeded(),
 		)
 		require.NoError(t, err)
+		require.False(t, known)
 
 		assertBalanceAndPayments(1000, AccountPayments{
 			hash1: &PaymentEntry{
@@ -311,10 +313,11 @@ func TestAccountUpdateMethods(t *testing.T) {
 		// Add a second payment to the account and again don't update
 		// the balance.
 		hash2 := lntypes.Hash{5, 6, 7, 8}
-		err = store.UpsertAccountPayment(
+		known, err = store.UpsertAccountPayment(
 			ctx, acct.ID, hash2, 100, lnrpc.Payment_UNKNOWN,
 		)
 		require.NoError(t, err)
+		require.False(t, known)
 
 		assertBalanceAndPayments(1000, AccountPayments{
 			hash1: &PaymentEntry{
@@ -329,11 +332,12 @@ func TestAccountUpdateMethods(t *testing.T) {
 
 		// Now, update the first payment to have a new status and this
 		// time, debit the account.
-		err = store.UpsertAccountPayment(
+		known, err = store.UpsertAccountPayment(
 			ctx, acct.ID, hash1, 600, lnrpc.Payment_SUCCEEDED,
 			WithDebitAccount(),
 		)
 		require.NoError(t, err)
+		require.True(t, known)
 
 		// The account should now have a balance of 400 and the first
 		// payment should have a status of succeeded.
@@ -350,10 +354,11 @@ func TestAccountUpdateMethods(t *testing.T) {
 
 		// Calling the same method again with the same payment hash
 		// should have no effect by default.
-		err = store.UpsertAccountPayment(
+		known, err = store.UpsertAccountPayment(
 			ctx, acct.ID, hash1, 600, lnrpc.Payment_SUCCEEDED,
 		)
 		require.NoError(t, err)
+		require.True(t, known)
 
 		assertBalanceAndPayments(400, AccountPayments{
 			hash1: &PaymentEntry{
@@ -368,11 +373,46 @@ func TestAccountUpdateMethods(t *testing.T) {
 
 		// But, if we use the WithErrIfAlreadyPending option, we should
 		// get an error since the payment already exists.
-		err = store.UpsertAccountPayment(
+		known, err = store.UpsertAccountPayment(
 			ctx, acct.ID, hash1, 600, lnrpc.Payment_SUCCEEDED,
 			WithErrIfAlreadyPending(),
 		)
 		require.ErrorContains(t, err, "is already in flight")
+		require.True(t, known)
+
+		// Do the above call again but this time, use the
+		// WithErrIfAlreadySucceeded option. This should return the
+		// ErrAlreadySucceeded error since the payment has already
+		// succeeded.
+		known, err = store.UpsertAccountPayment(
+			ctx, acct.ID, hash1, 600, lnrpc.Payment_SUCCEEDED,
+			WithErrIfAlreadySucceeded(),
+		)
+		require.ErrorIs(t, err, ErrAlreadySucceeded)
+		require.True(t, known)
+
+		// We now call the method again for hash 2 and update its status
+		// to SUCCEEDED. This time, we will use the WithPendingAmount
+		// option which means that whatever `fullAmount` is passed in
+		// should be ignored and the pending amount should be used
+		// instead.
+		known, err = store.UpsertAccountPayment(
+			ctx, acct.ID, hash2, 0, lnrpc.Payment_SUCCEEDED,
+			WithPendingAmount(),
+		)
+		require.NoError(t, err)
+		require.True(t, known)
+
+		assertBalanceAndPayments(400, AccountPayments{
+			hash1: &PaymentEntry{
+				Status:     lnrpc.Payment_SUCCEEDED,
+				FullAmount: 600,
+			},
+			hash2: &PaymentEntry{
+				Status:     lnrpc.Payment_SUCCEEDED,
+				FullAmount: 100,
+			},
+		})
 	})
 }