Add tests

safe_cli/operators/hw_wallets/hw_wallet.py

@@ -48,11 +48,14 @@ def sign_typed_hash(self, domain_hash: bytes, message_hash: bytes) -> bytes:
         """
 
     @abstractmethod
-    def get_signed_raw_transaction(self, tx_parameters: TxParams) -> HexStr:
+    def get_signed_raw_transaction(
+        self, tx_parameters: TxParams, chain_id: int
+    ) -> HexStr:
         """
 
+        :param chain_id:
         :param tx_parameters:
-        :return:
+        :return: raw transaction signed
         """
 
     def __str__(self):

safe_cli/operators/hw_wallets/hw_wallet_manager.py

@@ -75,10 +75,7 @@ def get_accounts(
         return accounts
 
     def add_account(
-        self,
-        hw_wallet_type: HwWalletType,
-        derivation_path: str,
-        set_as_sender: Optional[bool] = False,
+        self, hw_wallet_type: HwWalletType, derivation_path: str
     ) -> ChecksumAddress:
         """
         Add an account to ledger manager set and return the added address
@@ -91,8 +88,6 @@ def add_account(
 
         wallet = hw_wallet(derivation_path)
         self.wallets.add(wallet)
-        if set_as_sender:
-            self.sender = wallet
         return wallet.address
 
     def set_sender(self, hw_wallet_type: HwWalletType, derivation_path: str):
@@ -116,6 +111,8 @@ def delete_accounts(self, addresses: List[ChecksumAddress]) -> Set:
         for address in addresses:
             for account in self.wallets:
                 if account.address == address:
+                    if self.sender and self.sender.address == address:
+                        self.sender = None
                     accounts_to_remove.add(account)
         self.wallets = self.wallets.difference(accounts_to_remove)
         return accounts_to_remove
@@ -156,7 +153,7 @@ def sign_eip712(self, safe_tx: SafeTx, wallets: List[HwWallet]) -> SafeTx:
 
         return safe_tx
 
-    def execute(
+    def execute_safe_tx(
         self,
         safe_tx: SafeTx,
         tx_gas: Optional[int] = None,
@@ -202,7 +199,7 @@ def execute(
             tx_gas or (max(safe_tx.tx["gas"] + 75000, safe_tx.recommended_gas()))
         )
         signed_raw_transaction = self.sender.get_signed_raw_transaction(
-            safe_tx.tx
+            safe_tx.tx, safe_tx.ethereum_client.get_chain_id()
         )  # sign with ledger
         safe_tx.tx_hash = safe_tx.ethereum_client.w3.eth.send_raw_transaction(
             signed_raw_transaction

safe_cli/operators/hw_wallets/ledger_wallet.py

@@ -52,21 +52,24 @@ def sign_typed_hash(self, domain_hash: bytes, message_hash: bytes) -> bytes:
         return signature_to_bytes(signed.v, signed.r, signed.s)
 
     @raise_ledger_exception_as_hw_wallet_exception
-    def get_signed_raw_transaction(self, tx_parameters: TxParams) -> HexStr:
+    def get_signed_raw_transaction(
+        self, tx_parameters: TxParams, chain_id: int
+    ) -> HexStr:
         """
 
+        :param chain_id:
         :param tx_parameters:
-        :return:
+        :return: raw transaction signed
         """
         signed_transaction = create_transaction(
             destination=tx_parameters["to"],
             amount=tx_parameters["value"],
             gas=tx_parameters["gas"],
             nonce=tx_parameters["nonce"],
-            data=tx_parameters["data"],
-            max_priority_fee_per_gas=tx_parameters["maxPriorityFeePerGas"],
-            max_fee_per_gas=tx_parameters["maxPriorityFeePerGas"],
-            chain_id=5,
+            data=tx_parameters.get("data"),
+            max_priority_fee_per_gas=tx_parameters.get("maxPriorityFeePerGas"),
+            max_fee_per_gas=tx_parameters.get("maxPriorityFeePerGas"),
+            chain_id=chain_id,
             sender_path=self.derivation_path,
             dongle=self.dongle,
         )

safe_cli/operators/hw_wallets/trezor_wallet.py

@@ -54,7 +54,8 @@ def sign_typed_hash(self, domain_hash: bytes, message_hash: bytes) -> bytes:
     def get_signed_raw_transaction(self, tx_parameters: TxParams) -> HexStr:
         """
 
+        :param chain_id:
         :param tx_parameters:
-        :return:
+        :return: raw transaction signed
         """
         raise NotImplementedError

safe_cli/operators/safe_operator.py

@@ -870,7 +870,7 @@ def execute_safe_transaction(self, safe_tx: SafeTx):
                         self.default_sender.key, eip1559_speed=TxSpeed.NORMAL
                     )
                 else:
-                    tx_hash, tx = self.hw_wallet_manager.execute(
+                    tx_hash, tx = self.hw_wallet_manager.execute_safe_tx(
                         safe_tx, eip1559_speed=TxSpeed.NORMAL
                     )
                 self.executed_transactions.append(tx_hash.hex())

tests/test_hw_wallet_manager.py

@@ -3,8 +3,11 @@
 from unittest.mock import MagicMock
 
 from eth_account import Account
+from hexbytes import HexBytes
 from ledgerblue.Dongle import Dongle
+from ledgereth import SignedTransaction
 
+from gnosis.safe import SafeTx
 from gnosis.safe.tests.safe_test_case import SafeTestCaseMixin
 
 from safe_cli.operators.hw_wallets.hw_wallet_manager import (
@@ -133,3 +136,81 @@ def test_delete_account(
             2,
         )
         self.assertEqual(len(hw_wallet_manager.wallets), 0)
+
+    @mock.patch(
+        "safe_cli.operators.hw_wallets.ledger_wallet.create_transaction",
+        autospec=True,
+        return_value=Dongle(),
+    )
+    @mock.patch(
+        "safe_cli.operators.hw_wallets.ledger_wallet.LedgerWallet.get_address",
+        autospec=True,
+    )
+    @mock.patch(
+        "safe_cli.operators.hw_wallets.ledger_wallet.init_dongle",
+        autospec=True,
+        return_value=Dongle(),
+    )
+    def test_execute(
+        self,
+        mock_init_dongle: MagicMock,
+        mock_get_address: MagicMock,
+        mock_ledger_create_transaction: MagicMock,
+    ):
+        owner = self.ethereum_test_account
+        to = Account.create()
+        derivation_path = "44'/60'/0'/0"
+        hw_wallet_manager = HwWalletManager()
+        mock_get_address.return_value = owner.address
+        hw_wallet_manager.add_account(HwWalletType.LEDGER, derivation_path)
+        hw_wallet_manager.set_sender(HwWalletType.LEDGER, derivation_path)
+        self.assertEqual(hw_wallet_manager.sender.address, owner.address)
+        safe = self.deploy_test_safe(
+            owners=[owner.address],
+            threshold=1,
+            initial_funding_wei=self.w3.to_wei(0.1, "ether"),
+        )
+        safe_tx = SafeTx(
+            self.ethereum_client,
+            safe.address,
+            to.address,
+            10,
+            b"",
+            0,
+            200000,
+            200000,
+            self.gas_price,
+            None,
+            None,
+            safe_nonce=0,
+        )
+        safe_tx.sign(owner.key)
+        tx_parameters = {
+            "from": owner.address,
+            "gasPrice": safe_tx.w3.eth.gas_price,
+            "nonce": 0,
+            "gas": safe_tx.recommended_gas(),
+        }
+        safe_tx.tx = safe_tx.w3_tx.build_transaction(tx_parameters)
+        signed_fields = safe_tx.w3.eth.account.sign_transaction(
+            safe_tx.tx, private_key=owner.key
+        )
+
+        mocked_signed_transaction_response = SignedTransaction(
+            nonce=0,
+            gas_price=safe_tx.tx["gasPrice"],
+            gas_limit=safe_tx.tx["gas"],
+            destination=HexBytes(safe_tx.tx["to"]),
+            amount=safe_tx.tx["value"],
+            data=HexBytes(safe_tx.tx["data"]),
+            v=signed_fields.v,
+            r=signed_fields.r,
+            s=signed_fields.s,
+        )
+
+        mock_ledger_create_transaction.return_value = mocked_signed_transaction_response
+        tx_hash, tx = hw_wallet_manager.execute_safe_tx(safe_tx)
+        self.assertEqual(tx["data"], safe_tx.tx["data"])
+        self.assertIsNotNone(
+            self.ethereum_client.w3.eth.get_transaction_receipt(tx_hash)
+        )

tests/test_ledger_wallet.py

@@ -4,14 +4,15 @@
 from unittest.mock import MagicMock
 
 from eth_account import Account
+from hexbytes import HexBytes
 from ledgerblue.Dongle import Dongle
 from ledgereth.exceptions import (
     LedgerAppNotOpened,
     LedgerCancel,
     LedgerLocked,
     LedgerNotFound,
 )
-from ledgereth.objects import LedgerAccount
+from ledgereth.objects import LedgerAccount, SignedTransaction
 
 from gnosis.eth.eip712 import eip712_encode
 from gnosis.safe import SafeTx
@@ -167,3 +168,79 @@ def test_sign_typed_hash(
         mock_init_dongle.return_value.exchange.assert_called_once_with(
             expected_exchange_payload
         )
+
+    @mock.patch(
+        "safe_cli.operators.hw_wallets.ledger_wallet.create_transaction",
+        autospec=True,
+        return_value=Dongle(),
+    )
+    @mock.patch(
+        "safe_cli.operators.hw_wallets.ledger_wallet.get_account_by_path",
+        autospec=True,
+    )
+    @mock.patch(
+        "safe_cli.operators.hw_wallets.ledger_wallet.init_dongle",
+        autospec=True,
+        return_value=Dongle(),
+    )
+    def test_get_signed_raw_transaction(
+        self,
+        mock_init_dongle: MagicMock,
+        mock_get_account_by_path: MagicMock,
+        mock_ledger_create_transaction: MagicMock,
+    ):
+        owner = Account.create()
+        to = Account.create()
+        derivation_path = "44'/60'/0'/0"
+        mock_get_account_by_path.return_value = LedgerAccount(
+            derivation_path, owner.address
+        )
+        ledger_wallet = LedgerWallet(derivation_path)
+        safe = self.deploy_test_safe(
+            owners=[owner.address],
+            threshold=1,
+            initial_funding_wei=self.w3.to_wei(0.1, "ether"),
+        )
+        safe_tx = SafeTx(
+            self.ethereum_client,
+            safe.address,
+            to.address,
+            10,
+            b"",
+            0,
+            200000,
+            200000,
+            self.gas_price,
+            None,
+            None,
+            safe_nonce=0,
+        )
+        safe_tx.sign(owner.key)
+        tx_parameters = {
+            "from": owner.address,
+            "gasPrice": safe_tx.w3.eth.gas_price,
+            "nonce": 0,
+            "gas": safe_tx.recommended_gas(),
+        }
+        safe_tx.tx = safe_tx.w3_tx.build_transaction(tx_parameters)
+        signed_fields = safe_tx.w3.eth.account.sign_transaction(
+            safe_tx.tx, private_key=owner.key
+        )
+
+        mocked_signed_transaction_response = SignedTransaction(
+            nonce=0,
+            gas_price=safe_tx.tx["gasPrice"],
+            gas_limit=safe_tx.tx["gas"],
+            destination=HexBytes(safe_tx.tx["to"]),
+            amount=safe_tx.tx["value"],
+            data=HexBytes(safe_tx.tx["data"]),
+            v=signed_fields.v,
+            r=signed_fields.r,
+            s=signed_fields.s,
+        )
+        mock_ledger_create_transaction.return_value = mocked_signed_transaction_response
+
+        raw_signed_tx = ledger_wallet.get_signed_raw_transaction(
+            safe_tx.tx, safe_tx.ethereum_client.get_chain_id()
+        )  # return raw signed transaction
+        self.assertEqual(signed_fields.rawTransaction, HexBytes(raw_signed_tx))

tests/test_safe_operator.py

@@ -8,6 +8,7 @@
 from ledgerblue.Dongle import Dongle
 from ledgereth.objects import LedgerAccount
 from web3 import Web3
+from web3.types import Wei
 
 from gnosis.eth import EthereumClient
 from gnosis.safe import Safe
@@ -113,14 +114,15 @@ def test_load_ledger_cli_owner(
         )
         safe_operator.load_ledger_cli_owners()
         self.assertEqual(len(safe_operator.hw_wallet_manager.wallets), 1)
+        # Wallet without funds shouldn't be added as sender
+        self.assertIsNone(safe_operator.hw_wallet_manager.sender)
         self.assertEqual(
             safe_operator.hw_wallet_manager.wallets.pop().address, random_address
         )
 
         # Only accept ethereum derivation paths
         with self.assertRaises(HardwareWalletException):
             safe_operator.load_ledger_cli_owners(derivation_path="44'/137'/0'/0/1")
-
         mock_get_account_by_path.return_value = LedgerAccount(
             "44'/60'/0'/0/0", owner_address
         )
@@ -130,14 +132,23 @@ def test_load_ledger_cli_owner(
             safe_operator.hw_wallet_manager.wallets.pop().address, owner_address
         )
 
-        # test unload ledger owner
+        # Should be loaded as sender
         ledger_random_address = Account.create().address
-        safe_operator.hw_wallet_manager.wallets.add(
-            LedgerAccount("44'/60'/0'/1", ledger_random_address)
+        # Send funds to define it as sender
+        self.send_ether(ledger_random_address, Wei(1000))
+        mock_get_account_by_path.return_value = LedgerAccount(
+            "44'/60'/0'/0/1", ledger_random_address
         )
+        safe_operator.load_ledger_cli_owners(derivation_path="44'/60'/0'/0/1")
         self.assertEqual(len(safe_operator.hw_wallet_manager.wallets), 1)
+        self.assertEqual(
+            safe_operator.hw_wallet_manager.sender.address, ledger_random_address
+        )
+
+        # test unload ledger owner
         safe_operator.unload_cli_owners([ledger_random_address])
         self.assertEqual(len(safe_operator.hw_wallet_manager.wallets), 0)
+        self.assertIsNone(safe_operator.hw_wallet_manager.sender)
 
     def test_approve_hash(self):
         safe_address = self.deploy_test_safe(