Refactor delegate endpoint signatures to use EIP712 #2001

falvaradorodriguez · 2024-04-19T11:15:14Z

Closes #1989

Points of interest:

Added endpoints in v2 to manage delegates. The delete and add endpoint has an optional "safe" field to manage the case of adding/deleting delegates for a safe (previously there were two delete endpoints).
A valid signature can be obtained with the following python script:

import environ

from eth_account import Account
from gnosis.eth.eip712 import eip712_encode_hash
from gnosis.eth import EthereumClient

env = environ.Env()

ethereum_client = EthereumClient(env.str("RPC_URL"))
account_a = Account.from_key(env.str("ACCOUNT_A_PRIVATE_KEY"))
account_b = Account.from_key(env.str("ACCOUNT_B_PRIVATE_KEY"))

types = {'EIP712Domain': [{'name': 'name', 'type': 'string'},
                          {'name': 'version', 'type': 'string'},
                          {'name': 'chainId', 'type': 'uint256'}],
         'Delegate': [{"name": "delegateAddress", "type": "bytes32"},
                      {"name": "totp", "type": "uint256"}]}

message = {"delegateAddress": account_b.address,
           "totp": get_totp()}

payload = {'types': types,
           'primaryType': 'Delegate',
           'domain': {"name": "Safe Transaction Service",
                      "version": "1.0",
                      "chainId": ethereum_client.get_chain_id()},
           'message': message}

eip712_hash = eip712_encode_hash(payload)
signature = account_a.signHash(eip712_hash)

moisses89 · 2024-04-22T07:50:36Z

safe_transaction_service/history/serializers.py

    """
-    Deprecated in favour of DelegateDeleteSerializer
+    Deprecated in favour of DelegateDeleteDeprecatedSerializer


Suggested change

Deprecated in favour of DelegateDeleteDeprecatedSerializer

Deprecated in favour of DelegateDeleteSerializer

I can understand the mistake, in my opinion would be better indicate that the serializer is a V2 version of the serializer, same that we are doing with the endpoint.
I mean, new Serializer DelegateDeleteSerializerV2 previous serializer DelegateDeleteSerializerV1.

Finally I have added the deprecated documentation tag and added V2 to the new classes.

moisses89 · 2024-04-22T08:09:29Z

safe_transaction_service/history/serializers.py

            ):
-                return attrs
+                raise ValidationError("Only EOA and ETH_SIGN signatures are supported")


I couldn´t find this restriction in the previous version of serializer. Is this new restriction expected?
If for example there is a Safe which one owner is other Safe, and this Safe delegated using contract signature EIPT1271 with the previous version wouldn´t be able to remove the delegate with the new version of the endpoint?

As we didn't have the limit before, we shouldn't enforce now, good catch @moisses89

Reviewed and updated! 👍

safe_transaction_service/history/serializers.py

moisses89 · 2024-04-22T14:46:50Z

safe_transaction_service/history/tests/test_views_v2.py

+        safe_address = Account.create().address
+        delegate = Account.create()
+        delegator = Account.create()
+        label = "Saul Goodman"


moisses89 · 2024-04-22T14:56:46Z

safe_transaction_service/history/tests/test_views_v2.py

+            # Create delegate
+            self.assertEqual(SafeContractDelegate.objects.count(), 0)
+            chain_id = self.ethereum_client.get_chain_id()
+            hash_to_sign = DelegateSignatureHelperV2.calculate_hash(


In this case we are using to calculate the hash the same function that is used to validate.
If there is an error in the way that the hash is calculated the test will pass without detect it.
My suggestion would be use the same safe_address, totp and delegate info to hardcode the expected hash or add a unit test of helper hash caclulation .

Added a unit test. Agree that it is better :)

moisses89 · 2024-04-22T15:12:18Z

safe_transaction_service/history/tests/test_views_v2.py

+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+        self.assertEqual(SafeContractDelegate.objects.count(), 2)
+
+        # Test not internal server error on contract signature


What means this test? that we don't return a 500 for a specific case?

I have just updated the commentary to avoid confusion.

moisses89 · 2024-04-22T15:19:12Z

safe_transaction_service/history/tests/test_views_v2.py

+        response = self.client.get(url, format="json")
+        self.assertEqual(response.data[0], "At least one query param must be provided")
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+


Could you include a test for not found delegate address?

moisses89

Just some minor comments

moisses89 · 2024-04-24T07:40:15Z

safe_transaction_service/history/tests/test_helpers.py

+        delegate_address = "0x1234567890123456789012345678901234567890"
+        chain_id = 1
+
+        expected_hash_previous_false = HexBytes(


It takes some time to me to understand what means the false, previous_false did me to think that was a previous wrong hash.
I think that could help add a comment to this line as # Hash calculated when totp previous is false the same for the previous true.

moisses89 · 2024-04-24T07:44:53Z

safe_transaction_service/history/serializers.py

@@ -982,9 +970,10 @@ class AllTransactionsSchemaSerializer(serializers.Serializer):
 # Deprecated ---------------------------------------------------------------


-class SafeDelegateDeleteSerializer(serializers.Serializer):
+class SafeDelegateDeleteDeprecatedSerializer(serializers.Serializer):


Other deprecated classes were keeping the class name but with a comment that the class is deprecated, are you aware that this class is renamed including the Deprecated tag?

Updated the name, it had been forgotten. As for the reasons, it was in this PR, but I don't know why.

falvaradorodriguez added 2 commits April 19, 2024 13:03

Add new endpoints to manage delegates with EIP712 signatures

805e24b

Allow localhost origin for dev

0363681

falvaradorodriguez marked this pull request as ready for review April 19, 2024 11:19

falvaradorodriguez requested a review from a team as a code owner April 19, 2024 11:19

Add more test to increase coverage

cff3d19

moisses89 requested changes Apr 22, 2024

View reviewed changes

falvaradorodriguez added 3 commits April 22, 2024 13:28

Rename classes as PR suggestions

2f6681e

Merge branch 'main' into refactor-delegate-endpoint-use-eip712

134ba36

Fix signature_type validation

ed07058

moisses89 reviewed Apr 22, 2024

View reviewed changes

moisses89 requested changes Apr 22, 2024

View reviewed changes

Add more tests

9992a67

moisses89 approved these changes Apr 24, 2024

View reviewed changes

falvaradorodriguez added 2 commits April 24, 2024 10:06

Add some comments in test_helpers

56c0168

Merge branch 'main' into refactor-delegate-endpoint-use-eip712

d778302

falvaradorodriguez merged commit 0b7acde into main Apr 24, 2024
5 checks passed

falvaradorodriguez deleted the refactor-delegate-endpoint-use-eip712 branch April 24, 2024 08:19

github-actions bot locked and limited conversation to collaborators Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor delegate endpoint signatures to use EIP712 #2001

Refactor delegate endpoint signatures to use EIP712 #2001

falvaradorodriguez commented Apr 19, 2024 •

edited

Loading

moisses89 Apr 22, 2024

falvaradorodriguez Apr 22, 2024

moisses89 Apr 22, 2024

Uxio0 Apr 22, 2024

falvaradorodriguez Apr 22, 2024

moisses89 Apr 22, 2024

moisses89 Apr 22, 2024

falvaradorodriguez Apr 22, 2024

moisses89 Apr 22, 2024

falvaradorodriguez Apr 22, 2024

moisses89 Apr 22, 2024

falvaradorodriguez Apr 22, 2024

moisses89 left a comment

moisses89 Apr 24, 2024

falvaradorodriguez Apr 24, 2024

moisses89 Apr 24, 2024 •

edited

Loading

falvaradorodriguez Apr 24, 2024

	Deprecated in favour of DelegateDeleteDeprecatedSerializer
	Deprecated in favour of DelegateDeleteSerializer

Refactor delegate endpoint signatures to use EIP712 #2001

Refactor delegate endpoint signatures to use EIP712 #2001

Conversation

falvaradorodriguez commented Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moisses89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moisses89 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falvaradorodriguez commented Apr 19, 2024 •

edited

Loading

moisses89 Apr 24, 2024 •

edited

Loading