diff --git a/accounts/errors.go b/accounts/errors.go index 8b3a59afb..241060a2a 100644 --- a/accounts/errors.go +++ b/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") ) diff --git a/accounts/interface.go b/accounts/interface.go index ec2a8a5f6..533f05c7c 100644 --- a/accounts/interface.go +++ b/accounts/interface.go @@ -221,11 +221,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. @@ -316,16 +319,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, } } @@ -346,3 +353,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 + } +} diff --git a/accounts/service.go b/accounts/service.go index 4bcb557e3..6b411a9a8 100644 --- a/accounts/service.go +++ b/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 @@ -615,34 +617,27 @@ 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 !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 @@ -821,7 +816,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(), ) diff --git a/accounts/store_kvdb.go b/accounts/store_kvdb.go index 488602b16..e9702d5be 100644 --- a/accounts/store_kvdb.go +++ b/accounts/store_kvdb.go @@ -198,22 +198,33 @@ func (s *BoltStore) UpdateAccount(_ context.Context, // 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. @@ -225,6 +236,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{ @@ -239,7 +254,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, diff --git a/accounts/store_test.go b/accounts/store_test.go index eb87a20ce..fdae6345d 100644 --- a/accounts/store_test.go +++ b/accounts/store_test.go @@ -136,15 +136,17 @@ func TestAccountUpdateMethods(t *testing.T) { assertBalanceAndPayments(1000, nil) // 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{ @@ -156,10 +158,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{ @@ -174,11 +177,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. @@ -195,10 +199,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{ @@ -213,11 +218,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, + }, + }) }) }