Add support to remove not executed transactions (#352)

* Add support to remove not executed transactions

* Add test remove transaction

* Add documentation

* Apply suggestions from code review

Co-authored-by: Uxío <Uxio0@users.noreply.github.com>

* Refactor search_account
Add check for keccak hash

* Add warning when threeshold was reached

* Bump safe-eth-py to v6.0.0b16

* Apply suggestions from code review

Co-authored-by: Uxío <Uxio0@users.noreply.github.com>

* Fix linting

---------

Co-authored-by: Uxío <Uxio0@users.noreply.github.com>

README.md

@@ -216,6 +216,7 @@ There are 2 operation modes:
 - `get_delegates`: Returns a list of **delegates** for the Safe. A **delegate** can be used when you **trust an address to post transactions to the tx-service on your behalf**. If a transaction is not trusted (posted to the service not signed by a delegate or an owner of the Safe) it will be stored in the service but not shown in the UI or mobile applications.
 - `add_delegate <address> <label> <owner-address>`: Adds a new delegate `address` for the `owner` of the Safe.
 - `remove_delegate <address> <owner-address>`: Removes a delegate `address` from the Safe.
+- `remove_proposed_transaction <safe_tx_hsh>`: Removes a proposed non executed transaction with the signature of the owner that proposed the transaction.
 - `drain <address>`: Sends all Ether and ERC20 funds to the provided account. **WARNING: DON'T USE THIS IF YOU DON'T KNOW WHAT YOU ARE DOING. ALL YOUR FUNDS COULD BE LOST**
 
 If the information in the information bar is outdated or there's any problem you can force the `safe-cli` to update the information about the Safe using:

requirements.txt

@@ -5,7 +5,7 @@ packaging>=23.1
 prompt_toolkit==3.0.43
 pygments==2.17.2
 requests==2.31.0
-safe-eth-py==6.0.0b15
+safe-eth-py==6.0.0b16
 tabulate==0.9.0
 trezor==0.13.8
 web3==6.15.1

safe_cli/operators/hw_wallets/hw_wallet_manager.py

@@ -8,8 +8,9 @@
 from web3.types import TxParams, Wei
 
 from gnosis.eth import TxSpeed
-from gnosis.eth.eip712 import eip712_encode
+from gnosis.eth.eip712 import eip712_encode, eip712_encode_hash
 from gnosis.safe import SafeTx
+from gnosis.safe.safe_signature import SafeSignature, SafeSignatureEOA
 
 from .hw_wallet import HwWallet
 
@@ -113,18 +114,17 @@ def delete_accounts(self, addresses: List[ChecksumAddress]) -> Set:
         self.wallets = self.wallets.difference(accounts_to_remove)
         return accounts_to_remove
 
-    def sign_eip712(self, safe_tx: SafeTx, wallets: List[HwWallet]) -> SafeTx:
+    def sign_eip712(self, eip712_message: Dict, wallets: List[HwWallet]) -> HexBytes:
         """
-        Sign a safeTx EIP-712 hashes with supported hw wallet devices
+        Sign an EIP712 message
 
-        :param domain_hash:
-        :param message_hash:
-        :param wallets: list of HwWallet
-        :return: signed safeTx
+        :param eip712_message:
+        :param wallets:
+        :return: Appended sorted signatures for all the provided wallets
         """
-        encode_hash = eip712_encode(safe_tx.eip712_structured_data)
-        domain_hash = encode_hash[1]
-        message_hash = encode_hash[2]
+        _, domain_hash, message_hash = eip712_encode(eip712_message)
+        eip712_message_hash = eip712_encode_hash(eip712_message)
+        safe_signatures: List[SafeSignature] = []
         for wallet in wallets:
             print_formatted_text(
                 HTML(
@@ -134,19 +134,20 @@ def sign_eip712(self, safe_tx: SafeTx, wallets: List[HwWallet]) -> SafeTx:
             print_formatted_text(HTML(f"Domain_hash: <b>{domain_hash.hex()}</b>"))
             print_formatted_text(HTML(f"Message_hash: <b>{message_hash.hex()}</b>"))
             signature = wallet.sign_typed_hash(domain_hash, message_hash)
+            safe_signatures.append(SafeSignatureEOA(signature, eip712_message_hash))
 
-            # Insert signature sorted
-            if wallet.address not in safe_tx.signers:
-                new_owners = safe_tx.signers + [wallet.address]
-                new_owner_pos = sorted(new_owners, key=lambda x: int(x, 16)).index(
-                    wallet.address
-                )
-                safe_tx.signatures = (
-                    safe_tx.signatures[: 65 * new_owner_pos]
-                    + signature
-                    + safe_tx.signatures[65 * new_owner_pos :]
-                )
+        return SafeSignature.export_signatures(safe_signatures)
 
+    def sign_safe_tx(self, safe_tx: SafeTx, wallets: List[HwWallet]) -> SafeTx:
+        """
+        Sign a safe transaction with the provided hardware wallets
+
+        :param safe_tx:
+        :param wallets:
+        :return: SafeTx with signature.
+        """
+        signatures = self.sign_eip712(safe_tx.eip712_structured_data, wallets)
+        safe_tx.signatures = signatures
         return safe_tx
 
     def execute_safe_tx(

safe_cli/operators/safe_operator.py

@@ -1071,7 +1071,7 @@ def sign_transaction(self, safe_tx: SafeTx) -> SafeTx:
 
         # Sign with ledger
         if len(hw_wallets_signers):
-            safe_tx = self.hw_wallet_manager.sign_eip712(safe_tx, hw_wallets_signers)
+            safe_tx = self.hw_wallet_manager.sign_safe_tx(safe_tx, hw_wallets_signers)
 
         return safe_tx
 
@@ -1164,6 +1164,9 @@ def drain(self, to: str):
                 HTML("<ansigreen>Safe account is currently empty</ansigreen>")
             )
 
+    def remove_proposed_transaction(self, safe_tx_hash: bytes):
+        return self._require_tx_service_mode()
+
     def process_command(self, first_command: str, rest_command: List[str]) -> bool:
         if first_command == "help":
             print_formatted_text("I still cannot help you")

safe_cli/operators/safe_tx_service_operator.py

@@ -1,8 +1,10 @@
 import json
-from typing import Any, Dict, List, Optional, Sequence, Set
+from itertools import chain
+from typing import Any, Dict, List, Optional, Sequence, Set, Union
 
 from colorama import Fore, Style
 from eth_account.messages import defunct_hash_message
+from eth_account.signers.local import LocalAccount
 from eth_typing import ChecksumAddress
 from hexbytes import HexBytes
 from prompt_toolkit import HTML, print_formatted_text
@@ -12,6 +14,9 @@
 from gnosis.eth.eip712 import eip712_encode_hash
 from gnosis.safe import SafeOperation, SafeTx
 from gnosis.safe.api import SafeAPIException
+from gnosis.safe.api.transaction_service_api.transaction_service_messages import (
+    get_remove_transaction_message,
+)
 from gnosis.safe.multi_send import MultiSend, MultiSendOperation, MultiSendTx
 from gnosis.safe.safe_signature import SafeSignature, SafeSignatureEOA
 from gnosis.safe.signatures import signature_to_bytes
@@ -20,6 +25,7 @@
 
 from . import SafeServiceNotAvailable
 from .exceptions import AccountNotLoadedException, NonExistingOwnerException
+from .hw_wallets.hw_wallet import HwWallet
 from .safe_operator import SafeOperator
 
 
@@ -161,7 +167,7 @@ def submit_signatures(self, safe_tx_hash: bytes) -> bool:
                     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_eip712(
+                    safe_tx = self.hw_wallet_manager.sign_safe_tx(
                         safe_tx, selected_ledger_accounts
                     )
 
@@ -411,3 +417,63 @@ def drain(self, to: ChecksumAddress):
             print_formatted_text(
                 HTML("<ansigreen>Safe account is currently empty</ansigreen>")
             )
+
+    def search_account(
+        self, address: ChecksumAddress
+    ) -> Optional[Union[LocalAccount, HwWallet]]:
+        """
+        Search the provided address between loaded owners
+
+        :param address:
+        :return: LocalAccount or HwWallet of the provided address
+        """
+        for account in chain(self.accounts, self.hw_wallet_manager.wallets):
+            if account.address == address:
+                return account
+
+    def remove_proposed_transaction(self, safe_tx_hash: bytes):
+        eip712_message = get_remove_transaction_message(
+            self.address, safe_tx_hash, self.ethereum_client.get_chain_id()
+        )
+        message_hash = eip712_encode_hash(eip712_message)
+        try:
+            safe_tx, _ = self.safe_tx_service.get_safe_transaction(safe_tx_hash)
+            signer = self.search_account(safe_tx.proposer)
+            if not signer:
+                print_formatted_text(
+                    HTML(
+                        f"<ansired>The proposer with address: {safe_tx.proposer} wasn loaded</ansired>"
+                    )
+                )
+            if isinstance(signer, LocalAccount):
+                signature = signer.signHash(message_hash).signature
+            else:
+                signature = self.hw_wallet_manager.sign_eip712(eip712_message, [signer])
+
+            if len(safe_tx.signers) >= self.safe.retrieve_threshold():
+                print_formatted_text(
+                    HTML(
+                        "<ansired>The transaction has all the required signatures to be executed!!!\n"
+                        "This means that the transaction can be executed by a 3rd party monitoring your Safe even after removal!\n"
+                        f"Make sure you execute a transaction with nonce {safe_tx.safe_nonce} to void the current transaction"
+                        "</ansired>"
+                    )
+                )
+
+            if not yes_or_no_question(
+                f"Do you want to remove the tx with safe-tx-hash={safe_tx.safe_tx_hash.hex()}"
+            ):
+                return False
+
+            self.safe_tx_service.delete_transaction(safe_tx_hash.hex(), signature.hex())
+            print_formatted_text(
+                HTML(
+                    f"<ansigreen>Transaction {safe_tx_hash.hex()} was removed correctly</ansigreen>"
+                )
+            )
+            return True
+        except SafeAPIException as e:
+            print_formatted_text(
+                HTML(f"<ansired>Transaction wasn't removed due an error: {e}</ansired>")
+            )
+            return False

safe_cli/prompt_parser.py

@@ -315,6 +315,10 @@ def add_delegate(args):
     def remove_delegate(args):
         safe_operator.remove_delegate(args.address, args.signer)
 
+    @safe_exception
+    def remove_proposed_transaction(args):
+        safe_operator.remove_proposed_transaction(args.safe_tx_hash)
+
     # Cli owners
     parser_show_cli_owners = subparsers.add_parser("show_cli_owners")
     parser_show_cli_owners.set_defaults(func=show_cli_owners)
@@ -499,16 +503,18 @@ def remove_delegate(args):
 
     parser_tx_service = subparsers.add_parser("sign-tx")
     parser_tx_service.set_defaults(func=sign_tx)
-    parser_tx_service.add_argument("safe_tx_hash", type=check_hex_str)
+    parser_tx_service.add_argument("safe_tx_hash", type=check_keccak256_hash)
 
     parser_tx_service = subparsers.add_parser("batch-txs")
     parser_tx_service.set_defaults(func=batch_txs)
     parser_tx_service.add_argument("safe_nonce", type=int)
-    parser_tx_service.add_argument("safe_tx_hashes", type=check_hex_str, nargs="+")
+    parser_tx_service.add_argument(
+        "safe_tx_hashes", type=check_keccak256_hash, nargs="+"
+    )
 
     parser_tx_service = subparsers.add_parser("execute-tx")
     parser_tx_service.set_defaults(func=execute_tx)
-    parser_tx_service.add_argument("safe_tx_hash", type=check_hex_str)
+    parser_tx_service.add_argument("safe_tx_hash", type=check_keccak256_hash)
 
     # List delegates
     parser_delegates = subparsers.add_parser("get_delegates")
@@ -527,4 +533,13 @@ def remove_delegate(args):
     parser_remove_delegate.add_argument("address", type=check_ethereum_address)
     parser_remove_delegate.add_argument("signer", type=check_ethereum_address)
 
+    # Remove not executed proposed transaction
+    parser_remove_proposed_transaction = subparsers.add_parser(
+        "remove_proposed_transaction"
+    )
+    parser_remove_proposed_transaction.set_defaults(func=remove_proposed_transaction)
+    parser_remove_proposed_transaction.add_argument(
+        "safe_tx_hash", type=check_keccak256_hash
+    )
+
     return prompt_parser

safe_cli/safe_completer_constants.py

@@ -30,6 +30,7 @@
     "refresh": "",
     "remove_delegate": "<address> <signer-address>",
     "remove_owner": "<address> [--threshold <int>]",
+    "remove_proposed_transaction": "safe-tx-hash",
     "send_custom": "<address> <value-wei> <data> [--delegate] [--safe-nonce <int>]",
     "send_erc20": "<address> <token-address> <value-wei> [--safe-nonce <int>]",
     "send_erc721": "<address> <token-address> <token-id> [--safe-nonce <int>]",
@@ -118,6 +119,10 @@
         "Command <b>remove_delegate</b> will remove a delegate <u>&lt;address&gt;</u> from the "
         "current loaded safe."
     ),
+    "remove_proposed_transaction": HTML(
+        "Command <b>remove_proposed_transaction</b> will remove proposed not executed transaction for "
+        "provided<u>&lt;safe-tx-hash&gt;</u>"
+    ),
     "enable_module": HTML(
         "Command <b>enable_module</b> will enable a check-summed <u>&lt;address&gt;</u> module."
     ),

safe_cli/safe_lexer.py

@@ -32,6 +32,7 @@ class SafeLexer(BashLexer):
         "send_ether",
         "send_erc20",
         "send_erc721",
+        "remove_proposed_transaction",
     }
 
     def get_tokens_unprocessed(self, text: str) -> (int, Token, str):

setup.py

@@ -24,7 +24,7 @@
         "prompt_toolkit>=3",
         "pygments>=2",
         "requests>=2",
-        "safe-eth-py==6.0.0b15",
+        "safe-eth-py==6.0.0b16",
         "tabulate>=0.8",
     ],
     extras_require={"ledger": ["ledgereth==0.9.1"], "trezor": ["trezor==0.13.8"]},

tests/test_safe_tx_service_operator.py

@@ -99,6 +99,35 @@ def test_remove_delegate(self, remove_delegate_mock: MagicMock):
             safe_operator.address, delegate_address, signer
         )
 
+    @mock.patch.object(TransactionServiceApi, "delete_transaction", return_value=None)
+    @mock.patch.object(TransactionServiceApi, "get_safe_transaction")
+    @mock.patch(
+        "gnosis.safe.api.transaction_service_api.transaction_service_messages.get_totp",
+        return_value=8365,
+    )
+    def test_remove_transaction(
+        self,
+        mock_get_totp,
+        get_transaction_mock: MagicMock,
+        remove_transaction_mock: MagicMock,
+    ):
+        safe_operator = self.setup_operator(
+            number_owners=1, mode=SafeOperatorMode.TX_SERVICE
+        )
+        safe_operator.address = "0x23793e7Ce4f2Fd31C993893f658181fD39fB5dEb"
+        signer = list(safe_operator.accounts)[0]
+        safe_tx_mock = MagicMock()
+        safe_tx_mock.proposer = signer.address
+        get_transaction_mock.return_value = (safe_tx_mock, None)
+        safe_tx_hash = HexBytes(
+            "0x07eeea19b7d005b561f367e714bf65734d0ecc477de3e8b1fbc20ccb18c8747b"
+        )
+        expected_signature = "0xbc305f59a62c5bbb002645f658ee62cfa1966f20aca4dc1922b813e9d41bf1f02abd356891a3725af2066dd1758c930574298b4ba180117dc6146bbc4369cc0c1c"
+        self.assertTrue(safe_operator.remove_proposed_transaction(safe_tx_hash))
+        remove_transaction_mock.assert_called_with(
+            safe_tx_hash.hex(), expected_signature
+        )
+
     @mock.patch.object(SafeTxServiceOperator, "get_permitted_signers", return_value=[])
     @mock.patch.object(
         SafeTx, "safe_version", return_value="1.4.1", new_callable=mock.PropertyMock
@@ -139,7 +168,7 @@ def test_submit_signatures(
         with mock.patch.object(
             SafeTx, "signers", return_value=["signer"], new_callable=mock.PropertyMock
         ) as mock_safe_tx:
-            safe_operator.hw_wallet_manager.sign_eip712 = MagicMock(
+            safe_operator.hw_wallet_manager.sign_safe_tx = MagicMock(
                 return_value=mock_safe_tx
             )
             get_safe_transaction_mock.return_value = GetMultisigTxRequestMock(

tests/test_trezor_wallet.py

@@ -143,7 +143,7 @@ def test_sign_typed_hash(
         expected_signature = safe_tx.sign(owner.key)
 
         trezor_return_signature = EthereumTypedDataSignature(
-            signature=expected_signature
+            signature=expected_signature, address=trezor_wallet.address
         )
         mock_trezor_client.return_value.call = MagicMock(
             return_value=trezor_return_signature