diff --git a/safe_cli/operators/hw_wallets/hw_wallet.py b/safe_cli/operators/hw_wallets/hw_wallet.py index a77c858..ac17617 100644 --- a/safe_cli/operators/hw_wallets/hw_wallet.py +++ b/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): diff --git a/safe_cli/operators/hw_wallets/hw_wallet_manager.py b/safe_cli/operators/hw_wallets/hw_wallet_manager.py index a935bd1..a8f417a 100644 --- a/safe_cli/operators/hw_wallets/hw_wallet_manager.py +++ b/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 diff --git a/safe_cli/operators/hw_wallets/ledger_wallet.py b/safe_cli/operators/hw_wallets/ledger_wallet.py index e2ef548..4ed99a4 100644 --- a/safe_cli/operators/hw_wallets/ledger_wallet.py +++ b/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, ) diff --git a/safe_cli/operators/hw_wallets/trezor_wallet.py b/safe_cli/operators/hw_wallets/trezor_wallet.py index a99c3d1..896fdd3 100644 --- a/safe_cli/operators/hw_wallets/trezor_wallet.py +++ b/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 diff --git a/safe_cli/operators/safe_operator.py b/safe_cli/operators/safe_operator.py index 88a2ddb..2c47036 100644 --- a/safe_cli/operators/safe_operator.py +++ b/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()) diff --git a/tests/test_hw_wallet_manager.py b/tests/test_hw_wallet_manager.py index 6bfe29c..85214dd 100644 --- a/tests/test_hw_wallet_manager.py +++ b/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) + ) diff --git a/tests/test_ledger_wallet.py b/tests/test_ledger_wallet.py index bc9e0d4..e7ba4d2 100644 --- a/tests/test_ledger_wallet.py +++ b/tests/test_ledger_wallet.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock from eth_account import Account +from hexbytes import HexBytes from ledgerblue.Dongle import Dongle from ledgereth.exceptions import ( LedgerAppNotOpened, @@ -11,7 +12,7 @@ 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)) diff --git a/tests/test_safe_operator.py b/tests/test_safe_operator.py index c6a0502..991b35a 100644 --- a/tests/test_safe_operator.py +++ b/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,6 +114,8 @@ 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 ) @@ -120,7 +123,6 @@ def test_load_ledger_cli_owner( # 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(