Fix tests

config/settings/base.py

@@ -680,6 +680,7 @@
 
 # Compression level – an integer from 0 to 9. 0 means not compression
 CACHE_ALL_TXS_COMPRESSION_LEVEL = env.int("CACHE_ALL_TXS_COMPRESSION_LEVEL", default=0)
+DEFAULT_CACHE_PAGE_TIMEOUT = env.int("DEFAULT_CACHE_PAGE_TIMEOUT", default=60)
 
 # Contracts reindex batch configuration
 # ------------------------------------------------------------------------------

safe_transaction_service/history/services/safe_service.py

@@ -115,9 +115,7 @@ def get_safe_creation_info(
         try:
             # Get first the actual creation transaction for the safe
             creation_internal_tx = (
-                InternalTx.objects.filter(
-                    ethereum_tx__status=1  # Ignore Internal Transactions for failed Transactions
-                )
+                InternalTx.objects.filter(ethereum_tx__status=1)
                 .select_related("ethereum_tx__block")
                 .get(contract_address=safe_address)
             )

safe_transaction_service/history/signals.py

@@ -1,17 +1,16 @@
 from logging import getLogger
-from typing import List, Optional, Type, Union
+from typing import Type, Union
 
 from django.conf import settings
 from django.db.models import Model
 from django.db.models.signals import post_delete, post_save
 from django.dispatch import receiver
 from django.utils import timezone
 
-from eth_typing import ChecksumAddress
-
 from safe_transaction_service.notifications.tasks import send_notification_task
 
 from ..events.services.queue_service import get_queue_service
+from ..utils.redis import remove_cache_view_by_instance
 from .models import (
     ERC20Transfer,
     ERC721Transfer,
@@ -114,38 +113,6 @@ def safe_master_copy_clear_cache(
     SafeMasterCopy.objects.get_version_for_address.cache_clear()
 
 
-def get_safe_addresses_involved_from_db_instance(
-    instance: Union[
-        TokenTransfer,
-        InternalTx,
-        MultisigConfirmation,
-        MultisigTransaction,
-    ]
-) -> List[Optional[ChecksumAddress]]:
-    """
-    Retrieves the Safe addresses involved in the provided database instance.
-
-    :param instance:
-    :return: List of Safe addresses from the provided instance
-    """
-    addresses = []
-    if isinstance(instance, TokenTransfer):
-        addresses.append(instance.to)
-        addresses.append(instance._from)
-        return addresses
-    elif isinstance(instance, MultisigTransaction):
-        addresses.append(instance.safe)
-        return addresses
-    elif isinstance(instance, MultisigConfirmation) and instance.multisig_transaction:
-        addresses.append(instance.multisig_transaction.safe)
-        return addresses
-    elif isinstance(instance, InternalTx):
-        addresses.append(instance.to)
-        return addresses
-
-    return addresses
-
-
 def _process_notification_event(
     sender: Type[Model],
     instance: Union[
@@ -173,6 +140,8 @@ def _process_notification_event(
         created and deleted
     ), "An instance cannot be created and deleted at the same time"
 
+    logger.debug("Removing cache for object=%s", instance)
+    remove_cache_view_by_instance(instance)
     logger.debug("Start building payloads for created=%s object=%s", created, instance)
     payloads = build_event_payload(sender, instance, deleted=deleted)
     logger.debug(

safe_transaction_service/history/tests/test_views.py

@@ -33,7 +33,7 @@
 from safe_transaction_service.tokens.tests.factories import TokenFactory
 from safe_transaction_service.utils.utils import datetime_to_str
 
-from ...utils.redis import get_redis
+from ...utils.redis import get_redis, remove_cache_view_by_instance
 from ..helpers import DelegateSignatureHelper, DeleteMultisigTxSignatureHelper
 from ..models import (
     IndexingStatus,
@@ -1210,6 +1210,23 @@ def test_get_multisig_transactions(self):
         self.assertEqual(response.data["count"], 2)
         self.assertEqual(response.data["count_unique_nonce"], 1)
 
+        #
+        # Mock get_queryset with empty queryset return value to get proper error in case of fail
+        with mock.patch.object(
+            SafeMultisigTransactionListView,
+            "get_queryset",
+            return_value=MultisigTransaction.objects.none(),
+        ) as patched_queryset:
+            response = self.client.get(
+                reverse("v1:history:multisig-transactions", args=(safe_address,)),
+                format="json",
+            )
+            # view shouldn't be called
+            patched_queryset.assert_not_called()
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+            self.assertEqual(response.data["count"], 2)
+            self.assertEqual(response.data["count_unique_nonce"], 1)
+
     def test_get_multisig_transactions_unique_nonce(self):
         """
         Unique nonce should follow the trusted filter
@@ -1279,6 +1296,8 @@ def test_get_multisig_transactions_not_decoded(
             ContractFactory(
                 address=multisig_transaction.to, trusted_for_delegate_call=True
             )
+            # Force to clean cache because we are not cleaning the cache on contracts change
+            remove_cache_view_by_instance(multisig_transaction)
             response = self.client.get(
                 reverse("v1:history:multisig-transactions", args=(safe_address,)),
                 format="json",

safe_transaction_service/history/views.py

@@ -33,6 +33,11 @@
 from safe_transaction_service.utils.ethereum import get_chain_id
 from safe_transaction_service.utils.utils import parse_boolean_query_param
 
+from ..utils.redis import (
+    LIST_MODULETRANSACTIONS_VIEW_CACHE_KEY,
+    LIST_MULTISIGTRANSACTIONS_VIEW_CACHE_KEY,
+    cache_page_for_address,
+)
 from . import filters, pagination, serializers
 from .helpers import add_tokens_to_transfers, is_valid_unique_transfer_id
 from .models import (
@@ -460,6 +465,9 @@ def get_queryset(self):
     @swagger_auto_schema(
         responses={400: "Invalid data", 422: "Invalid ethereum address"}
     )
+    @cache_page_for_address(
+        cache_tag=LIST_MODULETRANSACTIONS_VIEW_CACHE_KEY, timeout=60
+    )
     def get(self, request, address, format=None):
         """
         Returns all the transactions executed from modules given a Safe address
@@ -630,6 +638,9 @@ def get_serializer_class(self):
     @swagger_auto_schema(
         responses={400: "Invalid data", 422: "Invalid ethereum address"}
     )
+    @cache_page_for_address(
+        cache_tag=LIST_MULTISIGTRANSACTIONS_VIEW_CACHE_KEY, timeout=2
+    )
     def get(self, request, *args, **kwargs):
         """
         Returns all the multi-signature transactions for a given Safe address.

safe_transaction_service/utils/redis.py

@@ -1,12 +1,25 @@
 import copyreg
+import json
 import logging
-import pickle
 from functools import cache, wraps
+from typing import List, Optional, Union
 from urllib.parse import urlencode
 
 from django.conf import settings
 
+from eth_typing import ChecksumAddress
 from redis import Redis
+from rest_framework import status
+from rest_framework.response import Response
+
+from safe_transaction_service.contracts.models import Contract
+from safe_transaction_service.history.models import (
+    InternalTx,
+    ModuleTransaction,
+    MultisigConfirmation,
+    MultisigTransaction,
+    TokenTransfer,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -21,7 +34,25 @@ def get_redis() -> Redis:
     return Redis.from_url(settings.REDIS_URL)
 
 
-def cache_view_response(timeout: int, cache_name: str):
+LIST_MULTISIGTRANSACTIONS_VIEW_CACHE_KEY = "multisigtransactionsview"
+LIST_MODULETRANSACTIONS_VIEW_CACHE_KEY = "moduletransactionsview"
+LIST_TRANSFERS_VIEW_CACHE_KEY = "transfersview"
+
+
+def get_cache_page_name(cache_tag: str, address: ChecksumAddress) -> str:
+    """
+    Calculate the cache_name from the cache_tag and provided address
+
+    :param cache_tag:
+    :param address:
+    :return:
+    """
+    return f"{cache_tag}:{address}"
+
+
+def cache_page_for_address(
+    cache_tag: str, timeout: int = settings.DEFAULT_CACHE_PAGE_TIMEOUT
+):
     """
     Custom cache decorator that caches the view response.
     This decorator caches the response of a view function for a specified timeout.
@@ -37,20 +68,38 @@ def decorator(view_func):
         def _wrapped_view(request, *args, **kwargs):
             redis = get_redis()
             # Get query parameters
-            query_params = request.GET.dict()
-            cache_path = f"{request.path}:{urlencode(query_params)}"
-
-            # Check if response is cached
-            response = redis.hget(cache_name, cache_path)
-            if response:
-                return pickle.loads(response)
+            query_params = request.request.GET.dict()
+            cache_path = f"{urlencode(query_params)}"
+            # Calculate cache_name
+            address = request.kwargs["address"]
+            if address:
+                cache_name = get_cache_page_name(cache_tag, address)
+            else:
+                logger.warning(
+                    "Address does not exist in the request, this will not be cached"
+                )
+                cache_name = None
+
+            if cache_name:
+                # Check if response is cached
+                response_data = redis.hget(cache_name, cache_path)
+                if response_data:
+                    logger.debug(f"Getting from cache {cache_name}{cache_path}")
+                    return Response(
+                        status=status.HTTP_200_OK, data=json.loads(response_data)
+                    )
 
             # Get response from the view
-            response = view_func(request, *args, **kwargs).render()
+            response = view_func(request, *args, **kwargs)
             if response.status_code == 200:
-                # We just store the success result
-                redis.hset(cache_name, cache_path, pickle.dumps(response))
-                redis.expire(cache_name, timeout)
+                # Just store if there were not issues calculating cache_name
+                if cache_name:
+                    # We just store the success result
+                    logger.debug(
+                        f"Setting cache {cache_name}{cache_path} with TTL {timeout} seconds"
+                    )
+                    redis.hset(cache_name, cache_path, json.dumps(response.data))
+                    redis.expire(cache_name, timeout)
 
             return response
 
@@ -59,11 +108,63 @@ def _wrapped_view(request, *args, **kwargs):
     return decorator
 
 
-def remove_cache_view_response(cache_name: str):
+def remove_cache_page_by_address(cache_tag: str, address: ChecksumAddress):
     """
-    Remove cache key stored in redis
+    Remove cache key stored in redis for the provided parameters
 
     :param cache_name:
     :return:
     """
+    cache_name = get_cache_page_name(cache_tag, address)
+
+    logger.debug(f"Removing all the cache for {cache_name}")
     get_redis().unlink(cache_name)
+
+
+def remove_cache_page_for_addresses(cache_tag: str, addresses: List[ChecksumAddress]):
+    """
+    Remove cache for provided addresses
+
+    :param cache_tag:
+    :param addresses:
+    :return:
+    """
+    for address in addresses:
+        remove_cache_page_by_address(cache_tag, address)
+
+
+def remove_cache_view_by_instance(
+    instance: Union[
+        TokenTransfer,
+        InternalTx,
+        MultisigConfirmation,
+        MultisigTransaction,
+        ModuleTransaction,
+        Contract,
+    ]
+):
+    """
+    Remove the cache stored for instance view.
+
+    :param instance:
+    """
+    addresses = []
+    cache_tag: Optional[str] = None
+    if isinstance(instance, TokenTransfer):
+        cache_tag = LIST_TRANSFERS_VIEW_CACHE_KEY
+        addresses.append(instance.to)
+        addresses.append(instance._from)
+    elif isinstance(instance, MultisigTransaction) or isinstance(
+        instance, MultisigConfirmation
+    ):
+        cache_tag = LIST_MULTISIGTRANSACTIONS_VIEW_CACHE_KEY
+        addresses.append(instance.safe)
+    elif isinstance(instance, InternalTx):
+        cache_tag = LIST_TRANSFERS_VIEW_CACHE_KEY
+        addresses.append(instance.to)
+    elif isinstance(instance, ModuleTransaction):
+        cache_tag = LIST_MODULETRANSACTIONS_VIEW_CACHE_KEY
+        addresses.append(instance.safe)
+
+    if cache_tag:
+        remove_cache_page_for_addresses(cache_tag, addresses)