From b8b7c08e04eab0827a3b5c0f23d9499b3d32223a Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Thu, 5 Oct 2023 16:01:30 +0200 Subject: [PATCH 1/2] Add proposer field --- .../0075_multisigtransaction_proposer.py | 20 +++++ safe_transaction_service/history/models.py | 1 + .../history/serializers.py | 8 ++ .../history/tests/test_migrations.py | 73 +++++++++++-------- .../history/tests/test_views.py | 25 ++++++- 5 files changed, 92 insertions(+), 35 deletions(-) create mode 100644 safe_transaction_service/history/migrations/0075_multisigtransaction_proposer.py diff --git a/safe_transaction_service/history/migrations/0075_multisigtransaction_proposer.py b/safe_transaction_service/history/migrations/0075_multisigtransaction_proposer.py new file mode 100644 index 000000000..5353854a3 --- /dev/null +++ b/safe_transaction_service/history/migrations/0075_multisigtransaction_proposer.py @@ -0,0 +1,20 @@ +# Generated by Django 4.2.5 on 2023-10-05 09:23 + +from django.db import migrations + +import gnosis.eth.django.models + + +class Migration(migrations.Migration): + + dependencies = [ + ("history", "0074_internaltx_history_internal_transfer_idx"), + ] + + operations = [ + migrations.AddField( + model_name="multisigtransaction", + name="proposer", + field=gnosis.eth.django.models.EthereumAddressV2Field(null=True), + ), + ] diff --git a/safe_transaction_service/history/models.py b/safe_transaction_service/history/models.py index 3ff15a5f9..a80d22859 100644 --- a/safe_transaction_service/history/models.py +++ b/safe_transaction_service/history/models.py @@ -1366,6 +1366,7 @@ class MultisigTransaction(TimeStampedModel): objects = MultisigTransactionManager.from_queryset(MultisigTransactionQuerySet)() safe_tx_hash = Keccak256Field(primary_key=True) safe = EthereumAddressV2Field(db_index=True) + proposer = EthereumAddressV2Field(null=True) ethereum_tx = models.ForeignKey( EthereumTx, null=True, diff --git a/safe_transaction_service/history/serializers.py b/safe_transaction_service/history/serializers.py index 27135249f..4bc256e85 100644 --- a/safe_transaction_service/history/serializers.py +++ b/safe_transaction_service/history/serializers.py @@ -256,6 +256,12 @@ def save(self, **kwargs): and (user := request.user) ): trusted = user.has_perm("history.create_trusted") + try: + proposer = SafeContractDelegate.objects.get( + delegate=self.validated_data["sender"] + ).delegator + except SafeContractDelegate.DoesNotExist: + proposer = self.validated_data["sender"] multisig_transaction, created = MultisigTransaction.objects.get_or_create( safe_tx_hash=safe_tx_hash, @@ -275,6 +281,7 @@ def save(self, **kwargs): "nonce": self.validated_data["nonce"], "origin": origin, "trusted": trusted, + "proposer": proposer, }, ) @@ -546,6 +553,7 @@ class SafeMultisigTransactionResponseSerializer(SafeMultisigTxSerializerV1): block_number = serializers.SerializerMethodField() transaction_hash = Sha3HashField(source="ethereum_tx_id") safe_tx_hash = Sha3HashField() + proposer = EthereumAddressField() executor = serializers.SerializerMethodField() value = serializers.CharField() is_executed = serializers.BooleanField(source="executed") diff --git a/safe_transaction_service/history/tests/test_migrations.py b/safe_transaction_service/history/tests/test_migrations.py index ee6a12b8b..bfa0060f7 100644 --- a/safe_transaction_service/history/tests/test_migrations.py +++ b/safe_transaction_service/history/tests/test_migrations.py @@ -7,8 +7,6 @@ from eth_account import Account from web3 import Web3 -from safe_transaction_service.history.tests.factories import MultisigTransactionFactory - class TestMigrations(TestCase): def setUp(self) -> None: @@ -252,23 +250,28 @@ def test_migration_forward_0073_safe_apps_links(self): new_state = self.migrator.apply_initial_migration( ("history", "0072_safecontract_banned_and_more"), ) - - # Factories can be used as there are no database definition changes - # Make sure there are no issues with empty `origin` or `origin` lacking `url` - MultisigTransactionFactory(origin={"not_url": "random"}) - - # Make sure other urls are not affected - MultisigTransactionFactory( - origin={"url": "https://app.zerion.io", "name": "Zerion"} - ) - - # This origin must be replaced - MultisigTransactionFactory( - origin={ + origins = [ + {"not_url": "random"}, + {"url": "https://app.zerion.io", "name": "Zerion"}, + { "url": "https://apps.gnosis-safe.io/tx-builder/", "name": "Transaction Builder", - } - ) + }, + ] + + MultisigTransaction = new_state.apps.get_model("history", "MultisigTransaction") + for origin in origins: + MultisigTransaction.objects.create( + safe_tx_hash=Web3.keccak(text=f"multisig-tx-{origin}").hex(), + safe=Account.create().address, + value=0, + operation=0, + safe_tx_gas=0, + base_gas=0, + gas_price=0, + nonce=0, + origin=origin, + ) new_state = self.migrator.apply_tested_migration( ("history", "0073_safe_apps_links"), @@ -295,22 +298,28 @@ def test_migration_backward_0073_safe_apps_links(self): ("history", "0073_safe_apps_links"), ) - # Factories can be used as there are no database definition changes - # Make sure there are no issues with empty `origin` or `origin` lacking `url` - MultisigTransactionFactory(origin={"not_url": "random"}) - - # Make sure other urls are not affected - MultisigTransactionFactory( - origin={"url": "https://app.zerion.io", "name": "Zerion"} - ) - - # This origin must be replaced - MultisigTransactionFactory( - origin={ - "url": "https://apps-portal.safe.global/tx-builder/", + origins = [ + {"not_url": "random"}, + {"url": "https://app.zerion.io", "name": "Zerion"}, + { + "url": "https://apps.gnosis-safe.io/tx-builder/", "name": "Transaction Builder", - } - ) + }, + ] + + MultisigTransaction = new_state.apps.get_model("history", "MultisigTransaction") + for origin in origins: + MultisigTransaction.objects.create( + safe_tx_hash=Web3.keccak(text=f"multisig-tx-{origin}").hex(), + safe=Account.create().address, + value=0, + operation=0, + safe_tx_gas=0, + base_gas=0, + gas_price=0, + nonce=0, + origin=origin, + ) new_state = self.migrator.apply_tested_migration( ("history", "0072_safecontract_banned_and_more"), diff --git a/safe_transaction_service/history/tests/test_views.py b/safe_transaction_service/history/tests/test_views.py index 91d6db192..e51f684a0 100644 --- a/safe_transaction_service/history/tests/test_views.py +++ b/safe_transaction_service/history/tests/test_views.py @@ -798,6 +798,7 @@ def test_get_multisig_transaction(self): "b1b3b164cf000000000000000000000000000000000000000000000000000000" "0000000001" ) + multisig_tx = MultisigTransactionFactory(data=add_owner_with_threshold_data) safe_tx_hash = multisig_tx.safe_tx_hash response = self.client.get( @@ -814,6 +815,7 @@ def test_get_multisig_transaction(self): self.assertFalse(response.data["trusted"]) self.assertIsNone(response.data["max_fee_per_gas"]) self.assertIsNone(response.data["max_priority_fee_per_gas"]) + self.assertIsNone(response.data["proposer"]) self.assertEqual( response.data["data_decoded"], { @@ -828,6 +830,7 @@ def test_get_multisig_transaction(self): ], }, ) + # Test camelCase self.assertEqual( response.json()["transactionHash"], multisig_tx.ethereum_tx.tx_hash @@ -855,8 +858,19 @@ def test_get_multisig_transaction(self): self.assertEqual(response.data["origin"], json.dumps(origin)) self.assertEqual(json.loads(response.data["origin"]), origin) + # Test proposer + proposer = Account.create().address + multisig_tx.proposer = proposer + multisig_tx.save() + response = self.client.get( + reverse("v1:history:multisig-transaction", args=(safe_tx_hash,)), + format="json", + ) + self.assertEqual(response.data["proposer"], proposer) + def test_get_multisig_transactions(self): safe_address = Account.create().address + proposer = Account.create().address response = self.client.get( reverse("v1:history:multisig-transactions", args=(safe_address,)), format="json", @@ -865,7 +879,7 @@ def test_get_multisig_transactions(self): self.assertEqual(response.data["count"], 0) self.assertEqual(response.data["count_unique_nonce"], 0) - multisig_tx = MultisigTransactionFactory(safe=safe_address) + multisig_tx = MultisigTransactionFactory(safe=safe_address, proposer=proposer) response = self.client.get( reverse("v1:history:multisig-transactions", args=(safe_address,)), format="json", @@ -889,7 +903,6 @@ def test_get_multisig_transactions(self): ) # Check Etag header self.assertTrue(response["Etag"]) - MultisigConfirmationFactory(multisig_transaction=multisig_tx) response = self.client.get( reverse("v1:history:multisig-transactions", args=(safe_address,)), @@ -898,6 +911,7 @@ def test_get_multisig_transactions(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(len(response.data["results"]), 1) self.assertEqual(len(response.data["results"][0]["confirmations"]), 1) + self.assertEqual(response.data["results"][0]["proposer"], proposer) MultisigTransactionFactory(safe=safe_address, nonce=multisig_tx.nonce) response = self.client.get( @@ -1182,6 +1196,7 @@ def test_post_multisig_transactions(self): self.assertEqual(len(response.data["results"]), 1) self.assertIsNone(response.data["results"][0]["executor"]) self.assertEqual(len(response.data["results"][0]["confirmations"]), 0) + self.assertEqual(response.data["results"][0]["proposer"], data["sender"]) # Test confirmation with signature data["signature"] = safe_owner_1.signHash(safe_tx.safe_tx_hash)[ @@ -1676,7 +1691,11 @@ def test_post_multisig_transactions_with_delegate(self): self.assertEqual(response.status_code, status.HTTP_201_CREATED) self.assertEqual(MultisigTransaction.objects.count(), 1) self.assertEqual(MultisigConfirmation.objects.count(), 0) - self.assertTrue(MultisigTransaction.objects.first().trusted) + multisig_transaction = MultisigTransaction.objects.first() + self.assertTrue(multisig_transaction.trusted) + # Proposer should be the owner address not the delegate + self.assertNotEqual(multisig_transaction.proposer, safe_delegate.address) + self.assertEqual(multisig_transaction.proposer, safe_owners[0].address) data["signature"] = data["signature"] + data["signature"][2:] response = self.client.post( From 64a11531e7c97423c8d12a04db9801803a7d59a4 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Mon, 9 Oct 2023 13:03:18 +0200 Subject: [PATCH 2/2] Reduce number of database queries --- safe_transaction_service/history/serializers.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/safe_transaction_service/history/serializers.py b/safe_transaction_service/history/serializers.py index 4bc256e85..375ca9a53 100644 --- a/safe_transaction_service/history/serializers.py +++ b/safe_transaction_service/history/serializers.py @@ -256,12 +256,13 @@ def save(self, **kwargs): and (user := request.user) ): trusted = user.has_perm("history.create_trusted") - try: + + if self.validated_data["sender"] in self.validated_data["safe_owners"]: + proposer = self.validated_data["sender"] + else: proposer = SafeContractDelegate.objects.get( delegate=self.validated_data["sender"] ).delegator - except SafeContractDelegate.DoesNotExist: - proposer = self.validated_data["sender"] multisig_transaction, created = MultisigTransaction.objects.get_or_create( safe_tx_hash=safe_tx_hash,