From 86b29ad3df94cfe04cf4863762e685788b75befe Mon Sep 17 00:00:00 2001 From: Felipe Alvarado Date: Wed, 23 Oct 2024 08:35:20 +0200 Subject: [PATCH] Add expiry date to safe contract delegates (#2273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add expiry date to safe contract delegates * Apply PR suggestion Co-authored-by: Uxío * Fix delete expired delegates task name --------- Co-authored-by: Uxío --- .../management/commands/setup_service.py | 5 +++ .../0089_safecontractdelegate_expiry_date.py | 18 ++++++++++ safe_transaction_service/history/models.py | 15 ++++++--- .../history/serializers.py | 23 +++++++++++++ safe_transaction_service/history/tasks.py | 17 +++++++++- .../history/tests/factories.py | 2 ++ .../history/tests/test_models.py | 26 +++++++++++++++ .../history/tests/test_tasks.py | 33 ++++++++++++++++++- .../history/tests/test_views.py | 6 ++++ .../history/tests/test_views_v2.py | 21 ++++++++++++ 10 files changed, 159 insertions(+), 7 deletions(-) create mode 100644 safe_transaction_service/history/migrations/0089_safecontractdelegate_expiry_date.py diff --git a/safe_transaction_service/history/management/commands/setup_service.py b/safe_transaction_service/history/management/commands/setup_service.py index 01b7d63b6..6fe97f720 100644 --- a/safe_transaction_service/history/management/commands/setup_service.py +++ b/safe_transaction_service/history/management/commands/setup_service.py @@ -134,6 +134,11 @@ def create_task(self) -> Tuple[PeriodicTask, bool]: description="Remove older than 1 month not trusted Multisig Txs (every day at 00:00)", cron=CronDefinition(minute=0, hour=0), # Every day at 00:00 - 0 0 * * * ), + CeleryTaskConfiguration( + name="safe_transaction_service.history.tasks.delete_expired_delegates_task", + description="Remove expired Safe Contract Delegates (every hour at minute 0)", + cron=CronDefinition(minute=0), # Every hour at minute 0 - 0 * * * * + ), CeleryTaskConfiguration( name="safe_transaction_service.contracts.tasks.create_missing_contracts_with_metadata_task", description="Index contract names and ABIs (every hour at minute 0)", diff --git a/safe_transaction_service/history/migrations/0089_safecontractdelegate_expiry_date.py b/safe_transaction_service/history/migrations/0089_safecontractdelegate_expiry_date.py new file mode 100644 index 000000000..115d4f3bf --- /dev/null +++ b/safe_transaction_service/history/migrations/0089_safecontractdelegate_expiry_date.py @@ -0,0 +1,18 @@ +# Generated by Django 5.0.9 on 2024-10-18 13:52 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("history", "0088_erc20transfer_history_erc__from_d88198_idx_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="safecontractdelegate", + name="expiry_date", + field=models.DateTimeField(db_index=True, null=True), + ), + ] diff --git a/safe_transaction_service/history/models.py b/safe_transaction_service/history/models.py index 41149fa34..74b442fd1 100644 --- a/safe_transaction_service/history/models.py +++ b/safe_transaction_service/history/models.py @@ -1784,11 +1784,15 @@ def get_for_safe( if not owner_addresses: return self.none() - return self.filter( - # If safe_contract is null on SafeContractDelegate, delegates are valid for every Safe - Q(safe_contract_id=safe_address) - | Q(safe_contract=None) - ).filter(delegator__in=owner_addresses) + return ( + self.filter( + # If safe_contract is null on SafeContractDelegate, delegates are valid for every Safe + Q(safe_contract_id=safe_address) + | Q(safe_contract=None) + ) + .filter(delegator__in=owner_addresses) + .filter(Q(expiry_date__isnull=True) | Q(expiry_date__gt=timezone.now())) + ) def get_for_safe_and_delegate( self, @@ -1844,6 +1848,7 @@ class SafeContractDelegate(models.Model): label = models.CharField(max_length=50) read = models.BooleanField(default=True) # For permissions in the future write = models.BooleanField(default=True) + expiry_date = models.DateTimeField(null=True, db_index=True) class Meta: constraints = [ diff --git a/safe_transaction_service/history/serializers.py b/safe_transaction_service/history/serializers.py index 9180047b2..50ba77860 100644 --- a/safe_transaction_service/history/serializers.py +++ b/safe_transaction_service/history/serializers.py @@ -1,9 +1,11 @@ +import datetime import itertools import json from enum import Enum from typing import Any, Dict, List, Optional from django.http import Http404 +from django.utils import timezone from drf_yasg.utils import swagger_serializer_method from eth_typing import ChecksumAddress @@ -377,6 +379,7 @@ class SafeDelegateResponseSerializer(serializers.Serializer): delegate = EthereumAddressField() delegator = EthereumAddressField() label = serializers.CharField(max_length=50) + expiry_date = serializers.DateTimeField() class DelegateSerializerMixin: @@ -444,6 +447,24 @@ class DelegateSerializerV2(DelegateSerializerMixin, serializers.Serializer): delegator = EthereumAddressField() signature = HexadecimalField(min_length=65, max_length=MAX_SIGNATURE_LENGTH) label = serializers.CharField(max_length=50) + expiry_date = serializers.DateTimeField( + allow_null=True, required=False, default=None + ) + + def validate_expiry_date( + self, expiry_date: Optional[datetime.datetime] + ) -> Optional[datetime.datetime]: + """ + Make sure ``expiry_date`` is not previous to the current timestamp + + :param expiry_date: + :return: `expiry_date` + """ + if expiry_date and expiry_date <= timezone.now(): + raise ValidationError( + "`expiry_date` cannot be previous to the current timestamp" + ) + return expiry_date def validate(self, attrs): super().validate(attrs) @@ -465,12 +486,14 @@ def save(self, **kwargs): delegate = self.validated_data["delegate"] delegator = self.validated_data["delegator"] label = self.validated_data["label"] + expiry_date = self.validated_data["expiry_date"] obj, _ = SafeContractDelegate.objects.update_or_create( safe_contract_id=safe_address, delegate=delegate, delegator=delegator, defaults={ "label": label, + "expiry_date": expiry_date, }, ) return obj diff --git a/safe_transaction_service/history/tasks.py b/safe_transaction_service/history/tasks.py index 84034d4b1..7a952dd6b 100644 --- a/safe_transaction_service/history/tasks.py +++ b/safe_transaction_service/history/tasks.py @@ -25,7 +25,13 @@ ProxyFactoryIndexerProvider, SafeEventsIndexerProvider, ) -from .models import EthereumBlock, InternalTxDecoded, MultisigTransaction, SafeContract +from .models import ( + EthereumBlock, + InternalTxDecoded, + MultisigTransaction, + SafeContract, + SafeContractDelegate, +) from .services import ( CollectiblesServiceProvider, IndexingException, @@ -499,3 +505,12 @@ def remove_not_trusted_multisig_txs_task( .delete() ) return deleted + + +@app.shared_task() +@close_gevent_db_connection_decorator +def delete_expired_delegates_task(): + logger.info("Deleting expired Safe Delegates") + now = timezone.now() + deleted, _ = SafeContractDelegate.objects.filter(expiry_date__lte=now).delete() + return deleted diff --git a/safe_transaction_service/history/tests/factories.py b/safe_transaction_service/history/tests/factories.py index e62ea20b0..6acbd06a4 100644 --- a/safe_transaction_service/history/tests/factories.py +++ b/safe_transaction_service/history/tests/factories.py @@ -1,3 +1,4 @@ +import datetime from typing import Any, Dict from django.utils import timezone @@ -335,6 +336,7 @@ class Meta: label = factory.Faker("name") read = True write = True + expiry_date = timezone.now() + datetime.timedelta(minutes=90) class MonitoredAddressFactory(DjangoModelFactory): diff --git a/safe_transaction_service/history/tests/test_models.py b/safe_transaction_service/history/tests/test_models.py index 9d75dc693..9941fe273 100644 --- a/safe_transaction_service/history/tests/test_models.py +++ b/safe_transaction_service/history/tests/test_models.py @@ -1,3 +1,4 @@ +import datetime import logging from datetime import timedelta from unittest import mock @@ -1094,6 +1095,31 @@ def test_get_for_safe(self): [safe_contract_delegate_another_safe, safe_contract_delegate_without_safe], ) + # Check expired delegate + safe_contract_delegate_expired = SafeContractDelegateFactory( + expiry_date=timezone.now() - datetime.timedelta(hours=1) + ) + safe_contract_delegate_not_expired = SafeContractDelegateFactory( + safe_contract=safe_contract_delegate_expired.safe_contract + ) + safe_contract_delegate_not_expired_2 = SafeContractDelegateFactory( + safe_contract=safe_contract_delegate_expired.safe_contract + ) + expired_delegate_safe_address = ( + safe_contract_delegate_expired.safe_contract.address + ) + self.assertCountEqual( + SafeContractDelegate.objects.get_for_safe( + expired_delegate_safe_address, + [ + safe_contract_delegate_expired.delegator, + safe_contract_delegate_not_expired.delegator, + safe_contract_delegate_not_expired_2.delegator, + ], + ), + [safe_contract_delegate_not_expired, safe_contract_delegate_not_expired_2], + ) + def test_get_for_safe_and_delegate(self): delegator = Account.create().address delegate = Account.create().address diff --git a/safe_transaction_service/history/tests/test_tasks.py b/safe_transaction_service/history/tests/test_tasks.py index ea6d90dc7..769eb1f65 100644 --- a/safe_transaction_service/history/tests/test_tasks.py +++ b/safe_transaction_service/history/tests/test_tasks.py @@ -16,7 +16,13 @@ InternalTxIndexerProvider, SafeEventsIndexerProvider, ) -from ..models import MultisigTransaction, SafeContract, SafeLastStatus, SafeStatus +from ..models import ( + MultisigTransaction, + SafeContract, + SafeContractDelegate, + SafeLastStatus, + SafeStatus, +) from ..services import ( CollectiblesService, CollectiblesServiceProvider, @@ -27,6 +33,7 @@ from ..tasks import ( check_reorgs_task, check_sync_status_task, + delete_expired_delegates_task, index_erc20_events_out_of_sync_task, index_erc20_events_task, index_internal_txs_task, @@ -46,6 +53,7 @@ EthereumBlockFactory, InternalTxDecodedFactory, MultisigTransactionFactory, + SafeContractDelegateFactory, SafeContractFactory, SafeStatusFactory, ) @@ -334,3 +342,26 @@ def test_remove_not_trusted_multisig_txs_task(self): safe_tx_hash=multisig_tx_expected_to_be_deleted.safe_tx_hash ).exists() ) + + def test_delete_expired_delegates_task(self): + self.assertEqual(delete_expired_delegates_task.delay().result, 0) + + SafeContractDelegateFactory() + SafeContractDelegateFactory(expiry_date=None) + + self.assertEqual(delete_expired_delegates_task.delay().result, 0) + + safe_contract_delegate_expected_to_be_deleted = SafeContractDelegateFactory( + expiry_date=timezone.now() - datetime.timedelta(hours=1) + ) + + self.assertEqual(SafeContractDelegate.objects.count(), 3) + self.assertEqual(delete_expired_delegates_task.delay().result, 1) + + self.assertFalse( + SafeContractDelegate.objects.filter( + safe_contract=safe_contract_delegate_expected_to_be_deleted.safe_contract, + delegate=safe_contract_delegate_expected_to_be_deleted.delegate, + delegator=safe_contract_delegate_expected_to_be_deleted.delegator, + ).exists() + ) diff --git a/safe_transaction_service/history/tests/test_views.py b/safe_transaction_service/history/tests/test_views.py index 861c609e8..ba734027d 100644 --- a/safe_transaction_service/history/tests/test_views.py +++ b/safe_transaction_service/history/tests/test_views.py @@ -2162,6 +2162,7 @@ def test_delegates_post(self): self.assertEqual(safe_contract_delegate.delegator, delegator.address) self.assertEqual(safe_contract_delegate.label, label) self.assertEqual(safe_contract_delegate.safe_contract_id, safe_address) + self.assertEqual(safe_contract_delegate.expiry_date, None) # Update label label = "Jimmy McGill" @@ -2171,6 +2172,7 @@ def test_delegates_post(self): self.assertEqual(SafeContractDelegate.objects.count(), 1) safe_contract_delegate = SafeContractDelegate.objects.get() self.assertEqual(safe_contract_delegate.label, label) + self.assertEqual(safe_contract_delegate.expiry_date, None) # Create delegate without a Safe another_label = "Kim Wexler" @@ -2232,12 +2234,14 @@ def test_delegates_get(self): "delegator": safe_contract_delegate_1.delegator, "label": safe_contract_delegate_1.label, "safe": safe_contract.address, + "expiry_date": datetime_to_str(safe_contract_delegate_1.expiry_date), }, { "delegate": safe_contract_delegate_2.delegate, "delegator": safe_contract_delegate_2.delegator, "label": safe_contract_delegate_2.label, "safe": safe_contract.address, + "expiry_date": datetime_to_str(safe_contract_delegate_2.expiry_date), }, ] response = self.client.get( @@ -2255,12 +2259,14 @@ def test_delegates_get(self): "delegator": safe_contract_delegate_1.delegator, "label": safe_contract_delegate_1.label, "safe": safe_contract.address, + "expiry_date": datetime_to_str(safe_contract_delegate_1.expiry_date), }, { "delegate": safe_contract_delegate_3.delegate, "delegator": safe_contract_delegate_3.delegator, "label": safe_contract_delegate_3.label, "safe": safe_contract_delegate_3.safe_contract_id, + "expiry_date": datetime_to_str(safe_contract_delegate_3.expiry_date), }, ] response = self.client.get( diff --git a/safe_transaction_service/history/tests/test_views_v2.py b/safe_transaction_service/history/tests/test_views_v2.py index 9a0b3e981..f7787707e 100644 --- a/safe_transaction_service/history/tests/test_views_v2.py +++ b/safe_transaction_service/history/tests/test_views_v2.py @@ -1,6 +1,8 @@ +from datetime import timedelta from unittest import mock from django.urls import reverse +from django.utils import timezone from eth_account import Account from hexbytes import HexBytes @@ -11,6 +13,7 @@ from safe_eth.safe.tests.safe_test_case import SafeTestCaseMixin from ...tokens.models import Token +from ...utils.utils import datetime_to_str from ..helpers import DelegateSignatureHelperV2 from ..models import SafeContractDelegate from .factories import ( @@ -169,6 +172,7 @@ def test_delegates_post(self): "delegator": delegator.address, "label": label, "signature": "0x" + "1" * 130, + "expiry_date": datetime_to_str(timezone.now() + timedelta(minutes=30)), } response = self.client.post(url, format="json", data=data) self.assertIn( @@ -225,6 +229,19 @@ def test_delegates_post(self): safe_contract_delegate = SafeContractDelegate.objects.get() self.assertEqual(safe_contract_delegate.label, label) + # Create expired delegate + data["expiry_date"] = datetime_to_str(timezone.now() - timedelta(hours=1)) + response = self.client.post(url, format="json", data=data) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + # Remove expiry date + data["expiry_date"] = None + response = self.client.post(url, format="json", data=data) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(SafeContractDelegate.objects.count(), 1) + safe_contract_delegate = SafeContractDelegate.objects.get() + self.assertIsNone(safe_contract_delegate.expiry_date) + # Create delegate without a Safe hash_to_sign = DelegateSignatureHelperV2.calculate_hash( delegate.address, chain_id, False @@ -327,12 +344,14 @@ def test_delegates_get(self): "delegator": safe_contract_delegate_1.delegator, "label": safe_contract_delegate_1.label, "safe": safe_contract.address, + "expiry_date": datetime_to_str(safe_contract_delegate_1.expiry_date), }, { "delegate": safe_contract_delegate_2.delegate, "delegator": safe_contract_delegate_2.delegator, "label": safe_contract_delegate_2.label, "safe": safe_contract.address, + "expiry_date": datetime_to_str(safe_contract_delegate_2.expiry_date), }, ] response = self.client.get( @@ -350,12 +369,14 @@ def test_delegates_get(self): "delegator": safe_contract_delegate_1.delegator, "label": safe_contract_delegate_1.label, "safe": safe_contract.address, + "expiry_date": datetime_to_str(safe_contract_delegate_1.expiry_date), }, { "delegate": safe_contract_delegate_3.delegate, "delegator": safe_contract_delegate_3.delegator, "label": safe_contract_delegate_3.label, "safe": safe_contract_delegate_3.safe_contract_id, + "expiry_date": datetime_to_str(safe_contract_delegate_3.expiry_date), }, ] response = self.client.get(