accounts+mid: support for error handlers in accounts

We also want to be able to see via the trace logs if a request has
errored. So first we need to update the framework to be ready for this.

Author: ellemouton

accounts/checkers.go

@@ -398,6 +398,30 @@ func NewAccountChecker(service Service,
 	}
 }
 
+// handleErrorResponse passes an error to a checker if the checker has
+// registered an error handler.
+func (a *AccountChecker) handleErrorResponse(ctx context.Context,
+	fullUri string, parsedErr error) (error, error) {
+
+	// If we don't have a handler for the URI, it means we don't support
+	// that RPC.
+	checker, ok := a.checkers[fullUri]
+	if !ok {
+		return nil, ErrNotSupportedWithAccounts
+	}
+
+	newErr, err := checker.HandleErrorResponse(ctx, parsedErr)
+	if err != nil {
+		return nil, err
+	}
+
+	if newErr != nil {
+		parsedErr = newErr
+	}
+
+	return parsedErr, nil
+}
+
 // checkIncomingRequest makes sure the type of incoming call is supported and
 // if it is, that it is allowed with the current account balance.
 func (a *AccountChecker) checkIncomingRequest(ctx context.Context,

accounts/checkers_test.go

@@ -80,6 +80,10 @@ func (m *mockService) AssociatePayment(id AccountID, paymentHash lntypes.Hash,
 	return nil
 }
 
+func (m *mockService) PaymentErrored(id AccountID, hash lntypes.Hash) error {
+	return nil
+}
+
 func (m *mockService) TrackPayment(_ AccountID, hash lntypes.Hash,
 	amt lnwire.MilliSatoshi) error {
 

accounts/interceptor.go

@@ -126,6 +126,30 @@ func (s *InterceptorService) Intercept(ctx context.Context,
 
 	// Parse and possibly manipulate outgoing responses.
 	case *lnrpc.RPCMiddlewareRequest_Response:
+		if r.Response.IsError {
+			parsedErr := mid.ParseResponseErr(r.Response.Serialized)
+
+			replacementErr, err := s.checkers.handleErrorResponse(
+				ctx, r.Response.MethodFullUri, parsedErr,
+			)
+			if err != nil {
+				return mid.RPCErr(req, err)
+			}
+
+			// No error occurred but the response error should be
+			// replaced with the given custom error. Wrap it in the
+			// correct RPC response of the interceptor now.
+			if replacementErr != nil {
+				return mid.RPCErrReplacement(
+					req, replacementErr,
+				)
+			}
+
+			// No error and no replacement, just return an empty
+			// response of the correct type.
+			return mid.RPCOk(req)
+		}
+
 		msg, err := parseRPCMessage(r.Response)
 		if err != nil {
 			return mid.RPCErr(req, err)

accounts/interface.go

@@ -258,6 +258,11 @@ type Service interface {
 	AssociatePayment(id AccountID, paymentHash lntypes.Hash,
 		fullAmt lnwire.MilliSatoshi) error
 
+	// PaymentErrored removes a pending payment from the accounts
+	// registered payment list. This should only ever be called if we are
+	// sure that the payment request errored out.
+	PaymentErrored(id AccountID, hash lntypes.Hash) error
+
 	RequestValuesStore
 }
 

accounts/service.go

@@ -437,6 +437,45 @@ func (s *InterceptorService) AssociateInvoice(id AccountID,
 	return s.store.UpdateAccount(account)
 }
 
+// PaymentErrored removes a pending payment from the account's registered
+// payment list. This should only ever be called if we are sure that the payment
+// request errored out.
+func (s *InterceptorService) PaymentErrored(id AccountID,
+	hash lntypes.Hash) error {
+
+	s.Lock()
+	defer s.Unlock()
+
+	// If we have already started tracking this payment, then RemovePayment
+	// should have been called instead.
+	_, ok := s.pendingPayments[hash]
+	if ok {
+		return fmt.Errorf("cannot disassociate payment if tracking " +
+			"has already started")
+	}
+
+	account, err := s.store.Account(id)
+	if err != nil {
+		return err
+	}
+
+	// Check that this payment is actually associated with this account.
+	_, ok = account.Payments[hash]
+	if !ok {
+		return fmt.Errorf("payment with hash %s is not associated "+
+			"with this account", hash)
+	}
+
+	// Delete the payment and update the persisted account.
+	delete(account.Payments, hash)
+
+	if err := s.store.UpdateAccount(account); err != nil {
+		return fmt.Errorf("error updating account: %w", err)
+	}
+
+	return nil
+}
+
 // AssociatePayment associates a payment (hash) with the given account,
 // ensuring that the payment will be tracked for a user when LiT is
 // restarted.
@@ -451,11 +490,26 @@ func (s *InterceptorService) AssociatePayment(id AccountID,
 		return err
 	}
 
-	// If the payment is already associated with the account, we don't need
-	// to associate it again.
+	// Check if this payment is associated with the account already.
 	_, ok := account.Payments[paymentHash]
 	if ok {
-		return nil
+		// We do not allow another payment to the same hash if the
+		// payment is already in-flight or succeeded. This mitigates a
+		// user being able to launch a second RPC-erring payment with
+		// the same hash that would remove the payment from being
+		// tracked. Note that this prevents launching multipart
+		// payments, but allows retrying a payment if it has failed.
+		if account.Payments[paymentHash].Status !=
+			lnrpc.Payment_FAILED {
+
+			return fmt.Errorf("payment with hash %s is already in "+
+				"flight or succeeded (status %v)", paymentHash,
+				account.Payments[paymentHash].Status)
+		}
+
+		// Otherwise, we fall through to correctly update the payment
+		// amount, in case we have a zero-amount invoice that is
+		// retried.
 	}
 
 	// Associate the payment with the account and store it.

rpcmiddleware/interface.go

@@ -35,7 +35,9 @@ var (
 
 	// PassThroughErrorHandler is an ErrorHandler that does not modify an
 	// error and instead just passes it through.
-	PassThroughErrorHandler ErrorHandler = func(error) (error, error) {
+	PassThroughErrorHandler ErrorHandler = func(context.Context, error) (
+		error, error) {
+
 		return nil, nil
 	}
 
@@ -81,7 +83,7 @@ type messageHandler func(context.Context, proto.Message) (proto.Message, error)
 // pass through the error unchanged (=return nil, nil), replace the error with
 // a different one (=return non-nil error, nil error) or abort by returning a
 // non-nil error.
-type ErrorHandler func(respErr error) (error, error)
+type ErrorHandler func(ctx context.Context, respErr error) (error, error)
 
 // RoundTripChecker is a type that represents a basic request/response round
 // trip checker.
@@ -115,7 +117,7 @@ type RoundTripChecker interface {
 	// The handler can pass through the error (=return nil, nil), replace
 	// the response error with a new one (=return non-nil error, nil) or
 	// abort by returning a non nil error (=return nil, non-nil error).
-	HandleErrorResponse(error) (error, error)
+	HandleErrorResponse(context.Context, error) (error, error)
 }
 
 // DefaultChecker is the default implementation of a round trip checker.
@@ -171,8 +173,10 @@ func (r *DefaultChecker) HandleResponse(ctx context.Context,
 // The handler can pass through the error (=return nil, nil), replace
 // the response error with a new one (=return non-nil error, nil) or
 // abort by returning a non nil error (=return nil, non-nil error).
-func (r *DefaultChecker) HandleErrorResponse(respErr error) (error, error) {
-	return r.errorHandler(respErr)
+func (r *DefaultChecker) HandleErrorResponse(ctx context.Context,
+	respErr error) (error, error) {
+
+	return r.errorHandler(ctx, respErr)
 }
 
 // NewPassThrough returns a round trip checker that allows the incoming request