diff --git a/backend/handlers/remittances.go b/backend/handlers/remittances.go index 38740eb..7dc9127 100644 --- a/backend/handlers/remittances.go +++ b/backend/handlers/remittances.go @@ -1,9 +1,11 @@ package handlers import ( + "encoding/base64" "encoding/json" "fmt" "net/http" + "strconv" "time" "github.com/gin-gonic/gin" @@ -34,7 +36,44 @@ func NewRemittanceHandler(db *gorm.DB, cfg *config.Config) *RemittanceHandler { } } -// Paginate is a GORM scope for pagination +const ( + // MaxPage prevents integer overflow in offset calculation (#198) + MaxPage = 10000 +) + +// PaginationCursor represents cursor-based pagination state +type PaginationCursor struct { + CreatedAt time.Time `json:"created_at"` + ID uint `json:"id"` +} + +// EncodeCursor encodes pagination cursor to base64 string +func EncodeCursor(cursor PaginationCursor) string { + data, _ := json.Marshal(cursor) + return base64.StdEncoding.EncodeToString(data) +} + +// DecodeCursor decodes base64 cursor string to PaginationCursor +func DecodeCursor(encoded string) (PaginationCursor, error) { + var cursor PaginationCursor + if encoded == "" { + return cursor, nil + } + + data, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return cursor, fmt.Errorf("invalid cursor encoding: %w", err) + } + + err = json.Unmarshal(data, &cursor) + if err != nil { + return cursor, fmt.Errorf("invalid cursor format: %w", err) + } + + return cursor, nil +} + +// Paginate is a GORM scope for pagination with overflow protection func Paginate(c *gin.Context) func(db *gorm.DB) *gorm.DB { return func(db *gorm.DB) *gorm.DB { page := 1 @@ -50,6 +89,11 @@ func Paginate(c *gin.Context) func(db *gorm.DB) *gorm.DB { if page <= 0 { page = 1 } + if page > MaxPage { + // Return error context to be handled by calling function + c.Set("pagination_error", errors.NewValidationError(fmt.Sprintf("Page number cannot exceed %d", MaxPage), nil)) + return db + } if pageSize <= 0 || pageSize > 100 { pageSize = 20 } @@ -212,9 +256,73 @@ func (h *RemittanceHandler) GetRemittance(c *gin.Context) { c.JSON(http.StatusOK, payment) } +type ListRemittancesResponse struct { + Data []models.Payment `json:"data"` + Page int `json:"page,omitempty"` // Deprecated: use cursor instead + PageSize int `json:"page_size,omitempty"` // Deprecated: use limit instead + NextCursor string `json:"next_cursor,omitempty"` + HasMore bool `json:"has_more"` +} + func (h *RemittanceHandler) ListRemittances(c *gin.Context) { var payments []models.Payment + // Support both cursor-based and legacy offset-based pagination + cursor := c.Query("cursor") + limitStr := c.Query("limit") + + // Cursor-based pagination (preferred) + if cursor != "" || limitStr != "" { + // Cursor-based pagination eliminates overflow risk (#198) + limit := 20 + if limitStr != "" { + if l, err := strconv.Atoi(limitStr); err == nil && l > 0 && l <= 100 { + limit = l + } + } + + decodedCursor, err := DecodeCursor(cursor) + if err != nil { + c.Error(errors.NewValidationError("Invalid cursor format", err.Error())) + return + } + + query := h.db.Model(&models.Payment{}).Order("created_at DESC, id DESC") + + // Apply cursor filtering: WHERE created_at < cursor.CreatedAt OR (created_at = cursor.CreatedAt AND id < cursor.ID) + if !decodedCursor.CreatedAt.IsZero() { + query = query.Where("created_at < ? OR (created_at = ? AND id < ?)", + decodedCursor.CreatedAt, decodedCursor.CreatedAt, decodedCursor.ID) + } + + if err := query.Limit(limit + 1).Find(&payments).Error; err != nil { + c.Error(errors.NewInternalError("Failed to fetch payments", err)) + return + } + + var nextCursor string + hasMore := len(payments) > limit + if hasMore { + // Remove the extra item used for has_more detection + lastItem := payments[limit-1] + payments = payments[:limit] + nextCursor = EncodeCursor(PaginationCursor{ + CreatedAt: lastItem.CreatedAt, + ID: lastItem.ID, + }) + } + + response := ListRemittancesResponse{ + Data: payments, + NextCursor: nextCursor, + HasMore: hasMore, + } + + c.JSON(http.StatusOK, response) + return + } + + // Legacy offset-based pagination for backward compatibility // Cache key based on query params cacheKey := fmt.Sprintf("payments:list:%s:%s", c.Query("page"), c.Query("page_size")) @@ -225,12 +333,18 @@ func (h *RemittanceHandler) ListRemittances(c *gin.Context) { return } - // DB query with pagination + // DB query with pagination - check for pagination error from MaxPage validation if err := h.db.Scopes(Paginate(c)).Order("created_at DESC").Find(&payments).Error; err != nil { c.Error(errors.NewInternalError("Failed to fetch payments", err)) return } + // Check if MaxPage validation failed + if paginationErr, exists := c.Get("pagination_error"); exists { + c.Error(paginationErr.(error)) + return + } + // Set cache for 30 seconds utils.SetCached(cacheKey, payments, 30*time.Second) diff --git a/backend/handlers/remittances_test.go b/backend/handlers/remittances_test.go index 044a925..782a6be 100644 --- a/backend/handlers/remittances_test.go +++ b/backend/handlers/remittances_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" @@ -189,3 +190,141 @@ func TestCreateRemittance(t *testing.T) { assert.Equal(t, http.StatusCreated, w.Code) }) } + +// Test pagination overflow fix (#198) +func TestPaginationOverflow(t *testing.T) { + gin.SetMode(gin.TestMode) + db := setupTestDB() + + // Create some test payments + for i := 0; i < 5; i++ { + payment := models.Payment{ + SenderID: 1, + RecipientID: 2, + Amount: float64(100 + i), + Currency: "USD", + Status: "pending", + } + db.Create(&payment) + } + + mockStellar := &MockStellarClient{} + testCfg := &config.Config{} + handler := &RemittanceHandler{ + db: db, + config: testCfg, + stellarClient: mockStellar, + fees: services.NewFeeService(testCfg), + } + + router := gin.Default() + router.GET("/remittances", handler.ListRemittances) + + t.Run("Valid MaxPage", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?page=10000&page_size=20", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + }) + + t.Run("Exceeds MaxPage", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?page=10001&page_size=20", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains(t, w.Body.String(), "cannot exceed 10000") + }) + + t.Run("Zero Page", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?page=0&page_size=20", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) // Should default to page 1 + }) + + t.Run("Negative Page", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?page=-1&page_size=20", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) // Should default to page 1 + }) +} + +// Test cursor-based pagination (#198) +func TestCursorPagination(t *testing.T) { + gin.SetMode(gin.TestMode) + db := setupTestDB() + + // Create test payments with known timestamps + now := time.Now() + for i := 0; i < 5; i++ { + payment := models.Payment{ + SenderID: 1, + RecipientID: 2, + Amount: float64(100 + i), + Currency: "USD", + Status: "pending", + CreatedAt: now.Add(time.Duration(i) * time.Hour), // Different timestamps + } + db.Create(&payment) + } + + mockStellar := &MockStellarClient{} + testCfg := &config.Config{} + handler := &RemittanceHandler{ + db: db, + config: testCfg, + stellarClient: mockStellar, + fees: services.NewFeeService(testCfg), + } + + router := gin.Default() + router.GET("/remittances", handler.ListRemittances) + + t.Run("Valid Cursor", func(t *testing.T) { + // First request with limit + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?limit=2", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + + var response ListRemittancesResponse + err := json.Unmarshal(w.Body.Bytes(), &response) + assert.NoError(t, err) + assert.Equal(t, 2, len(response.Data)) + assert.True(t, response.HasMore) + assert.NotEmpty(t, response.NextCursor) + }) + + t.Run("Invalid Cursor", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?cursor=invalid_base64&limit=2", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains(t, w.Body.String(), "Invalid cursor format") + }) + + t.Run("Empty Result Set", func(t *testing.T) { + // Clear all payments + db.Where("1 = 1").Delete(&models.Payment{}) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/remittances?limit=2", nil) + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + + var response ListRemittancesResponse + err := json.Unmarshal(w.Body.Bytes(), &response) + assert.NoError(t, err) + assert.Equal(t, 0, len(response.Data)) + assert.False(t, response.HasMore) + assert.Empty(t, response.NextCursor) + }) +} diff --git a/backend/services/webhook_delivery.go b/backend/services/webhook_delivery.go index a11815c..e9ac147 100644 --- a/backend/services/webhook_delivery.go +++ b/backend/services/webhook_delivery.go @@ -6,11 +6,14 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" + "net" "net/http" "strings" "sync/atomic" + "syscall" "time" "github.com/yourusername/gpay-remit/logger" @@ -108,22 +111,86 @@ func (s *WebhookDeliveryService) TriggerWebhook(event string, data map[string]in return nil } -// DeliverWebhook delivers a webhook with retry logic -func (s *WebhookDeliveryService) DeliverWebhook(webhook *models.Webhook, delivery *models.WebhookDelivery) { - maxAttempts := 5 - baseDelay := time.Second +// isConnectionRefused checks if the error is a connection refused error +func isConnectionRefused(opErr *net.OpError) bool { + if opErr.Err == nil { + return false + } + // Check for syscall.ECONNREFUSED in the error chain + return errors.Is(opErr.Err, syscall.ECONNREFUSED) +} - for attempt := 0; attempt < maxAttempts; attempt++ { - delivery.AttemptCount = attempt + 1 - - // Exponential backoff - if attempt > 0 { - delay := baseDelay * time.Duration(1<= 400 { + if statusCode >= 400 && statusCode < 500 { + return ErrorHTTP4xx } + if statusCode >= 500 { + return ErrorHTTP5xx + } + } - success, responseCode, responseBody, errMsg := s.sendWebhookRequest(webhook, delivery.Payload) + var netErr net.Error + var dnsErr *net.DNSError + var opErr *net.OpError + + switch { + case errors.As(err, &dnsErr): + return ErrorDNS + case errors.As(err, &netErr) && netErr.Timeout(): + return ErrorTimeout + case errors.As(err, &opErr) && isConnectionRefused(opErr): + return ErrorConnectionRefused + default: + return ErrorOther + } +} +// getRetryPolicy returns retry configuration based on error type (#197) +func getRetryPolicy(errorType ErrorClassification) (maxRetries int, baseDelay time.Duration, shouldRetry bool) { + switch errorType { + case ErrorTimeout: + // Timeouts indicate transient network congestion - retry with exponential backoff + return 3, time.Second, true + case ErrorDNS: + // DNS failures may be transient but aggressive retrying is counterproductive + return 2, 5 * time.Second, true + case ErrorConnectionRefused: + // Connection refused typically indicates server down - don't retry immediately + return 0, 30 * time.Second, false // Mark as failed, schedule for later + case ErrorHTTP5xx: + // Server errors - retry with exponential backoff + return 3, time.Second, true + case ErrorHTTP4xx: + // Client errors - malformed request, don't retry + return 0, 0, false + case ErrorOther: + // Unclassified errors - single retry with moderate delay + return 1, 2 * time.Second, true + default: + return 1, 2 * time.Second, true + } +} +// DeliverWebhook delivers a webhook with per-error-type retry logic (#197) +func (s *WebhookDeliveryService) DeliverWebhook(webhook *models.Webhook, delivery *models.WebhookDelivery) { + for { + delivery.AttemptCount++ + + success, responseCode, responseBody, errMsg, err := s.sendWebhookRequest(webhook, delivery.Payload) + delivery.ResponseCode = responseCode delivery.ResponseBody = responseBody delivery.ErrorMessage = errMsg @@ -142,43 +209,63 @@ func (s *WebhookDeliveryService) DeliverWebhook(webhook *models.Webhook, deliver return } - // Calculate next retry time - if attempt < maxAttempts-1 { - nextDelay := baseDelay * time.Duration(1<= maxRetries { + // Mark as failed + delivery.Status = "failed" + now := time.Now() + delivery.CompletedAt = &now + delivery.NextRetryAt = nil + s.db.Save(delivery) + webhookDeliveryFailureCount.Add(1) + + logLevel.Error("Webhook delivery permanently failed") + return + } + + // Calculate delay based on error type and attempt count + var delay time.Duration + if errorType == ErrorTimeout || errorType == ErrorHTTP5xx { + // Exponential backoff for timeout and 5xx errors + delay = baseDelay * time.Duration(1<= 200 && resp.StatusCode < 300 { - return true, resp.StatusCode, responseBody, "" + return true, resp.StatusCode, responseBody, "", nil } - return false, resp.StatusCode, responseBody, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, responseBody) + return false, resp.StatusCode, responseBody, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, responseBody), nil } // generateSignature creates HMAC-SHA256 signature for webhook verification diff --git a/backend/services/webhook_delivery_test.go b/backend/services/webhook_delivery_test.go new file mode 100644 index 0000000..0b35eee --- /dev/null +++ b/backend/services/webhook_delivery_test.go @@ -0,0 +1,257 @@ +package services + +import ( + "errors" + "net" + "net/http" + "net/http/httptest" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/yourusername/gpay-remit/models" + "gorm.io/driver/sqlite" + "gorm.io/gorm" +) + +func setupWebhookTestDB() *gorm.DB { + db, _ := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + db.AutoMigrate(&models.Webhook{}, &models.WebhookDelivery{}) + return db +} + +// Test error classification (#197) +func TestClassifyError(t *testing.T) { + tests := []struct { + name string + err error + statusCode int + expectedType ErrorClassification + }{ + { + name: "Timeout Error", + err: &net.OpError{Op: "dial", Net: "tcp", Err: &timeoutError{}}, + statusCode: 0, + expectedType: ErrorTimeout, + }, + { + name: "DNS Error", + err: &net.DNSError{Err: "no such host"}, + statusCode: 0, + expectedType: ErrorDNS, + }, + { + name: "Connection Refused", + err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + statusCode: 0, + expectedType: ErrorConnectionRefused, + }, + { + name: "HTTP 4xx", + err: nil, + statusCode: 400, + expectedType: ErrorHTTP4xx, + }, + { + name: "HTTP 5xx", + err: nil, + statusCode: 500, + expectedType: ErrorHTTP5xx, + }, + { + name: "Other Error", + err: errors.New("unknown error"), + statusCode: 0, + expectedType: ErrorOther, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := classifyError(tt.err, tt.statusCode) + assert.Equal(t, tt.expectedType, result) + }) + } +} + +// Test retry policies (#197) +func TestGetRetryPolicy(t *testing.T) { + tests := []struct { + name string + errorType ErrorClassification + expectRetries int + expectDelay time.Duration + shouldRetry bool + }{ + { + name: "Timeout - should retry with exponential backoff", + errorType: ErrorTimeout, + expectRetries: 3, + expectDelay: time.Second, + shouldRetry: true, + }, + { + name: "DNS - should retry with fixed delay", + errorType: ErrorDNS, + expectRetries: 2, + expectDelay: 5 * time.Second, + shouldRetry: true, + }, + { + name: "Connection Refused - should not retry immediately", + errorType: ErrorConnectionRefused, + expectRetries: 0, + expectDelay: 30 * time.Second, + shouldRetry: false, + }, + { + name: "HTTP 4xx - should not retry", + errorType: ErrorHTTP4xx, + expectRetries: 0, + expectDelay: 0, + shouldRetry: false, + }, + { + name: "HTTP 5xx - should retry", + errorType: ErrorHTTP5xx, + expectRetries: 3, + expectDelay: time.Second, + shouldRetry: true, + }, + { + name: "Other - should retry once", + errorType: ErrorOther, + expectRetries: 1, + expectDelay: 2 * time.Second, + shouldRetry: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + maxRetries, baseDelay, shouldRetry := getRetryPolicy(tt.errorType) + assert.Equal(t, tt.expectRetries, maxRetries) + assert.Equal(t, tt.expectDelay, baseDelay) + assert.Equal(t, tt.shouldRetry, shouldRetry) + }) + } +} + +// Test webhook delivery with different error types (#197) +func TestWebhookDeliveryErrorHandling(t *testing.T) { + db := setupWebhookTestDB() + + t.Run("Success on Second Attempt", func(t *testing.T) { + // Create a server that fails first then succeeds + attempt := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempt++ + if attempt == 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + w.Write([]byte("success")) + })) + defer server.Close() + + service := NewWebhookDeliveryService(db) + + webhook := &models.Webhook{ + ID: 1, + URL: server.URL, + Secret: "test-secret", + } + + delivery := &models.WebhookDelivery{ + ID: 1, + WebhookID: 1, + Event: "test.event", + Payload: `{"test": "data"}`, + Status: "pending", + } + db.Create(delivery) + + // This would normally run in a goroutine, but we'll call it directly for testing + service.DeliverWebhook(webhook, delivery) + + // Check that delivery was marked as successful + db.First(delivery) + assert.Equal(t, "success", delivery.Status) + assert.Equal(t, 2, delivery.AttemptCount) + assert.NotNil(t, delivery.CompletedAt) + }) + + t.Run("Permanent Failure After Max Retries", func(t *testing.T) { + // Create a server that always returns 4xx (should not retry) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte("bad request")) + })) + defer server.Close() + + service := NewWebhookDeliveryService(db) + + webhook := &models.Webhook{ + ID: 2, + URL: server.URL, + Secret: "test-secret", + } + + delivery := &models.WebhookDelivery{ + ID: 2, + WebhookID: 2, + Event: "test.event", + Payload: `{"test": "data"}`, + Status: "pending", + } + db.Create(delivery) + + service.DeliverWebhook(webhook, delivery) + + // Check that delivery was marked as permanently failed + db.First(delivery) + assert.Equal(t, "failed", delivery.Status) + assert.Equal(t, 1, delivery.AttemptCount) // Should not retry 4xx errors + assert.NotNil(t, delivery.CompletedAt) + }) +} + +// Mock timeout error for testing +type timeoutError struct{} + +func (e *timeoutError) Error() string { return "timeout" } +func (e *timeoutError) Timeout() bool { return true } +func (e *timeoutError) Temporary() bool { return true } + +// Test connection refused detection +func TestIsConnectionRefused(t *testing.T) { + t.Run("Valid Connection Refused", func(t *testing.T) { + opErr := &net.OpError{ + Op: "dial", + Net: "tcp", + Err: syscall.ECONNREFUSED, + } + assert.True(t, isConnectionRefused(opErr)) + }) + + t.Run("Different Error", func(t *testing.T) { + opErr := &net.OpError{ + Op: "dial", + Net: "tcp", + Err: errors.New("other error"), + } + assert.False(t, isConnectionRefused(opErr)) + }) + + t.Run("Nil Error", func(t *testing.T) { + opErr := &net.OpError{ + Op: "dial", + Net: "tcp", + Err: nil, + } + assert.False(t, isConnectionRefused(opErr)) + }) +} \ No newline at end of file diff --git a/backend/utils/cache.go b/backend/utils/cache.go index a9e93ce..f74e55f 100644 --- a/backend/utils/cache.go +++ b/backend/utils/cache.go @@ -3,6 +3,7 @@ package utils import ( "context" "encoding/json" + "errors" "fmt" "time" @@ -14,25 +15,45 @@ var ( ctx = context.Background() ) -// InitRedis initializes the Redis client +// ErrCacheUnavailable is returned when Redis is not available +var ErrCacheUnavailable = errors.New("cache service unavailable") + +// InitRedis initializes the Redis client with connection pool configuration (#202) func InitRedis(addr string, password string, db int) error { + // Configure connection pool for better resource management RedisClient = redis.NewClient(&redis.Options{ - Addr: addr, - Password: password, - DB: db, + Addr: addr, + Password: password, + DB: db, + PoolSize: 10, // Maximum number of connections + MinIdleConns: 2, // Minimum idle connections to maintain + ConnMaxIdleTime: 5 * time.Minute, // Close idle connections after this time + DialTimeout: 5 * time.Second, // Connection timeout + ReadTimeout: 3 * time.Second, // Read timeout + WriteTimeout: 3 * time.Second, // Write timeout }) - _, err := RedisClient.Ping(ctx).Result() + // Test connection with short timeout - don't panic if Redis is unavailable + pingCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + defer cancel() + + _, err := RedisClient.Ping(pingCtx).Result() if err != nil { - return fmt.Errorf("failed to connect to redis: %w", err) + // Log warning but continue - application should function without Redis + fmt.Printf("WARNING: Redis unavailable at startup: %v — caching disabled\n", err) + RedisClient = nil // Set to nil to trigger graceful degradation + return nil // Don't return error - this is graceful degradation } + fmt.Println("INFO: Redis connection established successfully") return nil } // GetCached retrieves a value from the cache and unmarshals it into dest +// Returns cache miss (false, nil) when Redis is unavailable for graceful degradation (#202) func GetCached(key string, dest interface{}) (bool, error) { if RedisClient == nil { + // Redis unavailable - return cache miss to fall back to source of truth return false, nil } @@ -40,7 +61,9 @@ func GetCached(key string, dest interface{}) (bool, error) { if err == redis.Nil { return false, nil } else if err != nil { - return false, err + // Redis error - log at debug level and return cache miss for graceful degradation + fmt.Printf("DEBUG: Redis Get error for key %s: %v\n", key, err) + return false, nil } err = json.Unmarshal([]byte(val), dest) @@ -52,8 +75,11 @@ func GetCached(key string, dest interface{}) (bool, error) { } // SetCached stores a value in the cache with a TTL +// Silently continues when Redis is unavailable for graceful degradation (#202) func SetCached(key string, value interface{}, ttl time.Duration) error { if RedisClient == nil { + // Redis unavailable - log at debug level and continue + fmt.Printf("DEBUG: Redis unavailable, skipping cache set for key %s\n", key) return nil } @@ -62,14 +88,49 @@ func SetCached(key string, value interface{}, ttl time.Duration) error { return err } - return RedisClient.Set(ctx, key, data, ttl).Err() + err = RedisClient.Set(ctx, key, data, ttl).Err() + if err != nil { + // Redis error - log at debug level and continue (don't propagate error) + fmt.Printf("DEBUG: Redis Set error for key %s: %v\n", key, err) + return nil + } + + return nil } // DeleteCached removes a value from the cache +// Silently continues when Redis is unavailable for graceful degradation (#202) func DeleteCached(key string) error { if RedisClient == nil { + // Redis unavailable - log at debug level and continue + fmt.Printf("DEBUG: Redis unavailable, skipping cache delete for key %s\n", key) + return nil + } + + err := RedisClient.Del(ctx, key).Err() + if err != nil { + // Redis error - log at debug level and continue (don't propagate error) + fmt.Printf("DEBUG: Redis Del error for key %s: %v\n", key, err) return nil } - return RedisClient.Del(ctx, key).Err() + return nil +} + +// PingRedis checks Redis availability for health checks +// Returns ErrCacheUnavailable when Redis is not available (#202) +func PingRedis() error { + if RedisClient == nil { + return ErrCacheUnavailable + } + + pingCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + defer cancel() + + _, err := RedisClient.Ping(pingCtx).Result() + if err != nil { + return ErrCacheUnavailable + } + + return nil } diff --git a/backend/utils/cache_test.go b/backend/utils/cache_test.go index dc644aa..d51b894 100644 --- a/backend/utils/cache_test.go +++ b/backend/utils/cache_test.go @@ -5,102 +5,126 @@ import ( "time" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -// resetRedisClient sets RedisClient to nil before each test so tests are -// isolated regardless of execution order. -func resetRedisClient() { - RedisClient = nil +// Test graceful degradation when Redis is unavailable (#202) +func TestRedisGracefulDegradation(t *testing.T) { + // Save original client + originalClient := RedisClient + defer func() { + RedisClient = originalClient + }() + + t.Run("GetCached returns cache miss when Redis unavailable", func(t *testing.T) { + // Set client to nil to simulate unavailability + RedisClient = nil + + var result interface{} + found, err := GetCached("test-key", &result) + + assert.False(t, found) + assert.NoError(t, err) + }) + + t.Run("SetCached continues silently when Redis unavailable", func(t *testing.T) { + // Set client to nil to simulate unavailability + RedisClient = nil + + err := SetCached("test-key", map[string]string{"test": "data"}, time.Minute) + + // Should not return error for graceful degradation + assert.NoError(t, err) + }) + + t.Run("DeleteCached continues silently when Redis unavailable", func(t *testing.T) { + // Set client to nil to simulate unavailability + RedisClient = nil + + err := DeleteCached("test-key") + + // Should not return error for graceful degradation + assert.NoError(t, err) + }) + + t.Run("PingRedis returns ErrCacheUnavailable when Redis unavailable", func(t *testing.T) { + // Set client to nil to simulate unavailability + RedisClient = nil + + err := PingRedis() + + assert.Equal(t, ErrCacheUnavailable, err) + }) } -// TestGetCachedNilClient verifies that GetCached is a no-op when the Redis -// client has not been initialised (returns false, nil). -func TestGetCachedNilClient(t *testing.T) { - resetRedisClient() - - var dest map[string]interface{} - found, err := GetCached("some-key", &dest) - - assert.NoError(t, err) - assert.False(t, found) - assert.Nil(t, dest) +// Test that InitRedis doesn't panic when Redis is unavailable (#202) +func TestInitRedisGracefulFailure(t *testing.T) { + t.Run("InitRedis handles connection failure gracefully", func(t *testing.T) { + // Try to connect to non-existent Redis instance + err := InitRedis("localhost:9999", "", 0) + + // Should not return error (graceful degradation) + assert.NoError(t, err) + + // Client should be nil after failed initialization + assert.Nil(t, RedisClient) + }) } -// TestSetCachedNilClient verifies that SetCached silently succeeds (no-op) -// when the Redis client is nil. -func TestSetCachedNilClient(t *testing.T) { - resetRedisClient() - - err := SetCached("some-key", map[string]string{"hello": "world"}, time.Minute) - - assert.NoError(t, err) -} - -// TestDeleteCachedNilClient verifies that DeleteCached silently succeeds -// (no-op) when the Redis client is nil. -func TestDeleteCachedNilClient(t *testing.T) { - resetRedisClient() - - err := DeleteCached("some-key") - - assert.NoError(t, err) -} - -// TestGetCachedNotFound verifies that GetCached returns (false, nil) for a key -// that has never been stored (requires a live Redis connection; skipped when -// REDIS_ADDR is not reachable). -func TestGetCachedNotFound(t *testing.T) { - if RedisClient == nil { - t.Skip("skipping: no Redis client available") +// Test cache operations with working Redis (integration test) +func TestCacheOperationsWithRedis(t *testing.T) { + // This test requires a running Redis instance + // Skip if Redis is not available + err := InitRedis("localhost:6379", "", 0) + if err != nil || RedisClient == nil { + t.Skip("Redis not available for integration test") } - var dest string - found, err := GetCached("nonexistent-key-xyz", &dest) - - require.NoError(t, err) - assert.False(t, found) + t.Run("Set and Get cache value", func(t *testing.T) { + testData := map[string]interface{}{ + "key1": "value1", + "key2": 123, + } + + err := SetCached("integration-test-key", testData, time.Minute) + assert.NoError(t, err) + + var result map[string]interface{} + found, err := GetCached("integration-test-key", &result) + + assert.True(t, found) + assert.NoError(t, err) + assert.Equal(t, "value1", result["key1"]) + assert.Equal(t, float64(123), result["key2"]) // JSON unmarshaling converts numbers to float64 + + // Clean up + err = DeleteCached("integration-test-key") + assert.NoError(t, err) + }) + + t.Run("Get non-existent key returns cache miss", func(t *testing.T) { + var result interface{} + found, err := GetCached("non-existent-key", &result) + + assert.False(t, found) + assert.NoError(t, err) + }) + + t.Run("PingRedis succeeds with working Redis", func(t *testing.T) { + err := PingRedis() + assert.NoError(t, err) + }) } -// TestSetAndGetCached verifies that a value stored with SetCached can be -// retrieved with GetCached (requires a live Redis connection). -func TestSetAndGetCached(t *testing.T) { - if RedisClient == nil { - t.Skip("skipping: no Redis client available") - } - - type payload struct { - Message string `json:"message"` - } - - key := "test-set-get-key" - want := payload{Message: "hello-redis"} - - require.NoError(t, SetCached(key, want, 10*time.Second)) - t.Cleanup(func() { _ = DeleteCached(key) }) - - var got payload - found, err := GetCached(key, &got) - - require.NoError(t, err) - assert.True(t, found) - assert.Equal(t, want, got) -} - -// TestDeleteCached verifies that after a key is deleted GetCached returns false -// (requires a live Redis connection). -func TestDeleteCached(t *testing.T) { - if RedisClient == nil { - t.Skip("skipping: no Redis client available") - } - - key := "test-delete-key" - require.NoError(t, SetCached(key, "value", 10*time.Second)) - - require.NoError(t, DeleteCached(key)) - - var dest string - found, err := GetCached(key, &dest) - require.NoError(t, err) - assert.False(t, found) -} +// Test connection pool configuration +func TestRedisConnectionPoolConfig(t *testing.T) { + // This test verifies that InitRedis sets up connection pool properly + // We can't easily test the actual pool settings without accessing internals, + // but we can verify the function completes successfully + + t.Run("InitRedis with valid config", func(t *testing.T) { + // This should not panic and should handle connection gracefully + assert.NotPanics(t, func() { + InitRedis("localhost:6379", "", 0) + }) + }) +} \ No newline at end of file diff --git a/contracts/src/remittance_hub.rs b/contracts/src/remittance_hub.rs index 4c20de9..ed767e0 100644 --- a/contracts/src/remittance_hub.rs +++ b/contracts/src/remittance_hub.rs @@ -98,8 +98,11 @@ pub struct Invoice { pub fees: i128, pub total_due: i128, pub status: InvoiceStatus, + /// All timestamps are UTC Unix seconds as returned by env.ledger().timestamp() (#201) pub created_at: u64, + /// Due date in UTC Unix seconds - timezone metadata implicit (#201) pub due_date: u64, + /// Payment timestamp in UTC Unix seconds (#201) pub paid_at: u64, pub description: String, pub escrow_id: u64, @@ -304,6 +307,7 @@ impl RemittanceHubContract { let cached = CachedRate { rate, denominator, + // All timestamps are UTC Unix seconds as returned by env.ledger().timestamp() (#201) timestamp: env.ledger().timestamp(), from_asset: from_asset.clone(), to_asset: to_asset.clone(), @@ -534,6 +538,7 @@ impl RemittanceHubContract { amount, risk_score: 0, status: AmlStatus::Reviewing, + // All timestamps are UTC Unix seconds as returned by env.ledger().timestamp() (#201) timestamp: env.ledger().timestamp(), }; env.storage() @@ -691,6 +696,7 @@ impl RemittanceHubContract { } let current_time = env.ledger().timestamp(); + // All timestamp comparisons use UTC Unix seconds as returned by env.ledger().timestamp() (#201) if due_date <= current_time { return Err(RemittanceError::DueDateInPast); } @@ -809,6 +815,7 @@ impl RemittanceHubContract { } invoice.status = InvoiceStatus::Paid; + // All timestamps are UTC Unix seconds as returned by env.ledger().timestamp() (#201) invoice.paid_at = env.ledger().timestamp(); env.storage() @@ -840,6 +847,7 @@ impl RemittanceHubContract { let current_time = env.ledger().timestamp(); + // All timestamp comparisons use UTC Unix seconds as returned by env.ledger().timestamp() (#201) if current_time <= invoice.due_date { return Err(RemittanceError::InvalidInvoiceStatus); } @@ -1016,6 +1024,7 @@ impl RemittanceHubContract { } let current_time = env.ledger().timestamp(); + // All timestamp comparisons use UTC Unix seconds as returned by env.ledger().timestamp() (#201) if request.expiration_timestamp <= current_time { return Err(RemittanceError::DueDateInPast); } @@ -1468,6 +1477,7 @@ impl RemittanceHubContract { return; } + // All timestamps are UTC Unix seconds as returned by env.ledger().timestamp() (#201) let now = env.ledger().timestamp(); let day = now / 86400; let week = now / (86400 * 7); diff --git a/contracts/tests/remittance_hub_test.rs b/contracts/tests/remittance_hub_test.rs index f605dea..78b75ab 100644 --- a/contracts/tests/remittance_hub_test.rs +++ b/contracts/tests/remittance_hub_test.rs @@ -431,3 +431,299 @@ fn test_aml_screening() { let config = client.get_aml_config(); assert!(config.is_some()); } +// ============================================================================ +// TIMEZONE AND UTC CONSISTENCY TESTS (#201) +// ============================================================================ + +use gpay_remit_contracts::remittance_hub::{Asset}; + +// Test that invoice due dates are stored as UTC Unix seconds +#[test] +fn test_invoice_due_date_utc_storage() { + let env = Env::default(); + env.mock_all_auths(); + env.ledger().with_mut(|li| { + li.timestamp = 1000; // Fixed timestamp for predictable testing + }); + + let (client, _admin, user1, user2) = setup_test(&env); + + let issuer = Address::generate(&env); + let asset = Asset { + code: String::from_str(&env, "USDC"), + issuer, + }; + + let due_date = 2000; // Future timestamp + let invoice_id = client.generate_invoice( + &user1, + &user2, + &1000, + &asset, + &due_date, + &String::from_str(&env, "Test invoice"), + &0, + &String::from_str(&env, "Test memo"), + ); + + let invoice = client.get_invoice(&invoice_id).unwrap(); + + // Verify timestamps are stored as provided (UTC Unix seconds) + assert_eq!(invoice.created_at, 1000); + assert_eq!(invoice.due_date, due_date); + assert_eq!(invoice.paid_at, 0); // Not paid yet +} + +// Test due date comparison uses UTC consistently +#[test] +fn test_due_date_comparison_utc() { + let env = Env::default(); + env.mock_all_auths(); + env.ledger().with_mut(|li| { + li.timestamp = 1000; + }); + + let (client, _admin, user1, user2) = setup_test(&env); + + let issuer = Address::generate(&env); + let asset = Asset { + code: String::from_str(&env, "USDC"), + issuer, + }; + + // Test past due date rejection + let past_due_date = 500; // Before current timestamp + let result = client.try_generate_invoice( + &user1, + &user2, + &1000, + &asset, + &past_due_date, + &String::from_str(&env, "Test invoice"), + &0, + &String::from_str(&env, "Test memo"), + ); + assert_eq!(result, Err(Ok(RemittanceError::DueDateInPast))); + + // Test future due date acceptance + let future_due_date = 2000; // After current timestamp + let invoice_id = client.generate_invoice( + &user1, + &user2, + &1000, + &asset, + &future_due_date, + &String::from_str(&env, "Test invoice"), + &0, + &String::from_str(&env, "Test memo"), + ); + assert_eq!(invoice_id, 1); +} + +// Test mark_invoice_overdue with UTC timestamp comparison +#[test] +fn test_mark_invoice_overdue_utc_comparison() { + let env = Env::default(); + env.mock_all_auths(); + env.ledger().with_mut(|li| { + li.timestamp = 1000; + }); + + let (client, _admin, user1, user2) = setup_test(&env); + + let issuer = Address::generate(&env); + let asset = Asset { + code: String::from_str(&env, "USDC"), + issuer, + }; + + // Create invoice with future due date + let due_date = 2000; + let invoice_id = client.generate_invoice( + &user1, + &user2, + &1000, + &asset, + &due_date, + &String::from_str(&env, "Test invoice"), + &0, + &String::from_str(&env, "Test memo"), + ); + + // Try to mark overdue before due date - should fail + let result = client.try_mark_invoice_overdue(&invoice_id); + assert_eq!(result, Err(Ok(RemittanceError::InvalidInvoiceStatus))); + + // Advance ledger timestamp past due date + env.ledger().with_mut(|li| { + li.timestamp = 2500; // Past the due date + }); + + // Now should successfully mark as overdue + client.mark_invoice_overdue(&invoice_id); + + let invoice = client.get_invoice(&invoice_id).unwrap(); + assert_eq!(invoice.status, InvoiceStatus::Overdue); +} + +// Test that paid_at timestamp uses UTC from ledger +#[test] +fn test_invoice_paid_at_utc_timestamp() { + let env = Env::default(); + env.mock_all_auths(); + env.ledger().with_mut(|li| { + li.timestamp = 1000; + }); + + let (client, _admin, user1, user2) = setup_test(&env); + + let issuer = Address::generate(&env); + let asset = Asset { + code: String::from_str(&env, "USDC"), + issuer, + }; + + let invoice_id = client.generate_invoice( + &user1, + &user2, + &1000, + &asset, + &2000, + &String::from_str(&env, "Test invoice"), + &0, + &String::from_str(&env, "Test memo"), + ); + + // Advance time and mark as paid + env.ledger().with_mut(|li| { + li.timestamp = 1500; + }); + + client.mark_invoice_paid(&invoice_id, &user1); + + let invoice = client.get_invoice(&invoice_id).unwrap(); + assert_eq!(invoice.status, InvoiceStatus::Paid); + assert_eq!(invoice.paid_at, 1500); // Should match ledger timestamp +} + +// Test escrow expiration timestamp UTC comparison +#[test] +fn test_escrow_expiration_utc_comparison() { + use gpay_remit_contracts::remittance_hub::EscrowRequest; + + let env = Env::default(); + env.mock_all_auths(); + env.ledger().with_mut(|li| { + li.timestamp = 1000; + }); + + let (client, _admin, user1, user2) = setup_test(&env); + + let issuer = Address::generate(&env); + let asset = Asset { + code: String::from_str(&env, "USDC"), + issuer, + }; + + // Test past expiration timestamp rejection + let past_expiration = 500; + let mut requests = soroban_sdk::Vec::new(&env); + requests.push_back(EscrowRequest { + recipient: user2.clone(), + amount: 1000, + asset: asset.clone(), + expiration_timestamp: past_expiration, + }); + + let result = client.try_batch_create_escrows(&user1, &requests); + assert_eq!(result, Err(Ok(RemittanceError::DueDateInPast))); + + // Test future expiration timestamp acceptance + let future_expiration = 2000; + requests.clear(); + requests.push_back(EscrowRequest { + recipient: user2.clone(), + amount: 1000, + asset: asset.clone(), + expiration_timestamp: future_expiration, + }); + + let escrow_ids = client.batch_create_escrows(&user1, &requests); + assert_eq!(escrow_ids.len(), 1); +} + +// Test AML timestamp consistency +#[test] +fn test_aml_timestamp_utc_consistency() { + let env = Env::default(); + env.mock_all_auths(); + env.ledger().with_mut(|li| { + li.timestamp = 1000; + }); + + let (client, admin, user1, user2) = setup_test(&env); + + // Configure AML with mock oracle + let aml_oracle = Address::generate(&env); + client.configure_aml(&admin, &aml_oracle, &50); + + // This will trigger AML screening which should use UTC timestamps + let remittance_id = client.send_remittance( + &user1, + &user2, + &5000, + &soroban_sdk::symbol_short!("USD"), + ); + + // Verify remittance was created (AML logic may set status based on mock behavior) + let remittance = client.get_remittance(&remittance_id); + assert!(remittance.is_some()); +} + +// Test metric tracking uses UTC timestamps +#[test] +fn test_metric_tracking_utc_timestamps() { + use gpay_remit_contracts::remittance_hub::MetricType; + + let env = Env::default(); + env.mock_all_auths(); + + // Set specific timestamp for predictable day/week calculation + let test_timestamp = 86400 * 10; // Day 10 + env.ledger().with_mut(|li| { + li.timestamp = test_timestamp; + }); + + let (client, _admin, user1, user2) = setup_test(&env); + + let issuer = Address::generate(&env); + let asset = Asset { + code: String::from_str(&env, "USDC"), + issuer, + }; + + // Generate invoice to trigger metric tracking + let invoice_id = client.generate_invoice( + &user1, + &user2, + &1000, + &asset, + &(test_timestamp + 1000), + &String::from_str(&env, "Test invoice"), + &0, + &String::from_str(&env, "Test memo"), + ); + + // Check metrics are tracked using the correct UTC timestamp + let daily_volume = client.get_metric(&MetricType::Volume, &test_timestamp, &false); + let weekly_volume = client.get_metric(&MetricType::Volume, &test_timestamp, &true); + + assert_eq!(daily_volume, 1000); + assert_eq!(weekly_volume, 1000); + + // Mark invoice as paid to trigger success metric + client.mark_invoice_paid(&invoice_id, &user1); + + let daily_success = client.get_metric(&MetricType::Success, &test_timestamp, &false); + assert_eq!(daily_success, 1); +} \ No newline at end of file