From a8196defa89a77220fe407904512769ab63284c0 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Mon, 17 Jun 2024 16:32:00 +0200 Subject: [PATCH 1/7] Fix sign messages with blank spaces --- src/safe_cli/operators/safe_operator.py | 12 +++++++++--- src/safe_cli/operators/safe_tx_service_operator.py | 7 ++++--- src/safe_cli/prompt_parser.py | 2 +- src/safe_cli/safe_completer_constants.py | 2 +- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/safe_cli/operators/safe_operator.py b/src/safe_cli/operators/safe_operator.py index 0592875..6632d4b 100644 --- a/src/safe_cli/operators/safe_operator.py +++ b/src/safe_cli/operators/safe_operator.py @@ -67,7 +67,12 @@ get_safe_contract_address, get_safe_l2_contract_address, ) -from safe_cli.utils import choose_option_from_list, get_erc_20_list, yes_or_no_question +from safe_cli.utils import ( + choose_option_from_list, + get_erc_20_list, + get_input, + yes_or_no_question, +) from ..contracts import safe_to_l2_migration from .hw_wallets.hw_wallet import HwWallet @@ -451,7 +456,7 @@ def approve_hash(self, hash_to_approve: HexBytes, sender: str) -> bool: def sign_message( self, - eip191_message: Optional[str] = None, + eip191_message: Optional[bool] = False, eip712_message_path: Optional[str] = None, ) -> bool: if eip712_message_path: @@ -461,7 +466,8 @@ def sign_message( except ValueError: raise ValueError else: - message = eip191_message + print_formatted_text("EIP191 message to sign:") + message = get_input() message_bytes = eip191_message.encode("UTF-8") safe_message_hash = self.safe.get_message_hash(message_bytes) diff --git a/src/safe_cli/operators/safe_tx_service_operator.py b/src/safe_cli/operators/safe_tx_service_operator.py index 22ca308..7a91e60 100644 --- a/src/safe_cli/operators/safe_tx_service_operator.py +++ b/src/safe_cli/operators/safe_tx_service_operator.py @@ -21,7 +21,7 @@ from gnosis.safe.safe_signature import SafeSignature, SafeSignatureEOA from gnosis.safe.signatures import signature_to_bytes -from safe_cli.utils import yes_or_no_question +from safe_cli.utils import get_input, yes_or_no_question from . import SafeServiceNotAvailable from .exceptions import AccountNotLoadedException, NonExistingOwnerException @@ -45,7 +45,7 @@ def approve_hash(self, hash_to_approve: HexBytes, sender: str) -> bool: def sign_message( self, - eip191_message: Optional[str] = None, + eip191_message: Optional[bool] = False, eip712_message_path: Optional[str] = None, ) -> bool: if eip712_message_path: @@ -55,7 +55,8 @@ def sign_message( except ValueError: raise ValueError else: - message = eip191_message + print_formatted_text("EIP191 message to sign:") + message = get_input() message_hash = defunct_hash_message(text=message) safe_message_hash = self.safe.get_message_hash(message_hash) diff --git a/src/safe_cli/prompt_parser.py b/src/safe_cli/prompt_parser.py index 2117adf..79a24e1 100644 --- a/src/safe_cli/prompt_parser.py +++ b/src/safe_cli/prompt_parser.py @@ -400,7 +400,7 @@ def terminate_cli(args): # Sign message parser_sign_message = subparsers.add_parser("sign_message") group = parser_sign_message.add_mutually_exclusive_group(required=True) - group.add_argument("--eip191_message", type=str) + group.add_argument("--eip191_message", action="store_true") group.add_argument("--eip712_path", type=str) parser_sign_message.set_defaults(func=sign_message) diff --git a/src/safe_cli/safe_completer_constants.py b/src/safe_cli/safe_completer_constants.py index 6160f80..6794fa2 100644 --- a/src/safe_cli/safe_completer_constants.py +++ b/src/safe_cli/safe_completer_constants.py @@ -36,7 +36,7 @@ "send_erc721": "
[--safe-nonce ]", "send_ether": "
[--safe-nonce ]", "show_cli_owners": "(read-only)", - "sign_message": "[--eip191_message ] [--eip712_path ]", + "sign_message": "[--eip191_message] [--eip712_path ]", "sign-tx": "", "unload_cli_owners": "
[
...]", "update": "", From 27121ef4e1276b26a5dd42c601514863aba1ea20 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:49:50 +0200 Subject: [PATCH 2/7] Add confirm message for transaction service mode --- src/safe_cli/operators/safe_operator.py | 3 + .../operators/safe_tx_service_operator.py | 65 ++++++++++++++----- src/safe_cli/prompt_parser.py | 10 +++ 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/safe_cli/operators/safe_operator.py b/src/safe_cli/operators/safe_operator.py index 6632d4b..38d2d5c 100644 --- a/src/safe_cli/operators/safe_operator.py +++ b/src/safe_cli/operators/safe_operator.py @@ -492,6 +492,9 @@ def sign_message( HTML(f"Message was signed correctly: {safe_message_hash.hex()}") ) + def confirm_message(self, safe_message_hash: bytes, sender: ChecksumAddress): + return self._require_tx_service_mode() + def add_owner(self, new_owner: str, threshold: Optional[int] = None) -> bool: threshold = threshold if threshold is not None else self.safe_cli_info.threshold if new_owner in self.safe_cli_info.owners: diff --git a/src/safe_cli/operators/safe_tx_service_operator.py b/src/safe_cli/operators/safe_tx_service_operator.py index 7a91e60..a3dd6b0 100644 --- a/src/safe_cli/operators/safe_tx_service_operator.py +++ b/src/safe_cli/operators/safe_tx_service_operator.py @@ -21,8 +21,7 @@ from gnosis.safe.safe_signature import SafeSignature, SafeSignatureEOA from gnosis.safe.signatures import signature_to_bytes -from safe_cli.utils import get_input, yes_or_no_question - +from ..utils import get_input, yes_or_no_question from . import SafeServiceNotAvailable from .exceptions import AccountNotLoadedException, NonExistingOwnerException from .hw_wallets.hw_wallet import HwWallet @@ -94,6 +93,52 @@ def sign_message( ) return False + def confirm_message(self, safe_message_hash: bytes, sender: ChecksumAddress): + # GET message + try: + safe_message = self.safe_tx_service.get_message(safe_message_hash) + except SafeAPIException: + print_formatted_text( + HTML( + f"Message with hash {safe_message_hash} does not exist" + ) + ) + if not yes_or_no_question( + f"Message: {safe_message['message']} \n Do you want to sign the following message?:" + ): + return False + + signer = self.search_account(sender) + if not signer: + print_formatted_text( + HTML(f"Owner with address {sender} was not loaded") + ) + + if isinstance(signer, LocalAccount): + signature = signer.signHash(safe_message_hash).signature + else: + print_formatted_text( + HTML( + "Signing messages is not currently supported by hardware wallets" + ) + ) + return False + + if self.safe_tx_service.post_message_signature(safe_message_hash, signature): + print_formatted_text( + HTML( + "Message was correctly confirmed on Safe Transaction Service" + ) + ) + return True + else: + print_formatted_text( + HTML( + "Something went wrong confirming message on Safe Transaction Service" + ) + ) + return False + def get_delegates(self): delegates = self.safe_tx_service.get_delegates(self.address) headers = ["delegate", "delegator", "label"] @@ -157,21 +202,7 @@ def submit_signatures(self, safe_tx_hash: bytes) -> bool: ) ) else: - owners = self.get_permitted_signers() - for account in self.accounts: - if account.address in owners: - safe_tx.sign(account.key) - # Check if there are ledger signers - if self.hw_wallet_manager.wallets: - selected_ledger_accounts = [] - for ledger_account in self.hw_wallet_manager.wallets: - if ledger_account.address in owners: - selected_ledger_accounts.append(ledger_account) - if len(selected_ledger_accounts) > 0: - safe_tx = self.hw_wallet_manager.sign_safe_tx( - safe_tx, selected_ledger_accounts - ) - + safe_tx = self.sign_transaction(safe_tx) if safe_tx.signers: self.safe_tx_service.post_signatures(safe_tx_hash, safe_tx.signatures) print_formatted_text( diff --git a/src/safe_cli/prompt_parser.py b/src/safe_cli/prompt_parser.py index 79a24e1..b864e26 100644 --- a/src/safe_cli/prompt_parser.py +++ b/src/safe_cli/prompt_parser.py @@ -196,6 +196,10 @@ def approve_hash(args): def sign_message(args): safe_operator.sign_message(args.eip191_message, args.eip712_path) + @safe_exception + def confirm_message(args): + safe_operator.confirm_message(args.safe_message_hash, args.sender) + @safe_exception def add_owner(args): safe_operator.add_owner(args.address, threshold=args.threshold) @@ -404,6 +408,12 @@ def terminate_cli(args): group.add_argument("--eip712_path", type=str) parser_sign_message.set_defaults(func=sign_message) + # Confirm message + parser_confirm_message = subparsers.add_parser("confirm_message") + parser_confirm_message.add_argument("safe_message_hash", type=check_keccak256_hash) + parser_confirm_message.add_argument("sender", type=check_ethereum_address) + parser_confirm_message.set_defaults(func=confirm_message) + # Add owner parser_add_owner = subparsers.add_parser("add_owner") parser_add_owner.add_argument("address", type=check_ethereum_address) From 3d252bc03fa6bc8d2f4956964117c4e5292a9ac8 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Tue, 18 Jun 2024 13:24:38 +0200 Subject: [PATCH 3/7] Fix sign_message for eip191 --- src/safe_cli/operators/safe_operator.py | 2 +- tests/test_safe_operator.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/safe_cli/operators/safe_operator.py b/src/safe_cli/operators/safe_operator.py index 38d2d5c..a6086a0 100644 --- a/src/safe_cli/operators/safe_operator.py +++ b/src/safe_cli/operators/safe_operator.py @@ -468,7 +468,7 @@ def sign_message( else: print_formatted_text("EIP191 message to sign:") message = get_input() - message_bytes = eip191_message.encode("UTF-8") + message_bytes = message.encode("UTF-8") safe_message_hash = self.safe.get_message_hash(message_bytes) diff --git a/tests/test_safe_operator.py b/tests/test_safe_operator.py index 44f737d..5f916c6 100644 --- a/tests/test_safe_operator.py +++ b/tests/test_safe_operator.py @@ -185,7 +185,9 @@ def test_sign_message(self, mock_sign_message_lib_address): safe_operator.accounts.add(self.ethereum_test_account) safe_operator.default_sender = self.ethereum_test_account message_hash = safe_operator.safe.get_message_hash(bytes(message, "utf-8")) - safe_operator.sign_message(eip191_message=message) + with mock.patch("builtins.input", return_value=message): + safe_operator.sign_message(eip191_message=True) + self.assertTrue(safe_operator.safe.retrieve_is_message_signed(message_hash)) eip712_path = "tests/mocks/mock_eip712.json" message = json.load(open(eip712_path, "r")) From 53e6bffefa6811bdcbf49f5a175cf9293c107ac8 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Tue, 18 Jun 2024 16:43:45 +0200 Subject: [PATCH 4/7] Add test for confirm_message --- .../operators/safe_tx_service_operator.py | 21 ++++++------ tests/test_safe_tx_service_operator.py | 33 ++++++++++++++++++- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/safe_cli/operators/safe_tx_service_operator.py b/src/safe_cli/operators/safe_tx_service_operator.py index a3dd6b0..2643082 100644 --- a/src/safe_cli/operators/safe_tx_service_operator.py +++ b/src/safe_cli/operators/safe_tx_service_operator.py @@ -124,20 +124,19 @@ def confirm_message(self, safe_message_hash: bytes, sender: ChecksumAddress): ) return False - if self.safe_tx_service.post_message_signature(safe_message_hash, signature): + try: + self.safe_tx_service.post_message_signature(safe_message_hash, signature) + except SafeAPIException as e: print_formatted_text( - HTML( - "Message was correctly confirmed on Safe Transaction Service" - ) - ) - return True - else: - print_formatted_text( - HTML( - "Something went wrong confirming message on Safe Transaction Service" - ) + HTML(f"Message wasn't confirmed due an error: {e}") ) return False + print_formatted_text( + HTML( + "Message was correctly confirmed on Safe Transaction Service" + ) + ) + return True def get_delegates(self): delegates = self.safe_tx_service.get_delegates(self.address) diff --git a/tests/test_safe_tx_service_operator.py b/tests/test_safe_tx_service_operator.py index 07a7ba8..65e12db 100644 --- a/tests/test_safe_tx_service_operator.py +++ b/tests/test_safe_tx_service_operator.py @@ -9,7 +9,7 @@ from gnosis.eth import EthereumClient from gnosis.safe import SafeTx -from gnosis.safe.api import TransactionServiceApi +from gnosis.safe.api import SafeAPIException, TransactionServiceApi from safe_cli.operators import SafeOperatorMode, SafeTxServiceOperator @@ -551,6 +551,37 @@ def test_data_decoded_to_text(self): decoded_data_text, ) + @mock.patch.object( + TransactionServiceApi, "post_message_signature", return_value=True + ) + @mock.patch.object(TransactionServiceApi, "get_message", return_value=None) + def test_confirm_message( + self, get_message: MagicMock, post_message_signature: MagicMock + ): + safe_operator = self.setup_operator( + number_owners=1, mode=SafeOperatorMode.TX_SERVICE + ) + # confirm_message just need the raw message to print + get_message.return_value = {"message": "We just need a message"} + safe_message_hash = HexBytes( + "0xfbf5d6f7faaec8fd7770cb532f87e38fb82ecb47f58474cbeb29a174f1afd829" + ) + expected_signature = HexBytes( + "0x82484cd1f17430915f940d7c589e1687253de7f4062748b6807d29c67af52e3957b90427a9202ba4bc2104a129183387d994be2e4a8623a1f1ba2bad75d3546d1b" + ) + sender = list(safe_operator.accounts)[0] + self.assertTrue( + safe_operator.confirm_message(safe_message_hash, sender.address) + ) + post_message_signature.assert_called_with(safe_message_hash, expected_signature) + + post_message_signature.side_effect = SafeAPIException( + "Error posting message signature" + ) + self.assertFalse( + safe_operator.confirm_message(safe_message_hash, sender.address) + ) + def test_drain(self): # TODO Drain is a complex to mock pass From b8b048e40a0e030de645df1c8e92c93bb885077d Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Thu, 20 Jun 2024 10:44:02 +0200 Subject: [PATCH 5/7] Add comments --- src/safe_cli/operators/safe_tx_service_operator.py | 4 ++-- src/safe_cli/safe_completer_constants.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/safe_cli/operators/safe_tx_service_operator.py b/src/safe_cli/operators/safe_tx_service_operator.py index 2643082..2569b4b 100644 --- a/src/safe_cli/operators/safe_tx_service_operator.py +++ b/src/safe_cli/operators/safe_tx_service_operator.py @@ -81,7 +81,7 @@ def sign_message( if self.safe_tx_service.post_message(self.address, message, signatures): print_formatted_text( HTML( - "Message was correctly created on Safe Transaction Service" + f"Message with safe-message-hash {safe_message_hash} was correctly created on Safe Transaction Service" ) ) return True @@ -133,7 +133,7 @@ def confirm_message(self, safe_message_hash: bytes, sender: ChecksumAddress): return False print_formatted_text( HTML( - "Message was correctly confirmed on Safe Transaction Service" + f"Message with safe-message-hash {safe_message_hash} was correctly confirmed on Safe Transaction Service" ) ) return True diff --git a/src/safe_cli/safe_completer_constants.py b/src/safe_cli/safe_completer_constants.py index 6794fa2..7f07130 100644 --- a/src/safe_cli/safe_completer_constants.py +++ b/src/safe_cli/safe_completer_constants.py @@ -37,6 +37,7 @@ "send_ether": "
[--safe-nonce ]", "show_cli_owners": "(read-only)", "sign_message": "[--eip191_message] [--eip712_path ]", + "confirm_message": " ", "sign-tx": "", "unload_cli_owners": "
[
...]", "update": "", From 734f8d57ed627d676447474761d0ed82346b3ccb Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:04:00 +0200 Subject: [PATCH 6/7] Remove unused parameter --- src/safe_cli/operators/safe_operator.py | 1 - src/safe_cli/operators/safe_tx_service_operator.py | 1 - src/safe_cli/prompt_parser.py | 2 +- tests/test_safe_operator.py | 2 +- 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/safe_cli/operators/safe_operator.py b/src/safe_cli/operators/safe_operator.py index a6086a0..90c8486 100644 --- a/src/safe_cli/operators/safe_operator.py +++ b/src/safe_cli/operators/safe_operator.py @@ -456,7 +456,6 @@ def approve_hash(self, hash_to_approve: HexBytes, sender: str) -> bool: def sign_message( self, - eip191_message: Optional[bool] = False, eip712_message_path: Optional[str] = None, ) -> bool: if eip712_message_path: diff --git a/src/safe_cli/operators/safe_tx_service_operator.py b/src/safe_cli/operators/safe_tx_service_operator.py index 2569b4b..71f0b51 100644 --- a/src/safe_cli/operators/safe_tx_service_operator.py +++ b/src/safe_cli/operators/safe_tx_service_operator.py @@ -44,7 +44,6 @@ def approve_hash(self, hash_to_approve: HexBytes, sender: str) -> bool: def sign_message( self, - eip191_message: Optional[bool] = False, eip712_message_path: Optional[str] = None, ) -> bool: if eip712_message_path: diff --git a/src/safe_cli/prompt_parser.py b/src/safe_cli/prompt_parser.py index b864e26..d332476 100644 --- a/src/safe_cli/prompt_parser.py +++ b/src/safe_cli/prompt_parser.py @@ -194,7 +194,7 @@ def approve_hash(args): @safe_exception def sign_message(args): - safe_operator.sign_message(args.eip191_message, args.eip712_path) + safe_operator.sign_message(args.eip712_path) @safe_exception def confirm_message(args): diff --git a/tests/test_safe_operator.py b/tests/test_safe_operator.py index 5f916c6..2575155 100644 --- a/tests/test_safe_operator.py +++ b/tests/test_safe_operator.py @@ -186,7 +186,7 @@ def test_sign_message(self, mock_sign_message_lib_address): safe_operator.default_sender = self.ethereum_test_account message_hash = safe_operator.safe.get_message_hash(bytes(message, "utf-8")) with mock.patch("builtins.input", return_value=message): - safe_operator.sign_message(eip191_message=True) + safe_operator.sign_message() self.assertTrue(safe_operator.safe.retrieve_is_message_signed(message_hash)) eip712_path = "tests/mocks/mock_eip712.json" From 0e4d42639dcfb89585fc2cd1f1c2097ab1ac60ca Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:43:23 +0200 Subject: [PATCH 7/7] Fix bytes print format --- src/safe_cli/operators/safe_tx_service_operator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/safe_cli/operators/safe_tx_service_operator.py b/src/safe_cli/operators/safe_tx_service_operator.py index 71f0b51..283c90e 100644 --- a/src/safe_cli/operators/safe_tx_service_operator.py +++ b/src/safe_cli/operators/safe_tx_service_operator.py @@ -80,7 +80,7 @@ def sign_message( if self.safe_tx_service.post_message(self.address, message, signatures): print_formatted_text( HTML( - f"Message with safe-message-hash {safe_message_hash} was correctly created on Safe Transaction Service" + f"Message with safe-message-hash {safe_message_hash.hex()} was correctly created on Safe Transaction Service" ) ) return True @@ -99,7 +99,7 @@ def confirm_message(self, safe_message_hash: bytes, sender: ChecksumAddress): except SafeAPIException: print_formatted_text( HTML( - f"Message with hash {safe_message_hash} does not exist" + f"Message with hash {safe_message_hash.hex()} does not exist" ) ) if not yes_or_no_question( @@ -132,7 +132,7 @@ def confirm_message(self, safe_message_hash: bytes, sender: ChecksumAddress): return False print_formatted_text( HTML( - f"Message with safe-message-hash {safe_message_hash} was correctly confirmed on Safe Transaction Service" + f"Message with safe-message-hash {safe_message_hash.hex()} was correctly confirmed on Safe Transaction Service" ) ) return True